genirq: Managed affinity fixes

PATCH Superseded Marc Zyngier 2 participants 8 messages Mar 21, 2022 – Mar 23, 2022

Subsystems (4)

irqchip drivers IRQ Arm generic interrupt controller drivers IRQ domains (IRQ number mapping library)

lore

Share & tools

lore

Marc Zyngier

author maintainer

2022-03-21 19:37 UTC #0 lore raw

v2 cover genirq: Managed affinity fixes

John (and later on David) reported[1] a while ago that booting with maxcpus=1, managed affinity devices would fail to get the interrupts that were associated with offlined CPUs.

Similarly, Xiongfeng reported[2] that the GICv3 ITS would sometime use non-housekeeping CPUs instead of the affinity that was passed down as a parameter.

[1] can be fixed by not trying to activate these interrupts if no CPU that can satisfy the affinity is present (a patch addressing this was already posted[3])

[2] is a consequence of affinities containing non-online CPUs being passed down to the interrupt controller driver and the ITS driver trying to paper over that by ignoring the affinity parameter and doing its own (stupid) thing. It would be better to (a) get the core code to remove the offline CPUs from the affinity mask at all times, and (b) fix the drivers so that they can trust the core code not to trip them.

This small series, based on 5.17, addresses the above.

Thanks,

M.

[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]
[3] https://lore.kernel.org/r/[email protected]

Marc Zyngier (3):
genirq/msi: Shutdown managed interrupts with unsatifiable affinities
genirq: Always limit the affinity to online CPUs irqchip/gic-v3: Always trust the managed affinity provided by the core
code

 drivers/irqchip/irq-gic-v3-its.c |  2 +-
 kernel/irq/manage.c              | 25 +++++++++++++++++--------
 kernel/irq/msi.c                 | 15 +++++++++++++++
 3 files changed, 33 insertions(+), 9 deletions(-)

Signature · 1 line

2.34.1

Marc Zyngier

author maintainer

2022-03-21 19:37 UTC #1 lore raw

PATCH v2 1/3 genirq/msi: Shutdown managed interrupts with unsatifiable affinities

When booting with maxcpus=<small number>, interrupt controllers such as the GICv3 ITS may not be able to satisfy the affinity of some managed interrupts, as some of the HW resources are simply not available.

The same thing happens when loading a driver using managed interrupts while CPUs are offline.

In order to deal with this, do not try to activate such interrupt if there is no online CPU capable of handling it. Instead, place it in shutdown state. Once a capable CPU shows up, it will be activated.

Reported-by John Garry <[email protected]>

Tested-by John Garry <[email protected]>

Reported-by David Decotigny <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

---
 kernel/irq/msi.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

kernel/irq/msi.c +15 -0

index 2bdfce5edafd..a9ee535293eb 100644
@@ -818,6 +818,21 @@ static int msi_init_virq(struct irq_domain *domain, int virq, unsigned int vflag
 		irqd_clr_can_reserve(irqd);
 		if (vflags & VIRQ_NOMASK_QUIRK)
 			irqd_set_msi_nomask_quirk(irqd);
+
+		/*
+		 * If the interrupt is managed but no CPU is available to
+		 * service it, shut it down until better times. Note that
+		 * we only do this on the !RESERVE path as x86 (the only
+		 * architecture using this flag) deals with this in a
+		 * different way by using a catch-all vector.
+		 */
+		if ((vflags & VIRQ_ACTIVATE) &&
+		    irqd_affinity_is_managed(irqd) &&
+		    !cpumask_intersects(irq_data_get_affinity_mask(irqd),
+					cpu_online_mask)) {
+			    irqd_set_managed_shutdown(irqd);
+			    return 0;
+		    }
 	}
 
 	if (!(vflags & VIRQ_ACTIVATE))

Signature · 1 line

2.34.1

Marc Zyngier

author maintainer

2022-03-21 19:37 UTC #2 lore raw

PATCH v2 2/3 genirq: Always limit the affinity to online CPUs

In reply to Marc Zyngier

When booting with maxcpus=<small number> (or even loading a driver while most CPUs are offline), it is pretty easy to observe managed affinities containing a mix of online and offline CPUs being passed to the irqchip driver.

