ARM: smpboot: Enable interrupts after marking CPU online/active

Frank Rowand reported:

I have a consistent (every boot) hang on boot with the RT patches. With a few hacks to get console output, I get:

rcu_preempt_state detected stalls on CPUs/tasks

I have also replicated the problem on the ARM RealView (in tree) and without the RT patches.

The problem ended up being caused by the allowed cpus mask being set to all possible cpus for the ksoftirqd on the secondary processors. So the RCU softirq was never executing on the secondary cpu.

The problem was that ksoftirqd was woken on the secondary processors before the secondary processors were online. This led to allowed cpus being set to all cpus.

     wake_up_process()
        try_to_wake_up()
           select_task_rq()
              if (... || !cpu_online(cpu))
                 select_fallback_rq(task_cpu(p), p)
                    ...
                    /* No more Mr. Nice Guy. */
                    dest_cpu = cpuset_cpus_allowed_fallback(p)
                       do_set_cpus_allowed(p, cpu_possible_mask)
                          #  Thus ksoftirqd can now run on any cpu...

</report>

The reason is that the ARM SMP boot code for the secondary CPUs enables interrupts before the newly brought up CPU is marked online and active.

That causes a wakeup of ksoftirqd or a wakeup of any other kernel thread which is affine to the brought up CPU break that threads affinity and therefor being scheduled on already online CPUs.

This problem has been observed on x86 before and the only solution is to mark the CPU online and wait for the CPU active bit before the point where interrupts are enabled.

This is safe as the percpu timer setup and the calibration code are not part of the critical setup path and the calibration code needs to have interrupts enabled anyway. We cannot schedule away at this point because we are still in the preempt disabled region which is released in cpu_idle().

Reported-and-tested-by Frank Rowand<[email protected]>

Link http://lkml.kernel.org/r/alpine.LFD.2.02.1109071115410.2723 at ionos

Signed-off-by Thomas Gleixner<[email protected]>

Frank Rowand reported:

?I have a consistent (every boot) hang on boot with the RT patches. ?With a few hacks to get console output, I get:

? rcu_preempt_state detected stalls on CPUs/tasks

?I have also replicated the problem on the ARM RealView (in tree) and ?without the RT patches.

?The problem ended up being caused by the allowed cpus mask being set ?to all possible cpus for the ksoftirqd on the secondary processors. ?So the RCU softirq was never executing on the secondary cpu.

?The problem was that ksoftirqd was woken on the secondary processors before ?the secondary processors were online. This led to allowed cpus being set ?to all cpus.

? ?wake_up_process()
? ? ? try_to_wake_up()
? ? ? ? ?select_task_rq()
? ? ? ? ? ? if (... || !cpu_online(cpu))
? ? ? ? ? ? ? ?select_fallback_rq(task_cpu(p), p)
? ? ? ? ? ? ? ? ? ...
? ? ? ? ? ? ? ? ? /* No more Mr. Nice Guy. */
? ? ? ? ? ? ? ? ? dest_cpu = cpuset_cpus_allowed_fallback(p) ? ? ? ? ? ? ? ? ? ? ?do_set_cpus_allowed(p, cpu_possible_mask) ? ? ? ? ? ? ? ? ? ? ? ? # ?Thus ksoftirqd can now run on any cpu... </report>

The reason is that the ARM SMP boot code for the secondary CPUs enables interrupts before the newly brought up CPU is marked online and active.

That causes a wakeup of ksoftirqd or a wakeup of any other kernel thread which is affine to the brought up CPU break that threads affinity and therefor being scheduled on already online CPUs.

This problem has been observed on x86 before and the only solution is to mark the CPU online and wait for the CPU active bit before the point where interrupts are enabled.

This is safe as the percpu timer setup and the calibration code are not part of the critical setup path and the calibration code needs to have interrupts enabled anyway. We cannot schedule away at this point because we are still in the preempt disabled region which is released in cpu_idle().

Reported-and-tested-by Frank Rowand <[email protected]>

Link http://lkml.kernel.org/r/alpine.LFD.2.02.1109071115410.2723 at ionos

Signed-off-by Thomas Gleixner <[email protected]>

---
?arch/arm/kernel/smp.c | ? 21 ++++++++++++--------- ?1 file changed, 12 insertions(+), 9 deletions(-)

Index: linux-2.6/arch/arm/kernel/smp.c
===================================================================

arch/arm/kernel/smp.c +0 -0

@@ -305,6 +305,18 @@ asmlinkage void __cpuinit secondary_star

? ? ? ? * Enable local interrupts.
? ? ? ? */
? ? ? ?notify_cpu_starting(cpu);

arch/arm/kernel/smp.c +12 -0

