arm: use cpu_online_mask when using forced irq_set_affinity

PATCH New Sudeep Holla 3 participants 12 messages May 9, 2014 – Aug 28, 2014 Arm (32-bit)

lore

Share & tools

lore

Sudeep Holla

author

2014-05-09 16:40 UTC #0 lore raw

PATCH 1/1 arm: use cpu_online_mask when using forced irq_set_affinity

From: Sudeep Holla <[email protected]>

Commit 01f8fa4f01d8("genirq: Allow forcing cpu affinity of interrupts") enabled the forced irq_set_affinity which previously refused to route an interrupt to an offline cpu.

Commit ffde1de64012("irqchip: Gic: Support forced affinity setting") implements this force logic and disables the cpu online check for GIC interrupt controller.

When __cpu_disable calls migrate_irqs, it disables the current cpu in cpu_online_mask and uses forced irq_set_affinity to migrate the IRQs away from the cpu but passes affinity mask with the cpu being offlined also included in it.

When calling irq_set_affinity with force == true in a cpu hotplug path, the caller must ensure that the cpu being offlined is not present in the affinity mask or it may be selected as the target CPU, leading to the interrupt not being migrated.

This patch uses cpu_online_mask when using forced irq_set_affinity so that the IRQs are properly migrated away.

Tested on TC2 hotpluging CPU0 in and out. Without this patch the system locks up as the IRQs are not migrated away from CPU0.

Cc Russell King <[email protected]>

Acked-by Mark Rutland <[email protected]>

Signed-off-by Sudeep Holla <[email protected]>

---
 arch/arm/kernel/irq.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

arch/arm/kernel/irq.c +7 -3

index 9723d17..1f216a5 100644
@@ -155,11 +155,15 @@ static bool migrate_one_irq(struct irq_desc *desc)
 	if (irqd_is_per_cpu(d) || !cpumask_test_cpu(smp_processor_id(), affinity))
 		return false;
 
-	if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids) {
-		affinity = cpu_online_mask;
+	if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids)
 		ret = true;
-	}
 
+	/*
+	 * when using forced irq_set_affinity we must ensure that the cpu
+	 * being offlined is not present in the affinity mask, it may be
+	 * selected as the target CPU otherwise
+	 */
+	affinity = cpu_online_mask;
 	c = irq_data_get_irq_chip(d);
 	if (!c->irq_set_affinity)
 		pr_debug("IRQ%u: unable to set affinity\n", d->irq);

Signature · 1 line

1.8.3.2

Sudeep Holla

author

2014-05-23 11:26 UTC #1 lore raw

PATCH 1/1 arm: use cpu_online_mask when using forced irq_set_affinity

Hi Russell,

Quoted 20 lines · Sudeep Holla

From: Sudeep Holla <[email protected]>
Commit 01f8fa4f01d8("genirq: Allow forcing cpu affinity of interrupts") enabled the forced irq_set_affinity which previously refused to route an interrupt to an offline cpu.
Commit ffde1de64012("irqchip: Gic: Support forced affinity setting") implements this force logic and disables the cpu online check for GIC interrupt controller.
When __cpu_disable calls migrate_irqs, it disables the current cpu in cpu_online_mask and uses forced irq_set_affinity to migrate the IRQs away from the cpu but passes affinity mask with the cpu being offlined also included in it.
When calling irq_set_affinity with force == true in a cpu hotplug path, the caller must ensure that the cpu being offlined is not present in the affinity mask or it may be selected as the target CPU, leading to the interrupt not being migrated.

Quoted 6 lines

This patch uses cpu_online_mask when using forced irq_set_affinity so that the IRQs are properly migrated away.
Tested on TC2 hotpluging CPU0 in and out. Without this patch the system locks up as the IRQs are not migrated away from CPU0.

If you are fine with this change, can I put this in the patch tracker?

Regards,
Sudeep

Quoted 31 lines

Cc Russell King <[email protected]>

Acked-by Mark Rutland <[email protected]>

Signed-off-by Sudeep Holla <[email protected]>

---
  arch/arm/kernel/irq.c | 10 +++++++---
  1 file changed, 7 insertions(+), 3 deletions(-)