This means that the irqchip cannot trust the affinity passed down from the core code, which is a bit annoying and requires (at least in theory) all drivers to implement some sort of affinity narrowing.

In order to address this, always limit the cpumask to the set of online CPUs.

Signed-off-by Marc Zyngier <[email protected]>

---
 kernel/irq/manage.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

kernel/irq/manage.c +17 -8

index f23ffd30385b..351024253ce8 100644
@@ -222,11 +222,16 @@ int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
 {
 	struct irq_desc *desc = irq_data_to_desc(data);
 	struct irq_chip *chip = irq_data_get_irq_chip(data);
+	const struct cpumask  *prog_mask;
 	int ret;
 
+	static DEFINE_RAW_SPINLOCK(tmp_mask_lock);
+	static struct cpumask tmp_mask;
+
 	if (!chip || !chip->irq_set_affinity)
 		return -EINVAL;
 
+	raw_spin_lock(&tmp_mask_lock);
 	/*
 	 * If this is a managed interrupt and housekeeping is enabled on
 	 * it check whether the requested affinity mask intersects with

@@ -248,24 +253,28 @@ int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
 	 */
 	if (irqd_affinity_is_managed(data) &&
 	    housekeeping_enabled(HK_FLAG_MANAGED_IRQ)) {
-		const struct cpumask *hk_mask, *prog_mask;
-
-		static DEFINE_RAW_SPINLOCK(tmp_mask_lock);
-		static struct cpumask tmp_mask;
+		const struct cpumask *hk_mask;
 
 		hk_mask = housekeeping_cpumask(HK_FLAG_MANAGED_IRQ);
 
-		raw_spin_lock(&tmp_mask_lock);
 		cpumask_and(&tmp_mask, mask, hk_mask);
 		if (!cpumask_intersects(&tmp_mask, cpu_online_mask))
 			prog_mask = mask;
 		else
 			prog_mask = &tmp_mask;
-		ret = chip->irq_set_affinity(data, prog_mask, force);
-		raw_spin_unlock(&tmp_mask_lock);
 	} else {
-		ret = chip->irq_set_affinity(data, mask, force);
+		prog_mask = mask;
 	}
+
+	/* Make sure we only provide online CPUs to the irqchip */
+	cpumask_and(&tmp_mask, prog_mask, cpu_online_mask);
+	if (!cpumask_empty(&tmp_mask))
+		ret = chip->irq_set_affinity(data, &tmp_mask, force);
+	else
+		ret = -EINVAL;
+
+	raw_spin_unlock(&tmp_mask_lock);
+
 	switch (ret) {
 	case IRQ_SET_MASK_OK:
 	case IRQ_SET_MASK_OK_DONE:

Signature · 1 line

2.34.1

Marc Zyngier

author maintainer

2022-03-21 19:37 UTC #3 lore raw

PATCH v2 3/3 irqchip/gic-v3: Always trust the managed affinity provided by the core code

In reply to Marc Zyngier

Now that the core code has been fixed to always give us an affinity that only includes online CPUs, directly use this affinity when computing a target CPU.

Signed-off-by Marc Zyngier <[email protected]>

---
 drivers/irqchip/irq-gic-v3-its.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/irqchip/irq-gic-v3-its.c +1 -1

index cd772973114a..2656efd5d2b6 100644
@@ -1624,7 +1624,7 @@ static int its_select_cpu(struct irq_data *d,
 
 		cpu = cpumask_pick_least_loaded(d, tmpmask);
 	} else {
-		cpumask_and(tmpmask, irq_data_get_affinity_mask(d), cpu_online_mask);
+		cpumask_copy(tmpmask, aff_mask);
 
 		/* If we cannot cross sockets, limit the search to that node */
 		if ((its_dev->its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_23144) &&

Signature · 1 line

2.34.1

Xiongfeng Wang

2022-03-23 03:52 UTC #4 lore raw

In reply to Marc Zyngier

Hi, Marc

Quoted 12 lines · Marc Zyngier

John (and later on David) reported[1] a while ago that booting with maxcpus=1, managed affinity devices would fail to get the interrupts that were associated with offlined CPUs.
Similarly, Xiongfeng reported[2] that the GICv3 ITS would sometime use non-housekeeping CPUs instead of the affinity that was passed down as a parameter.
[1] can be fixed by not trying to activate these interrupts if no CPU that can satisfy the affinity is present (a patch addressing this was already posted[3])

Quoted 8 lines

[2] is a consequence of affinities containing non-online CPUs being passed down to the interrupt controller driver and the ITS driver trying to paper over that by ignoring the affinity parameter and doing its own (stupid) thing. It would be better to (a) get the core code to remove the offline CPUs from the affinity mask at all times, and (b) fix the drivers so that they can trust the core code not to trip them.
This small series, based on 5.17, addresses the above.

I have tested this patchset on D06. It works well with kernel parameter 'maxcpus=1' or 'nohz_full=1-127 isolcpus=nohz,domain,managed_irq,1-127'. Also the 'effective_affinity' is correct. Thanks!
By the way, I merged the second patch manually because of conflicts. Maybe I lack some patches on your local repo.

Thanks,
Xiongfeng

Quoted 20 lines

Thanks,
	M.
[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]
[3] https://lore.kernel.org/r/[email protected]
Marc Zyngier (3):
genirq/msi: Shutdown managed interrupts with unsatifiable affinities
genirq: Always limit the affinity to online CPUs irqchip/gic-v3: Always trust the managed affinity provided by the core
code
 drivers/irqchip/irq-gic-v3-its.c |  2 +-
 kernel/irq/manage.c              | 25 +++++++++++++++++--------
 kernel/irq/msi.c                 | 15 +++++++++++++++
 3 files changed, 33 insertions(+), 9 deletions(-)

Marc Zyngier

author maintainer

2022-03-23 08:56 UTC #5 lore raw

Hi Xiongfeng,

Quoted 23 lines · Xiongfeng Wang

Hi, Marc
Quoted 19 lines · Marc Zyngier
John (and later on David) reported[1] a while ago that booting with maxcpus=1, managed affinity devices would fail to get the interrupts that were associated with offlined CPUs.
Similarly, Xiongfeng reported[2] that the GICv3 ITS would sometime use non-housekeeping CPUs instead of the affinity that was passed down as a parameter.
[1] can be fixed by not trying to activate these interrupts if no CPU that can satisfy the affinity is present (a patch addressing this was already posted[3])
[2] is a consequence of affinities containing non-online CPUs being passed down to the interrupt controller driver and the ITS driver trying to paper over that by ignoring the affinity parameter and doing its own (stupid) thing. It would be better to (a) get the core code to remove the offline CPUs from the affinity mask at all times, and (b) fix the drivers so that they can trust the core code not to trip them.

Quoted 5 lines

Quoted 1 line
This small series, based on 5.17, addresses the above.
I have tested this patchset on D06. It works well with kernel parameter 'maxcpus=1' or 'nohz_full=1-127 isolcpus=nohz,domain,managed_irq,1-127'. Also the 'effective_affinity' is correct. Thanks!

Thanks for having given it a go.

Quoted 2 lines

By the way, I merged the second patch manually because of conflicts. Maybe I lack some patches on your local repo.

That's odd, as the patches are directly sitting on top of 5.17 in my tree (see [1]). Do you have any out of tree patches around? Please make sure you test this without any extra change.

Thanks,

M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/managed-affinity-fixes

Signature · 1 line

Without deviation from the norm, progress is not possible.

Xiongfeng Wang

2022-03-23 10:58 UTC #6 lore raw

Quoted 33 lines · Marc Zyngier

Hi Xiongfeng,
Quoted 23 lines · Xiongfeng Wang
Hi, Marc
Quoted 19 lines · Marc Zyngier
John (and later on David) reported[1] a while ago that booting with maxcpus=1, managed affinity devices would fail to get the interrupts that were associated with offlined CPUs.
Similarly, Xiongfeng reported[2] that the GICv3 ITS would sometime use non-housekeeping CPUs instead of the affinity that was passed down as a parameter.
[1] can be fixed by not trying to activate these interrupts if no CPU that can satisfy the affinity is present (a patch addressing this was already posted[3])
[2] is a consequence of affinities containing non-online CPUs being passed down to the interrupt controller driver and the ITS driver trying to paper over that by ignoring the affinity parameter and doing its own (stupid) thing. It would be better to (a) get the core code to remove the offline CPUs from the affinity mask at all times, and (b) fix the drivers so that they can trust the core code not to trip them.
Quoted 5 lines
Quoted 1 line
This small series, based on 5.17, addresses the above.
I have tested this patchset on D06. It works well with kernel parameter 'maxcpus=1' or 'nohz_full=1-127 isolcpus=nohz,domain,managed_irq,1-127'. Also the 'effective_affinity' is correct. Thanks!

Quoted 8 lines

Thanks for having given it a go.
Quoted 2 lines
By the way, I merged the second patch manually because of conflicts. Maybe I lack some patches on your local repo.
That's odd, as the patches are directly sitting on top of 5.17 in my tree (see [1]). Do you have any out of tree patches around? Please make sure you test this without any extra change.

I apply the patchset based on the latest mainline kernel. The latest commit is
commit 3bf03b9a0839c9fb06927ae53ebd0f960b19d408
Merge branch 'akpm' (patches from Andrew)
I didn't change the modification of the second patch. Only resolve the context conflicts, which is cause by the following commit.
commit 04d4e665a60902cf36e7ad39af1179cb5df542ad
sched/isolation: Use single feature type while referring to housekeeping cpumask
It changed 'HK_FLAG_MANAGED_IRQ' to 'HK_TYPE_MANAGED_IRQ'.

Thanks,
Xiongfeng

Quoted 7 lines

Thanks,
	M.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/managed-affinity-fixes

Marc Zyngier

author maintainer

2022-03-23 11:31 UTC #7 lore raw

Quoted 46 lines · Xiongfeng Wang

Quoted 33 lines · Marc Zyngier
Hi Xiongfeng,
Quoted 23 lines · Xiongfeng Wang
Hi, Marc
Quoted 19 lines · Marc Zyngier
John (and later on David) reported[1] a while ago that booting with maxcpus=1, managed affinity devices would fail to get the interrupts that were associated with offlined CPUs.
Similarly, Xiongfeng reported[2] that the GICv3 ITS would sometime use non-housekeeping CPUs instead of the affinity that was passed down as a parameter.
[1] can be fixed by not trying to activate these interrupts if no CPU that can satisfy the affinity is present (a patch addressing this was already posted[3])
[2] is a consequence of affinities containing non-online CPUs being passed down to the interrupt controller driver and the ITS driver trying to paper over that by ignoring the affinity parameter and doing its own (stupid) thing. It would be better to (a) get the core code to remove the offline CPUs from the affinity mask at all times, and (b) fix the drivers so that they can trust the core code not to trip them.
Quoted 5 lines
Quoted 1 line
This small series, based on 5.17, addresses the above.
I have tested this patchset on D06. It works well with kernel parameter 'maxcpus=1' or 'nohz_full=1-127 isolcpus=nohz,domain,managed_irq,1-127'. Also the 'effective_affinity' is correct. Thanks!
Quoted 8 lines
Thanks for having given it a go.
Quoted 2 lines
By the way, I merged the second patch manually because of conflicts. Maybe I lack some patches on your local repo.
That's odd, as the patches are directly sitting on top of 5.17 in my tree (see [1]). Do you have any out of tree patches around? Please make sure you test this without any extra change.

Quoted 8 lines

I apply the patchset based on the latest mainline kernel. The latest commit is
commit 3bf03b9a0839c9fb06927ae53ebd0f960b19d408
Merge branch 'akpm' (patches from Andrew)
I didn't change the modification of the second patch. Only resolve the context conflicts, which is cause by the following commit.
commit 04d4e665a60902cf36e7ad39af1179cb5df542ad
sched/isolation: Use single feature type while referring to housekeeping cpumask
It changed 'HK_FLAG_MANAGED_IRQ' to 'HK_TYPE_MANAGED_IRQ'.

Ah, that's on top of linux/master then. Yeah, I expect some small conflicts (this is a popular spot). I'll rebase things at some point once (and if) we agree that patch #2 is the right thing to do.

Thanks,

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.