+
+ ? ? ? /*
+ ? ? ? ?* OK, now it's safe to let the boot CPU continue. ?Wait for
+ ? ? ? ?* the CPU migration code to notice that the CPU is online
+ ? ? ? ?* before we continue. We need to do that before we enable
+ ? ? ? ?* interrupts otherwise a wakeup of a kernel thread affine to
+ ? ? ? ?* this CPU might break the affinity and let hell break lose.
+ ? ? ? ?*/
+ ? ? ? set_cpu_online(cpu, true);
+ ? ? ? while (!cpu_active(cpu))
+ ? ? ? ? ? ? ? cpu_relax();
+

? ? ? ?local_irq_enable();
? ? ? ?local_fiq_enable();

arch/arm/kernel/smp.c +0 -0

@@ -318,15 +330,6 @@ asmlinkage void __cpuinit secondary_star
 ? ? ? ?smp_store_cpu_info(cpu);
 

? ? ? ?/*
- ? ? ? ?* OK, now it's safe to let the boot CPU continue. ?Wait for
- ? ? ? ?* the CPU migration code to notice that the CPU is online
- ? ? ? ?* before we continue.
- ? ? ? ?*/
- ? ? ? set_cpu_online(cpu, true);
- ? ? ? while (!cpu_active(cpu))
- ? ? ? ? ? ? ? cpu_relax();
-
- ? ? ? /*
? ? ? ? * OK, it's off to the idle thread for us
? ? ? ? */
? ? ? ?cpu_idle();

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Thomas,

