[PATCH 0/4] arch/CPU hotplug: Add missing CPU Hotplug bits to fix nasty issues

[PATCH 0/4] arch/CPU hotplug: Add missing CPU Hotplug bits to fix nasty issues

[Srivatsa S. Bhat]

Unfortunately, some of the CPU Hotplug code has been duplicated in all of
the architectures. And in some cases, very poorly (some architectures left
out some of the important bits), to add to the woes.

Commit 5fbd036b552f633abb394a319f7c62a5c86a9cd7 (sched: Cleanup cpu_active
madness) introduced some changes that made the scheduler rely on the
CPU_STARTING notifier. And hence those architectures which forgot to
send out the CPU_STARTING notification will almost surely get into trouble.
(Xen is one example[1]).

The proper fix would be to pull out these bits into generic CPU Hotplug code.
But for now, fix this regression by adding the missing bits in the respective
architectures.

[1].https://lkml.org/lkml/2012/3/20/459
--
 Srivatsa S. Bhat (4):
      hexagon/CPU hotplug: Add missing call to notify_cpu_starting()
      mn10300/CPU hotplug: Add missing call to notify_cpu_starting()
      parisc/CPU hotplug: Add missing call to notify_cpu_starting()
      tile/CPU hotplug: Add missing call to notify_cpu_starting()


  arch/hexagon/kernel/smp.c  |    5 +++++
 arch/mn10300/kernel/smp.c  |    8 ++++++--
 arch/parisc/kernel/smp.c   |    7 ++++++-
 arch/tile/kernel/smpboot.c |    2 ++
 4 files changed, 19 insertions(+), 3 deletions(-)


Regards,
Srivatsa S. Bhat
IBM Linux Technology Center

[PATCH 1/4] hexagon/CPU hotplug: Add missing call to notify_cpu_starting()

[Srivatsa S. Bhat]

The scheduler depends on receiving the CPU_STARTING notification, without
which we end up into a lot of trouble. So add the missing call to
notify_cpu_starting() in the bringup code.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 arch/hexagon/kernel/smp.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/hexagon/kernel/smp.c b/arch/hexagon/kernel/smp.c
index 0123c63..71ad6e9 100644
--- a/arch/hexagon/kernel/smp.c
+++ b/arch/hexagon/kernel/smp.c
@@ -178,7 +178,12 @@ void __cpuinit start_secondary(void)
 
 	printk(KERN_INFO "%s cpu %d\n", __func__, current_thread_info()->cpu);
 
+	notify_cpu_starting(cpu);
+
+	ipi_call_lock();
 	set_cpu_online(cpu, true);
+	ipi_call_unlock();
+
 	local_irq_enable();
 
 	cpu_idle();

[PATCH 2/4] mn10300/CPU hotplug: Add missing call to notify_cpu_starting()

[Srivatsa S. Bhat]

The scheduler depends on receiving the CPU_STARTING notification, without
which we end up into a lot of trouble. So add the missing call to
notify_cpu_starting() in the bringup code.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

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

diff --git a/arch/mn10300/kernel/smp.c b/arch/mn10300/kernel/smp.c
index 9242e9f..25ca7fe 100644
--- a/arch/mn10300/kernel/smp.c
+++ b/arch/mn10300/kernel/smp.c
@@ -875,10 +875,14 @@ static void __init smp_online(void)
 
 	cpu = smp_processor_id();
 
-	local_irq_enable();
+	notify_cpu_starting(cpu);
 
+	ipi_call_lock();
 	set_cpu_online(cpu, true);
-	smp_wmb();
+	ipi_call_unlock();
+
+	local_irq_enable();
+
 }
 
 /**

[PATCH 3/4] parisc/CPU hotplug: Add missing call to notify_cpu_starting()

[Srivatsa S. Bhat]

The scheduler depends on receiving the CPU_STARTING notification, without
which we end up into a lot of trouble. So add the missing call to
notify_cpu_starting() in the bringup code.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 arch/parisc/kernel/smp.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/parisc/kernel/smp.c b/arch/parisc/kernel/smp.c
index 32d5884..50a4581 100644
--- a/arch/parisc/kernel/smp.c
+++ b/arch/parisc/kernel/smp.c
@@ -297,8 +297,13 @@ smp_cpu_init(int cpunum)
 
 		printk(KERN_CRIT "CPU#%d already initialized!\n", cpunum);
 		machine_halt();
-	}  
+	}
+
+	notify_cpu_starting(cpunum);
+
+	ipi_call_lock();
 	set_cpu_online(cpunum, true);
+	ipi_call_unlock();
 
 	/* Initialise the idle task for this CPU */
 	atomic_inc(&init_mm.mm_count);

[PATCH 4/4] tile/CPU hotplug: Add missing call to notify_cpu_starting()

[Srivatsa S. Bhat]

The scheduler depends on receiving the CPU_STARTING notification, without
which we end up into a lot of trouble. So add the missing call to
notify_cpu_starting() in the bringup code.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 arch/tile/kernel/smpboot.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/tile/kernel/smpboot.c b/arch/tile/kernel/smpboot.c
index b949edc..172aef7 100644
--- a/arch/tile/kernel/smpboot.c
+++ b/arch/tile/kernel/smpboot.c
@@ -196,6 +196,8 @@ void __cpuinit online_secondary(void)
 	/* This must be done before setting cpu_online_mask */
 	wmb();
 
