[PATCH 03/27] smpboot: Define and use cpu_state per-cpu variable in generic code

[PATCH 03/27] smpboot: Define and use cpu_state per-cpu variable in generic code

[Srivatsa S. Bhat]

The per-cpu variable cpu_state is used in x86 and also used in other
architectures, to track the state of the cpu during bringup and hotplug.
Pull it out into generic code.

Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: Chris Metcalf <cmetcalf@tilera.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Yong Zhang <yong.zhang0@gmail.com>
Cc: Venkatesh Pallipadi <venki@google.com>
Cc: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: linux-ia64@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-sh@vger.kernel.org
Cc: xen-devel@lists.xensource.com
Cc: virtualization@lists.linux-foundation.org
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 arch/ia64/include/asm/cpu.h   |    2 --
 arch/ia64/kernel/process.c    |    1 +
 arch/ia64/kernel/smpboot.c    |    6 +-----
 arch/mips/cavium-octeon/smp.c |    4 +---
 arch/powerpc/kernel/smp.c     |    6 +-----
 arch/sh/include/asm/smp.h     |    2 --
 arch/sh/kernel/smp.c          |    4 +---
 arch/tile/kernel/smpboot.c    |    4 +---
 arch/x86/include/asm/cpu.h    |    2 --
 arch/x86/kernel/smpboot.c     |    4 +---
 arch/x86/xen/smp.c            |    1 +
 include/linux/smpboot.h       |    1 +
 kernel/smpboot.c              |    4 ++++
 13 files changed, 13 insertions(+), 28 deletions(-)