This movement of set_cpu_online before percpu_timer_setup is usefull for samsung exyno4 platforms where the external gic timer initialisation needs the secondary cpu to be online.
In addition to your modifications the following changes fixes the race condition crash happening when sched_mc configuration flags (CONFIG_ARM_CPU_TOPOLOGY, CONFIG_SCHED_MC ,CONFIG_SCHED_SMT) are enabled. Sched_mc patches are recently submitted by Vincent and accepted by Russell.(https://lkml.org/lkml/2011/7/5/209). I have not attached the crash log here. If needed i can do so.

---
 arch/arm/kernel/smp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/kernel/smp.c +1 -0

index 8cb94c8..04a8630 100644
@@ -316,6 +316,7 @@ asmlinkage void __cpuinit secondary_start_kernel(void)
         * interrupts otherwise a wakeup of a kernel thread affine to
         * this CPU might break the affinity and let hell break lose.
         */
+       smp_store_cpu_info(cpu);
        set_cpu_online(cpu, true);
        while (!cpu_active(cpu))
                cpu_relax();

Quoted 32 lines · amit kachhap

Hi Thomas,

This movement of set_cpu_online before percpu_timer_setup is usefull for samsung exyno4 platforms where the external gic timer initialisation needs the secondary cpu to be online.
In addition to your modifications the following changes fixes the race condition crash happening when sched_mc configuration flags (CONFIG_ARM_CPU_TOPOLOGY, CONFIG_SCHED_MC ,CONFIG_SCHED_SMT) are enabled. Sched_mc patches are recently submitted by Vincent and accepted by Russell.(https://lkml.org/lkml/2011/7/5/209). I have not attached the crash log here. If needed i can do so.

---
?arch/arm/kernel/smp.c | ? ?2 +-
?1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/kernel/smp.c +0 -0

index 8cb94c8..04a8630 100644
@@ -316,6 +316,7 @@ asmlinkage void __cpuinit secondary_start_kernel(void)

? ? ? ? ?* interrupts otherwise a wakeup of a kernel thread affine to ? ? ? ? ?* this CPU might break the affinity and let hell break lose. ? ? ? ? ?*/
+ ? ? ? smp_store_cpu_info(cpu);
? ? ? ? set_cpu_online(cpu, true);
? ? ? ? while (!cpu_active(cpu))
? ? ? ? ? ? ? ? cpu_relax();

arch/arm/kernel/smp.c +0 -0

@@ -330,7 +331,6 @@ asmlinkage void __cpuinit secondary_start_kernel(void)
 
 ? ? ? ? calibrate_delay();
 

arch/arm/kernel/smp.c asmlinkage void __cpuinit secondary_start_kernel(void) · line 333

Quoted 1 line

arch/arm/kernel/smp.c +0 -1

- ? ? ? smp_store_cpu_info(cpu);

This is totally wrong - the order of things here is quite specific. You can't store the CPU info until after you've calibrated the delay loop.

Please continue playing cat-and-mouse with this.

The assumption done in the 1st patch that smp_store_cpu_info can be delayed is no more true. The smp_store_cpu_info is now also used to store the cpu topology information
(https://lkml.org/lkml/2011/7/5/209). This must be done before calling sched_init_smp, which will use this information for building sched_domain.
If we move set_cpu_online before smp_store_cpu_info, sched_init_smp can be called before having called smp_store_cpu_info on all cpus.

Quoted 8 lines · Vincent Guittot

The assumption done in the 1st patch that smp_store_cpu_info can be delayed is no more true. The smp_store_cpu_info is now also used to store the cpu topology information
(https://lkml.org/lkml/2011/7/5/209). This must be done before calling sched_init_smp, which will use this information for building sched_domain.
If we move set_cpu_online before smp_store_cpu_info, sched_init_smp can be called before having called smp_store_cpu_info on all cpus.

Right. We hold off returning from cpu_up() by watching for the upcoming CPU setting its online bit.

The bug which Thomas' patch introduces is to move the setting of that before we've finished bringing the CPU up - specifically, allowing the requesting CPU to continue while the brought-up CPU is still calibrating loops_per_jiffy, and before it's stored that information and setup the scheduler domain information.

The other issue is that moving the marking of the CPU online in the way Thomas has means that we then invite the delivery of IPIs to the CPU which is still in the process of coming up. Whether that's an issue depends on what the IPIs are.

So, we must have the setting of CPU online _after_ we've setup the scheduler domain information etc - so the following is a strict ordering:

1. calibrate_delay()
2. smp_store_cpu_info()
3. set_cpu_online()

Now, the question is do we need interrupts enabled to setup timers via percpu_timer_setup() and calibrate delay. Can we move enabling interrupts after smp_store_cpu_info(). IOW, instead of moving the setting of cpu online before all this, can we move notify_cpu_starting() and the enabling of _both_ interrupts after smp_store_cpu_info()... No idea at the moment.

The question is whether you really need to calibrate the delay for each core or if you simply can take the calibration of the first core. That would avoid enabling interrupts all the way.

Quoted 8 lines · Vincent Guittot

The assumption done in the 1st patch that smp_store_cpu_info can be delayed is no more true. The smp_store_cpu_info is now also used to store the cpu topology information
(https://lkml.org/lkml/2011/7/5/209). This must be done before calling sched_init_smp, which will use this information for building sched_domain.
If we move set_cpu_online before smp_store_cpu_info, sched_init_smp can be called before having called smp_store_cpu_info on all cpus.

Right. We hold off returning from cpu_up() by watching for the upcoming CPU setting its online bit.

The bug which Thomas' patch introduces is to move the setting of that before we've finished bringing the CPU up - specifically, allowing the requesting CPU to continue while the brought-up CPU is still calibrating loops_per_jiffy, and before it's stored that information and setup the scheduler domain information.

The other issue is that moving the marking of the CPU online in the way Thomas has means that we then invite the delivery of IPIs to the CPU which is still in the process of coming up. Whether that's an issue depends on what the IPIs are.

So, we must have the setting of CPU online _after_ we've setup the scheduler domain information etc - so the following is a strict ordering:

1. calibrate_delay()
2. smp_store_cpu_info()
3. set_cpu_online()

Now, the question is do we need interrupts enabled to setup timers via percpu_timer_setup() and calibrate delay. Can we move enabling interrupts after smp_store_cpu_info(). IOW, instead of moving the setting of cpu online before all this, can we move notify_cpu_starting() and the enabling of _both_ interrupts after smp_store_cpu_info()... No idea at the moment.

Quoted 32 lines · Russell King - ARM Linux

Quoted 8 lines · Vincent Guittot

The assumption done in the 1st patch that smp_store_cpu_info can be delayed is no more true. The smp_store_cpu_info is now also used to store the cpu topology information
(https://lkml.org/lkml/2011/7/5/209). This must be done before calling sched_init_smp, which will use this information for building sched_domain.
If we move set_cpu_online before smp_store_cpu_info, sched_init_smp can be called before having called smp_store_cpu_info on all cpus.

Right. ?We hold off returning from cpu_up() by watching for the upcoming CPU setting its online bit.

The bug which Thomas' patch introduces is to move the setting of that before we've finished bringing the CPU up - specifically, allowing the requesting CPU to continue while the brought-up CPU is still calibrating loops_per_jiffy, and before it's stored that information and setup the scheduler domain information.

The other issue is that moving the marking of the CPU online in the way Thomas has means that we then invite the delivery of IPIs to the CPU which is still in the process of coming up. ?Whether that's an issue depends on what the IPIs are.

So, we must have the setting of CPU online _after_ we've setup the scheduler domain information etc - so the following is a strict ordering:

1. calibrate_delay()
2. smp_store_cpu_info()
3. set_cpu_online()

Quoted 6 lines

Now, the question is do we need interrupts enabled to setup timers via percpu_timer_setup() and calibrate delay. ?Can we move enabling interrupts after smp_store_cpu_info(). ?IOW, instead of moving the setting of cpu online before all this, can we move notify_cpu_starting() and the enabling of _both_ interrupts after smp_store_cpu_info()... No idea at the moment.

Modified the patch from Thomas to move enabling interrupts after smp_store_cpu_info(), as suggested by Russell.

Tested on RealView (3.0.1, 3.0.1-rt11), Panda (3.0.0, 3.0.3-rt12).

The calibrate_delay() is platform specific, so be aware that there are more platforms that I did not test.

Note that these kernel versions do not have the store_cpu_topology() that Vincent pointed out.

Signed-off-by Frank Rowand <[email protected]>

---
?arch/arm/kernel/smp.c | ? 14 ? 7 + ? ? 7 - ? ? 0 ! ?1 file changed, 7 insertions(+), 7 deletions(-)

Index: b/arch/arm/kernel/smp.c
===================================================================

arch/arm/kernel/smp.c +0 -0

@@ -303,13 +303,6 @@ asmlinkage void __cpuinit secondary_star
 ? ? ? ?platform_secondary_init(cpu);
 

? ? ? ?/*
- ? ? ? ?* Enable local interrupts.
- ? ? ? ?*/
- ? ? ? notify_cpu_starting(cpu);
- ? ? ? local_irq_enable();
- ? ? ? local_fiq_enable();
-
- ? ? ? /*
? ? ? ? * Setup the percpu timer for this CPU.
? ? ? ? */
? ? ? ?percpu_timer_setup();

arch/arm/kernel/smp.c +0 -0

@@ -328,6 +321,13 @@ asmlinkage void __cpuinit secondary_star
 ? ? ? ? ? ? ? ?cpu_relax();
 

? ? ? ?/*
+ ? ? ? ?* Enable local interrupts.
+ ? ? ? ?*/
+ ? ? ? notify_cpu_starting(cpu);
+ ? ? ? local_irq_enable();
+ ? ? ? local_fiq_enable();
+
+ ? ? ? /*
? ? ? ? * OK, it's off to the idle thread for us
? ? ? ? */
? ? ? ?cpu_idle();

So, we must have the setting of CPU online _after_ we've setup the scheduler domain information etc - so the following is a strict ordering:

1. calibrate_delay()
2. smp_store_cpu_info()
3. set_cpu_online()

Now, the question is do we need interrupts enabled to setup timers via percpu_timer_setup() and calibrate delay. Can we move enabling interrupts after smp_store_cpu_info(). IOW, instead of moving the setting of cpu online before all this, can we move notify_cpu_starting() and the enabling of _both_ interrupts after smp_store_cpu_info()... No idea at the moment.

Quoted 8 lines · Russell King - ARM Linux

So, we must have the setting of CPU online _after_ we've setup the scheduler domain information etc - so the following is a strict ordering:

1. calibrate_delay()
2. smp_store_cpu_info()
3. set_cpu_online()

Quoted 6 lines

Now, the question is do we need interrupts enabled to setup timers via percpu_timer_setup() and calibrate delay. ?Can we move enabling interrupts after smp_store_cpu_info(). ?IOW, instead of moving the setting of cpu online before all this, can we move notify_cpu_starting() and the enabling of _both_ interrupts after smp_store_cpu_info()... No idea at the moment.

And to make things worse... 4bd0fe1c78623062263cf5ae875fd484c5b8256d has appeared in mainline today.

arch/arm/mach-exynos4/platsmp.c +0 -0

index 7c2282c..df6ef1b 100644
@@ -106,6 +106,8 @@ void __cpuinit platform_secondary_init(unsigned int cpu)

? ? ? ? */
? ? ? ?spin_lock(&boot_lock);
? ? ? ?spin_unlock(&boot_lock);
+
+ ? ? ? set_cpu_online(cpu, true);
?}

?int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)

I think some work needs to be done to eliminate some of the dependencies in this code so that we can have a *sane* order for bringup of secondary CPUs.

I'm just going to sit on the fence and watch what platform people do during the next merge window when the support for the topological scheduler goes in.

Quoted 8 lines · Russell King - ARM Linux

So, we must have the setting of CPU online _after_ we've setup the scheduler domain information etc - so the following is a strict ordering:

1. calibrate_delay()
2. smp_store_cpu_info()
3. set_cpu_online()

Quoted 6 lines

Now, the question is do we need interrupts enabled to setup timers via percpu_timer_setup() and calibrate delay. Can we move enabling interrupts after smp_store_cpu_info(). IOW, instead of moving the setting of cpu online before all this, can we move notify_cpu_starting() and the enabling of _both_ interrupts after smp_store_cpu_info()... No idea at the moment.

arch/arm/mach-exynos4/platsmp.c +2 -0

         */
        spin_lock(&boot_lock);
        spin_unlock(&boot_lock);
+
+       set_cpu_online(cpu, true);
 }
 
 int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)

I think some work needs to be done to eliminate some of the dependencies in this code so that we can have a *sane* order for bringup of secondary CPUs.

I'm just going to sit on the fence and watch what platform people do during the next merge window when the support for the topological scheduler goes in.

Russell King - ARM Linux wrote:
But as Amit said, current EXYNOS4 SoCs which are using SPI for local timers has problem that the irq_set_affinity() method is called in percpu_timer_setup() before CPU1 becomes online with the commit 5dfc54e087c15f823ee9b6541d2f0f314e69cbed ("ARM: GIC: avoid routing interrupts to offline CPUs"). So I will check again.

Quoted 6 lines · Kukjin Kim

Russell King - ARM Linux wrote:
But as Amit said, current EXYNOS4 SoCs which are using SPI for local timers has problem that the irq_set_affinity() method is called in percpu_timer_setup() before CPU1 becomes online with the commit 5dfc54e087c15f823ee9b6541d2f0f314e69cbed ("ARM: GIC: avoid routing interrupts to offline CPUs"). So I will check again.

Why is a per cpu interrupt having an irq_set_affinity() function at all?

Quoted 2 lines · Russell King - ARM Linux

On Tue, Sep 13, 2011 at 06:53:12PM +0100, Russell King - ARM Linux

Quoted 4 lines

Quoted 4 lines

So, we must have the setting of CPU online _after_ we've setup the scheduler domain information etc - so the following is a strict ordering:

Quoted 8 lines

Quoted 8 lines

1. calibrate_delay()
2. smp_store_cpu_info()
3. set_cpu_online()

Now, the question is do we need interrupts enabled to setup timers via percpu_timer_setup() and calibrate delay. Can we move enabling interrupts after smp_store_cpu_info(). IOW, instead of moving the setting of cpu online before all this, can we move

Quoted 6 lines

Quoted 2 lines

and the enabling of _both_ interrupts after smp_store_cpu_info()... No idea at the moment.

And to make things worse... 4bd0fe1c78623062263cf5ae875fd484c5b8256d has appeared in mainline today.

arch/arm/mach-

Quoted 1 line

arch/arm/mach- renamed from arch/arm/mach-exynos4/platsmp.c +0 -0

exynos4/platsmp.c

arch/arm/mach-exynos4/platsmp.c void __cpuinit platform_secondary_init(unsigned int

Quoted 4 lines

index 7c2282c..df6ef1b 100644

arch/arm/mach-exynos4/platsmp.c +0 -0

@@ -106,6 +106,8 @@ void __cpuinit platform_secondary_init(unsigned int

arch/arm/mach-exynos4/platsmp.c void __cpuinit platform_secondary_init(unsigned int cpu)

Quoted 8 lines

arch/arm/mach-exynos4/platsmp.c +2 -0

         */
        spin_lock(&boot_lock);
        spin_unlock(&boot_lock);
+
+       set_cpu_online(cpu, true);
 }
 
 int __cpuinit boot_secondary(unsigned int cpu, struct task_struct

Quoted 5 lines

I think some work needs to be done to eliminate some of the dependencies in this code so that we can have a *sane* order for bringup of secondary CPUs.

Hi Russell,

Oops, as you said, it seems not proper place, platform_secondary_init() to

secondary CPU online so I will submit its revert patch.

But as Amit said, current EXYNOS4 SoCs which are using SPI for local

problem that the irq_set_affinity() method is called in

CPU1 becomes online with the commit 5dfc54e087c15f823ee9b6541d2f0f314e69cbed ("ARM: GIC: avoid routing interrupts to offline CPUs"). So I will check again.

If you have any ideas, please kindly let me know.

Quoted 3 lines

I'm just going to sit on the fence and watch what platform people do during the next merge window when the support for the topological scheduler goes in.

I think, basically, if SPIs in GIC are used in local timer, the routing interrupt like calling irq_set_affinity() to offline CPUs should be available when boot time before calling set_cpu_online() because as you know the local_timer_setup() which includes setup interrupt is called in percpu_timer_setup() now...

Is there any way to get the flags of struct irqaction from struct irq_data in gic_set_affinity()? Or?

Quoted 8 lines · Kukjin Kim

I think, basically, if SPIs in GIC are used in local timer, the routing interrupt like calling irq_set_affinity() to offline CPUs should be available when boot time before calling set_cpu_online() because as you know the local_timer_setup() which includes setup interrupt is called in percpu_timer_setup() now...

Is there any way to get the flags of struct irqaction from struct irq_data in gic_set_affinity()? Or?

No, and you should not even think about it at all.

The problem I can see from all this discussion is related to the early enabling of interrupts and the per cpu timer setup before the cpu is marked online. I really do not understand at all why this needs to be done in order to call calibrate_delay().

calibrate_delay() monitors jiffies, which are updated from the CPU which is waiting for the new CPU to set the online bit.

So you simply can call calibrate_delay() on the new CPU just from the interrupt disabled region and move the local timer setup after you stored the cpu data and before enabling interrupts.

This solves both the cpu_online vs. cpu_active problem and the affinity setting of the per cpu timers.

Thanks,

? ? ? ?tglx
----
Index: linux-2.6/arch/arm/kernel/smp.c
===================================================================

arch/arm/kernel/smp.c +0 -0

@@ -301,17 +301,7 @@ asmlinkage void __cpuinit secondary_star

? ? ? ? */
? ? ? ?platform_secondary_init(cpu);

- ? ? ? /*
- ? ? ? ?* Enable local interrupts.
- ? ? ? ?*/
? ? ? ?notify_cpu_starting(cpu);
- ? ? ? local_irq_enable();
- ? ? ? local_fiq_enable();
-
- ? ? ? /*
- ? ? ? ?* Setup the percpu timer for this CPU.
- ? ? ? ?*/
- ? ? ? percpu_timer_setup();

? ? ? ?calibrate_delay();

arch/arm/kernel/smp.c +0 -0

@@ -323,10 +313,23 @@ asmlinkage void __cpuinit secondary_star

? ? ? ? * before we continue.
? ? ? ? */
? ? ? ?set_cpu_online(cpu, true);
+
+ ? ? ? /*
+ ? ? ? ?* Setup the percpu timer for this CPU.
+ ? ? ? ?*/
+ ? ? ? percpu_timer_setup();

+
? ? ? ?while (!cpu_active(cpu))
? ? ? ? ? ? ? ?cpu_relax();

? ? ? ?/*
+ ? ? ? ?* cpu_active bit is set, so it's safe to enable interrupts
+ ? ? ? ?* now.
+ ? ? ? ?*/
+ ? ? ? local_irq_enable();
+ ? ? ? local_fiq_enable();
+
+ ? ? ? /*
? ? ? ? * OK, it's off to the idle thread for us
? ? ? ? */
? ? ? ?cpu_idle();

Quoted 3 lines · Kukjin Kim

I think, basically, if SPIs in GIC are used in local timer, the routing interrupt like calling irq_set_affinity() to offline CPUs should be available when boot time before calling set_cpu_online() because as you

Quoted 4 lines

the local_timer_setup() which includes setup interrupt is called in percpu_timer_setup() now...

Is there any way to get the flags of struct irqaction from struct

Quoted 1 line

in gic_set_affinity()? Or?

No, and you should not even think about it at all.

The problem I can see from all this discussion is related to the early enabling of interrupts and the per cpu timer setup before the cpu is marked online. I really do not understand at all why this needs to be done in order to call calibrate_delay().

calibrate_delay() monitors jiffies, which are updated from the CPU which is waiting for the new CPU to set the online bit.

So you simply can call calibrate_delay() on the new CPU just from the interrupt disabled region and move the local timer setup after you stored the cpu data and before enabling interrupts.

This solves both the cpu_online vs. cpu_active problem and the affinity setting of the per cpu timers.

Thanks,

tglx
----
Index: linux-2.6/arch/arm/kernel/smp.c
=============================================================
======

arch/arm/kernel/smp.c +6 -10

@@ -301,17 +301,7 @@ asmlinkage void __cpuinit secondary_star
 	 */
 	platform_secondary_init(cpu);
 
-	/*
-	 * Enable local interrupts.
-	 */
 	notify_cpu_starting(cpu);
-	local_irq_enable();
-	local_fiq_enable();
-
-	/*
-	 * Setup the percpu timer for this CPU.
-	 */
-	percpu_timer_setup();
 
 	calibrate_delay();
 

@@ -323,10 +313,23 @@ asmlinkage void __cpuinit secondary_star
 	 * before we continue.
 	 */
 	set_cpu_online(cpu, true);
+
+	/*
+	 * Setup the percpu timer for this CPU.
+	 */
+	percpu_timer_setup();
+
 	while (!cpu_active(cpu))
 		cpu_relax();
 
 	/*

Quoted 8 lines · Kukjin Kim

I think, basically, if SPIs in GIC are used in local timer, the routing interrupt like calling irq_set_affinity() to offline CPUs should be available when boot time before calling set_cpu_online() because as you know the local_timer_setup() which includes setup interrupt is called in percpu_timer_setup() now...

Is there any way to get the flags of struct irqaction from struct irq_data in gic_set_affinity()? Or?

No, and you should not even think about it at all.

The problem I can see from all this discussion is related to the early enabling of interrupts and the per cpu timer setup before the cpu is marked online. I really do not understand at all why this needs to be done in order to call calibrate_delay().

calibrate_delay() monitors jiffies, which are updated from the CPU which is waiting for the new CPU to set the online bit.

So you simply can call calibrate_delay() on the new CPU just from the interrupt disabled region and move the local timer setup after you stored the cpu data and before enabling interrupts.

This solves both the cpu_online vs. cpu_active problem and the affinity setting of the per cpu timers.

Thanks,

? ? ? ?tglx
----
Index: linux-2.6/arch/arm/kernel/smp.c
===================================================================

arch/arm/kernel/smp.c +0 -0

@@ -301,17 +301,7 @@ asmlinkage void __cpuinit secondary_star

? ? ? ? */
? ? ? ?platform_secondary_init(cpu);

- ? ? ? /*
- ? ? ? ?* Enable local interrupts.
- ? ? ? ?*/
? ? ? ?notify_cpu_starting(cpu);
- ? ? ? local_irq_enable();
- ? ? ? local_fiq_enable();
-
- ? ? ? /*
- ? ? ? ?* Setup the percpu timer for this CPU.
- ? ? ? ?*/
- ? ? ? percpu_timer_setup();

? ? ? ?calibrate_delay();

arch/arm/kernel/smp.c +0 -0

@@ -323,10 +313,23 @@ asmlinkage void __cpuinit secondary_star

? ? ? ? * before we continue.
? ? ? ? */
? ? ? ?set_cpu_online(cpu, true);

arch/arm/kernel/smp.c +6 -0

+
+ ? ? ? /*
+ ? ? ? ?* Setup the percpu timer for this CPU.
+ ? ? ? ?*/
+ ? ? ? percpu_timer_setup();
+

? ? ? ?while (!cpu_active(cpu))
? ? ? ? ? ? ? ?cpu_relax();

? ? ? ?/*
+ ? ? ? ?* cpu_active bit is set, so it's safe to enable interrupts
+ ? ? ? ?* now.
+ ? ? ? ?*/
+ ? ? ? local_irq_enable();
+ ? ? ? local_fiq_enable();
+
+ ? ? ? /*
? ? ? ? * OK, it's off to the idle thread for us
? ? ? ? */
? ? ? ?cpu_idle();

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Thomas,

I was seeing something similar, and this patch did address part of it, but I still think we have a problem. What I am seeing is the following:

We have a SCHED_FIFO kernel thread which tries to do a blocking IPI (smp_call_function_single) to all online cpus (for_each_online_cpu). This introduces a deadlock where the FIFO thread could be preempting the suspend thread which is the one that's trying to set the second CPU active after it sees it going online. The second CPU marked itself online and is then waiting for the first CPU to mark it active before enabling irqs to service the IPI.

It feels to me like we should probably not be sending blocking IPIs from a FIFO thread for sanity reasons, but that does mean there's still a deadlock present here.

Thoughts?

Thanks in advance.

--Dima

Quoted 83 lines · Thomas Gleixner

Quoted 8 lines · Kukjin Kim

I think, basically, if SPIs in GIC are used in local timer, the routing interrupt like calling irq_set_affinity() to offline CPUs should be available when boot time before calling set_cpu_online() because as you know the local_timer_setup() which includes setup interrupt is called in percpu_timer_setup() now...

Is there any way to get the flags of struct irqaction from struct irq_data in gic_set_affinity()? Or?

No, and you should not even think about it at all.

The problem I can see from all this discussion is related to the early enabling of interrupts and the per cpu timer setup before the cpu is marked online. I really do not understand at all why this needs to be done in order to call calibrate_delay().

calibrate_delay() monitors jiffies, which are updated from the CPU which is waiting for the new CPU to set the online bit.

So you simply can call calibrate_delay() on the new CPU just from the interrupt disabled region and move the local timer setup after you stored the cpu data and before enabling interrupts.

This solves both the cpu_online vs. cpu_active problem and the affinity setting of the per cpu timers.

Thanks,

? ? ? ?tglx
----
Index: linux-2.6/arch/arm/kernel/smp.c
===================================================================

arch/arm/kernel/smp.c +0 -0

@@ -301,17 +301,7 @@ asmlinkage void __cpuinit secondary_star

? ? ? ? */
? ? ? ?platform_secondary_init(cpu);

- ? ? ? /*
- ? ? ? ?* Enable local interrupts.
- ? ? ? ?*/
? ? ? ?notify_cpu_starting(cpu);
- ? ? ? local_irq_enable();
- ? ? ? local_fiq_enable();
-
- ? ? ? /*
- ? ? ? ?* Setup the percpu timer for this CPU.
- ? ? ? ?*/
- ? ? ? percpu_timer_setup();

? ? ? ?calibrate_delay();

arch/arm/kernel/smp.c +0 -0

@@ -323,10 +313,23 @@ asmlinkage void __cpuinit secondary_star

? ? ? ? * before we continue.
? ? ? ? */
? ? ? ?set_cpu_online(cpu, true);

arch/arm/kernel/smp.c +6 -0

+
+ ? ? ? /*
+ ? ? ? ?* Setup the percpu timer for this CPU.
+ ? ? ? ?*/
+ ? ? ? percpu_timer_setup();
+

? ? ? ?while (!cpu_active(cpu))
? ? ? ? ? ? ? ?cpu_relax();

? ? ? ?/*
+ ? ? ? ?* cpu_active bit is set, so it's safe to enable interrupts
+ ? ? ? ?* now.
+ ? ? ? ?*/
+ ? ? ? local_irq_enable();
+ ? ? ? local_fiq_enable();
+
+ ? ? ? /*
? ? ? ? * OK, it's off to the idle thread for us
? ? ? ? */
? ? ? ?cpu_idle();

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

<go go gadget message retractor>

Looks like the issue ended up being the fact that the caller was accessing the cpu_online_mask without calling get_online_cpus anywhere in the path and could have picked up the the intermediate state where CPU was online but not yet active..

--Dima

Quoted 24 lines · Dima Zavin

Thomas,

I was seeing something similar, and this patch did address part of it, but I still think we have a problem. What I am seeing is the following:

We have a SCHED_FIFO kernel thread which tries to do a blocking IPI (smp_call_function_single) to all online cpus (for_each_online_cpu). This introduces a deadlock where the FIFO thread could be preempting the suspend thread which is the one that's trying to set the second CPU active after it sees it going online. The second CPU marked itself online and is then waiting for the first CPU to mark it active before enabling irqs to service the IPI.

It feels to me like we should probably not be sending blocking IPIs from a FIFO thread for sanity reasons, but that does mean there's still a deadlock present here.

Thoughts?

Thanks in advance.

--Dima

I am seeing a deadlock when executing hotplug operations with this patch applied. When the secondary CPU gets brought up in _cpu_up, the cpu is turned on
and then the online notifier gets called, which is what marks the secondary CPU
as active. If _cpu_up on the primary CPU is preempted before the secondary CPU is marked active, it is possible that the primary CPU will want to call smp_call_function (or send an IPI) to the secondary CPU because it is marked online. However, with this patch, the secondary CPU is still spinning on !cpu_active(cpu)
with interrupts disabled. So, the primary CPU is now stuck in csd_lock_wait(), waiting for the secondary CPU to respond, while the secondary CPU spins with interrupts disabled, waiting for the primary CPU to mark it as active. So, while
your approach to not call smp_function_single may work for you in your specific
case, I believe there is still a problem in the general case.

One suggestion for resolving this might be making smp_call_function look at the
active CPUs rather than online CPUs, or to just let the secondary CPU mark itself as active rather than having the primary CPU do this, though this might defeat the original intended purpose of the active mask.

Quoted 18 lines · Stepan Moskovchenko

I am seeing a deadlock when executing hotplug operations with this patch applied. When the secondary CPU gets brought up in _cpu_up, the cpu is turned on
and then the online notifier gets called, which is what marks the secondary CPU
as active. If _cpu_up on the primary CPU is preempted before the secondary CPU is marked active, it is possible that the primary CPU will want to call smp_call_function (or send an IPI) to the secondary CPU because it is marked online. However, with this patch, the secondary CPU is still spinning on !cpu_active(cpu)
with interrupts disabled. So, the primary CPU is now stuck in csd_lock_wait(), waiting for the secondary CPU to respond, while the secondary CPU spins with interrupts disabled, waiting for the primary CPU to mark it as active. So, while
your approach to not call smp_function_single may work for you in your specific
case, I believe there is still a problem in the general case.

Quoted 5 lines

One suggestion for resolving this might be making smp_call_function look at the
active CPUs rather than online CPUs, or to just let the secondary CPU mark itself as active rather than having the primary CPU do this, though this might defeat the original intended purpose of the active mask.

What a mess. I'll have a look tomorrow.

Thomas,

Did you ever get a chance to look into this further?

Quoted 36 lines · Dima Zavin

<go go gadget message retractor>

Looks like the issue ended up being the fact that the caller was accessing the cpu_online_mask without calling get_online_cpus anywhere in the path and could have picked up the the intermediate state where CPU was online but not yet active..

--Dima

Quoted 25 lines · On Wed, Oct 19, 2011 at 2:16 PM, Dima Zavin<[email protected]> ?wrote:

Thomas,

I was seeing something similar, and this patch did address part of it, but I still think we have a problem. What I am seeing is the following:

We have a SCHED_FIFO kernel thread which tries to do a blocking IPI (smp_call_function_single) to all online cpus (for_each_online_cpu). This introduces a deadlock where the FIFO thread could be preempting the suspend thread which is the one that's trying to set the second CPU active after it sees it going online. The second CPU marked itself online and is then waiting for the first CPU to mark it active before enabling irqs to service the IPI.

It feels to me like we should probably not be sending blocking IPIs from a FIFO thread for sanity reasons, but that does mean there's still a deadlock present here.

Thoughts?

Thanks in advance.

--Dima

Hello

I am seeing a deadlock when executing hotplug operations with this patch applied. When the secondary CPU gets brought up in _cpu_up, the cpu is turned on
and then the online notifier gets called, which is what marks the secondary CPU
as active. If _cpu_up on the primary CPU is preempted before the secondary CPU
is marked active, it is possible that the primary CPU will want to call smp_call_function (or send an IPI) to the secondary CPU because it is marked online. However, with this patch, the secondary CPU is still spinning on !cpu_active(cpu)
with interrupts disabled. So, the primary CPU is now stuck in csd_lock_wait(),
waiting for the secondary CPU to respond, while the secondary CPU spins with interrupts disabled, waiting for the primary CPU to mark it as active. So, while
your approach to not call smp_function_single may work for you in your specific
case, I believe there is still a problem in the general case.

One suggestion for resolving this might be making smp_call_function look at the
active CPUs rather than online CPUs, or to just let the secondary CPU mark itself as active rather than having the primary CPU do this, though this might
defeat the original intended purpose of the active mask.

Steve