arch/arm/kernel/irq.c +7 -3

index 9723d17..1f216a5 100644
@@ -155,11 +155,15 @@ static bool migrate_one_irq(struct irq_desc *desc)
  	if (irqd_is_per_cpu(d) || !cpumask_test_cpu(smp_processor_id(), affinity))
  		return false;
 
-	if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids) {
-		affinity = cpu_online_mask;
+	if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids)
  		ret = true;
-	}
 
+	/*
+	 * when using forced irq_set_affinity we must ensure that the cpu
+	 * being offlined is not present in the affinity mask, it may be
+	 * selected as the target CPU otherwise
+	 */
+	affinity = cpu_online_mask;
  	c = irq_data_get_irq_chip(d);
  	if (!c->irq_set_affinity)
  		pr_debug("IRQ%u: unable to set affinity\n", d->irq);

Russell King - ARM Linux

2014-05-23 12:10 UTC #2 lore raw

PATCH 1/1 arm: use cpu_online_mask when using forced irq_set_affinity

In reply to Sudeep Holla

Quoted 20 lines · Sudeep Holla

From: Sudeep Holla <[email protected]>
Commit 01f8fa4f01d8("genirq: Allow forcing cpu affinity of interrupts") enabled the forced irq_set_affinity which previously refused to route an interrupt to an offline cpu.
Commit ffde1de64012("irqchip: Gic: Support forced affinity setting") implements this force logic and disables the cpu online check for GIC interrupt controller.
When __cpu_disable calls migrate_irqs, it disables the current cpu in cpu_online_mask and uses forced irq_set_affinity to migrate the IRQs away from the cpu but passes affinity mask with the cpu being offlined also included in it.
When calling irq_set_affinity with force == true in a cpu hotplug path, the caller must ensure that the cpu being offlined is not present in the affinity mask or it may be selected as the target CPU, leading to the interrupt not being migrated.

Quoted 5 lines

This patch uses cpu_online_mask when using forced irq_set_affinity so that the IRQs are properly migrated away.
Tested on TC2 hotpluging CPU0 in and out. Without this patch the system locks up as the IRQs are not migrated away from CPU0.

You don't explain /how/ this happens, and I'm not convinced that you've properly diagnosed this bug.

arch/arm/kernel/irq.c static bool migrate_one_irq(struct irq_desc *desc) · line 161

Quoted 9 lines

arch/arm/kernel/irq.c +1 -3

@@ -155,11 +155,15 @@ static bool migrate_one_irq(struct irq_desc *desc)
 	if (irqd_is_per_cpu(d) || !cpumask_test_cpu(smp_processor_id(), affinity))
 		return false;
 
-	if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids) {
-		affinity = cpu_online_mask;
+	if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids)
 		ret = true;
-	}

The idea here with the original code is:

