| Message ID | 1606486531-25719-2-git-send-email-hanks.chen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/3] irqchip/gic: enable irq target all | expand |
On 2020-11-27 14:15, Hanks Chen wrote: > Support for interrupt distribution design for SMP system solutions. As far as I know, we have been supporting interrupt distribution on ARM SMP systems pretty well for the past... what... 15 years? I'm sure Russell can dig out an ARM926 SMP system that even predates that. > With this feature enabled ,the SPI interrupts would be routed to > all the cores rather than boot core to achieve better > load balance of interrupt handling. Please quantify this compared to the current distribution method. > That is, interrupts might be serviced simultaneously on different CPUs. They already can. What is new here? Or do you mean the *same* interrupt being serviced by different CPUs *at the same time*? That'd be fun. > Signed-off-by: Hanks Chen <hanks.chen@mediatek.com> > --- > drivers/irqchip/Kconfig | 12 ++++ > drivers/irqchip/irq-gic-v3.c | 107 +++++++++++++++++++++-------- > include/linux/irqchip/arm-gic-v3.h | 1 + > kernel/irq/cpuhotplug.c | 22 ++++++ > kernel/irq/manage.c | 7 ++ > 5 files changed, 122 insertions(+), 27 deletions(-) > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index c6098eee0c7c..c88ee7731e92 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -597,4 +597,16 @@ config MST_IRQ > help > Support MStar Interrupt Controller. > > +config ARM_IRQ_TARGET_ALL > + bool "Distribute interrupts across processors on SMP system" > + depends on SMP && ARM_GIC_V3 > + help > + Support for interrupt distribution design for > + SMP system solutions. With this feature enabled ,the > + SPI interrupts would be routed to all the cores rather > + than boot cpu to achieve better load balance of interrupt > + handling > + > + If you don't know what to do here, say N. There is no way we start introducing conditional compilation for architectural features. Either this works at all times, or it doesn't exist. > + > endmenu > diff --git a/drivers/irqchip/irq-gic-v3.c > b/drivers/irqchip/irq-gic-v3.c > index 16fecc0febe8..62a878ce4681 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -381,6 +381,12 @@ static inline bool gic_supports_nmi(void) > static_branch_likely(&supports_pseudo_nmis); > } > > +static inline bool gic_supports_1n(void) > +{ > + return (IS_ENABLED(CONFIG_ARM_IRQ_TARGET_ALL) && > + ~(readl_relaxed(gic_data.dist_base + GICD_TYPER) & > GICD_TYPER_No1N)); > +} > + > static int gic_irq_set_irqchip_state(struct irq_data *d, > enum irqchip_irq_state which, bool val) > { > @@ -716,6 +722,7 @@ static void __init gic_dist_init(void) > { > unsigned int i; > u64 affinity; > + Spurious whitespace. > void __iomem *base = gic_data.dist_base; > u32 val; > > @@ -759,16 +766,27 @@ static void __init gic_dist_init(void) > /* Enable distributor with ARE, Group1 */ > writel_relaxed(val, base + GICD_CTLR); > > - /* > - * Set all global interrupts to the boot CPU only. ARE must be > - * enabled. > - */ > - affinity = > gic_mpidr_to_affinity(cpu_logical_map(smp_processor_id())); > - for (i = 32; i < GIC_LINE_NR; i++) > - gic_write_irouter(affinity, base + GICD_IROUTER + i * 8); > + if (!gic_supports_1n()) { > + /* > + * Set all global interrupts to the boot CPU only. ARE must be > + * enabled. > + */ > + affinity = > gic_mpidr_to_affinity(cpu_logical_map(smp_processor_id())); > + for (i = 32; i < GIC_LINE_NR; i++) > + gic_write_irouter(affinity, base + GICD_IROUTER + i * 8); > > - for (i = 0; i < GIC_ESPI_NR; i++) > - gic_write_irouter(affinity, base + GICD_IROUTERnE + i * 8); > + for (i = 0; i < GIC_ESPI_NR; i++) > + gic_write_irouter(affinity, base + GICD_IROUTERnE + i * 8); > + } else { > + /* default set target all for all SPI */ > + for (i = 32; i < GIC_LINE_NR; i++) > + gic_write_irouter(GICD_IROUTER_SPI_MODE_ANY, > + base + GICD_IROUTER + i * 8); > + > + for (i = 0; i < GIC_ESPI_NR; i++) > + gic_write_irouter(GICD_IROUTER_SPI_MODE_ANY, > + base + GICD_IROUTERnE + i * 8); Why should we decide to distribute interrupts to all CPUs by default when we don't even know where to route them? > + } > } > > static int gic_iterate_rdists(int (*fn)(struct redist_region *, void > __iomem *)) > @@ -1191,29 +1209,64 @@ static int gic_set_affinity(struct irq_data > *d, const struct cpumask *mask_val, > if (gic_irq_in_rdist(d)) > return -EINVAL; > > - /* If interrupt was enabled, disable it first */ > - enabled = gic_peek_irq(d, GICD_ISENABLER); > - if (enabled) > - gic_mask_irq(d); > + if (!gic_supports_1n()) { > + /* If interrupt was enabled, disable it first */ > + enabled = gic_peek_irq(d, GICD_ISENABLER); > + if (enabled) > + gic_mask_irq(d); > > - offset = convert_offset_index(d, GICD_IROUTER, &index); > - reg = gic_dist_base(d) + offset + (index * 8); > - val = gic_mpidr_to_affinity(cpu_logical_map(cpu)); > + offset = convert_offset_index(d, GICD_IROUTER, &index); > + reg = gic_dist_base(d) + offset + (index * 8); > + val = gic_mpidr_to_affinity(cpu_logical_map(cpu)); > > - gic_write_irouter(val, reg); > + gic_write_irouter(val, reg); > > - /* > - * If the interrupt was enabled, enabled it again. Otherwise, > - * just wait for the distributor to have digested our changes. > - */ > - if (enabled) > - gic_unmask_irq(d); > - else > - gic_dist_wait_for_rwp(); > + /* > + * If the interrupt was enabled, enabled it again. Otherwise, > + * just wait for the distributor to have digested our changes. > + */ > + if (enabled) > + gic_unmask_irq(d); > + else > + gic_dist_wait_for_rwp(); > + > + irq_data_update_effective_affinity(d, cpumask_of(cpu)); > + > + } else { > + /* > + * no need to update when: > + * input mask is equal to the current setting > + */ > + if (cpumask_equal(irq_data_get_affinity_mask(d), mask_val)) > + return IRQ_SET_MASK_OK_NOCOPY; > + > + /* If interrupt was enabled, disable it first */ > + enabled = gic_peek_irq(d, GICD_ISENABLER); > + if (enabled) > + gic_mask_irq(d); > + > + offset = convert_offset_index(d, GICD_IROUTER, &index); > + reg = gic_dist_base(d) + offset + (index * 8); > > - irq_data_update_effective_affinity(d, cpumask_of(cpu)); > + /* GICv3 supports target is 1 or all */ > + if (cpumask_weight(mask_val) > 1) > + val = GICD_IROUTER_SPI_MODE_ANY; There is a massive difference between targeting more than one CPU and targeting all the CPUs. This breaks all existing drivers and userspace that need to manage interrupt affinity. > + else > + val = gic_mpidr_to_affinity(cpu_logical_map(cpu)); > + > + gic_write_irouter(val, reg); > + > + /* > + * If the interrupt was enabled, enabled it again. Otherwise, > + * just wait for the distributor to have digested our changes. > + */ > + if (enabled) > + gic_unmask_irq(d); > + else > + gic_dist_wait_for_rwp(); Why so much code duplication? > + } > > - return IRQ_SET_MASK_OK_DONE; > + return IRQ_SET_MASK_OK; > } > #else > #define gic_set_affinity NULL > diff --git a/include/linux/irqchip/arm-gic-v3.h > b/include/linux/irqchip/arm-gic-v3.h > index f6d092fdb93d..c24336d506a3 100644 > --- a/include/linux/irqchip/arm-gic-v3.h > +++ b/include/linux/irqchip/arm-gic-v3.h > @@ -80,6 +80,7 @@ > #define GICD_CTLR_ENABLE_SS_G0 (1U << 0) > > #define GICD_TYPER_RSS (1U << 26) > +#define GICD_TYPER_No1N (1U << 25) > #define GICD_TYPER_LPIS (1U << 17) > #define GICD_TYPER_MBIS (1U << 16) > #define GICD_TYPER_ESPI (1U << 8) > diff --git a/kernel/irq/cpuhotplug.c b/kernel/irq/cpuhotplug.c > index 02236b13b359..779512e44960 100644 > --- a/kernel/irq/cpuhotplug.c > +++ b/kernel/irq/cpuhotplug.c > @@ -87,6 +87,18 @@ static bool migrate_one_irq(struct irq_desc *desc) > return false; > } > > +#ifdef CONFIG_ARM_IRQ_TARGET_ALL No way. > + /* > + * No move required, if interrupt is 1 of N IRQ. > + * write current cpu_online_mask into affinity mask. > + */ > + if (cpumask_weight(desc->irq_common_data.affinity) > 1) { > + cpumask_copy(desc->irq_common_data.affinity, cpu_online_mask); Again, this is totally bogus. > + > + return false; > + } > +#endif > + > /* > * Complete an eventually pending irq move cleanup. If this > * interrupt was moved in hard irq context, then the vectors need > @@ -191,6 +203,16 @@ static void irq_restore_affinity_of_irq(struct > irq_desc *desc, unsigned int cpu) > struct irq_data *data = irq_desc_get_irq_data(desc); > const struct cpumask *affinity = irq_data_get_affinity_mask(data); > > +#ifdef CONFIG_ARM_IRQ_TARGET_ALL > + /* > + * No restore required, if interrupt is 1 of N IRQ. > + */ > + if (cpumask_weight(affinity) > 1) { > + cpumask_set_cpu(cpu, irq_data_get_affinity_mask(data)); > + return; > + } Same thing. > +#endif > + > if (!irqd_affinity_is_managed(data) || !desc->action || > !irq_data_get_irq_chip(data) || !cpumask_test_cpu(cpu, affinity)) > return; > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index c460e0496006..770b97e326bd 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -270,7 +270,14 @@ int irq_do_set_affinity(struct irq_data *data, > const struct cpumask *mask, > switch (ret) { > case IRQ_SET_MASK_OK: > case IRQ_SET_MASK_OK_DONE: > +#ifndef CONFIG_ARM_IRQ_TARGET_ALL > cpumask_copy(desc->irq_common_data.affinity, mask); > +#else > + if (cpumask_weight(mask) > 1) > + cpumask_copy(desc->irq_common_data.affinity, cpu_online_mask); > + else > + cpumask_copy(desc->irq_common_data.affinity, mask); And again. > +#endif > fallthrough; > case IRQ_SET_MASK_OK_NOCOPY: > irq_validate_effective_affinity(data); To sum it up: - You claim that the current interrupt distribution model is either non functional or inefficient. I expect you to demonstrate this. - You claim that this brings some kind of improvement: I expect you to describe workloads and numbers that demonstrate the improvements. 1:N distribution has repeatedly shown to have much worse behaviour that 1:1 distribution, so I really wonder what's new here. - You break existing APIs where a driver or userspace can legitimately decide to route an interrupt to 2 CPUs out of 256 if it decides to. That's not acceptable. - You pollute the core code with hacks that should never be there. If the current behaviour is a problem, please state what the problem is. - You don't factor in the requirements of the pseudo-NMI masking, meaning the IRI doesn't know about the current PMR and will not be able to efficiently dispatch interrupts. - I don't see anything that configures the participating nodes to the 1:N distribution, meaning it currently works by chance. - How does it work with CPU hotplug, where we actively move the interrupts away from a CPU going down? With this scheme, the interrupts keep being delivered to the wrong CPU, resulting in lost interrupts if they were edge triggered. As it stands, I can't imagine this code making it in the kernel. Thanks, M.
On Fri, Nov 27, 2020 at 06:11:01PM +0000, Marc Zyngier wrote: > On 2020-11-27 14:15, Hanks Chen wrote: > > Support for interrupt distribution design for SMP system solutions. > > As far as I know, we have been supporting interrupt distribution on > ARM SMP systems pretty well for the past... what... 15 years? > I'm sure Russell can dig out an ARM926 SMP system that even predates > that. > > > With this feature enabled ,the SPI interrupts would be routed to > > all the cores rather than boot core to achieve better > > load balance of interrupt handling. > > Please quantify this compared to the current distribution method. > > > That is, interrupts might be serviced simultaneously on different CPUs. > > They already can. What is new here? Or do you mean the *same* interrupt > being serviced by different CPUs *at the same time*? That'd be fun. IIRC (it's been many years since I looked at the GIC), more than one CPU is woken and if they all read the INTACK, only one of them gets the actual IRQ number, the others see a spurious interrupt. I thought we decided that's not an efficient way to handle interrupts, so a software irqbalancer is better. Has anything changed since then? I'm also concerned that in a big.LITTLE system, you may see the big CPUs taking the interrupts all the time, which is nice for energy efficiency.
On 2020-11-27 18:56, Catalin Marinas wrote: > On Fri, Nov 27, 2020 at 06:11:01PM +0000, Marc Zyngier wrote: >> On 2020-11-27 14:15, Hanks Chen wrote: >> > Support for interrupt distribution design for SMP system solutions. >> >> As far as I know, we have been supporting interrupt distribution on >> ARM SMP systems pretty well for the past... what... 15 years? >> I'm sure Russell can dig out an ARM926 SMP system that even predates >> that. >> >> > With this feature enabled ,the SPI interrupts would be routed to >> > all the cores rather than boot core to achieve better >> > load balance of interrupt handling. >> >> Please quantify this compared to the current distribution method. >> >> > That is, interrupts might be serviced simultaneously on different CPUs. >> >> They already can. What is new here? Or do you mean the *same* >> interrupt >> being serviced by different CPUs *at the same time*? That'd be fun. > > IIRC (it's been many years since I looked at the GIC), more than one > CPU > is woken and if they all read the INTACK, only one of them gets the > actual IRQ number, the others see a spurious interrupt. I thought we > decided that's not an efficient way to handle interrupts, so a software > irqbalancer is better. > > Has anything changed since then? What you describe is mostly the GICv1/v2 behaviour. GICv3 *can* be slightly better in that respect, as long as you force the synchronization from the CPU interface back to the redistributor via PHME and a DSB SY on each priority change. Which means what whatever you gain from not interrupting the other CPUs, you waste it by forcing synchronization system wide. > I'm also concerned that in a big.LITTLE system, you may see the big > CPUs > taking the interrupts all the time, which is nice for energy > efficiency. Yes, this is bound to happen. It means you cannot place interrupts on a small cluster unless you completely hotplug the big cores off, thanks to the "more than one means all" nonsense. And since these patches seem to also break hotplug, that's... fun. M.
Hi Marc, Sorry for the late reply. On Fri, 2020-11-27 at 18:11 +0000, Marc Zyngier wrote: > On 2020-11-27 14:15, Hanks Chen wrote: > > Support for interrupt distribution design for SMP system solutions. > > As far as I know, we have been supporting interrupt distribution on > ARM SMP systems pretty well for the past... what... 15 years? > I'm sure Russell can dig out an ARM926 SMP system that even predates > that. > > > With this feature enabled ,the SPI interrupts would be routed to > > all the cores rather than boot core to achieve better > > load balance of interrupt handling. > > Please quantify this compared to the current distribution method. > Got it, but I need some time to prepare for the demonstration. I'll show it soon. Btw, it is good on IO bandwidth-sensitive applications, we could also use androbench to recognize it. > > That is, interrupts might be serviced simultaneously on different CPUs. > > They already can. What is new here? Or do you mean the *same* interrupt > being serviced by different CPUs *at the same time*? That'd be fun. > It is my fault, I used the imprecise word to describe it. Not mean 'same interrupt being serviced by different CPUs'. You are right, we can use affinity API to achieve 'different CPUs' However, I focus on HW behavior. If we don't change the setting of affinity(target to boot CPU), SPI IRQs might still be serviced on different CPUs after applying target all. > > Signed-off-by: Hanks Chen <hanks.chen@mediatek.com> > > --- > > drivers/irqchip/Kconfig | 12 ++++ > > drivers/irqchip/irq-gic-v3.c | 107 +++++++++++++++++++++-------- > > include/linux/irqchip/arm-gic-v3.h | 1 + > > kernel/irq/cpuhotplug.c | 22 ++++++ > > kernel/irq/manage.c | 7 ++ > > 5 files changed, 122 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > > index c6098eee0c7c..c88ee7731e92 100644 > > --- a/drivers/irqchip/Kconfig > > +++ b/drivers/irqchip/Kconfig > > @@ -597,4 +597,16 @@ config MST_IRQ > > help > > Support MStar Interrupt Controller. > > > > +config ARM_IRQ_TARGET_ALL > > + bool "Distribute interrupts across processors on SMP system" > > + depends on SMP && ARM_GIC_V3 > > + help > > + Support for interrupt distribution design for > > + SMP system solutions. With this feature enabled ,the > > + SPI interrupts would be routed to all the cores rather > > + than boot cpu to achieve better load balance of interrupt > > + handling > > + > > + If you don't know what to do here, say N. > > There is no way we start introducing conditional compilation for > architectural features. Either this works at all times, or it doesn't > exist. > I have no idea how to solve it. Indeed, it is not good to enable the feature on every ARM-based platform. maybe I could use device tree to determine whether or not the feature should be enabled. e.g. gic: interrupt-controller { compatible = "arm,gic-v3"; ... irq-target-all-enable = <1>; }; Would this be better? > > + > > endmenu > > diff --git a/drivers/irqchip/irq-gic-v3.c > > b/drivers/irqchip/irq-gic-v3.c > > index 16fecc0febe8..62a878ce4681 100644 > > --- a/drivers/irqchip/irq-gic-v3.c > > +++ b/drivers/irqchip/irq-gic-v3.c > > @@ -381,6 +381,12 @@ static inline bool gic_supports_nmi(void) > > static_branch_likely(&supports_pseudo_nmis); > > } > > > > +static inline bool gic_supports_1n(void) > > +{ > > + return (IS_ENABLED(CONFIG_ARM_IRQ_TARGET_ALL) && > > + ~(readl_relaxed(gic_data.dist_base + GICD_TYPER) & > > GICD_TYPER_No1N)); > > +} > > + > > static int gic_irq_set_irqchip_state(struct irq_data *d, > > enum irqchip_irq_state which, bool val) > > { > > @@ -716,6 +722,7 @@ static void __init gic_dist_init(void) > > { > > unsigned int i; > > u64 affinity; > > + > > Spurious whitespace. Got it, I'll fix it. > > > void __iomem *base = gic_data.dist_base; > > u32 val; > > > > @@ -759,16 +766,27 @@ static void __init gic_dist_init(void) > > /* Enable distributor with ARE, Group1 */ > > writel_relaxed(val, base + GICD_CTLR); > > > > - /* > > - * Set all global interrupts to the boot CPU only. ARE must be > > - * enabled. > > - */ > > - affinity = > > gic_mpidr_to_affinity(cpu_logical_map(smp_processor_id())); > > - for (i = 32; i < GIC_LINE_NR; i++) > > - gic_write_irouter(affinity, base + GICD_IROUTER + i * 8); > > + if (!gic_supports_1n()) { > > + /* > > + * Set all global interrupts to the boot CPU only. ARE must be > > + * enabled. > > + */ > > + affinity = > > gic_mpidr_to_affinity(cpu_logical_map(smp_processor_id())); > > + for (i = 32; i < GIC_LINE_NR; i++) > > + gic_write_irouter(affinity, base + GICD_IROUTER + i * 8); > > > > - for (i = 0; i < GIC_ESPI_NR; i++) > > - gic_write_irouter(affinity, base + GICD_IROUTERnE + i * 8); > > + for (i = 0; i < GIC_ESPI_NR; i++) > > + gic_write_irouter(affinity, base + GICD_IROUTERnE + i * 8); > > + } else { > > + /* default set target all for all SPI */ > > + for (i = 32; i < GIC_LINE_NR; i++) > > + gic_write_irouter(GICD_IROUTER_SPI_MODE_ANY, > > + base + GICD_IROUTER + i * 8); > > + > > + for (i = 0; i < GIC_ESPI_NR; i++) > > + gic_write_irouter(GICD_IROUTER_SPI_MODE_ANY, > > + base + GICD_IROUTERnE + i * 8); > > Why should we decide to distribute interrupts to all CPUs by default > when > we don't even know where to route them? > It makes sense to dispatch SPI interrupts defaultly by GICD when we transfer the control to GIC HW. It's more real time to track the IRQ balance by HW controller and the policy is based on CPU state or low power purpose. btw, we also want to introduce the interrupt class to our big-little system.It is an implementation-defined feature that the GIC600 provides for the 1 of N SPIs. > > + } > > } > > > > static int gic_iterate_rdists(int (*fn)(struct redist_region *, void > > __iomem *)) > > @@ -1191,29 +1209,64 @@ static int gic_set_affinity(struct irq_data > > *d, const struct cpumask *mask_val, > > if (gic_irq_in_rdist(d)) > > return -EINVAL; > > > > - /* If interrupt was enabled, disable it first */ > > - enabled = gic_peek_irq(d, GICD_ISENABLER); > > - if (enabled) > > - gic_mask_irq(d); > > + if (!gic_supports_1n()) { > > + /* If interrupt was enabled, disable it first */ > > + enabled = gic_peek_irq(d, GICD_ISENABLER); > > + if (enabled) > > + gic_mask_irq(d); > > > > - offset = convert_offset_index(d, GICD_IROUTER, &index); > > - reg = gic_dist_base(d) + offset + (index * 8); > > - val = gic_mpidr_to_affinity(cpu_logical_map(cpu)); > > + offset = convert_offset_index(d, GICD_IROUTER, &index); > > + reg = gic_dist_base(d) + offset + (index * 8); > > + val = gic_mpidr_to_affinity(cpu_logical_map(cpu)); > > > > - gic_write_irouter(val, reg); > > + gic_write_irouter(val, reg); > > > > - /* > > - * If the interrupt was enabled, enabled it again. Otherwise, > > - * just wait for the distributor to have digested our changes. > > - */ > > - if (enabled) > > - gic_unmask_irq(d); > > - else > > - gic_dist_wait_for_rwp(); > > + /* > > + * If the interrupt was enabled, enabled it again. Otherwise, > > + * just wait for the distributor to have digested our changes. > > + */ > > + if (enabled) > > + gic_unmask_irq(d); > > + else > > + gic_dist_wait_for_rwp(); > > + > > + irq_data_update_effective_affinity(d, cpumask_of(cpu)); > > + > > + } else { > > + /* > > + * no need to update when: > > + * input mask is equal to the current setting > > + */ > > + if (cpumask_equal(irq_data_get_affinity_mask(d), mask_val)) > > + return IRQ_SET_MASK_OK_NOCOPY; > > + > > + /* If interrupt was enabled, disable it first */ > > + enabled = gic_peek_irq(d, GICD_ISENABLER); > > + if (enabled) > > + gic_mask_irq(d); > > + > > + offset = convert_offset_index(d, GICD_IROUTER, &index); > > + reg = gic_dist_base(d) + offset + (index * 8); > > > > - irq_data_update_effective_affinity(d, cpumask_of(cpu)); > > + /* GICv3 supports target is 1 or all */ > > + if (cpumask_weight(mask_val) > 1) > > + val = GICD_IROUTER_SPI_MODE_ANY; > > There is a massive difference between targeting more than one CPU > and targeting all the CPUs. This breaks all existing drivers and > userspace that need to manage interrupt affinity. > Yes, it is not a good implementation that cause the user-space confused. Should I modify the code as follows? If (bitmap_equal(mask_val, online_mask, nr_cpumask_bits)) val = GICD_IROUTER_SPI_MODE_ANY; redefine the coverage > > + else > > + val = gic_mpidr_to_affinity(cpu_logical_map(cpu)); > > + > > + gic_write_irouter(val, reg); > > + > > + /* > > + * If the interrupt was enabled, enabled it again. Otherwise, > > + * just wait for the distributor to have digested our changes. > > + */ > > + if (enabled) > > + gic_unmask_irq(d); > > + else > > + gic_dist_wait_for_rwp(); > > Why so much code duplication? Got it, I'll remove the duplicated code. > > > + } > > > > - return IRQ_SET_MASK_OK_DONE; > > + return IRQ_SET_MASK_OK; > > } > > #else > > #define gic_set_affinity NULL > > diff --git a/include/linux/irqchip/arm-gic-v3.h > > b/include/linux/irqchip/arm-gic-v3.h > > index f6d092fdb93d..c24336d506a3 100644 > > --- a/include/linux/irqchip/arm-gic-v3.h > > +++ b/include/linux/irqchip/arm-gic-v3.h > > @@ -80,6 +80,7 @@ > > #define GICD_CTLR_ENABLE_SS_G0 (1U << 0) > > > > #define GICD_TYPER_RSS (1U << 26) > > +#define GICD_TYPER_No1N (1U << 25) > > #define GICD_TYPER_LPIS (1U << 17) > > #define GICD_TYPER_MBIS (1U << 16) > > #define GICD_TYPER_ESPI (1U << 8) > > diff --git a/kernel/irq/cpuhotplug.c b/kernel/irq/cpuhotplug.c > > index 02236b13b359..779512e44960 100644 > > --- a/kernel/irq/cpuhotplug.c > > +++ b/kernel/irq/cpuhotplug.c > > @@ -87,6 +87,18 @@ static bool migrate_one_irq(struct irq_desc *desc) > > return false; > > } > > > > +#ifdef CONFIG_ARM_IRQ_TARGET_ALL > > No way. > > > + /* > > + * No move required, if interrupt is 1 of N IRQ. > > + * write current cpu_online_mask into affinity mask. > > + */ > > + if (cpumask_weight(desc->irq_common_data.affinity) > 1) { > > + cpumask_copy(desc->irq_common_data.affinity, cpu_online_mask); > > Again, this is totally bogus. > If I add the target all flag into irq_common_data stucture to distinguish it. Would this be better? remove #ifdef CONFIG_ARM_IRQ_TARGET_ALL and if (desc->irq_common_data.irq_target_all) cpumask_copy(desc->irq_common_data.affinity, cpu_online_mask); > > + > > + return false; > > + } > > +#endif > > + > > /* > > * Complete an eventually pending irq move cleanup. If this > > * interrupt was moved in hard irq context, then the vectors need > > @@ -191,6 +203,16 @@ static void irq_restore_affinity_of_irq(struct > > irq_desc *desc, unsigned int cpu) > > struct irq_data *data = irq_desc_get_irq_data(desc); > > const struct cpumask *affinity = irq_data_get_affinity_mask(data); > > > > +#ifdef CONFIG_ARM_IRQ_TARGET_ALL > > + /* > > + * No restore required, if interrupt is 1 of N IRQ. > > + */ > > + if (cpumask_weight(affinity) > 1) { > > + cpumask_set_cpu(cpu, irq_data_get_affinity_mask(data)); > > + return; > > + } > > Same thing. > as above remove #ifdef CONFIG_ARM_IRQ_TARGET_ALL and if (desc->irq_common_data.irq_target_all) { cpumask_set_cpu(cpu, irq_data_get_affinity_mask(data)); return; } > > +#endif > > + > > if (!irqd_affinity_is_managed(data) || !desc->action || > > !irq_data_get_irq_chip(data) || !cpumask_test_cpu(cpu, affinity)) > > return; > > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > > index c460e0496006..770b97e326bd 100644 > > --- a/kernel/irq/manage.c > > +++ b/kernel/irq/manage.c > > @@ -270,7 +270,14 @@ int irq_do_set_affinity(struct irq_data *data, > > const struct cpumask *mask, > > switch (ret) { > > case IRQ_SET_MASK_OK: > > case IRQ_SET_MASK_OK_DONE: > > +#ifndef CONFIG_ARM_IRQ_TARGET_ALL > > cpumask_copy(desc->irq_common_data.affinity, mask); > > +#else > > + if (cpumask_weight(mask) > 1) > > + cpumask_copy(desc->irq_common_data.affinity, cpu_online_mask); > > + else > > + cpumask_copy(desc->irq_common_data.affinity, mask); > > And again. > as above if (!desc->irq_common_data.irq_target_all) cpumask_copy(desc->irq_common_data.affinity, mask); else cpumask_copy(desc->irq_common_data.affinity, cpu_online_mask); > > +#endif > > fallthrough; > > case IRQ_SET_MASK_OK_NOCOPY: > > irq_validate_effective_affinity(data); > > To sum it up: > > - You claim that the current interrupt distribution model is either non > functional or inefficient. I expect you to demonstrate this. > > - You claim that this brings some kind of improvement: I expect you to > describe workloads and numbers that demonstrate the improvements. > 1:N distribution has repeatedly shown to have much worse behaviour > that 1:1 distribution, so I really wonder what's new here. > I'll show the demonstration to answer first two questions. > - You break existing APIs where a driver or userspace can legitimately > decide to route an interrupt to 2 CPUs out of 256 if it decides to. > That's not acceptable. > I redefine the coverage as above > - You pollute the core code with hacks that should never be there. If > the > current behaviour is a problem, please state what the problem is. > We found the RCU warn when IRQs target to a offline CPU without I bit masked in CPU hot-plug flow I'll reproduce the issue again and show the log analysis for it. > - You don't factor in the requirements of the pseudo-NMI masking, > meaning > the IRI doesn't know about the current PMR and will not be able to > efficiently dispatch interrupts. > Yes. mask interrupts using PMR becomes very expensive. (require a dsb) We don't use the pseduo-NMI in our platform. Thank you for pointing that out, I need some time to mull it over. > - I don't see anything that configures the participating nodes to the > 1:N > distribution, meaning it currently works by chance. > 1 of N SPI is dispatched by GICD with round robin policy by default. However, GIC support the interface to control the policy of dispatching SPI interrupt. In MTK platform, GICD could dispatch SPI interrupts to suitable target CPU by CPU state or low power purpose. > - How does it work with CPU hotplug, where we actively move the > interrupts > away from a CPU going down? With this scheme, the interrupts keep > being > delivered to the wrong CPU, resulting in lost interrupts if they were > edge triggered. > Hot-plug off CPU would assert the GICR_WAKER.ProcessorSleep and GICR_CTLR.DPG in trusted firmware cortex-a (TF-a) 1) When ProcessorSleep is 1 All interrupts that arrive at the re-distributor are not communicated to the CPU interface 2) When DPG is 1 Any interrupts that have not reached a core at the time of the change are recalled and reprioritized by the GIC hot-plug off sequence. (PSCI in TF-a) 1. Mask interrupts on the core 2. Clear the CPU interface enables 3. GICR_WAKER.ProcessorSleep = 1 4. GICR_CTLR.DPG = 1 (we need to add it when 1 of N SPIs feature is enabled) According to the above, I think it would keep the edge-triggered interrupt in GIC. > As it stands, I can't imagine this code making it in the kernel. > > Thanks, > > M. In MTK platform, We change the dispatch policy of 1:N SPIs by interface of interrupt routing infrastructure and it would improve our system performance. GICR_CTLR.DPG, cpu_active and interrupt class could also change the policy of 1:N SPIs. So we want to introduce the interface for 1 of N SPIs to provide more choices. But I need to carefully consider the various scenario on all arm64 system and don't break the established API. I need some time to mull it over. Thank you for your review Hanks
Hanks, On Tue, Dec 01 2020 at 21:54, Hanks Chen wrote: > On Fri, 2020-11-27 at 18:11 +0000, Marc Zyngier wrote: >> On 2020-11-27 14:15, Hanks Chen wrote: >> > + /* >> > + * No move required, if interrupt is 1 of N IRQ. >> > + * write current cpu_online_mask into affinity mask. >> > + */ >> > + if (cpumask_weight(desc->irq_common_data.affinity) > 1) { >> > + cpumask_copy(desc->irq_common_data.affinity, cpu_online_mask); >> >> Again, this is totally bogus. >> > > If I add the target all flag into irq_common_data stucture to > distinguish it. Would this be better? > > remove #ifdef CONFIG_ARM_IRQ_TARGET_ALL > > and > > if (desc->irq_common_data.irq_target_all) > cpumask_copy(desc->irq_common_data.affinity, cpu_online_mask); No. We are not adding random members to irq_common_data just to support this. If at all this is a flag of the interrupt chip. Also this copy is wrong to begin with. The affinity mask is only updated on cpu hot-unplug when the outgoing CPU is the last online CPU in the mask. Then we break the user/kernel supplied affinity mask and we only do that when the interrupt is not affinity managed. We do not remove offline CPUs from the affinity mask. The affinity code of the irq chip has to ensure that only online CPUs can be targeted. That's what ends up in effective_affinity. >> > +#ifdef CONFIG_ARM_IRQ_TARGET_ALL >> > + /* >> > + * No restore required, if interrupt is 1 of N IRQ. >> > + */ >> > + if (cpumask_weight(affinity) > 1) { >> > + cpumask_set_cpu(cpu, irq_data_get_affinity_mask(data)); >> > + return; >> > + } Heck no. This breaks managed interrupts and some more. Fiddling with the irq affinity mask is wrong to begin with. Don't touch it ever. >> > --- a/kernel/irq/manage.c >> > +++ b/kernel/irq/manage.c >> > @@ -270,7 +270,14 @@ int irq_do_set_affinity(struct irq_data *data, >> > const struct cpumask *mask, >> > switch (ret) { >> > case IRQ_SET_MASK_OK: >> > case IRQ_SET_MASK_OK_DONE: >> > +#ifndef CONFIG_ARM_IRQ_TARGET_ALL >> > cpumask_copy(desc->irq_common_data.affinity, mask); >> > +#else >> > + if (cpumask_weight(mask) > 1) >> > + cpumask_copy(desc->irq_common_data.affinity, cpu_online_mask); >> > + else >> > + cpumask_copy(desc->irq_common_data.affinity, mask); Again, no. Touching the affinity mask is a NONO. The affinity mask is handed in from either the kernel or from user space. This code has no business to override that. The only case where the kernel touches it is on CPU hot-unplug when the last cpu in the affinity mask goes offline and if the interrupt is not affinity managed. >> - You pollute the core code with hacks that should never be there. If >> the >> current behaviour is a problem, please state what the problem is. >> > We found the RCU warn when IRQs target to a offline CPU without I bit > masked in CPU hot-plug flow > I'll reproduce the issue again and show the log analysis for it. That has absolutely nothing to do with any of the functions where you fiddle with the affinity mask. Thanks, tglx
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index c6098eee0c7c..c88ee7731e92 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -597,4 +597,16 @@ config MST_IRQ help Support MStar Interrupt Controller. +config ARM_IRQ_TARGET_ALL + bool "Distribute interrupts across processors on SMP system" + depends on SMP && ARM_GIC_V3 + help + Support for interrupt distribution design for + SMP system solutions. With this feature enabled ,the + SPI interrupts would be routed to all the cores rather + than boot cpu to achieve better load balance of interrupt + handling + + If you don't know what to do here, say N. + endmenu diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 16fecc0febe8..62a878ce4681 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -381,6 +381,12 @@ static inline bool gic_supports_nmi(void) static_branch_likely(&supports_pseudo_nmis); } +static inline bool gic_supports_1n(void) +{ + return (IS_ENABLED(CONFIG_ARM_IRQ_TARGET_ALL) && + ~(readl_relaxed(gic_data.dist_base + GICD_TYPER) & GICD_TYPER_No1N)); +} + static int gic_irq_set_irqchip_state(struct irq_data *d, enum irqchip_irq_state which, bool val) { @@ -716,6 +722,7 @@ static void __init gic_dist_init(void) { unsigned int i; u64 affinity; + void __iomem *base = gic_data.dist_base; u32 val; @@ -759,16 +766,27 @@ static void __init gic_dist_init(void) /* Enable distributor with ARE, Group1 */ writel_relaxed(val, base + GICD_CTLR); - /* - * Set all global interrupts to the boot CPU only. ARE must be - * enabled. - */ - affinity = gic_mpidr_to_affinity(cpu_logical_map(smp_processor_id())); - for (i = 32; i < GIC_LINE_NR; i++) - gic_write_irouter(affinity, base + GICD_IROUTER + i * 8); + if (!gic_supports_1n()) { + /* + * Set all global interrupts to the boot CPU only. ARE must be + * enabled. + */ + affinity = gic_mpidr_to_affinity(cpu_logical_map(smp_processor_id())); + for (i = 32; i < GIC_LINE_NR; i++) + gic_write_irouter(affinity, base + GICD_IROUTER + i * 8); - for (i = 0; i < GIC_ESPI_NR; i++) - gic_write_irouter(affinity, base + GICD_IROUTERnE + i * 8); + for (i = 0; i < GIC_ESPI_NR; i++) + gic_write_irouter(affinity, base + GICD_IROUTERnE + i * 8); + } else { + /* default set target all for all SPI */ + for (i = 32; i < GIC_LINE_NR; i++) + gic_write_irouter(GICD_IROUTER_SPI_MODE_ANY, + base + GICD_IROUTER + i * 8); + + for (i = 0; i < GIC_ESPI_NR; i++) + gic_write_irouter(GICD_IROUTER_SPI_MODE_ANY, + base + GICD_IROUTERnE + i * 8); + } } static int gic_iterate_rdists(int (*fn)(struct redist_region *, void __iomem *)) @@ -1191,29 +1209,64 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, if (gic_irq_in_rdist(d)) return -EINVAL; - /* If interrupt was enabled, disable it first */ - enabled = gic_peek_irq(d, GICD_ISENABLER); - if (enabled) - gic_mask_irq(d); + if (!gic_supports_1n()) { + /* If interrupt was enabled, disable it first */ + enabled = gic_peek_irq(d, GICD_ISENABLER); + if (enabled) + gic_mask_irq(d); - offset = convert_offset_index(d, GICD_IROUTER, &index); - reg = gic_dist_base(d) + offset + (index * 8); - val = gic_mpidr_to_affinity(cpu_logical_map(cpu)); + offset = convert_offset_index(d, GICD_IROUTER, &index); + reg = gic_dist_base(d) + offset + (index * 8); + val = gic_mpidr_to_affinity(cpu_logical_map(cpu)); - gic_write_irouter(val, reg); + gic_write_irouter(val, reg); - /* - * If the interrupt was enabled, enabled it again. Otherwise, - * just wait for the distributor to have digested our changes. - */ - if (enabled) - gic_unmask_irq(d); - else - gic_dist_wait_for_rwp(); + /* + * If the interrupt was enabled, enabled it again. Otherwise, + * just wait for the distributor to have digested our changes. + */ + if (enabled) + gic_unmask_irq(d); + else + gic_dist_wait_for_rwp(); + + irq_data_update_effective_affinity(d, cpumask_of(cpu)); + + } else { + /* + * no need to update when: + * input mask is equal to the current setting + */ + if (cpumask_equal(irq_data_get_affinity_mask(d), mask_val)) + return IRQ_SET_MASK_OK_NOCOPY; + + /* If interrupt was enabled, disable it first */ + enabled = gic_peek_irq(d, GICD_ISENABLER); + if (enabled) + gic_mask_irq(d); + + offset = convert_offset_index(d, GICD_IROUTER, &index); + reg = gic_dist_base(d) + offset + (index * 8); - irq_data_update_effective_affinity(d, cpumask_of(cpu)); + /* GICv3 supports target is 1 or all */ + if (cpumask_weight(mask_val) > 1) + val = GICD_IROUTER_SPI_MODE_ANY; + else + val = gic_mpidr_to_affinity(cpu_logical_map(cpu)); + + gic_write_irouter(val, reg); + + /* + * If the interrupt was enabled, enabled it again. Otherwise, + * just wait for the distributor to have digested our changes. + */ + if (enabled) + gic_unmask_irq(d); + else + gic_dist_wait_for_rwp(); + } - return IRQ_SET_MASK_OK_DONE; + return IRQ_SET_MASK_OK; } #else #define gic_set_affinity NULL diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h index f6d092fdb93d..c24336d506a3 100644 --- a/include/linux/irqchip/arm-gic-v3.h +++ b/include/linux/irqchip/arm-gic-v3.h @@ -80,6 +80,7 @@ #define GICD_CTLR_ENABLE_SS_G0 (1U << 0) #define GICD_TYPER_RSS (1U << 26) +#define GICD_TYPER_No1N (1U << 25) #define GICD_TYPER_LPIS (1U << 17) #define GICD_TYPER_MBIS (1U << 16) #define GICD_TYPER_ESPI (1U << 8) diff --git a/kernel/irq/cpuhotplug.c b/kernel/irq/cpuhotplug.c index 02236b13b359..779512e44960 100644 --- a/kernel/irq/cpuhotplug.c +++ b/kernel/irq/cpuhotplug.c @@ -87,6 +87,18 @@ static bool migrate_one_irq(struct irq_desc *desc) return false; } +#ifdef CONFIG_ARM_IRQ_TARGET_ALL + /* + * No move required, if interrupt is 1 of N IRQ. + * write current cpu_online_mask into affinity mask. + */ + if (cpumask_weight(desc->irq_common_data.affinity) > 1) { + cpumask_copy(desc->irq_common_data.affinity, cpu_online_mask); + + return false; + } +#endif + /* * Complete an eventually pending irq move cleanup. If this * interrupt was moved in hard irq context, then the vectors need @@ -191,6 +203,16 @@ static void irq_restore_affinity_of_irq(struct irq_desc *desc, unsigned int cpu) struct irq_data *data = irq_desc_get_irq_data(desc); const struct cpumask *affinity = irq_data_get_affinity_mask(data); +#ifdef CONFIG_ARM_IRQ_TARGET_ALL + /* + * No restore required, if interrupt is 1 of N IRQ. + */ + if (cpumask_weight(affinity) > 1) { + cpumask_set_cpu(cpu, irq_data_get_affinity_mask(data)); + return; + } +#endif + if (!irqd_affinity_is_managed(data) || !desc->action || !irq_data_get_irq_chip(data) || !cpumask_test_cpu(cpu, affinity)) return; diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index c460e0496006..770b97e326bd 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -270,7 +270,14 @@ int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask, switch (ret) { case IRQ_SET_MASK_OK: case IRQ_SET_MASK_OK_DONE: +#ifndef CONFIG_ARM_IRQ_TARGET_ALL cpumask_copy(desc->irq_common_data.affinity, mask); +#else + if (cpumask_weight(mask) > 1) + cpumask_copy(desc->irq_common_data.affinity, cpu_online_mask); + else + cpumask_copy(desc->irq_common_data.affinity, mask); +#endif fallthrough; case IRQ_SET_MASK_OK_NOCOPY: irq_validate_effective_affinity(data);
Support for interrupt distribution design for SMP system solutions. With this feature enabled ,the SPI interrupts would be routed to all the cores rather than boot core to achieve better load balance of interrupt handling. That is, interrupts might be serviced simultaneously on different CPUs. Signed-off-by: Hanks Chen <hanks.chen@mediatek.com> --- drivers/irqchip/Kconfig | 12 ++++ drivers/irqchip/irq-gic-v3.c | 107 +++++++++++++++++++++-------- include/linux/irqchip/arm-gic-v3.h | 1 + kernel/irq/cpuhotplug.c | 22 ++++++ kernel/irq/manage.c | 7 ++ 5 files changed, 122 insertions(+), 27 deletions(-)