[patch] ARM: smpboot: Enable interrupts after marking CPU online/active
From: vincent.guittot@linaro.org (Vincent Guittot)
Date: 2011-09-14 06:55:01
On 14 September 2011 03:10, Frank Rowand [off-list ref] wrote:
On 09/13/11 10:53, Russell King - ARM Linux wrote:quoted
On Tue, Sep 13, 2011 at 07:22:16PM +0200, Vincent Guittot wrote:quoted
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.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.
I have tested your patch on a snowball with a kernel based on 3.1-rc4 and cpu topology patch
quoted hunk ↗ jump to hunk
Signed-off-by: Frank Rowand <redacted> --- ?arch/arm/kernel/smp.c | ? 14 ? 7 + ? ? 7 - ? ? 0 ! ?1 file changed, 7 insertions(+), 7 deletions(-) Index: b/arch/arm/kernel/smp.c ===================================================================--- a/arch/arm/kernel/smp.c +++ b/arch/arm/kernel/smp.c@@ -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();@@ -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();