- if the current CPU (which is the one being offlined) is not in the affinity mask, do nothing.
- if "affinity & cpu_online_mask" indicates that there's no CPUs in the new set (cpu_online_mask must have been updated to indicate that the current CPU is offline) then re-set the affinity mask and report that we forced a change.
- otherwise, re-set the existing affinity (which will force the IRQ controller to re-evaluate it's routing given the affinity and online CPUs.)

This code is correct. In fact, changing it as you have, you /always/ reset the affinity mask whether or not the CPU being offlined is the last CPU in the affinity set.

If you are finding that CPU0 is left with interrupts afterwards, the bug lies elsewhere - probably in the IRQ controller code.

Signature · 2 lines

FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Sudeep Holla

author

2014-05-23 12:51 UTC #3 lore raw

PATCH 1/1 arm: use cpu_online_mask when using forced irq_set_affinity

Quoted 21 lines · Russell King - ARM Linux

Quoted 20 lines · Sudeep Holla
From: Sudeep Holla <[email protected]>
Commit 01f8fa4f01d8("genirq: Allow forcing cpu affinity of interrupts") enabled the forced irq_set_affinity which previously refused to route an interrupt to an offline cpu.
Commit ffde1de64012("irqchip: Gic: Support forced affinity setting") implements this force logic and disables the cpu online check for GIC interrupt controller.
When __cpu_disable calls migrate_irqs, it disables the current cpu in cpu_online_mask and uses forced irq_set_affinity to migrate the IRQs away from the cpu but passes affinity mask with the cpu being offlined also included in it.
When calling irq_set_affinity with force == true in a cpu hotplug path, the caller must ensure that the cpu being offlined is not present in the affinity mask or it may be selected as the target CPU, leading to the interrupt not being migrated.

Quoted 9 lines

Quoted 5 lines
This patch uses cpu_online_mask when using forced irq_set_affinity so that the IRQs are properly migrated away.
Tested on TC2 hotpluging CPU0 in and out. Without this patch the system locks up as the IRQs are not migrated away from CPU0.
You don't explain /how/ this happens, and I'm not convinced that you've properly diagnosed this bug.

Sorry for not being elaborate enough.
- On boot by default all the irqs have cpu_online_mask as affinity
- Now if CPU0 is being hotplugged out, CPU0 is removed from cpu_online_mask and migrate_irqs is called
- In migrate_one_irq, when affinity is read from the irq_desc, it still contains CPU0 which is expected.
- irq_set_affinity is called with affinity with CPU0 set and force = true, which chooses CPU0 resulting in not migrating the IRQ.

Quoted 12 lines

arch/arm/kernel/irq.c static bool migrate_one_irq(struct irq_desc *desc) · line 161

Quoted 9 lines

arch/arm/kernel/irq.c +1 -3

@@ -155,11 +155,15 @@ static bool migrate_one_irq(struct irq_desc *desc)
  	if (irqd_is_per_cpu(d) || !cpumask_test_cpu(smp_processor_id(), affinity))
  		return false;
 
-	if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids) {
-		affinity = cpu_online_mask;
+	if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids)
  		ret = true;
-	}

The idea here with the original code is:

Quoted 10 lines

- if the current CPU (which is the one being offlined) is not in the affinity mask, do nothing.
- if "affinity & cpu_online_mask" indicates that there's no CPUs in the new set (cpu_online_mask must have been updated to indicate that the current CPU is offline) then re-set the affinity mask and report that we forced a change.
- otherwise, re-set the existing affinity (which will force the IRQ controller to re-evaluate it's routing given the affinity and online CPUs.)

I completely understand the above idea, except that the new feature added to allow forced affinity setting(as mentioned in the commit log by 2 commits), changes the behaviour of last step.

IRQ controller now re-evaluates it's routing based on the given affinity alone and doesn't consider online CPUs when force = true is set. This will result in the CPU being offlined chosen as the target if it happens to be the first in the affinity mask.

Quoted 7 lines

This code is correct. In fact, changing it as you have, you /always/ reset the affinity mask whether or not the CPU being offlined is the last CPU in the affinity set.
If you are finding that CPU0 is left with interrupts afterwards, the bug lies elsewhere - probably in the IRQ controller code.

Since the IRQ controller code is changed to provide that feature, either
- we have to choose not to use forced option, or
- we need to make sure we pass valid affinity mask with force = true option

I chose latter in this patch. Let me know your opinion.

Regards,
Sudeep

Sudeep Holla

author

2014-06-20 13:04 UTC #4 lore raw

PATCH 1/1 arm: use cpu_online_mask when using forced irq_set_affinity

Hi Russell,

Quoted 83 lines · Sudeep Holla

Quoted 21 lines · Russell King - ARM Linux
Quoted 20 lines · Sudeep Holla
From: Sudeep Holla <[email protected]>
Commit 01f8fa4f01d8("genirq: Allow forcing cpu affinity of interrupts") enabled the forced irq_set_affinity which previously refused to route an interrupt to an offline cpu.
Commit ffde1de64012("irqchip: Gic: Support forced affinity setting") implements this force logic and disables the cpu online check for GIC interrupt controller.
When __cpu_disable calls migrate_irqs, it disables the current cpu in cpu_online_mask and uses forced irq_set_affinity to migrate the IRQs away from the cpu but passes affinity mask with the cpu being offlined also included in it.
When calling irq_set_affinity with force == true in a cpu hotplug path, the caller must ensure that the cpu being offlined is not present in the affinity mask or it may be selected as the target CPU, leading to the interrupt not being migrated.
Quoted 9 lines
Quoted 5 lines
This patch uses cpu_online_mask when using forced irq_set_affinity so that the IRQs are properly migrated away.
Tested on TC2 hotpluging CPU0 in and out. Without this patch the system locks up as the IRQs are not migrated away from CPU0.
You don't explain /how/ this happens, and I'm not convinced that you've properly diagnosed this bug.
Sorry for not being elaborate enough.
- On boot by default all the irqs have cpu_online_mask as affinity
- Now if CPU0 is being hotplugged out, CPU0 is removed from cpu_online_mask and migrate_irqs is called
- In migrate_one_irq, when affinity is read from the irq_desc, it still contains CPU0 which is expected.
- irq_set_affinity is called with affinity with CPU0 set and force = true, which chooses CPU0 resulting in not migrating the IRQ.
Quoted 12 lines
arch/arm/kernel/irq.c static bool migrate_one_irq(struct irq_desc *desc) · line 161
Quoted 9 lines
arch/arm/kernel/irq.c +1 -3
@@ -155,11 +155,15 @@ static bool migrate_one_irq(struct irq_desc *desc)
   	if (irqd_is_per_cpu(d) || !cpumask_test_cpu(smp_processor_id(), affinity))
   		return false;
 
-	if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids) {
-		affinity = cpu_online_mask;
+	if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids)
   		ret = true;