diff --git a/arch/ia64/include/asm/cpu.h b/arch/ia64/include/asm/cpu.h
index fcca30b..1c3acac 100644
--- a/arch/ia64/include/asm/cpu.h
+++ b/arch/ia64/include/asm/cpu.h
@@ -12,8 +12,6 @@ struct ia64_cpu {
 
 DECLARE_PER_CPU(struct ia64_cpu, cpu_devices);
 
-DECLARE_PER_CPU(int, cpu_state);
-
 #ifdef CONFIG_HOTPLUG_CPU
 extern int arch_register_cpu(int num);
 extern void arch_unregister_cpu(int);
diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
index 5e0e86d..32566c7 100644
--- a/arch/ia64/kernel/process.c
+++ b/arch/ia64/kernel/process.c
@@ -29,6 +29,7 @@
 #include <linux/kdebug.h>
 #include <linux/utsname.h>
 #include <linux/tracehook.h>
+#include <linux/smpboot.h>
 
 #include <asm/cpu.h>
 #include <asm/delay.h>
diff --git a/arch/ia64/kernel/smpboot.c b/arch/ia64/kernel/smpboot.c
index 963d2db..df00a3c 100644
--- a/arch/ia64/kernel/smpboot.c
+++ b/arch/ia64/kernel/smpboot.c
@@ -39,6 +39,7 @@
 #include <linux/efi.h>
 #include <linux/percpu.h>
 #include <linux/bitops.h>
+#include <linux/smpboot.h>
 
 #include <linux/atomic.h>
 #include <asm/cache.h>
@@ -111,11 +112,6 @@ extern unsigned long ia64_iobase;
 
 struct task_struct *task_for_booting_cpu;
 
-/*
- * State for each CPU
- */
-DEFINE_PER_CPU(int, cpu_state);
-
 cpumask_t cpu_core_map[NR_CPUS] __cacheline_aligned;
 EXPORT_SYMBOL(cpu_core_map);
 DEFINE_PER_CPU_SHARED_ALIGNED(cpumask_t, cpu_sibling_map);
diff --git a/arch/mips/cavium-octeon/smp.c b/arch/mips/cavium-octeon/smp.c
index 97e7ce9..93cd4b0 100644
--- a/arch/mips/cavium-octeon/smp.c
+++ b/arch/mips/cavium-octeon/smp.c
@@ -13,6 +13,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/sched.h>
 #include <linux/module.h>
+#include <linux/smpboot.h>
 
 #include <asm/mmu_context.h>
 #include <asm/time.h>
@@ -252,9 +253,6 @@ static void octeon_cpus_done(void)
 
 #ifdef CONFIG_HOTPLUG_CPU
 
-/* State of each CPU. */
-DEFINE_PER_CPU(int, cpu_state);
-
 extern void fixup_irqs(void);
 
 static DEFINE_SPINLOCK(smp_reserve_lock);
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index e1417c4..1928058a 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -31,6 +31,7 @@
 #include <linux/cpu.h>
 #include <linux/notifier.h>
 #include <linux/topology.h>
+#include <linux/smpboot.h>
 
 #include <asm/ptrace.h>
 #include <linux/atomic.h>
@@ -57,11 +58,6 @@
 #define DBG(fmt...)
 #endif
 
-#ifdef CONFIG_HOTPLUG_CPU
-/* State of each CPU during hotplug phases */
-static DEFINE_PER_CPU(int, cpu_state) = { 0 };
-#endif
-
 struct thread_info *secondary_ti;
 
 DEFINE_PER_CPU(cpumask_var_t, cpu_sibling_map);
diff --git a/arch/sh/include/asm/smp.h b/arch/sh/include/asm/smp.h
index 78b0d0f4..bda041e 100644
--- a/arch/sh/include/asm/smp.h
+++ b/arch/sh/include/asm/smp.h
@@ -31,8 +31,6 @@ enum {
 	SMP_MSG_NR,	/* must be last */
 };
 
-DECLARE_PER_CPU(int, cpu_state);
-
 void smp_message_recv(unsigned int msg);
 void smp_timer_broadcast(const struct cpumask *mask);
 
diff --git a/arch/sh/kernel/smp.c b/arch/sh/kernel/smp.c
index b86e9ca..8e0fde0 100644
--- a/arch/sh/kernel/smp.c
+++ b/arch/sh/kernel/smp.c
@@ -22,6 +22,7 @@
 #include <linux/interrupt.h>
 #include <linux/sched.h>
 #include <linux/atomic.h>
+#include <linux/smpboot.h>
 #include <asm/processor.h>
 #include <asm/mmu_context.h>
 #include <asm/smp.h>
@@ -34,9 +35,6 @@ int __cpu_logical_map[NR_CPUS];		/* Map logical to physical */
 
 struct plat_smp_ops *mp_ops = NULL;
 
-/* State of each CPU */
-DEFINE_PER_CPU(int, cpu_state) = { 0 };
-
 void __cpuinit register_smp_ops(struct plat_smp_ops *ops)
 {
 	if (mp_ops)
diff --git a/arch/tile/kernel/smpboot.c b/arch/tile/kernel/smpboot.c
index e686c5a..24a9c06 100644
--- a/arch/tile/kernel/smpboot.c
+++ b/arch/tile/kernel/smpboot.c
@@ -25,13 +25,11 @@
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/irq.h>
+#include <linux/smpboot.h>
 #include <asm/mmu_context.h>
 #include <asm/tlbflush.h>
 #include <asm/sections.h>
 
-/* State of each CPU. */
-static DEFINE_PER_CPU(int, cpu_state) = { 0 };
-
 /* The messaging code jumps to this pointer during boot-up */
 unsigned long start_cpu_function_addr;
 
diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index 4564c8e..2d0b239 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -30,8 +30,6 @@ extern int arch_register_cpu(int num);
 extern void arch_unregister_cpu(int);
 #endif
 
-DECLARE_PER_CPU(int, cpu_state);
-
 int mwait_usable(const struct cpuinfo_x86 *);
 
 #endif /* _ASM_X86_CPU_H */
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index bfbe30e..269bc1f 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -51,6 +51,7 @@
 #include <linux/stackprotector.h>
 #include <linux/gfp.h>
 #include <linux/cpuidle.h>
+#include <linux/smpboot.h>
 
 #include <asm/acpi.h>
 #include <asm/desc.h>
@@ -73,9 +74,6 @@
 #include <asm/smpboot_hooks.h>
 #include <asm/i8259.h>
 
-/* State of each CPU */
-DEFINE_PER_CPU(int, cpu_state) = { 0 };
-
 #ifdef CONFIG_HOTPLUG_CPU
 /*
  * We need this for trampoline_base protection from concurrent accesses when
diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index 2ef5948..09a7199 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -16,6 +16,7 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/smp.h>
+#include <linux/smpboot.h>
 
 #include <asm/paravirt.h>
 #include <asm/desc.h>
diff --git a/include/linux/smpboot.h b/include/linux/smpboot.h
index 63bbedd..834d90c 100644
--- a/include/linux/smpboot.h
+++ b/include/linux/smpboot.h
@@ -5,6 +5,7 @@
 #ifndef SMPBOOT_H
 #define SMPBOOT_H
 
+DECLARE_PER_CPU(int, cpu_state);
 extern void smpboot_start_secondary(void *arg);
 
 #endif
diff --git a/kernel/smpboot.c b/kernel/smpboot.c
index 5ae1805..0df43b0 100644
--- a/kernel/smpboot.c
+++ b/kernel/smpboot.c
@@ -67,6 +67,8 @@ void __init idle_threads_init(void)
 }
 #endif
 
+/* State of each CPU during bringup/teardown */
+DEFINE_PER_CPU(int, cpu_state) = { 0 };
 
 /* Implement the following functions in your architecture, as appropriate. */
 
@@ -141,6 +143,8 @@ void __cpuinit smpboot_start_secondary(void *arg)
 	set_cpu_online(cpu, true);
 	arch_vector_unlock();
 
+	per_cpu(cpu_state, cpu) = CPU_ONLINE;
+
 	__cpu_post_online(arg);
 
 	/* Enable local interrupts now */

Re: [PATCH 03/27] smpboot: Define and use cpu_state per-cpu variable in generic code

[David Daney]

On 06/01/2012 02:10 AM, Srivatsa S. Bhat wrote:
> The per-cpu variable cpu_state is used in x86 and also used in other
> architectures, to track the state of the cpu during bringup and hotplug.
> Pull it out into generic code.
>
> Cc: Tony Luck<tony.luck@intel.com>
> Cc: Fenghua Yu<fenghua.yu@intel.com>
> Cc: Ralf Baechle<ralf@linux-mips.org>
> Cc: Benjamin Herrenschmidt<benh@kernel.crashing.org>
> Cc: Paul Mundt<lethal@linux-sh.org>
> Cc: Chris Metcalf<cmetcalf@tilera.com>
> Cc: Thomas Gleixner<tglx@linutronix.de>
> Cc: Ingo Molnar<mingo@redhat.com>
> Cc: "H. Peter Anvin"<hpa@zytor.com>
> Cc: x86@kernel.org
> Cc: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
> Cc: Jeremy Fitzhardinge<jeremy@goop.org>
> Cc: Peter Zijlstra<a.p.zijlstra@chello.nl>
> Cc: Andrew Morton<akpm@linux-foundation.org>
> Cc: Mike Frysinger<vapier@gentoo.org>
> Cc: Yong Zhang<yong.zhang0@gmail.com>
> Cc: Venkatesh Pallipadi<venki@google.com>
> Cc: Suresh Siddha<suresh.b.siddha@intel.com>
> Cc: linux-ia64@vger.kernel.org
> Cc: linux-mips@linux-mips.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-sh@vger.kernel.org
> Cc: xen-devel@lists.xensource.com
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Srivatsa S. Bhat<srivatsa.bhat@linux.vnet.ibm.com>
> ---
>
>   arch/ia64/include/asm/cpu.h   |    2 --
>   arch/ia64/kernel/process.c    |    1 +
>   arch/ia64/kernel/smpboot.c    |    6 +-----
>   arch/mips/cavium-octeon/smp.c |    4 +---
>   arch/powerpc/kernel/smp.c     |    6 +-----
>   arch/sh/include/asm/smp.h     |    2 --
>   arch/sh/kernel/smp.c          |    4 +---
>   arch/tile/kernel/smpboot.c    |    4 +---
>   arch/x86/include/asm/cpu.h    |    2 --
>   arch/x86/kernel/smpboot.c     |    4 +---
>   arch/x86/xen/smp.c            |    1 +
>   include/linux/smpboot.h       |    1 +
>   kernel/smpboot.c              |    4 ++++
>   13 files changed, 13 insertions(+), 28 deletions(-)
>
[...]
> diff --git a/arch/mips/cavium-octeon/smp.c b/arch/mips/cavium-octeon/smp.c
> index 97e7ce9..93cd4b0 100644
> --- a/arch/mips/cavium-octeon/smp.c
> +++ b/arch/mips/cavium-octeon/smp.c
> @@ -13,6 +13,7 @@
>   #include<linux/kernel_stat.h>
>   #include<linux/sched.h>
>   #include<linux/module.h>
> +#include<linux/smpboot.h>
>
>   #include<asm/mmu_context.h>
>   #include<asm/time.h>
> @@ -252,9 +253,6 @@ static void octeon_cpus_done(void)
>
>   #ifdef CONFIG_HOTPLUG_CPU
>
> -/* State of each CPU. */
> -DEFINE_PER_CPU(int, cpu_state);
> -
>   extern void fixup_irqs(void);
>
>   static DEFINE_SPINLOCK(smp_reserve_lock);

The Octeon bit:

Acked-by: David Daney <david.daney@cavium.com>


FWIW, the rest looks good too.

[PATCH 05/27] xen, cpu hotplug: Don't call cpu_bringup() in xen_play_dead()

[Srivatsa S. Bhat]

xen_play_dead calls cpu_bringup() which looks weird, because xen_play_dead()
is invoked in the cpu down path, whereas cpu_bringup() (as the name suggests)
is useful in the cpu bringup path.

Getting rid of xen_play_dead()'s dependency on cpu_bringup() helps in hooking
on to the generic SMP booting framework.

Also remove the extra call to preempt_enable() added by commit 41bd956
(xen/smp: Fix CPU online/offline bug triggering a BUG: scheduling while
atomic) because it becomes unnecessary after this change.

Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: xen-devel@lists.xensource.com
Cc: virtualization@lists.linux-foundation.org
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 arch/x86/xen/smp.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index 09a7199..602d6b7 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -417,14 +417,6 @@ static void __cpuinit xen_play_dead(void) /* used only with HOTPLUG_CPU */
 {
 	play_dead_common();
 	HYPERVISOR_vcpu_op(VCPUOP_down, smp_processor_id(), NULL);
-	cpu_bringup();
-	/*
-	 * Balance out the preempt calls - as we are running in cpu_idle
-	 * loop which has been called at bootup from cpu_bringup_and_idle.
-	 * The cpucpu_bringup_and_idle called cpu_bringup which made a
-	 * preempt_disable() So this preempt_enable will balance it out.
-	 */
-	preempt_enable();
 }
 
 #else /* !CONFIG_HOTPLUG_CPU */

Re: [Xen-devel] [PATCH 05/27] xen, cpu hotplug: Don't call cpu_bringup() in xen_play_dead()

[Jan Beulich]

>>> On 01.06.12 at 11:11, "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
wrote:
> xen_play_dead calls cpu_bringup() which looks weird, because xen_play_dead()
> is invoked in the cpu down path, whereas cpu_bringup() (as the name 
> suggests) is useful in the cpu bringup path.

This might not be correct - the code as it is without this change is
safe even when the vCPU gets onlined back later by an external
entity (e.g. the Xen tool stack), and it would in that case resume
at the return point of the VCPUOP_down hypercall. That might
be a heritage from the original XenoLinux tree though, and be
meaningless in pv-ops context - Jeremy, Konrad?

Possibly it was bogus/unused even in that original tree - Keir?

Jan

> Getting rid of xen_play_dead()'s dependency on cpu_bringup() helps in 
> hooking on to the generic SMP booting framework.
> 
> Also remove the extra call to preempt_enable() added by commit 41bd956
> (xen/smp: Fix CPU online/offline bug triggering a BUG: scheduling while
> atomic) because it becomes unnecessary after this change.
> 
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org 
> Cc: xen-devel@lists.xensource.com 
> Cc: virtualization@lists.linux-foundation.org 
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> 
>  arch/x86/xen/smp.c |    8 --------
>  1 files changed, 0 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> index 09a7199..602d6b7 100644
> --- a/arch/x86/xen/smp.c
> +++ b/arch/x86/xen/smp.c
> @@ -417,14 +417,6 @@ static void __cpuinit xen_play_dead(void) /* used only 
> with HOTPLUG_CPU */
>  {
>  	play_dead_common();
>  	HYPERVISOR_vcpu_op(VCPUOP_down, smp_processor_id(), NULL);
> -	cpu_bringup();
> -	/*
> -	 * Balance out the preempt calls - as we are running in cpu_idle
> -	 * loop which has been called at bootup from cpu_bringup_and_idle.
> -	 * The cpucpu_bringup_and_idle called cpu_bringup which made a
> -	 * preempt_disable() So this preempt_enable will balance it out.
> -	 */
> -	preempt_enable();
>  }
>  
>  #else /* !CONFIG_HOTPLUG_CPU */
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [Xen-devel] [PATCH 05/27] xen, cpu hotplug: Don't call cpu_bringup() in xen_play_dead()

[Srivatsa S. Bhat]

On 06/01/2012 06:29 PM, Jan Beulich wrote:

>>>> On 01.06.12 at 11:11, "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
> wrote:
>> xen_play_dead calls cpu_bringup() which looks weird, because xen_play_dead()
>> is invoked in the cpu down path, whereas cpu_bringup() (as the name 
>> suggests) is useful in the cpu bringup path.
> 
> This might not be correct - the code as it is without this change is
> safe even when the vCPU gets onlined back later by an external
> entity (e.g. the Xen tool stack), and it would in that case resume
> at the return point of the VCPUOP_down hypercall. That might
> be a heritage from the original XenoLinux tree though, and be
> meaningless in pv-ops context - Jeremy, Konrad?
> 
> Possibly it was bogus/unused even in that original tree - Keir?
>


Thanks for your comments Jan!

In case this change is wrong, the other method I had in mind was to call
cpu_bringup_and_idle() in xen_play_dead(). (Even ARM does something similar,
in the sense that it runs the cpu bringup code including cpu_idle(), in the
cpu offline path, namely the cpu_die() function). Would that approach work
for xen as well? If yes, then we wouldn't have any issues to convert xen to
generic code.

Regards,
Srivatsa S. Bhat

> 
>> Getting rid of xen_play_dead()'s dependency on cpu_bringup() helps in 
>> hooking on to the generic SMP booting framework.
>>
>> Also remove the extra call to preempt_enable() added by commit 41bd956
>> (xen/smp: Fix CPU online/offline bug triggering a BUG: scheduling while
>> atomic) because it becomes unnecessary after this change.
>>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: x86@kernel.org 
>> Cc: xen-devel@lists.xensource.com 
>> Cc: virtualization@lists.linux-foundation.org 
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> ---
>>
>>  arch/x86/xen/smp.c |    8 --------
>>  1 files changed, 0 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
>> index 09a7199..602d6b7 100644
>> --- a/arch/x86/xen/smp.c
>> +++ b/arch/x86/xen/smp.c
>> @@ -417,14 +417,6 @@ static void __cpuinit xen_play_dead(void) /* used only 
>> with HOTPLUG_CPU */
>>  {
>>  	play_dead_common();
>>  	HYPERVISOR_vcpu_op(VCPUOP_down, smp_processor_id(), NULL);
>> -	cpu_bringup();
>> -	/*
>> -	 * Balance out the preempt calls - as we are running in cpu_idle
>> -	 * loop which has been called at bootup from cpu_bringup_and_idle.
>> -	 * The cpucpu_bringup_and_idle called cpu_bringup which made a
>> -	 * preempt_disable() So this preempt_enable will balance it out.
>> -	 */
>> -	preempt_enable();
>>  }
>>  
>>  #else /* !CONFIG_HOTPLUG_CPU */
>>
>>

Re: [Xen-devel] [PATCH 05/27] xen, cpu hotplug: Don't call cpu_bringup() in xen_play_dead()

[Jan Beulich]

>>> On 01.06.12 at 17:13, "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
wrote:
> On 06/01/2012 06:29 PM, Jan Beulich wrote:
> 
>>>>> On 01.06.12 at 11:11, "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
>> wrote:
>>> xen_play_dead calls cpu_bringup() which looks weird, because xen_play_dead()
>>> is invoked in the cpu down path, whereas cpu_bringup() (as the name 
>>> suggests) is useful in the cpu bringup path.
>> 
>> This might not be correct - the code as it is without this change is
>> safe even when the vCPU gets onlined back later by an external
>> entity (e.g. the Xen tool stack), and it would in that case resume
>> at the return point of the VCPUOP_down hypercall. That might
>> be a heritage from the original XenoLinux tree though, and be
>> meaningless in pv-ops context - Jeremy, Konrad?
>> 
>> Possibly it was bogus/unused even in that original tree - Keir?
>>
> 
> 
> Thanks for your comments Jan!
> 
> In case this change is wrong, the other method I had in mind was to call
> cpu_bringup_and_idle() in xen_play_dead(). (Even ARM does something similar,
> in the sense that it runs the cpu bringup code including cpu_idle(), in the
> cpu offline path, namely the cpu_die() function). Would that approach work
> for xen as well? If yes, then we wouldn't have any issues to convert xen to
> generic code.

No, that wouldn't work either afaict - the function is expected
to return.

Jan

Re: [Xen-devel] [PATCH 05/27] xen, cpu hotplug: Don't call cpu_bringup() in xen_play_dead()

[Srivatsa S. Bhat]

On 06/01/2012 09:06 PM, Jan Beulich wrote:

>>>> On 01.06.12 at 17:13, "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
> wrote:
>> On 06/01/2012 06:29 PM, Jan Beulich wrote:
>>
>>>>>> On 01.06.12 at 11:11, "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
>>> wrote:
>>>> xen_play_dead calls cpu_bringup() which looks weird, because xen_play_dead()
>>>> is invoked in the cpu down path, whereas cpu_bringup() (as the name 
>>>> suggests) is useful in the cpu bringup path.
>>>
>>> This might not be correct - the code as it is without this change is
>>> safe even when the vCPU gets onlined back later by an external
>>> entity (e.g. the Xen tool stack), and it would in that case resume
>>> at the return point of the VCPUOP_down hypercall. That might
>>> be a heritage from the original XenoLinux tree though, and be
>>> meaningless in pv-ops context - Jeremy, Konrad?
>>>
>>> Possibly it was bogus/unused even in that original tree - Keir?
>>>
>>
>>
>> Thanks for your comments Jan!
>>
>> In case this change is wrong, the other method I had in mind was to call
>> cpu_bringup_and_idle() in xen_play_dead(). (Even ARM does something similar,
>> in the sense that it runs the cpu bringup code including cpu_idle(), in the
>> cpu offline path, namely the cpu_die() function). Would that approach work
>> for xen as well? If yes, then we wouldn't have any issues to convert xen to
>> generic code.
> 
> No, that wouldn't work either afaict - the function is expected
> to return.
> 


Ok.. So, I would love to hear a confirmation about whether this patch (which
removes cpu_bringup() in xen_play_dead()) will break things or it is good as is.

If its not correct, then we can probably make __cpu_post_online() return an int,
with the meaning:

0 => success, go ahead and call cpu_idle()
non-zero => stop here, thanks for your services so far.. now leave the rest to me.

So all other archs will return 0, Xen will return non-zero, and it will handle
when to call cpu_idle() and when not to do so.

Might sound a bit ugly, but I don't see much other option. Suggestions are
appreciated!

Regards,
Srivatsa S. Bhat

Re: [Xen-devel] [PATCH 05/27] xen, cpu hotplug: Don't call cpu_bringup() in xen_play_dead()

[Konrad Rzeszutek Wilk]

On Sat, Jun 02, 2012 at 11:36:00PM +0530, Srivatsa S. Bhat wrote:
> On 06/01/2012 09:06 PM, Jan Beulich wrote:
> 
> >>>> On 01.06.12 at 17:13, "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
> > wrote:
> >> On 06/01/2012 06:29 PM, Jan Beulich wrote:
> >>
> >>>>>> On 01.06.12 at 11:11, "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
> >>> wrote:
> >>>> xen_play_dead calls cpu_bringup() which looks weird, because xen_play_dead()
> >>>> is invoked in the cpu down path, whereas cpu_bringup() (as the name 
> >>>> suggests) is useful in the cpu bringup path.
> >>>
> >>> This might not be correct - the code as it is without this change is
> >>> safe even when the vCPU gets onlined back later by an external
> >>> entity (e.g. the Xen tool stack), and it would in that case resume
> >>> at the return point of the VCPUOP_down hypercall. That might
> >>> be a heritage from the original XenoLinux tree though, and be
> >>> meaningless in pv-ops context - Jeremy, Konrad?
> >>>
> >>> Possibly it was bogus/unused even in that original tree - Keir?
> >>>
> >>
> >>
> >> Thanks for your comments Jan!
> >>
> >> In case this change is wrong, the other method I had in mind was to call
> >> cpu_bringup_and_idle() in xen_play_dead(). (Even ARM does something similar,
> >> in the sense that it runs the cpu bringup code including cpu_idle(), in the
> >> cpu offline path, namely the cpu_die() function). Would that approach work
> >> for xen as well? If yes, then we wouldn't have any issues to convert xen to
> >> generic code.
> > 
> > No, that wouldn't work either afaict - the function is expected
> > to return.
> > 
> 
> 
> Ok.. So, I would love to hear a confirmation about whether this patch (which
> removes cpu_bringup() in xen_play_dead()) will break things or it is good as is.

I think it will break - are these patches available on some git tree to test them out?

> 
> If its not correct, then we can probably make __cpu_post_online() return an int,
> with the meaning:
> 
> 0 => success, go ahead and call cpu_idle()
> non-zero => stop here, thanks for your services so far.. now leave the rest to me.
> 
> So all other archs will return 0, Xen will return non-zero, and it will handle
> when to call cpu_idle() and when not to do so.

The call-chain (this is taken from 41bd956de3dfdc3a43708fe2e0c8096c69064a1e):

    cpu_bringup_and_idle:
     \- cpu_bringup
      |   \-[preempt_disable]
      |
      |- cpu_idle
           \- play_dead [assuming the user offlined the VCPU]
           |     \
           |     +- (xen_play_dead)
           |          \- HYPERVISOR_VCPU_off [so VCPU is dead, once user
           |          |                       onlines it starts from here]
           |          \- cpu_bringup [preempt_disable]
           |
           +- preempt_enable_no_reschedule()
           +- schedule()
           \- preempt_enable()

Which I think is a bit different from your use-case?

> 
> Might sound a bit ugly, but I don't see much other option. Suggestions are
> appreciated!
> 
> Regards,
> Srivatsa S. Bhat

Re: [Xen-devel] [PATCH 05/27] xen, cpu hotplug: Don't call cpu_bringup() in xen_play_dead()

[Srivatsa S. Bhat]

On 06/05/2012 10:19 PM, Konrad Rzeszutek Wilk wrote:

> On Sat, Jun 02, 2012 at 11:36:00PM +0530, Srivatsa S. Bhat wrote:
>> On 06/01/2012 09:06 PM, Jan Beulich wrote:
>>
>>>>>> On 01.06.12 at 17:13, "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
>>> wrote:
>>>> On 06/01/2012 06:29 PM, Jan Beulich wrote:
>>>>
>>>>>>>> On 01.06.12 at 11:11, "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
>>>>> wrote:
>>>>>> xen_play_dead calls cpu_bringup() which looks weird, because xen_play_dead()
>>>>>> is invoked in the cpu down path, whereas cpu_bringup() (as the name 
>>>>>> suggests) is useful in the cpu bringup path.
>>>>>
>>>>> This might not be correct - the code as it is without this change is
>>>>> safe even when the vCPU gets onlined back later by an external
>>>>> entity (e.g. the Xen tool stack), and it would in that case resume
>>>>> at the return point of the VCPUOP_down hypercall. That might
>>>>> be a heritage from the original XenoLinux tree though, and be
>>>>> meaningless in pv-ops context - Jeremy, Konrad?
>>>>>
>>>>> Possibly it was bogus/unused even in that original tree - Keir?
>>>>>
>>>>
>>>>
>>>> Thanks for your comments Jan!
>>>>
>>>> In case this change is wrong, the other method I had in mind was to call
>>>> cpu_bringup_and_idle() in xen_play_dead(). (Even ARM does something similar,
>>>> in the sense that it runs the cpu bringup code including cpu_idle(), in the
>>>> cpu offline path, namely the cpu_die() function). Would that approach work
>>>> for xen as well? If yes, then we wouldn't have any issues to convert xen to
>>>> generic code.
>>>
>>> No, that wouldn't work either afaict - the function is expected
>>> to return.
>>>
>>
>>
>> Ok.. So, I would love to hear a confirmation about whether this patch (which
>> removes cpu_bringup() in xen_play_dead()) will break things or it is good as is.
> 
> I think it will break - are these patches available on some git tree to test them out?
> 


Oh, sorry I haven't hosted them on any git tree.. 

>>
>> If its not correct, then we can probably make __cpu_post_online() return an int,
>> with the meaning:
>>
>> 0 => success, go ahead and call cpu_idle()
>> non-zero => stop here, thanks for your services so far.. now leave the rest to me.
>>
>> So all other archs will return 0, Xen will return non-zero, and it will handle
>> when to call cpu_idle() and when not to do so.
> 
> The call-chain (this is taken from 41bd956de3dfdc3a43708fe2e0c8096c69064a1e):
> 
>     cpu_bringup_and_idle:
>      \- cpu_bringup
>       |   \-[preempt_disable]
>       |
>       |- cpu_idle
>            \- play_dead [assuming the user offlined the VCPU]
>            |     \
>            |     +- (xen_play_dead)
>            |          \- HYPERVISOR_VCPU_off [so VCPU is dead, once user
>            |          |                       onlines it starts from here]
>            |          \- cpu_bringup [preempt_disable]
>            |
>            +- preempt_enable_no_reschedule()
>            +- schedule()
>            \- preempt_enable()
> 
> Which I think is a bit different from your use-case?
> 


Yes, when this patch is applied, the call to cpu_bringup() after HYPERVISOR_VCPU_off
gets removed. So it will look like this:

    cpu_bringup_and_idle:
     \- cpu_bringup
      |   \-[preempt_disable]
      |
      |- cpu_idle
           \- play_dead [assuming the user offlined the VCPU]
           |     \
           |     +- (xen_play_dead)
           |          \- HYPERVISOR_VCPU_off [so VCPU is dead, once user
           |                                  onlines it starts from here]
           |
           |
           +- preempt_enable_no_reschedule()
           +- schedule()
           \- preempt_enable()


And hence we wouldn't have the preempt imbalance, hence no need for the
extra preempt_enable() in xen_play_dead().

So, basically our problem is this:
The generic smp booting code calls cpu_idle() after setting the cpu in
cpu_online_mask etc, because this call to cpu_idle() is used in every
architecture. But just for xen, that too only in the cpu down path, this
call is omitted - which makes it difficult to convert xen to the generic
smp booting framework.

So I proposed 3 solutions, of which we can choose whichever that doesn't
break stuff and whichever looks the least ugly:

1. Omit the call to cpu_bringup() after HYPERVISOR_VCPU_off (which this
patch does).

2. Or, invoke cpu_bringup_and_idle() after HYPERVISOR_VCPU_off (Jan said
this might not work since we are expected to return). ARM actually does
something like this (it does the complete bringup including idle in the
cpu down path).

3. Or, Just for the sake of Xen, convert the __cpu_post_online() function to
return an int - Xen will return non-zero and do the next steps itself,
also deciding between whether or not to call cpu_idle(). All other archs
will just return 0, and allow the generic smp booting code to continue
on to cpu_idle().

Please let me know your thoughts.

Regards,
Srivatsa S. Bhat

Re: [Xen-devel] [PATCH 05/27] xen, cpu hotplug: Don't call cpu_bringup() in xen_play_dead()

[Thomas Gleixner]

On Sat, 2 Jun 2012, Srivatsa S. Bhat wrote:
> Ok.. So, I would love to hear a confirmation about whether this patch (which
> removes cpu_bringup() in xen_play_dead()) will break things or it is good as is.
> 
> If its not correct, then we can probably make __cpu_post_online() return an int,
> with the meaning:
> 
> 0 => success, go ahead and call cpu_idle()
> non-zero => stop here, thanks for your services so far.. now leave the rest to me.
> 
> So all other archs will return 0, Xen will return non-zero, and it will handle
> when to call cpu_idle() and when not to do so.
> 
> Might sound a bit ugly, but I don't see much other option. Suggestions are
> appreciated!

Yes, it's butt ugly. 

You are tripping over the main misconception of the current hotplug
code: It's asymetric.

So people added warts and workarounds like the xen one. What you are
proposing is another wart and workaround.

The real way to avoid it, is to have the symetric state machine in
place first and then convert everything to that instead of introducing
an intermediate state which resembles the existing state.

One of the main things we need to do to make it symetric is to kill
the play_dead() thing in the idle loop and make idle a function which
returns on cpu_should_die().

Give me a day or two and I get you a working version of that. (Up is
functional, just down refuses to play along)

Thanks,

	tglx

Re: [Xen-devel] [PATCH 05/27] xen, cpu hotplug: Don't call cpu_bringup() in xen_play_dead()

[Srivatsa S. Bhat]

On 06/05/2012 11:10 PM, Thomas Gleixner wrote:

> On Sat, 2 Jun 2012, Srivatsa S. Bhat wrote:
>> Ok.. So, I would love to hear a confirmation about whether this patch (which
>> removes cpu_bringup() in xen_play_dead()) will break things or it is good as is.
>>
>> If its not correct, then we can probably make __cpu_post_online() return an int,
>> with the meaning:
>>
>> 0 => success, go ahead and call cpu_idle()
>> non-zero => stop here, thanks for your services so far.. now leave the rest to me.
>>
>> So all other archs will return 0, Xen will return non-zero, and it will handle
>> when to call cpu_idle() and when not to do so.
>>
>> Might sound a bit ugly, but I don't see much other option. Suggestions are
>> appreciated!
> 
> Yes, it's butt ugly. 
> 
> You are tripping over the main misconception of the current hotplug
> code: It's asymetric.
> 
> So people added warts and workarounds like the xen one. What you are
> proposing is another wart and workaround.
> 
> The real way to avoid it, is to have the symetric state machine in
> place first and then convert everything to that instead of introducing
> an intermediate state which resembles the existing state.
> 
> One of the main things we need to do to make it symetric is to kill
> the play_dead() thing in the idle loop and make idle a function which
> returns on cpu_should_die().
> 
> Give me a day or two and I get you a working version of that. (Up is
> functional, just down refuses to play along)
> 


Oh great! So, then I'll wait for your patches and then adapt this patchset
to your model then. Let me know if I can help out with something..

Regards,
Srivatsa S. Bhat

[PATCH 06/27] xen, smpboot: Use generic SMP booting infrastructure

[Srivatsa S. Bhat]

Convert xen to use the generic framework to boot secondary CPUs.

Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: xen-devel@lists.xensource.com
Cc: virtualization@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 arch/x86/xen/smp.c |   21 ++++-----------------
 1 files changed, 4 insertions(+), 17 deletions(-)

diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index 602d6b7..46c96f9 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -58,13 +58,12 @@ static irqreturn_t xen_reschedule_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static void __cpuinit cpu_bringup(void)
+void __cpuinit xen_cpu_pre_starting(void *unused)
 {
 	int cpu;
 
 	cpu_init();
 	touch_softlockup_watchdog();
-	preempt_disable();
 
 	xen_enable_sysenter();
 	xen_enable_syscall();
@@ -75,25 +74,11 @@ static void __cpuinit cpu_bringup(void)
 	set_cpu_sibling_map(cpu);
 
 	xen_setup_cpu_clockevents();
-
-	notify_cpu_starting(cpu);
-
-	set_cpu_online(cpu, true);
-
-	this_cpu_write(cpu_state, CPU_ONLINE);
-
-	wmb();
-
-	/* We can take interrupts now: we're officially "up". */
-	local_irq_enable();
-
-	wmb();			/* make sure everything is out */
 }
 
 static void __cpuinit cpu_bringup_and_idle(void)
 {
-	cpu_bringup();
-	cpu_idle();
+	smpboot_start_secondary(NULL);
 }
 
 static int xen_smp_intr_init(unsigned int cpu)
@@ -515,6 +500,8 @@ static const struct smp_ops xen_smp_ops __initconst = {
 	.smp_prepare_cpus = xen_smp_prepare_cpus,
 	.smp_cpus_done = xen_smp_cpus_done,
 
+	.cpu_pre_starting = xen_cpu_pre_starting,
+
 	.cpu_up = xen_cpu_up,
 	.cpu_die = xen_cpu_die,
 	.cpu_disable = xen_cpu_disable,