Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
Marc, Marc Zyngier writes: > On Fri, 24 Jul 2020 21:03:50 +0100, > Thomas Gleixner wrote: >> Right. My brain tricked me to believe that we made activation mandatory, >> but that's not. >> >> I have some ideas for a trivial generic way to solve this without >> undoing the commit in question and without going through all the irq >> chip drivers. So far everything I came up with is butt ugly. Maybe Marc >> has some brilliant idea. > > Not really. We have contradicting behaviours here, where some > interrupts want to see the set_affinity early (the above case), and > some cannot handle that (x86 vectors and the GICv3 ITS). We could key > it on the presence of an activate callback, but it feels fragile. Yes, I thought about that briefly, but yeah, it's fragile and bound to break in weird ways. That said, we should make activate mandatory and actually set up the affinity during activation correctly. Thanks, tglx
Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
Hi Thomas,
On Fri, 24 Jul 2020 21:44:41 +0100,
Thomas Gleixner wrote:
>
> John,
>
> Thomas Gleixner writes:
> > I have some ideas for a trivial generic way to solve this without
> > undoing the commit in question and without going through all the irq
> > chip drivers. So far everything I came up with is butt ugly. Maybe Marc
> > has some brilliant idea.
> >
> > Sorry for the wreckage and thanks for the excellent problem
> > description. I'll come back to you in the next days.
>
> couldn't give up :)
>
> So after staring in too many drivers, I resorted to make this mode
> opt-in and mark the interrupts accordingly for the two drivers which are
> known to want this. Not that I love it, but it's the least dangerous
> option. Completely untested patch below.
>
> Thanks,
>
> tglx
> ---
> arch/x86/kernel/apic/vector.c|4
> drivers/irqchip/irq-gic-v3-its.c |5 -
> include/linux/irq.h | 13 +
> kernel/irq/manage.c |6 +-
> 4 files changed, 26 insertions(+), 2 deletions(-)
>
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -560,6 +560,10 @@ static int x86_vector_alloc_irqs(struct
>* as that can corrupt the affinity move state.
>*/
> irqd_set_handle_enforce_irqctx(irqd);
> +
> + /* Don't invoke affinity setter on deactivated interrupts */
> + irqd_set_affinity_on_activate(irqd);
> +
> /*
>* Legacy vectors are already assigned when the IOAPIC
>* takes them over. They stay on the same vector. This is
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -3523,6 +3523,7 @@ static int its_irq_domain_alloc(struct i
> msi_alloc_info_t *info = args;
> struct its_device *its_dev = info->scratchpad[0].ptr;
> struct its_node *its = its_dev->its;
> + struct irq_data *irqd;
> irq_hw_number_t hwirq;
> int err;
> int i;
> @@ -3542,7 +3543,9 @@ static int its_irq_domain_alloc(struct i
>
> irq_domain_set_hwirq_and_chip(domain, virq + i,
> hwirq + i, &its_irq_chip,
> its_dev);
> - irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(virq +
> i)));
> + irqd = irq_get_irq_data(virq + i);
> + irqd_set_single_target(irqd);
> + irqd_set_affinity_on_activate(irqd);
> pr_debug("ID:%d pID:%d vID:%d\n",
>(int)(hwirq + i - its_dev->event_map.lpi_base),
>(int)(hwirq + i), virq + i);
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -213,6 +213,8 @@ struct irq_data {
> * required
> * IRQD_HANDLE_ENFORCE_IRQCTX- Enforce that handle_irq_*() is only
> invoked
> * from actual interrupt context.
> + * IRQD_AFFINITY_ON_ACTIVATE - Affinity is set on activation. Don't call
> + * irq_chip::irq_set_affinity() when deactivated.
> */
> enum {
> IRQD_TRIGGER_MASK = 0xf,
> @@ -237,6 +239,7 @@ enum {
> IRQD_CAN_RESERVE= (1 << 26),
> IRQD_MSI_NOMASK_QUIRK = (1 << 27),
> IRQD_HANDLE_ENFORCE_IRQCTX = (1 << 28),
> + IRQD_AFFINITY_ON_ACTIVATE = (1 << 29),
> };
>
> #define __irqd_to_state(d) ACCESS_PRIVATE((d)->common, state_use_accessors)
> @@ -421,6 +424,16 @@ static inline bool irqd_msi_nomask_quirk
> return __irqd_to_state(d) & IRQD_MSI_NOMASK_QUIRK;
> }
>
> +static inline void irqd_set_affinity_on_activate(struct irq_data *d)
> +{
> + __irqd_to_state(d) |= IRQD_AFFINITY_ON_ACTIVATE;
> +}
> +
> +static inline bool irqd_affinity_on_activate(struct irq_data *d)
> +{
> + return __irqd_to_state(d) & IRQD_AFFINITY_ON_ACTIVATE;
> +}
> +
> #undef __irqd_to_state
>
> static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -320,12 +320,16 @@ static bool irq_set_affinity_deactivated
> struct irq_desc *desc = irq_data_to_desc(data);
>
> /*
> + * Handle irq chips which can handle affinity only in activated
> + * state correctly
> + *
>* If the interrupt is not yet activated, just store the affinity
>* mask and do not call the chip driver at all. On activation the
>* driver has to make sure anyway that the interrupt is in a
>* useable state so startup works.
>*/
> - if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) || irqd_is_activated(data))
> + if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) ||
> + irqd_is_activated(data) || !irqd_affinity_on_activate(data))
> return false;
>
> cpumask_copy(desc->irq_common_data.affinity, mask);
>
I have given this a go on two systems: one with a GICv2 that has its
PMUs wired in a si
Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
Hi both, On Fri, 24 Jul 2020 21:03:50 +0100, Thomas Gleixner wrote: > > John, > > John Keeping writes: > > On Fri, 17 Jul 2020 18:00:02 +0200 > > Thomas Gleixner wrote: > > It seems that this patch breaks perf events on RK3288 because the PMU > > interrupts that should be per-cpu are now all on CPU0 so no events are > > collected from CPUs 1-3 and those interrupts are killed as spurious > > after a few seconds. SPI-backed PMUs. Urgh... > > > > I'm seeing this on 4.19.134 and 5.4.53 but as far as I can tell the > > relevant code hasn't changed through to next-20200723. Reverting the > > backport of this change fixes the problem. > > Bah. > > > It looks like what happens is that because the interrupts are not > > per-CPU in the hardware, armpmu_request_irq() calls irq_force_affinity() > > while the interrupt is deactivated and then request_irq() with > > IRQF_PERCPU | IRQF_NOBALANCING. > > > > Now when irq_startup() runs with IRQ_STARTUP_NORMAL, it calls > > irq_setup_affinity() which returns early because IRQF_PERCPU and > > IRQF_NOBALANCING are set, leaving the interrupt on its original CPU. > > Right. My brain tricked me to believe that we made activation mandatory, > but that's not. > > I have some ideas for a trivial generic way to solve this without > undoing the commit in question and without going through all the irq > chip drivers. So far everything I came up with is butt ugly. Maybe Marc > has some brilliant idea. Not really. We have contradicting behaviours here, where some interrupts want to see the set_affinity early (the above case), and some cannot handle that (x86 vectors and the GICv3 ITS). We could key it on the presence of an activate callback, but it feels fragile. I'll follow up on your patch in the next email, which seems like a sensible approach. M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
John,
Thomas Gleixner writes:
> I have some ideas for a trivial generic way to solve this without
> undoing the commit in question and without going through all the irq
> chip drivers. So far everything I came up with is butt ugly. Maybe Marc
> has some brilliant idea.
>
> Sorry for the wreckage and thanks for the excellent problem
> description. I'll come back to you in the next days.
couldn't give up :)
So after staring in too many drivers, I resorted to make this mode
opt-in and mark the interrupts accordingly for the two drivers which are
known to want this. Not that I love it, but it's the least dangerous
option. Completely untested patch below.
Thanks,
tglx
---
arch/x86/kernel/apic/vector.c|4
drivers/irqchip/irq-gic-v3-its.c |5 -
include/linux/irq.h | 13 +
kernel/irq/manage.c |6 +-
4 files changed, 26 insertions(+), 2 deletions(-)
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -560,6 +560,10 @@ static int x86_vector_alloc_irqs(struct
* as that can corrupt the affinity move state.
*/
irqd_set_handle_enforce_irqctx(irqd);
+
+ /* Don't invoke affinity setter on deactivated interrupts */
+ irqd_set_affinity_on_activate(irqd);
+
/*
* Legacy vectors are already assigned when the IOAPIC
* takes them over. They stay on the same vector. This is
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3523,6 +3523,7 @@ static int its_irq_domain_alloc(struct i
msi_alloc_info_t *info = args;
struct its_device *its_dev = info->scratchpad[0].ptr;
struct its_node *its = its_dev->its;
+ struct irq_data *irqd;
irq_hw_number_t hwirq;
int err;
int i;
@@ -3542,7 +3543,9 @@ static int its_irq_domain_alloc(struct i
irq_domain_set_hwirq_and_chip(domain, virq + i,
hwirq + i, &its_irq_chip,
its_dev);
- irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(virq +
i)));
+ irqd = irq_get_irq_data(virq + i);
+ irqd_set_single_target(irqd);
+ irqd_set_affinity_on_activate(irqd);
pr_debug("ID:%d pID:%d vID:%d\n",
(int)(hwirq + i - its_dev->event_map.lpi_base),
(int)(hwirq + i), virq + i);
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -213,6 +213,8 @@ struct irq_data {
* required
* IRQD_HANDLE_ENFORCE_IRQCTX - Enforce that handle_irq_*() is only invoked
* from actual interrupt context.
+ * IRQD_AFFINITY_ON_ACTIVATE - Affinity is set on activation. Don't call
+ * irq_chip::irq_set_affinity() when deactivated.
*/
enum {
IRQD_TRIGGER_MASK = 0xf,
@@ -237,6 +239,7 @@ enum {
IRQD_CAN_RESERVE= (1 << 26),
IRQD_MSI_NOMASK_QUIRK = (1 << 27),
IRQD_HANDLE_ENFORCE_IRQCTX = (1 << 28),
+ IRQD_AFFINITY_ON_ACTIVATE = (1 << 29),
};
#define __irqd_to_state(d) ACCESS_PRIVATE((d)->common, state_use_accessors)
@@ -421,6 +424,16 @@ static inline bool irqd_msi_nomask_quirk
return __irqd_to_state(d) & IRQD_MSI_NOMASK_QUIRK;
}
+static inline void irqd_set_affinity_on_activate(struct irq_data *d)
+{
+ __irqd_to_state(d) |= IRQD_AFFINITY_ON_ACTIVATE;
+}
+
+static inline bool irqd_affinity_on_activate(struct irq_data *d)
+{
+ return __irqd_to_state(d) & IRQD_AFFINITY_ON_ACTIVATE;
+}
+
#undef __irqd_to_state
static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -320,12 +320,16 @@ static bool irq_set_affinity_deactivated
struct irq_desc *desc = irq_data_to_desc(data);
/*
+* Handle irq chips which can handle affinity only in activated
+* state correctly
+*
* If the interrupt is not yet activated, just store the affinity
* mask and do not call the chip driver at all. On activation the
* driver has to make sure anyway that the interrupt is in a
* useable state so startup works.
*/
- if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) || irqd_is_activated(data))
+ if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) ||
+ irqd_is_activated(data) || !irqd_affinity_on_activate(data))
return false;
cpumask_copy(desc->irq_common_data.affinity, mask);
Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
John, John Keeping writes: > On Fri, 17 Jul 2020 18:00:02 +0200 > Thomas Gleixner wrote: > It seems that this patch breaks perf events on RK3288 because the PMU > interrupts that should be per-cpu are now all on CPU0 so no events are > collected from CPUs 1-3 and those interrupts are killed as spurious > after a few seconds. > > I'm seeing this on 4.19.134 and 5.4.53 but as far as I can tell the > relevant code hasn't changed through to next-20200723. Reverting the > backport of this change fixes the problem. Bah. > It looks like what happens is that because the interrupts are not > per-CPU in the hardware, armpmu_request_irq() calls irq_force_affinity() > while the interrupt is deactivated and then request_irq() with > IRQF_PERCPU | IRQF_NOBALANCING. > > Now when irq_startup() runs with IRQ_STARTUP_NORMAL, it calls > irq_setup_affinity() which returns early because IRQF_PERCPU and > IRQF_NOBALANCING are set, leaving the interrupt on its original CPU. Right. My brain tricked me to believe that we made activation mandatory, but that's not. I have some ideas for a trivial generic way to solve this without undoing the commit in question and without going through all the irq chip drivers. So far everything I came up with is butt ugly. Maybe Marc has some brilliant idea. Sorry for the wreckage and thanks for the excellent problem description. I'll come back to you in the next days. Thanks, tglx
Re: [PATCH V2] genirq/affinity: Handle affinity setting on inactive interrupts correctly
Hi,
On Fri, 17 Jul 2020 18:00:02 +0200
Thomas Gleixner wrote:
> Setting interrupt affinity on inactive interrupts is inconsistent when
> hierarchical irq domains are enabled. The core code should just store the
> affinity and not call into the irq chip driver for inactive interrupts
> because the chip drivers may not be in a state to handle such requests.
>
> X86 has a hacky workaround for that but all other irq chips have not which
> causes problems e.g. on GIC V3 ITS.
>
> Instead of adding more ugly hacks all over the place, solve the problem in
> the core code. If the affinity is set on an inactive interrupt then:
>
> - Store it in the irq descriptors affinity mask
> - Update the effective affinity to reflect that so user space has
> a consistent view
> - Don't call into the irq chip driver
>
> This is the core equivalent of the X86 workaround and works correctly
> because the affinity setting is established in the irq chip when the
> interrupt is activated later on.
>
> Note, that this is only effective when hierarchical irq domains are enabled
> by the architecture. Doing it unconditionally would break legacy irq chip
> implementations.
>
> For hierarchial irq domains this works correctly as none of the drivers can
> have a dependency on affinity setting in inactive state by design.
>
> Remove the X86 workaround as it is not longer required.
>
> Fixes: 02edee152d6e ("x86/apic/vector: Ignore set_affinity call for inactive
> interrupts")
> Reported-by: Ali Saidi
> Signed-off-by: Thomas Gleixner
> Cc: sta...@vger.kernel.org
> Link: https://lore.kernel.org/r/20200529015501.15771-1-alisa...@amazon.com
> ---
> V2: Fix the fallout for CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK=n (0day)
It seems that this patch breaks perf events on RK3288 because the PMU
interrupts that should be per-cpu are now all on CPU0 so no events are
collected from CPUs 1-3 and those interrupts are killed as spurious
after a few seconds.
I'm seeing this on 4.19.134 and 5.4.53 but as far as I can tell the
relevant code hasn't changed through to next-20200723. Reverting the
backport of this change fixes the problem.
It looks like what happens is that because the interrupts are not
per-CPU in the hardware, armpmu_request_irq() calls irq_force_affinity()
while the interrupt is deactivated and then request_irq() with
IRQF_PERCPU | IRQF_NOBALANCING.
Now when irq_startup() runs with IRQ_STARTUP_NORMAL, it calls
irq_setup_affinity() which returns early because IRQF_PERCPU and
IRQF_NOBALANCING are set, leaving the interrupt on its original CPU.
At this point /proc/interrupts clearly shows the interrupts occurring on
CPU0 despite /proc/irq/N/effective_affinity and /proc/irq/N/smp_affinity
showing them spread across the cores as expected.
I don't think I understand what's meant to happen well enough to propose
a patch, but hopefully the above explanation explains the problem.
Regards,
John
> ---
> arch/x86/kernel/apic/vector.c | 22 +-
> kernel/irq/manage.c | 37 +++--
> 2 files changed, 40 insertions(+), 19 deletions(-)
>
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -446,12 +446,10 @@ static int x86_vector_activate(struct ir
> trace_vector_activate(irqd->irq, apicd->is_managed,
> apicd->can_reserve, reserve);
>
> - /* Nothing to do for fixed assigned vectors */
> - if (!apicd->can_reserve && !apicd->is_managed)
> - return 0;
> -
> raw_spin_lock_irqsave(&vector_lock, flags);
> - if (reserve || irqd_is_managed_and_shutdown(irqd))
> + if (!apicd->can_reserve && !apicd->is_managed)
> + assign_irq_vector_any_locked(irqd);
> + else if (reserve || irqd_is_managed_and_shutdown(irqd))
> vector_assign_managed_shutdown(irqd);
> else if (apicd->is_managed)
> ret = activate_managed(irqd);
> @@ -775,20 +773,10 @@ void lapic_offline(void)
> static int apic_set_affinity(struct irq_data *irqd,
>const struct cpumask *dest, bool force)
> {
> - struct apic_chip_data *apicd = apic_chip_data(irqd);
> int err;
>
> - /*
> - * Core code can call here for inactive interrupts. For inactive
> - * interrupts which use managed or reservation mode there is no
> - * point in going through the vector assignment right now as the
> - * activation will assign a vector which fits the destination
> - * cpumask. Let the core code store the destination mask and be
> - * done with it.
> - */
> - if (!irqd_is_activated(irqd) &&
> - (apicd->is_managed || apicd->can_reserve))
> - return IRQ_SET_MASK_OK;
> + if (WARN_ON_ONCE(!irqd_is_activated(irqd)))
> + return -EIO;
>
> raw_spin_lock(&vector_lock);
> cpumask_and(vector_searchmask, dest, cpu_online_mask);
> --- a/kernel/irq/manage.c
> +++ b/kernel/i