-	}
The idea here with the original code is:
Quoted 10 lines
- if the current CPU (which is the one being offlined) is not in the affinity mask, do nothing.
- if "affinity & cpu_online_mask" indicates that there's no CPUs in the new set (cpu_online_mask must have been updated to indicate that the current CPU is offline) then re-set the affinity mask and report that we forced a change.
- otherwise, re-set the existing affinity (which will force the IRQ controller to re-evaluate it's routing given the affinity and online CPUs.)
I completely understand the above idea, except that the new feature added to allow forced affinity setting(as mentioned in the commit log by 2 commits), changes the behaviour of last step.
IRQ controller now re-evaluates it's routing based on the given affinity alone and doesn't consider online CPUs when force = true is set. This will result in the CPU being offlined chosen as the target if it happens to be the first in the affinity mask.
Quoted 7 lines
This code is correct. In fact, changing it as you have, you /always/ reset the affinity mask whether or not the CPU being offlined is the last CPU in the affinity set.
If you are finding that CPU0 is left with interrupts afterwards, the bug lies elsewhere - probably in the IRQ controller code.

Quoted 6 lines

Since the IRQ controller code is changed to provide that feature, either
- we have to choose not to use forced option, or
- we need to make sure we pass valid affinity mask with force = true option
I chose latter in this patch. Let me know your opinion.

Any suggestions on this ? Since commit 01f8fa4f01d8 and ffde1de64012 are now in stable releases, CPU0 hotplug is broken there now.

Regards,
Sudeep

Russell King - ARM Linux

2014-06-20 15:16 UTC #5 lore raw

PATCH 1/1 arm: use cpu_online_mask when using forced irq_set_affinity

Quoted 90 lines · Sudeep Holla

Hi Russell,
Quoted 87 lines · Sudeep Holla
Quoted 21 lines · Russell King - ARM Linux
Quoted 20 lines · Sudeep Holla
From: Sudeep Holla <[email protected]>
Commit 01f8fa4f01d8("genirq: Allow forcing cpu affinity of interrupts") enabled the forced irq_set_affinity which previously refused to route an interrupt to an offline cpu.
Commit ffde1de64012("irqchip: Gic: Support forced affinity setting") implements this force logic and disables the cpu online check for GIC interrupt controller.
When __cpu_disable calls migrate_irqs, it disables the current cpu in cpu_online_mask and uses forced irq_set_affinity to migrate the IRQs away from the cpu but passes affinity mask with the cpu being offlined also included in it.
When calling irq_set_affinity with force == true in a cpu hotplug path, the caller must ensure that the cpu being offlined is not present in the affinity mask or it may be selected as the target CPU, leading to the interrupt not being migrated.
Quoted 9 lines
Quoted 5 lines
This patch uses cpu_online_mask when using forced irq_set_affinity so that the IRQs are properly migrated away.
Tested on TC2 hotpluging CPU0 in and out. Without this patch the system locks up as the IRQs are not migrated away from CPU0.
You don't explain /how/ this happens, and I'm not convinced that you've properly diagnosed this bug.
Sorry for not being elaborate enough.
- On boot by default all the irqs have cpu_online_mask as affinity
- Now if CPU0 is being hotplugged out, CPU0 is removed from cpu_online_mask and migrate_irqs is called
- In migrate_one_irq, when affinity is read from the irq_desc, it still contains CPU0 which is expected.
- irq_set_affinity is called with affinity with CPU0 set and force = true, which chooses CPU0 resulting in not migrating the IRQ.
Quoted 12 lines
arch/arm/kernel/irq.c static bool migrate_one_irq(struct irq_desc *desc) · line 161
Quoted 9 lines
arch/arm/kernel/irq.c +1 -3
@@ -155,11 +155,15 @@ static bool migrate_one_irq(struct irq_desc *desc)
   	if (irqd_is_per_cpu(d) || !cpumask_test_cpu(smp_processor_id(), affinity))
   		return false;
 
-	if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids) {
-		affinity = cpu_online_mask;
+	if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids)
   		ret = true;
