x86: Fix the irq affinity in fixup_cpus
RFC PATCH 1/1 x86: Fix the irq affinity in fixup_cpus
fixup_cpus() is to set appropriate irq affinity once the CPU has been brought down, however we should also adjust the desc->irq_common_data.affinity otherwise we will get an incorrect irqmask during cpu offline:
cat /proc/irq/31/smp_affinity
00000000,80000000
echo 0 > /sys/devices/system/cpu/cpu31/online
cat /proc/irq/31/smp_affinity
00000000,80000000
This might bring potential problems, as reported we saw plenty of irq flood during hibernation restore:do_IRQ: 1.51 No irq handler for vector
Maybe it is due to some drivers get incorrect irq mask during hibernation.
Fix this by invoking the interface of irq_set_affinity_locked() to also update the desc->irq_common_data.affinity.
--- arch/x86/kernel/irq.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
arch/x86/kernel/irq.c +7 -6
@@ -480,13 +480,14 @@ void fixup_irqs(void)if (!irqd_can_move_in_process_context (data) && chip->irq_mask) chip->irq_mask (data); -if (chip->irq_set_affinity) { - ret = chip->irq_set_affinity (data, affinity, true); -if (ret == -ENOSPC ) + ret =irq_set_affinity_locked (data, affinity, true); +if (ret) { +if (ret == -ENOSPC ) {pr_crit ("IRQ %d set affinity failed because there are no available vectors. The device assigned to this IRQ is unstable.\n" , irq); - }else { -if (!(warned++)) - set_affinity =0 ; + }else { +if (!(warned++)) + set_affinity =0 ; + } }/*
Signature · 1 line
2.7.4
Quoted 40 lines · Chen Yu
fixup_cpus()is to set appropriate irq affinity once the CPU has been brought down, however we should also adjust thedesc->irq_common_data.affinityotherwise we will get an incorrect irqmask during cpu offline:cat
/proc/irq/31/smp_affinity
00000000,80000000
echo 0 >/sys/devices/system/cpu/cpu31/online
cat/proc/irq/31/smp_affinity
00000000,80000000This might bring potential problems, as reported we saw plenty of irq flood during hibernation restore:
do_IRQ: 1.51 No irq handler for vector
Maybe it is due to some drivers get incorrect irq mask during hibernation.Fix this by invoking the interface of
irq_set_affinity_locked()to also update thedesc->irq_common_data.affinity.Reported-and-tested-by Thomas Mitterfellner <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Ingo Molnar <[email protected]>Cc "H. Peter Anvin" <[email protected]>Cc Borislav Petkov <[email protected]>Cc Len Brown <[email protected]>Cc Rafael J. Wysocki <[email protected]>Signed-off-by Chen Yu <[email protected]>--- arch/x86/kernel/irq.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)arch/x86/kernel/irq.c +0
-0
arch/x86/kernel/irq.c void fixup_irqs(void) · line 483
Quoted 7 lines
arch/x86/kernel/irq.c +1
-3@@ -480,13 +480,14 @@ void fixup_irqs(void)if (!irqd_can_move_in_process_context (data) && chip->irq_mask) chip->irq_mask (data); -if (chip->irq_set_affinity) { - ret = chip->irq_set_affinity (data, affinity, true); -if (ret == -ENOSPC ) + ret =irq_set_affinity_locked (data, affinity, true);
This can't work. For interrupts which cannot set the affinity in normal context irq_set_affinity_locked() will queue the interrupt to move at the next arrival of an interrupt. So the irq stays affine to the dying CPU.
After looking at the callsites, it's safe to change irq_set_affinity_locked() so that it uses the direct affinity setter function when force == true.
So we need that change first, before we can switch fixups_irqs() over.
Thanks,
tglx
Quoted 45 lines · Thomas Gleixner
Quoted 43 lines · Chen Yu
fixup_cpus()is to set appropriate irq affinity once the CPU has been brought down, however we should also adjust thedesc->irq_common_data.affinityotherwise we will get an incorrect irqmask during cpu offline:cat
/proc/irq/31/smp_affinity
00000000,80000000
echo 0 >/sys/devices/system/cpu/cpu31/online
cat/proc/irq/31/smp_affinity
00000000,80000000This might bring potential problems, as reported we saw plenty of irq flood during hibernation restore:
do_IRQ: 1.51 No irq handler for vector
Maybe it is due to some drivers get incorrect irq mask during hibernation.Fix this by invoking the interface of
irq_set_affinity_locked()to also update thedesc->irq_common_data.affinity.Reported-and-tested-by Thomas Mitterfellner <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Ingo Molnar <[email protected]>Cc "H. Peter Anvin" <[email protected]>Cc Borislav Petkov <[email protected]>Cc Len Brown <[email protected]>Cc Rafael J. Wysocki <[email protected]>Signed-off-by Chen Yu <[email protected]>--- arch/x86/kernel/irq.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)arch/x86/kernel/irq.c +0
-0@@ -480,13 +480,14 @@ void fixup_irqs(void)if (!irqd_can_move_in_process_context (data) && chip->irq_mask) chip->irq_mask (data);
Quoted 10 lines
arch/x86/kernel/irq.c void fixup_irqs(void) · line 483
Quoted 4 lines
arch/x86/kernel/irq.c +1
-3-if (chip->irq_set_affinity) { - ret = chip->irq_set_affinity (data, affinity, true); -if (ret == -ENOSPC ) + ret =irq_set_affinity_locked (data, affinity, true);This can't work. For interrupts which cannot set the affinity in normal context
irq_set_affinity_locked()will queue the interrupt to move at the next arrival of an interrupt. So the irq stays affine to the dying CPU.
Ok, got it.
Quoted 4 lines
After looking at the callsites, it's safe to change
irq_set_affinity_locked()so that it uses the direct affinity setter function when force == true.
Sorry it took me sometime to understand this point(this is why I did not reply to you at the first time :-)
I thought the defination of the word 'safe' here means, we should not adjust the irq affinity in the process context if the ISR is still running, otherwise there might be a race condition.
Currently, there are four drivers would set the force flag to true(AKA, invoking irq_force_affinity()).
1. exynos4_mct_starting_cpu()
The irq affinity is set before the clockevent is registered, so there would be no interrupt triggered when adjusting the irq affinity in the process context. Safe.
2. sirfsoc_local_timer_starting_cpu()
The same as above. Safe.
3. arm_perf_starting_cpu()
During cpu offline, the pmu interrupt(non percpu pmu interrupt) might be migrated to other online cpus. Then once the same cpu is put online, the interrupt will be set back to this cpu again by invoking irq_force_affinity(), but currently the pmu interrupt might be still running on other cpus, so it would be unsafe to adjust its irq affinity in the process context?
4. sunhv_migrate_hvcons_irq()
The cpu who encountered a panic needs to migrate the hvcons irq to the current alive cpu, and send ipi to stop other cpus. So at the time to adjust the irq affinity for the hvcons, the interrupt of the latter might be running and it might be unsafe to adjust the irq affinity in the process context?
Not sure if my understanding is correct, or do I miss something?
thanks,
Yu
Quoted 5 lines
So we need that change first, before we can switch
fixups_irqs()over.Thanks,
tglx
Quoted 22 lines · Chen Yu
Quoted 4 lines · Thomas Gleixner
After looking at the callsites, it's safe to change
irq_set_affinity_locked()so that it uses the direct affinity setter function when force == true.Sorry it took me sometime to understand this point(this is why I did not reply to you at the first time :-)
I thought the defination of the word 'safe' here means, we should not adjust the irq affinity in the process context if the ISR is still running, otherwise there might be a race condition.Currently, there are four drivers would set the force flag to
true(AKA, invoking irq_force_affinity()).1.
exynos4_mct_starting_cpu()
The irq affinity is set before the clockevent is registered, so there would be no interrupt triggered when adjusting the irq affinity in the process context. Safe.2.
sirfsoc_local_timer_starting_cpu()
The same as above. Safe.
Quoted 7 lines
3.
arm_perf_starting_cpu()
During cpu offline, the pmu interrupt(non percpu pmu interrupt) might be migrated to other online cpus. Then once the same cpu is put online, the interrupt will be set back to this cpu again by invokingirq_force_affinity(), but currently the pmu interrupt might be still running on other cpus, so it would be unsafe to adjust its irq affinity in the process context?
No, that's not an issue. The ability to move interrupts in process context, or better said in any context, has nothing to do with a concurrent interrupt. The normal mechanics for most architectures/interrupt controllers is just to program the new affinity setting which will take effect with the next delivered interrupt.
We just have these ill designed hardware implementations which do not allow that. They require to change the interrupt affinity at the point when the interrupt is handled on the original target CPU. But that's hard to achieve when the CPU is about to go offline, because we might wait forever for an interrupt to be raised. So in that case we need to forcefully move them away and take the risk of colliding with an actual interrupt being raised in hardware concurrently which has the potential to confuse the interrupt chip.
Quoted 6 lines
4.
sunhv_migrate_hvcons_irq()
The cpu who encountered a panic needs to migrate the hvcons irq to the current alive cpu, and send ipi to stop other cpus. So at the time to adjust the irq affinity for the hvcons, the interrupt of the latter might be running and it might be unsafe to adjust the irq affinity in the process context?
None of these are related to that problem. All of these architectures can move interrupts in process context unconditionally. It's also not relevant which callsites invoke irq_set_affinity_locked() with force=true.
The point is whether we can change the semantics of irq_set_affinity_locked() without breaking something.
But please answer my other mail in that thread [1] first before we start about changing anything in that area. The affinity related changes are in Linus tree now, so please retest against that as well.
Thanks,
tglx
[1] http://lkml.kernel.org/r/alpine.DEB.2.20.1706282036330.1890@nanos
Quoted 37 lines · Thomas Gleixner
Quoted 22 lines · Chen Yu
Quoted 4 lines · Thomas Gleixner
After looking at the callsites, it's safe to change
irq_set_affinity_locked()so that it uses the direct affinity setter function when force == true.Sorry it took me sometime to understand this point(this is why I did not reply to you at the first time :-)
I thought the defination of the word 'safe' here means, we should not adjust the irq affinity in the process context if the ISR is still running, otherwise there might be a race condition.Currently, there are four drivers would set the force flag to
true(AKA, invoking irq_force_affinity()).1.
exynos4_mct_starting_cpu()
The irq affinity is set before the clockevent is registered, so there would be no interrupt triggered when adjusting the irq affinity in the process context. Safe.2.
sirfsoc_local_timer_starting_cpu()
The same as above. Safe.Quoted 7 lines
3.
arm_perf_starting_cpu()
During cpu offline, the pmu interrupt(non percpu pmu interrupt) might be migrated to other online cpus. Then once the same cpu is put online, the interrupt will be set back to this cpu again by invokingirq_force_affinity(), but currently the pmu interrupt might be still running on other cpus, so it would be unsafe to adjust its irq affinity in the process context?No, that's not an issue. The ability to move interrupts in process context, or better said in any context, has nothing to do with a concurrent interrupt. The normal mechanics for most architectures/interrupt controllers is just to program the new affinity setting which will take effect with the next delivered interrupt.
Quoted 9 lines
We just have these ill designed hardware implementations which do not allow that. They require to change the interrupt affinity at the point when the interrupt is handled on the original target CPU. But that's hard to achieve when the CPU is about to go offline, because we might wait forever for an interrupt to be raised. So in that case we need to forcefully move them away and take the risk of colliding with an actual interrupt being raised in hardware concurrently which has the potential to confuse the interrupt chip.
Thanks for the detailed explaination! Got it.
Quoted 11 lines
Quoted 6 lines
4.
sunhv_migrate_hvcons_irq()
The cpu who encountered a panic needs to migrate the hvcons irq to the current alive cpu, and send ipi to stop other cpus. So at the time to adjust the irq affinity for the hvcons, the interrupt of the latter might be running and it might be unsafe to adjust the irq affinity in the process context?None of these are related to that problem. All of these architectures can move interrupts in process context unconditionally. It's also not relevant which callsites invoke
irq_set_affinity_locked()with force=true.
Okay.
Quoted 3 lines
The point is whether we can change the semantics of
irq_set_affinity_locked()without breaking something.
Yes, this might change the semantics of force flag.
Quoted 3 lines
But please answer my other mail in that thread [1] first before we start about changing anything in that area. The affinity related changes are in Linus tree now, so please retest against that as well.
Okay, I'll switch to that thread.
Here's the test result for affinity:
# uname -r 4.12.0+ # cat /proc/irq/32/smp_affinity 00000000,80000000 # echo 0 > /sys/devices/system/cpu/cpu31/online # cat /proc/irq/32/smp_affinity 00000000,ffffffff Looks like cpu31 is till included in the irq mask.
Thanks,
Yu
Quoted 8 lines
Thanks,
tglx
[1] http://lkml.kernel.org/r/alpine.DEB.2.20.1706282036330.1890@nanos
Quoted 10 lines · Chen Yu
On Tue, Jul 04, 2017 at 10:50:33AM +0200, Thomas Gleixner wrote:
Here's the test result for affinity:# uname -r 4.12.0+ # cat /proc/irq/32/smp_affinity 00000000,80000000 # echo 0 > /sys/devices/system/cpu/cpu31/online # cat /proc/irq/32/smp_affinity 00000000,ffffffff Looks like cpu31 is till included in the irq mask.
Yes, because the offline broke affinity and now it's reset to the default affinity mask. /proc/irq/*/affinity is the affinity which is allowed by the admin, not the one which is effective.
Look at /proc/irq/32/effective_affinity to see where it's really routed to.
Thanks,
tglx
Quoted 12 lines · Thomas Gleixner
Quoted 10 lines · Chen Yu
On Tue, Jul 04, 2017 at 10:50:33AM +0200, Thomas Gleixner wrote:
Here's the test result for affinity:# uname -r 4.12.0+ # cat /proc/irq/32/smp_affinity 00000000,80000000 # echo 0 > /sys/devices/system/cpu/cpu31/online # cat /proc/irq/32/smp_affinity 00000000,ffffffff Looks like cpu31 is till included in the irq mask.
Quoted 6 lines
Yes, because the offline broke affinity and now it's reset to the default affinity mask.
/proc/irq/*/affinityis the affinity which is allowed by the admin, not the one which is effective.Look at
/proc/irq/32/effective_affinityto see where it's really routed to.
ok, got it, this is normal now.
cat /proc/irq/32/effective_affinity
00000000,00000001
Thanks,
Yu