Re: [PATCH] irq: fasteoi handler re-runs on concurrent invoke
From: Yipeng Zou
Date: Wed Mar 22 2023 - 02:26:56 EST
在 2023/3/17 19:49, Gowans, James 写道:
On Fri, 2023-03-17 at 18:12 +0800, Yipeng Zou wrote:
It seems that we have the same solution.
That's a good sign! :D
(I introduced a new flow handler).
I considered this, but IMO a new handler isn't the way to go: we already have a
bit of handler proliferation going on here. As mentioned in my commit message
there this is starting to get closer to handle_edge_eoi_irq, and adding a new
generic handler which is a mix of the two existing seems to just add more
confusion: which one should a driver owner use? I think it'll be great if we can
enhance the existing generic handlers to cater for the various edge cases and
perhaps even merge these generic handlers in future.
What are your thoughts on this approach compared to your proposal?
Hi,
I also agree with you, enhance the existing generic handlers is a good
way to go.
Too many generic handlers really confuse developers.
There is also the "delay the affinity change of LPI until the next interrupt
acknowledge" option described in the previous thread [0]. I also considered that
but seeing as the handle_edge_irq does the approach implemented here of setting
the PENDING flag and then re-running it, it seemed like good prior art to draw
on. Is that option of enabling CONFIG_GENERIC_PENDING_IRQ a viable? IMO the
generic handlers should be resilient to this so I would prefer this fix than
depending on the user to know to set this config option.
About CONFIG_GENERIC_PENDING_IRQ is actually some attempts we made
before under the suggestion of Thomas.
This patch is valid for our problem. However, the current config is only
supported on x86, and some code modifications are required on arm.
In our patch, the config cannot be perfectly supported on arm like x86.
Refer to the patch below.
This has led to some changes in the original behavior of modifying
interrupting affinity, from the next interrupt taking effect to the next
to the next interrupt taking effect.
So, in general, I also prefer the fix which make generic handlers be
resilient than introduce an CONFIG_GENERIC_PENDING_IRQ for gic.
diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index 1cb392fb16d0..64cfa5e38d89 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1678,6 +1678,12 @@ static int its_select_cpu_other(const struct
cpumask *mask_val)
}
#endif
+static void its_irq_chip_eoi(struct irq_data *d)
+{
+ irq_move_irq(d);
+ irq_chip_eoi_parent(d);
+}
+
static int its_set_affinity(struct irq_data *d, const struct cpumask
*mask_val,
bool force)
{
@@ -2026,7 +2032,7 @@ static struct irq_chip its_irq_chip = {
.name = "ITS",
.irq_mask = its_mask_irq,
.irq_unmask = its_unmask_irq,
- .irq_eoi = irq_chip_eoi_parent,
+ .irq_eoi = its_irq_chip_eoi,
.irq_set_affinity = its_set_affinity,
.irq_compose_msi_msg = its_irq_compose_msi_msg,
.irq_set_irqchip_state = its_irq_set_irqchip_state,
diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
index ab5505d8caf1..3c829bb4f649 100644
--- a/kernel/irq/Kconfig
+++ b/kernel/irq/Kconfig
@@ -33,7 +33,9 @@ config GENERIC_IRQ_LEGACY_ALLOC_HWIRQ
# Support for delayed migration from interrupt context
config GENERIC_PENDING_IRQ
- bool
+ bool "Support for delayed migration from interrupt context"
+ depends on SMP
+ default n
# Support for generic irq migrating off cpu before the cpu is offline.
config GENERIC_IRQ_MIGRATION
diff --git a/kernel/irq/migration.c b/kernel/irq/migration.c
index def48589ea48..bcb61ee69c20 100644
--- a/kernel/irq/migration.c
+++ b/kernel/irq/migration.c
@@ -117,3 +117,5 @@ void __irq_move_irq(struct irq_data *idata)
if (!masked)
idata->chip->irq_unmask(idata);
}
+
+void __weak irq_force_complete_move(struct irq_desc *desc) { }
JG
[0]https://lore.kernel.org/all/b0f2623b-ec70-d57e-b744-26c62b1ce523@xxxxxxxxxx/
--
Regards,
Yipeng Zou