-	}
The idea here with the original code is:
Quoted 10 lines
- if the current CPU (which is the one being offlined) is not in the affinity mask, do nothing.
- if "affinity & cpu_online_mask" indicates that there's no CPUs in the new set (cpu_online_mask must have been updated to indicate that the current CPU is offline) then re-set the affinity mask and report that we forced a change.
- otherwise, re-set the existing affinity (which will force the IRQ controller to re-evaluate it's routing given the affinity and online CPUs.)
I completely understand the above idea, except that the new feature added to allow forced affinity setting(as mentioned in the commit log by 2 commits), changes the behaviour of last step.
IRQ controller now re-evaluates it's routing based on the given affinity alone and doesn't consider online CPUs when force = true is set. This will result in the CPU being offlined chosen as the target if it happens to be the first in the affinity mask.
Quoted 7 lines
This code is correct. In fact, changing it as you have, you /always/ reset the affinity mask whether or not the CPU being offlined is the last CPU in the affinity set.
If you are finding that CPU0 is left with interrupts afterwards, the bug lies elsewhere - probably in the IRQ controller code.
Since the IRQ controller code is changed to provide that feature, either
- we have to choose not to use forced option, or
- we need to make sure we pass valid affinity mask with force = true option

Quoted 5 lines

Quoted 2 lines
I chose latter in this patch. Let me know your opinion.
Any suggestions on this ? Since commit 01f8fa4f01d8 and ffde1de64012 are now in stable releases, CPU0 hotplug is broken there now.

Maybe we should ask Thomas, as he's (a) the maintainer of the irqchip stuff, and (b) the author of the patch causing the breakage.

Quoted 1 line

From what I can see looking at the x86 code, the work-around in

ffde1de64012 is wrong.

Signature · 2 lines

FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Sudeep Holla

author

2014-07-17 09:58 UTC #6 lore raw

PATCH 1/1 arm: use cpu_online_mask when using forced irq_set_affinity

Hi Thomas,

Quoted 94 lines · Russell King - ARM Linux

Quoted 93 lines · Sudeep Holla
Hi Russell,
Quoted 89 lines · Sudeep Holla
Quoted 21 lines · Russell King - ARM Linux
Quoted 20 lines · Sudeep Holla
From: Sudeep Holla <[email protected]>
Commit 01f8fa4f01d8("genirq: Allow forcing cpu affinity of interrupts") enabled the forced irq_set_affinity which previously refused to route an interrupt to an offline cpu.
Commit ffde1de64012("irqchip: Gic: Support forced affinity setting") implements this force logic and disables the cpu online check for GIC interrupt controller.
When __cpu_disable calls migrate_irqs, it disables the current cpu in cpu_online_mask and uses forced irq_set_affinity to migrate the IRQs away from the cpu but passes affinity mask with the cpu being offlined also included in it.
When calling irq_set_affinity with force == true in a cpu hotplug path, the caller must ensure that the cpu being offlined is not present in the affinity mask or it may be selected as the target CPU, leading to the interrupt not being migrated.
Quoted 9 lines
Quoted 5 lines
This patch uses cpu_online_mask when using forced irq_set_affinity so that the IRQs are properly migrated away.
Tested on TC2 hotpluging CPU0 in and out. Without this patch the system locks up as the IRQs are not migrated away from CPU0.
You don't explain /how/ this happens, and I'm not convinced that you've properly diagnosed this bug.
Sorry for not being elaborate enough.
- On boot by default all the irqs have cpu_online_mask as affinity
- Now if CPU0 is being hotplugged out, CPU0 is removed from cpu_online_mask and migrate_irqs is called
- In migrate_one_irq, when affinity is read from the irq_desc, it still contains CPU0 which is expected.
- irq_set_affinity is called with affinity with CPU0 set and force = true, which chooses CPU0 resulting in not migrating the IRQ.
Quoted 12 lines
arch/arm/kernel/irq.c static bool migrate_one_irq(struct irq_desc *desc) · line 161
Quoted 9 lines
arch/arm/kernel/irq.c +1 -3
@@ -155,11 +155,15 @@ static bool migrate_one_irq(struct irq_desc *desc)
    	if (irqd_is_per_cpu(d) || !cpumask_test_cpu(smp_processor_id(), affinity))
    		return false;
 
-	if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids) {
-		affinity = cpu_online_mask;
+	if (cpumask_any_and(affinity, cpu_online_mask) >= nr_cpu_ids)
    		ret = true;
-	}
The idea here with the original code is:
Quoted 10 lines
- if the current CPU (which is the one being offlined) is not in the affinity mask, do nothing.
- if "affinity & cpu_online_mask" indicates that there's no CPUs in the new set (cpu_online_mask must have been updated to indicate that the current CPU is offline) then re-set the affinity mask and report that we forced a change.
- otherwise, re-set the existing affinity (which will force the IRQ controller to re-evaluate it's routing given the affinity and online CPUs.)
I completely understand the above idea, except that the new feature added to allow forced affinity setting(as mentioned in the commit log by 2 commits), changes the behaviour of last step.
IRQ controller now re-evaluates it's routing based on the given affinity alone and doesn't consider online CPUs when force = true is set. This will result in the CPU being offlined chosen as the target if it happens to be the first in the affinity mask.
Quoted 7 lines
This code is correct. In fact, changing it as you have, you /always/ reset the affinity mask whether or not the CPU being offlined is the last CPU in the affinity set.
If you are finding that CPU0 is left with interrupts afterwards, the bug lies elsewhere - probably in the IRQ controller code.
Since the IRQ controller code is changed to provide that feature, either
- we have to choose not to use forced option, or
- we need to make sure we pass valid affinity mask with force = true option
I chose latter in this patch. Let me know your opinion.

Quoted 9 lines

Quoted 2 lines
Any suggestions on this ? Since commit 01f8fa4f01d8 and ffde1de64012 are now in stable releases, CPU0 hotplug is broken there now.
Maybe we should ask Thomas, as he's (a) the maintainer of the irqchip stuff, and (b) the author of the patch causing the breakage.
From what I can see looking at the x86 code, the work-around in
ffde1de64012 is wrong.

Can provide your thoughts on how to solve this issue ?

Is it expected from all the irqchip implementation to use force flag in irq_set_affinity to ignore cpu_online_mask similar to GIC ?

Regards,
Sudeep

Thomas Gleixner

2014-08-25 09:55 UTC #7 lore raw

PATCH 1/1 arm: use cpu_online_mask when using forced irq_set_affinity

Quoted 4 lines · Sudeep Holla

Quoted 4 lines
Quoted 3 lines
Any suggestions on this ? Since commit 01f8fa4f01d8 and ffde1de64012 are now in
stable releases, CPU0 hotplug is broken there now.

Quoted 8 lines

Quoted 6 lines
Maybe we should ask Thomas, as he's (a) the maintainer of the irqchip stuff, and (b) the author of the patch causing the breakage.
From what I can see looking at the x86 code, the work-around in
ffde1de64012 is wrong.
Can provide your thoughts on how to solve this issue ?

ffde1de64012 is not about offlining a cpu, it's about onlining where we need to make sure that we assign the affinity to a not yet online marked cpu.

Quoted 2 lines

Is it expected from all the irqchip implementation to use force flag in irq_set_affinity to ignore cpu_online_mask similar to GIC ?

No, it's only relevant for the cases where we need to route irqs to not yet online cpus.

Now the wreckage of offlining was definitely not intended and I wonder why set_affinity() is called there with force = true. This was introduced in commit 1dbfa187dad. I acked it back then, but I have no idea why, because the force argument did not have any effect at that time.

Changing it to false should solve the issue.

Thanks,

tglx

arch/arm/kernel/irq.c +1 -1

index 2c4257604513..5c4d38e32a51 100644
@@ -175,7 +175,7 @@ static bool migrate_one_irq(struct irq_desc *desc)
 	c = irq_data_get_irq_chip(d);
 	if (!c->irq_set_affinity)
 		pr_debug("IRQ%u: unable to set affinity\n", d->irq);
-	else if (c->irq_set_affinity(d, affinity, true) == IRQ_SET_MASK_OK && ret)
+	else if (c->irq_set_affinity(d, affinity, false) == IRQ_SET_MASK_OK && ret)
 		cpumask_copy(d->affinity, affinity);
 
 	return ret;

Sudeep Holla

author

2014-08-26 15:19 UTC #8 lore raw

PATCH 1/1 arm: use cpu_online_mask when using forced irq_set_affinity

Hi Thomas,

Thanks for your feedback.

Quoted 18 lines · Thomas Gleixner

Quoted 4 lines · Sudeep Holla
Quoted 4 lines
Quoted 3 lines
Any suggestions on this ? Since commit 01f8fa4f01d8 and ffde1de64012 are now in
stable releases, CPU0 hotplug is broken there now.
Quoted 8 lines
Quoted 6 lines
Maybe we should ask Thomas, as he's (a) the maintainer of the irqchip stuff, and (b) the author of the patch causing the breakage.
From what I can see looking at the x86 code, the work-around in
ffde1de64012 is wrong.
Can provide your thoughts on how to solve this issue ?
ffde1de64012 is not about offlining a cpu, it's about onlining where we need to make sure that we assign the affinity to a not yet online marked cpu.

Quoted 6 lines

Quoted 2 lines
Is it expected from all the irqchip implementation to use force flag in irq_set_affinity to ignore cpu_online_mask similar to GIC ?
No, it's only relevant for the cases where we need to route irqs to not yet online cpus.

Ok. IIUC Russell's main concern was if irqchip implementation uses force flag differently, then we can't change the core code to false. Also x86 core code also uses forced irq_set_affinity in arch/x86/kernel/irq.c

Russell, any comments on this or are you fine with changing to false.

Regards,
Sudeep

Quoted 26 lines

Now the wreckage of offlining was definitely not intended and I wonder why set_affinity() is called there with force = true. This was introduced in commit 1dbfa187dad. I acked it back then, but I have no idea why, because the force argument did not have any effect at that time.
Changing it to false should solve the issue.
Thanks,
tglx
arch/arm/kernel/irq.c +1 -1
index 2c4257604513..5c4d38e32a51 100644
@@ -175,7 +175,7 @@ static bool migrate_one_irq(struct irq_desc *desc)
  	c = irq_data_get_irq_chip(d);
  	if (!c->irq_set_affinity)
  		pr_debug("IRQ%u: unable to set affinity\n", d->irq);
-	else if (c->irq_set_affinity(d, affinity, true) == IRQ_SET_MASK_OK && ret)
+	else if (c->irq_set_affinity(d, affinity, false) == IRQ_SET_MASK_OK && ret)
  		cpumask_copy(d->affinity, affinity);
 
  	return ret;

Thomas Gleixner

2014-08-28 09:32 UTC #9 lore raw

PATCH 1/1 arm: use cpu_online_mask when using forced irq_set_affinity

Quoted 9 lines · Sudeep Holla

Quoted 9 lines
Quoted 1 line
Can provide your thoughts on how to solve this issue ?
ffde1de64012 is not about offlining a cpu, it's about onlining where we need to make sure that we assign the affinity to a not yet online marked cpu.
Quoted 2 lines
Is it expected from all the irqchip implementation to use force flag in irq_set_affinity to ignore cpu_online_mask similar to GIC ?

Quoted 7 lines

Quoted 3 lines
No, it's only relevant for the cases where we need to route irqs to not yet online cpus.
Ok. IIUC Russell's main concern was if irqchip implementation uses force flag differently, then we can't change the core code to false. Also x86 core code also uses forced irq_set_affinity in arch/x86/kernel/irq.c

Which is pointless as none of the x86 irq chip implementations actually honours the force argument.

In fact until the point where I implemented it in the GIC driver, nothing ever used that argument. So the GIC conversion actually added semantics to the argument. Any driver which will make use of it, has to follow that now. I'll add documentation to the core code for it ...

Thanks,

tglx

Sudeep Holla

author

2014-08-28 10:12 UTC #10 lore raw

PATCH 1/1 arm: use cpu_online_mask when using forced irq_set_affinity

Quoted 18 lines · Thomas Gleixner

Quoted 9 lines · Sudeep Holla
Quoted 9 lines
Quoted 1 line
Can provide your thoughts on how to solve this issue ?
ffde1de64012 is not about offlining a cpu, it's about onlining where we need to make sure that we assign the affinity to a not yet online marked cpu.
Quoted 2 lines
Is it expected from all the irqchip implementation to use force flag in irq_set_affinity to ignore cpu_online_mask similar to GIC ?
Quoted 7 lines
Quoted 3 lines
No, it's only relevant for the cases where we need to route irqs to not yet online cpus.
Ok. IIUC Russell's main concern was if irqchip implementation uses force flag differently, then we can't change the core code to false. Also x86 core code also uses forced irq_set_affinity in arch/x86/kernel/irq.c

Quoted 8 lines

Which is pointless as none of the x86 irq chip implementations actually honours the force argument.
In fact until the point where I implemented it in the GIC driver, nothing ever used that argument. So the GIC conversion actually added semantics to the argument. Any driver which will make use of it, has to follow that now. I'll add documentation to the core code for it ...

Thanks Thomas for confirming.

Hi Russell,

Can I post the patch changing force to false in irq_set_affinity call to fix the issue and cc stable ? It's broken in stable kernels(v3.10 and v3.14)

Regards,
Sudeep

Russell King - ARM Linux

2014-08-28 10:16 UTC #11 lore raw

PATCH 1/1 arm: use cpu_online_mask when using forced irq_set_affinity

Quoted 30 lines · Sudeep Holla

Quoted 18 lines · Thomas Gleixner
Quoted 9 lines · Sudeep Holla
Quoted 9 lines
Quoted 1 line
Can provide your thoughts on how to solve this issue ?
ffde1de64012 is not about offlining a cpu, it's about onlining where we need to make sure that we assign the affinity to a not yet online marked cpu.
Quoted 2 lines
Is it expected from all the irqchip implementation to use force flag in irq_set_affinity to ignore cpu_online_mask similar to GIC ?
Quoted 7 lines
Quoted 3 lines
No, it's only relevant for the cases where we need to route irqs to not yet online cpus.
Ok. IIUC Russell's main concern was if irqchip implementation uses force flag differently, then we can't change the core code to false. Also x86 core code also uses forced irq_set_affinity in arch/x86/kernel/irq.c
Quoted 8 lines
Which is pointless as none of the x86 irq chip implementations actually honours the force argument.
In fact until the point where I implemented it in the GIC driver, nothing ever used that argument. So the GIC conversion actually added semantics to the argument. Any driver which will make use of it, has to follow that now. I'll add documentation to the core code for it ...

Quoted 7 lines

Thanks Thomas for confirming.
Hi Russell,
Can I post the patch changing force to false in irq_set_affinity call to fix the issue and cc stable ? It's broken in stable kernels(v3.10 and v3.14)

I think it's up to Thomas to suggest what the correct solution is to the problem he introduced, and it's not clear from Thomas' email you quote above what he thinks would be the right solution. It sounds like passing false there would be the right thing, but really it needs a clear and unambiguous statement from Thomas.

Signature · 2 lines

FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.