+	notify_cpu_starting(smp_processor_id());
+
 	/*
 	 * We need to hold call_lock, so there is no inconsistency
 	 * between the time smp_call_function() determines number of

Re: [PATCH 0/4] arch/CPU hotplug: Add missing CPU Hotplug bits to fix nasty issues

[Peter Zijlstra]

On Thu, 2012-03-22 at 16:58 +0530, Srivatsa S. Bhat wrote:
> Commit 5fbd036b552f633abb394a319f7c62a5c86a9cd7 (sched: Cleanup cpu_active
> madness) introduced some changes that made the scheduler rely on the
> CPU_STARTING notifier. And hence those architectures which forgot to
> send out the CPU_STARTING notification will almost surely get into trouble.
> (Xen is one example[1]). 

The requirement is that CPU_STARTING is ran before set_cpu_online() or
the open-coded equivalent -- eg. blackfin gets this wrong.

However, it is also required that all this happens while local IRQs are
still disabled, since the moment we enable IRQs interrupts can happen
and interrupts can cause wakeups, and wakeups need to have this state
set up -- blackfin, cris, m32r, mips, sh, sparc64, sparc32, um?, x86 get
this wrong.

Furthermore, all archs that use CONFIG_USE_GENERIC_SMP_HELPERS should
hold ipi_call_lock() over setting the cpu online -- alpha, arm, m32r,
mips, sh, sparc32 seem wrong.

Furthermore, I was pondering the scenario where a 3rd cpu IPIs the newly
booting cpu, I suspect we need a smp_wmb() after setting cpu_active and
a rmb in select_fallback_rq() before reading active.

All in all its a complete friggin trainwreck.

Re: [PATCH 0/4] arch/CPU hotplug: Add missing CPU Hotplug bits to fix nasty issues

[Peter Zijlstra]

On Thu, 2012-03-22 at 13:13 +0100, Peter Zijlstra wrote:
> 
> Furthermore, I was pondering the scenario where a 3rd cpu IPIs the newly
> booting cpu, I suspect we need a smp_wmb() after setting cpu_active and
> a rmb in select_fallback_rq() before reading active.

Hmm, not IPI, that would be covered by the disabling of interrupts on
bringup, but somehow manage to wake a thread that's strictly affine that
the fresh cpu, like queue_work_on() or so.

Re: [PATCH 0/4] arch/CPU hotplug: Add missing CPU Hotplug bits to fix nasty issues

[James Bottomley]

On Thu, 2012-03-22 at 13:13 +0100, Peter Zijlstra wrote:
> On Thu, 2012-03-22 at 16:58 +0530, Srivatsa S. Bhat wrote:
> > Commit 5fbd036b552f633abb394a319f7c62a5c86a9cd7 (sched: Cleanup cpu_active
> > madness) introduced some changes that made the scheduler rely on the
> > CPU_STARTING notifier. And hence those architectures which forgot to
> > send out the CPU_STARTING notification will almost surely get into trouble.
> > (Xen is one example[1]). 
> 
> The requirement is that CPU_STARTING is ran before set_cpu_online() or
> the open-coded equivalent -- eg. blackfin gets this wrong.
> 
> However, it is also required that all this happens while local IRQs are
> still disabled, since the moment we enable IRQs interrupts can happen
> and interrupts can cause wakeups, and wakeups need to have this state
> set up -- blackfin, cris, m32r, mips, sh, sparc64, sparc32, um?, x86 get
> this wrong.
> 
> Furthermore, all archs that use CONFIG_USE_GENERIC_SMP_HELPERS should
> hold ipi_call_lock() over setting the cpu online -- alpha, arm, m32r,
> mips, sh, sparc32 seem wrong.

So for parisc, anywhere in smp_callin() should be fine.  Our last act on
CPU bringup before going into cpu_idle() is to enable interrupts.

> Furthermore, I was pondering the scenario where a 3rd cpu IPIs the newly
> booting cpu, I suspect we need a smp_wmb() after setting cpu_active and
> a rmb in select_fallback_rq() before reading active.
> 
> All in all its a complete friggin trainwreck.

I haven't yet actually tried git head on parisc (and I won't be able to
until I actually get my machines back [currently being shipped across
the atlantic]) but if there's a problem it will show up quickly:  parisc
actually uses cpu hotplug to boot its secondary cpus.

James

Re: [PATCH 0/4] arch/CPU hotplug: Add missing CPU Hotplug bits to fix nasty issues

[Peter Zijlstra]

On Thu, 2012-03-22 at 14:53 +0000, James Bottomley wrote:
> parisc actually uses cpu hotplug to boot its secondary cpus. 

I think we all do these days..

Re: [PATCH 4/4] tile/CPU hotplug: Add missing call to notify_cpu_starting()

[Chris Metcalf]

On 3/22/2012 7:29 AM, Srivatsa S. Bhat wrote:
> The scheduler depends on receiving the CPU_STARTING notification, without
> which we end up into a lot of trouble. So add the missing call to
> notify_cpu_starting() in the bringup code.
>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
>
>  arch/tile/kernel/smpboot.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

Thanks!  Taken into the tile tree.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH 1/4] hexagon/CPU hotplug: Add missing call to notify_cpu_starting()

[Richard Kuo]

On Thu, Mar 22, 2012 at 04:58:25PM +0530, Srivatsa S. Bhat wrote:
> The scheduler depends on receiving the CPU_STARTING notification, without
> which we end up into a lot of trouble. So add the missing call to
> notify_cpu_starting() in the bringup code.
> 
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> 
>  arch/hexagon/kernel/smp.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 

Tested in my tree; thanks!


Acked-by: Richard Kuo <rkuo@codeaurora.org>


-- 

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.