[PATCH 0/8] Online/offline BSP on x86

[PATCH 0/8] Online/offline BSP on x86

[Fenghua Yu]

From: Fenghua Yu <fenghua.yu@intel.com>

BSP or CPU0 has been the last obstacle to CPU hotplug on x86. This patch set
implements BSP online and offline and removes this obstacle to CPU hotplug.

Fenghua Yu (8):
  x86, apic.c: Disable irq0 if CPU enables ARAT for local apic timer
  x86/mtrr/main.c: Ask the first online CPU to save mtrr
  x86, i387.c: thread xstate is initialized only on BSP once
  kernel/workqueue.c: unbound work queue rescuer runs on first cpu in
    cpumask_online_cpu
  x86, common.c, smpboot.c: Init BSP during BSP online and don't
    offline BSP if irq is bound to it
  x86, topology.c: Enable CPU0 online/offline
  kernel/power/main.c: Not suspend/resume if CPU0 is offlined
  kernel/cpu.c: Define bsp_hotpluggable variable

 Documentation/kernel-parameters.txt |    7 ++++++
 arch/x86/include/asm/processor.h    |    1 +
 arch/x86/kernel/apic/apic.c         |    5 ++++
 arch/x86/kernel/cpu/common.c        |   13 +++++++++--
 arch/x86/kernel/cpu/mtrr/main.c     |    6 +++-
 arch/x86/kernel/i387.c              |    9 +++++++-
 arch/x86/kernel/smpboot.c           |   38 ++++++++++++++++++++++++++++------
 arch/x86/kernel/topology.c          |   22 +++++++++++++------
 include/linux/cpu.h                 |    2 +
 kernel/cpu.c                        |    5 ++++
 kernel/power/main.c                 |    9 ++++++++
 kernel/workqueue.c                  |    4 +-
 12 files changed, 99 insertions(+), 22 deletions(-)

[PATCH 1/8] x86, apic.c: Disable irq0 if CPU enables ARAT for local apic timer

[Fenghua Yu]

From: Fenghua Yu <fenghua.yu@intel.com>

irq0 won't generate any interrupt after local apic timers is enabled and ARAT
is enabled. Disable irq0 in this case. Thus irq0 won't block BSP offline.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/kernel/apic/apic.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 52fa563..8813acf 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -506,6 +506,7 @@ static DEFINE_PER_CPU(struct clock_event_device, lapic_events);
 static void __cpuinit setup_APIC_timer(void)
 {
 	struct clock_event_device *levt = &__get_cpu_var(lapic_events);
+	int cpu = smp_processor_id();
 
 	if (this_cpu_has(X86_FEATURE_ARAT)) {
 		lapic_clockevent.features &= ~CLOCK_EVT_FEAT_C3STOP;
@@ -517,6 +518,10 @@ static void __cpuinit setup_APIC_timer(void)
 	levt->cpumask = cpumask_of(smp_processor_id());
 
 	clockevents_register_device(levt);
+
+	/* irq0 won't be used any more if CPU supports ARAT feature. */
+	if (cpu == 0 && this_cpu_has(X86_FEATURE_ARAT))
+		disable_irq(0);
 }
 
 /*
-- 
1.6.0.3

Re: [PATCH 1/8] x86, apic.c: Disable irq0 if CPU enables ARAT for local apic timer

[Thomas Gleixner]

On Wed, 5 Oct 2011, Fenghua Yu wrote:

> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> irq0 won't generate any interrupt after local apic timers is enabled and ARAT
> is enabled. Disable irq0 in this case. Thus irq0 won't block BSP offline.

Why would it do so ?
 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  arch/x86/kernel/apic/apic.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 52fa563..8813acf 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -506,6 +506,7 @@ static DEFINE_PER_CPU(struct clock_event_device, lapic_events);
>  static void __cpuinit setup_APIC_timer(void)
>  {
>  	struct clock_event_device *levt = &__get_cpu_var(lapic_events);
> +	int cpu = smp_processor_id();
>  
>  	if (this_cpu_has(X86_FEATURE_ARAT)) {
>  		lapic_clockevent.features &= ~CLOCK_EVT_FEAT_C3STOP;
> @@ -517,6 +518,10 @@ static void __cpuinit setup_APIC_timer(void)
>  	levt->cpumask = cpumask_of(smp_processor_id());
>  
>  	clockevents_register_device(levt);
> +
> +	/* irq0 won't be used any more if CPU supports ARAT feature. */
> +	if (cpu == 0 && this_cpu_has(X86_FEATURE_ARAT))
> +		disable_irq(0);

This is completely wrong. If we want to shut that interrupt down, then
we do it in the clockevents set mode functions of PIT or HPET and not
at some random place in the apic timer code.

Thanks,

	tglx

RE: [PATCH 1/8] x86, apic.c: Disable irq0 if CPU enables ARAT for local apic timer

[Yu, Fenghua]

> 
> > From: Fenghua Yu <fenghua.yu@intel.com>
> >
> > irq0 won't generate any interrupt after local apic timers is enabled
> and ARAT
> > is enabled. Disable irq0 in this case. Thus irq0 won't block BSP
> offline.
> 
> Why would it do so ?
Irq0 is set as IRQF_NOBALANCING. And it's not used any more after boot time if CPU using local apic timer supports ARAT. Although irq0 is useless, it blocks CPU0 offline.

That's why we need to treat irq0 specially for CPU0 offline.

> > +
> > +	/* irq0 won't be used any more if CPU supports ARAT feature. */
> > +	if (cpu == 0 && this_cpu_has(X86_FEATURE_ARAT))
> > +		disable_irq(0);
> 
> This is completely wrong. If we want to shut that interrupt down, then
> we do it in the clockevents set mode functions of PIT or HPET and not
> at some random place in the apic timer code.

Agree with you.

Or my original irq0 handling is just ignoring irq0 when ARAT is enabled during CPU0 offline procedure. Is this way cleaner and limited to CPU0 offline path? I'm afraid shutting that interrupt down or disabling it may cause any other (legacy) issue?

Thanks.

-Fenghua

RE: [PATCH 1/8] x86, apic.c: Disable irq0 if CPU enables ARAT for local apic timer

[Thomas Gleixner]

On Wed, 5 Oct 2011, Yu, Fenghua wrote:

Please remove Zwane Mwaikambo <zwane@arm.linux.org.uk> from the cc
list, that address does not exist anymore.

> > 
> > > From: Fenghua Yu <fenghua.yu@intel.com>
> > >
> > > irq0 won't generate any interrupt after local apic timers is enabled
> > and ARAT
> > > is enabled. Disable irq0 in this case. Thus irq0 won't block BSP
> > offline.
> > 
> > Why would it do so ?

> Irq0 is set as IRQF_NOBALANCING. And it's not used any more after
> boot time if CPU using local apic timer supports ARAT. Although irq0
> is useless, it blocks CPU0 offline.

Please use proper line breaks around 78

> That's why we need to treat irq0 specially for CPU0 offline.

That's utter nonsense. Your whole approach is broken. irq0 is not
special at all and any other interrupt could be marked
IRQF_NOBALANCING as well.

Special casing stuff is always a sign of bandaids and your whole patch
set is just a big cobbled together duct tape thing.
 
> > > +
> > > +	/* irq0 won't be used any more if CPU supports ARAT feature. */
> > > +	if (cpu == 0 && this_cpu_has(X86_FEATURE_ARAT))
> > > +		disable_irq(0);
> > 
> > This is completely wrong. If we want to shut that interrupt down, then
> > we do it in the clockevents set mode functions of PIT or HPET and not
> > at some random place in the apic timer code.
> 
> Agree with you.
> 
> Or my original irq0 handling is just ignoring irq0 when ARAT is
> enabled during CPU0 offline procedure. Is this way cleaner and
> limited to CPU0 offline path? I'm afraid shutting that interrupt
> down or disabling it may cause any other (legacy) issue?

That's still wrong and your offline check code is broken beyond repair
anyway.

Thanks,

	tglx

RE: [PATCH 1/8] x86, apic.c: Disable irq0 if CPU enables ARAT for local apic timer

[Yu, Fenghua]

timer
> 
> On Wed, 5 Oct 2011, Yu, Fenghua wrote:
> 
> Please remove Zwane Mwaikambo <zwane@arm.linux.org.uk> from the cc
> list, that address does not exist anymore.

OK. I'll remove his address from the list.

I just got Zwane's address from MAINTAINERS. So his info should be fixed in MAINTAINERS.

SC1200 WDT DRIVER
M:      Zwane Mwaikambo <zwane@arm.linux.org.uk>
S:      Maintained
F:      drivers/watchdog/sc1200wdt.c

I was hoping Zwane knows which PCI quirks depends on CPU0.

Thanks.

-Fenghua

Re: [PATCH 1/8] x86, apic.c: Disable irq0 if CPU enables ARAT for local apic timer

[Andi Kleen]

"Yu, Fenghua" <fenghua.yu@intel.com> writes:
>
> SC1200 WDT DRIVER
> M:      Zwane Mwaikambo <zwane@arm.linux.org.uk>
> S:      Maintained
> F:      drivers/watchdog/sc1200wdt.c
>
> I was hoping Zwane knows which PCI quirks depends on CPU0.

At least one AMD SIS chipset relied on IRQ0 always being on CPU0 
Not sure we got a quirk for it because the existing code handled it
(I guess it's reasonable to just blacklist for all of SIS,
i don't think they ever did anything multi-socket)
Alan may remember more.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

[PATCH 2/8] x86/mtrr/main.c: Ask the first online CPU to save mtrr

[Fenghua Yu]

From: Fenghua Yu <fenghua.yu@intel.com>

Ask the first online CPU to save mtrr instead of asking BSP. BSP can be
offlined when mtrr_save_state() is called.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/kernel/cpu/mtrr/main.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c
index 6b96110..e18a4d3 100644
--- a/arch/x86/kernel/cpu/mtrr/main.c
+++ b/arch/x86/kernel/cpu/mtrr/main.c
@@ -695,11 +695,13 @@ void mtrr_ap_init(void)
 }
 
 /**
- * Save current fixed-range MTRR state of the BSP
+ * Save current fixed-range MTRR state of the first cpu in cpu_online_mask.
  */
 void mtrr_save_state(void)
 {
-	smp_call_function_single(0, mtrr_save_fixed_ranges, NULL, 1);
+	int first_cpu = cpumask_first(cpu_online_mask);
+
+	smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
 }
 
 void set_mtrr_aps_delayed_init(void)
-- 
1.6.0.3

Re: [PATCH 2/8] x86/mtrr/main.c: Ask the first online CPU to save mtrr

[Srivatsa S. Bhat]

Hi,

On 10/05/2011 10:09 PM, Fenghua Yu wrote:
> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> Ask the first online CPU to save mtrr instead of asking BSP. BSP can be
> offlined when mtrr_save_state() is called.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  arch/x86/kernel/cpu/mtrr/main.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c
> index 6b96110..e18a4d3 100644
> --- a/arch/x86/kernel/cpu/mtrr/main.c
> +++ b/arch/x86/kernel/cpu/mtrr/main.c
> @@ -695,11 +695,13 @@ void mtrr_ap_init(void)
>  }
> 
>  /**
> - * Save current fixed-range MTRR state of the BSP
> + * Save current fixed-range MTRR state of the first cpu in cpu_online_mask.
>   */
>  void mtrr_save_state(void)
>  {
> -	smp_call_function_single(0, mtrr_save_fixed_ranges, NULL, 1);
> +	int first_cpu = cpumask_first(cpu_online_mask);

There is a race condition here as well. What if, at this point, the cpu
represented by first_cpu is taken offline (due to a cpu hotplug operation)?

> +	smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
>  }
> 
>  void set_mtrr_aps_delayed_init(void)


-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@linux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab

[PATCH 3/8] x86, i387.c: thread xstate is initialized only on BSP once

[Fenghua Yu]

From: Fenghua Yu <fenghua.yu@intel.com>

init_thread_xstate() is only called on BSP once to avoid to override
xstate_size.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/kernel/i387.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 739d859..de2b48a 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -93,6 +93,7 @@ void __cpuinit fpu_init(void)
 {
 	unsigned long cr0;
 	unsigned long cr4_mask = 0;
+	static int once;
 
 	if (cpu_has_fxsr)
 		cr4_mask |= X86_CR4_OSFXSR;
@@ -107,8 +108,14 @@ void __cpuinit fpu_init(void)
 		cr0 |= X86_CR0_EM;
 	write_cr0(cr0);
 
-	if (!smp_processor_id())
+	/*
+	 * init_thread_xstate is only called on BSP once to avoid to override
+	 * xstate_size.
+	 */
+	if (!smp_processor_id() && once) {
+		once = 0;
 		init_thread_xstate();
+	}
 
 	mxcsr_feature_mask_init();
 	/* clean state in init */
-- 
1.6.0.3

Re: [PATCH 3/8] x86, i387.c: thread xstate is initialized only on BSP once

[Thomas Gleixner]

On Wed, 5 Oct 2011, Fenghua Yu wrote:

> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> init_thread_xstate() is only called on BSP once to avoid to override
> xstate_size.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  arch/x86/kernel/i387.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> index 739d859..de2b48a 100644
> --- a/arch/x86/kernel/i387.c
> +++ b/arch/x86/kernel/i387.c
> @@ -93,6 +93,7 @@ void __cpuinit fpu_init(void)
>  {
>  	unsigned long cr0;
>  	unsigned long cr4_mask = 0;
> +	static int once;
>  
>  	if (cpu_has_fxsr)
>  		cr4_mask |= X86_CR4_OSFXSR;
> @@ -107,8 +108,14 @@ void __cpuinit fpu_init(void)
>  		cr0 |= X86_CR0_EM;
>  	write_cr0(cr0);
>  
> -	if (!smp_processor_id())
> +	/*
> +	 * init_thread_xstate is only called on BSP once to avoid to override
> +	 * xstate_size.
> +	 */
> +	if (!smp_processor_id() && once) {

Brilliant change. Makes sure that init_thread_xstate() is NEVER
called. Was this crap tested at all?

> +		once = 0;
>  		init_thread_xstate();
> +	}
>  
>  	mxcsr_feature_mask_init();
>  	/* clean state in init */
> -- 
> 1.6.0.3
> 
> 

RE: [PATCH 3/8] x86, i387.c: thread xstate is initialized only on BSP once

[Yu, Fenghua]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Wednesday, October 05, 2011 11:50 AM
> To: Yu, Fenghua
> Cc: Ingo Molnar; H Peter Anvin; Zwane Mwaikambo; Luck, Tony; Mallick,
> Asit K; Siddha, Suresh B; Len Brown; linux-kernel
> Subject: Re: [PATCH 3/8] x86, i387.c: thread xstate is initialized only
> on BSP once
> 
> On Wed, 5 Oct 2011, Fenghua Yu wrote:
> 
> > From: Fenghua Yu <fenghua.yu@intel.com>
> >
> > init_thread_xstate() is only called on BSP once to avoid to override
> > xstate_size.
> >
> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > ---
> >  arch/x86/kernel/i387.c |    9 ++++++++-
> >  1 files changed, 8 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> > index 739d859..de2b48a 100644
> > --- a/arch/x86/kernel/i387.c
> > +++ b/arch/x86/kernel/i387.c
> > @@ -93,6 +93,7 @@ void __cpuinit fpu_init(void)
> >  {
> >  	unsigned long cr0;
> >  	unsigned long cr4_mask = 0;
> > +	static int once;
> >
> >  	if (cpu_has_fxsr)
> >  		cr4_mask |= X86_CR4_OSFXSR;
> > @@ -107,8 +108,14 @@ void __cpuinit fpu_init(void)
> >  		cr0 |= X86_CR0_EM;
> >  	write_cr0(cr0);
> >
> > -	if (!smp_processor_id())
> > +	/*
> > +	 * init_thread_xstate is only called on BSP once to avoid to
> override
> > +	 * xstate_size.
> > +	 */
> > +	if (!smp_processor_id() && once) {
> 
> Brilliant change. Makes sure that init_thread_xstate() is NEVER
> called. Was this crap tested at all?

Ooh, "once" should be initialized as 1.

The code passes stress test with CPU0 online/offline, though...xstate_size will be re-initialized/overriden in xstate_enable_boot_cpu() later. So not calling init_thread_xstate() doesn't cause problem in my situation (where HAVE_HWFP=1).

Anyway, this patch is wrong. I'll fix it in next version.

Thanks.

-Fenghua

[PATCH 4/8] kernel/workqueue.c: unbound work queue rescuer runs on first cpu in cpumask_online_cpu

[Fenghua Yu]

From: Fenghua Yu <fenghua.yu@intel.com>

If work queue is unbound to a specific cpu, run rescuer on first cpu in
cpumask_online_cpu instead BSP which could be offlined.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 kernel/workqueue.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 1783aab..d26e411 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1496,9 +1496,9 @@ static bool send_mayday(struct work_struct *work)
 
 	/* mayday mayday mayday */
 	cpu = cwq->gcwq->cpu;
-	/* WORK_CPU_UNBOUND can't be set in cpumask, use cpu 0 instead */
+	/* WORK_CPU_UNBOUND can't be set in cpumask, use first cpu instead */
 	if (cpu == WORK_CPU_UNBOUND)
-		cpu = 0;
+		cpu = cpumask_first(cpu_online_mask);
 	if (!mayday_test_and_set_cpu(cpu, wq->mayday_mask))
 		wake_up_process(wq->rescuer->task);
 	return true;
-- 
1.6.0.3

Re: [PATCH 4/8] kernel/workqueue.c: unbound work queue rescuer runs on first cpu in cpumask_online_cpu

[Thomas Gleixner]

On Wed, 5 Oct 2011, Fenghua Yu wrote:

> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> If work queue is unbound to a specific cpu, run rescuer on first cpu in
> cpumask_online_cpu instead BSP which could be offlined.

This patch needs to be applied separate from that series as it fixes a
bug which affects all architectures which can offline the boot cpu.

Thanks,

	tglx

 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  kernel/workqueue.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 1783aab..d26e411 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -1496,9 +1496,9 @@ static bool send_mayday(struct work_struct *work)
>  
>  	/* mayday mayday mayday */
>  	cpu = cwq->gcwq->cpu;
> -	/* WORK_CPU_UNBOUND can't be set in cpumask, use cpu 0 instead */
> +	/* WORK_CPU_UNBOUND can't be set in cpumask, use first cpu instead */
>  	if (cpu == WORK_CPU_UNBOUND)
> -		cpu = 0;
> +		cpu = cpumask_first(cpu_online_mask);
>  	if (!mayday_test_and_set_cpu(cpu, wq->mayday_mask))
>  		wake_up_process(wq->rescuer->task);
>  	return true;
> -- 
> 1.6.0.3
> 
> 

Re: [PATCH 4/8] kernel/workqueue.c: unbound work queue rescuer runs on first cpu in cpumask_online_cpu

[Peter Zijlstra]

On Wed, 2011-10-05 at 20:52 +0200, Thomas Gleixner wrote:
> On Wed, 5 Oct 2011, Fenghua Yu wrote:
> 
> > From: Fenghua Yu <fenghua.yu@intel.com>
> > 
> > If work queue is unbound to a specific cpu, run rescuer on first cpu in
> > cpumask_online_cpu instead BSP which could be offlined.
> 
> This patch needs to be applied separate from that series as it fixes a
> bug which affects all architectures which can offline the boot cpu.

AFAIKT it really doesn't matter in this case, all the rescuer_thread()
needs is some bit set in that mask, since its UNBOUND it won't actually
try and migrate to any cpu and just run wherever it woke up.

Re: [PATCH 4/8] kernel/workqueue.c: unbound work queue rescuer runs on first cpu in cpumask_online_cpu

[Tejun Heo]

On Wed, Oct 05, 2011 at 09:19:22PM +0200, Peter Zijlstra wrote:
> On Wed, 2011-10-05 at 20:52 +0200, Thomas Gleixner wrote:
> > On Wed, 5 Oct 2011, Fenghua Yu wrote:
> > 
> > > From: Fenghua Yu <fenghua.yu@intel.com>
> > > 
> > > If work queue is unbound to a specific cpu, run rescuer on first cpu in
> > > cpumask_online_cpu instead BSP which could be offlined.
> > 
> > This patch needs to be applied separate from that series as it fixes a
> > bug which affects all architectures which can offline the boot cpu.
> 
> AFAIKT it really doesn't matter in this case, all the rescuer_thread()
> needs is some bit set in that mask, since its UNBOUND it won't actually
> try and migrate to any cpu and just run wherever it woke up.

Yeah, for unbound, it's just a random fixed ID.  Changing it
dynamically wouldn't break anything but is pointless.  So, NACK.

Thanks.

-- 
tejun

[PATCH 5/8] x86, common.c, smpboot.c: Init BSP during BSP online and don't offline BSP if irq is bound to it

[Fenghua Yu]

From: Fenghua Yu <fenghua.yu@intel.com>

During BSP online, enable x2apic and initialize BSP. Don't offline BSP if
any irq is bound to it.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/include/asm/processor.h |    1 +
 arch/x86/kernel/cpu/common.c     |   13 ++++++++++---
 arch/x86/kernel/smpboot.c        |   38 +++++++++++++++++++++++++++++++-------
 3 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 0d1171c..d648cae 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -161,6 +161,7 @@ extern struct pt_regs *idle_regs(struct pt_regs *);
 
 extern void early_cpu_init(void);
 extern void identify_boot_cpu(void);
+extern void identify_boot_cpu_online(void);
 extern void identify_secondary_cpu(struct cpuinfo_x86 *);
 extern void print_cpu_info(struct cpuinfo_x86 *);
 extern void init_scattered_cpuid_features(struct cpuinfo_x86 *c);
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 6218439..2ceefb9 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -911,6 +911,14 @@ void __init identify_boot_cpu(void)
 #endif
 }
 
+void __cpuinit identify_boot_cpu_online(void)
+{
+	numa_add_cpu(smp_processor_id());
+#ifdef CONFIG_X86_32
+	enable_sep_cpu();
+#endif
+}
+
 void __cpuinit identify_secondary_cpu(struct cpuinfo_x86 *c)
 {
 	BUG_ON(c == &boot_cpu_data);
@@ -1158,7 +1166,7 @@ void __cpuinit cpu_init(void)
 	oist = &per_cpu(orig_ist, cpu);
 
 #ifdef CONFIG_NUMA
-	if (cpu != 0 && percpu_read(numa_node) == 0 &&
+	if (percpu_read(numa_node) == 0 &&
 	    early_cpu_to_node(cpu) != NUMA_NO_NODE)
 		set_numa_node(early_cpu_to_node(cpu));
 #endif
@@ -1190,8 +1198,7 @@ void __cpuinit cpu_init(void)
 	barrier();
 
 	x86_configure_nx();
-	if (cpu != 0)
-		enable_x2apic();
+	enable_x2apic();
 
 	/*
 	 * set up and load the per-CPU TSS
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 9f548cb..cdfa425 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -50,6 +50,7 @@
 #include <linux/tboot.h>
 #include <linux/stackprotector.h>
 #include <linux/gfp.h>
+#include <linux/kernel_stat.h>
 
 #include <asm/acpi.h>
 #include <asm/desc.h>
@@ -136,8 +137,8 @@ EXPORT_PER_CPU_SYMBOL(cpu_info);
 atomic_t init_deasserted;
 
 /*
- * Report back to the Boot Processor.
- * Running on AP.
+ * Report back to the Boot Processor during boot time or to the caller processor
+ * during CPU online.
  */
 static void __cpuinit smp_callin(void)
 {
@@ -224,6 +225,13 @@ static void __cpuinit smp_callin(void)
 	smp_store_cpu_info(cpuid);
 
 	/*
+	 * This function won't run on the BSP during boot time. It run
+	 * on BSP only when BSP is offlined and onlined again.
+	 */
+	if (cpuid == 0)
+		identify_boot_cpu_online();
+
+	/*
 	 * This must be done before setting cpu_online_mask
 	 * or calling notify_cpu_starting.
 	 */
@@ -839,7 +847,7 @@ int __cpuinit native_cpu_up(unsigned int cpu)
 
 	pr_debug("++++++++++++++++++++=_---CPU UP  %u\n", cpu);
 
-	if (apicid == BAD_APICID || apicid == boot_cpu_physical_apicid ||
+	if (apicid == BAD_APICID ||
 	    !physid_isset(apicid, phys_cpu_present_map)) {
 		printk(KERN_ERR "%s: bad cpu %d\n", __func__, cpu);
 		return -EINVAL;
@@ -1283,12 +1291,28 @@ int native_cpu_disable(void)
 	 * Perhaps use cpufreq to drop frequency, but that could go
 	 * into generic code.
 	 *
-	 * We won't take down the boot processor on i386 due to some
+	 * We won't take down the boot processor on i386 if some
 	 * interrupts only being able to be serviced by the BSP.
-	 * Especially so if we're not using an IOAPIC	-zwane
+	 * Especially so if we're not using an IOAPIC
+	 * -original comment from zwane, changed by fyu
 	 */
-	if (cpu == 0)
-		return -EBUSY;
+	if (cpu == 0) {
+		int i;
+		for (i = 0; i < NR_IRQS; i++) {
+			struct irq_desc *desc = irq_to_desc(i);
+
+			if (!desc)
+				continue;
+
+			if (irqd_irq_disabled(&desc->irq_data))
+				continue;
+
+			if (!irq_can_set_affinity(i)) {
+				pr_debug("irq%d can't move out of BSP\n", i);
+				return -EBUSY;
+			}
+		}
+	}
 
 	clear_local_APIC();
 
-- 
1.6.0.3

Re: [PATCH 5/8] x86, common.c, smpboot.c: Init BSP during BSP online and don't offline BSP if irq is bound to it

[Thomas Gleixner]

On Wed, 5 Oct 2011, Fenghua Yu wrote:

> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> During BSP online, enable x2apic and initialize BSP. Don't offline BSP if
> any irq is bound to it.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  arch/x86/include/asm/processor.h |    1 +
>  arch/x86/kernel/cpu/common.c     |   13 ++++++++++---
>  arch/x86/kernel/smpboot.c        |   38 +++++++++++++++++++++++++++++++-------
>  3 files changed, 42 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 0d1171c..d648cae 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -161,6 +161,7 @@ extern struct pt_regs *idle_regs(struct pt_regs *);
>  
>  extern void early_cpu_init(void);
>  extern void identify_boot_cpu(void);
> +extern void identify_boot_cpu_online(void);
>  extern void identify_secondary_cpu(struct cpuinfo_x86 *);
>  extern void print_cpu_info(struct cpuinfo_x86 *);
>  extern void init_scattered_cpuid_features(struct cpuinfo_x86 *c);
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 6218439..2ceefb9 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -911,6 +911,14 @@ void __init identify_boot_cpu(void)
>  #endif
>  }
>  
> +void __cpuinit identify_boot_cpu_online(void)
> +{
> +	numa_add_cpu(smp_processor_id());
> +#ifdef CONFIG_X86_32
> +	enable_sep_cpu();
> +#endif

Why this change? This has nothing to do with the changelog. And we
have 3 other call sites for enable_sep_cpu().

> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 9f548cb..cdfa425 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -50,6 +50,7 @@
>  #include <linux/tboot.h>
>  #include <linux/stackprotector.h>
>  #include <linux/gfp.h>
> +#include <linux/kernel_stat.h>

What is this include for?
  
>  #include <asm/acpi.h>
>  #include <asm/desc.h>
> @@ -136,8 +137,8 @@ EXPORT_PER_CPU_SYMBOL(cpu_info);
>  atomic_t init_deasserted;
>  
>  /*
> - * Report back to the Boot Processor.
> - * Running on AP.
> + * Report back to the Boot Processor during boot time or to the caller processor
> + * during CPU online.
>   */
>  static void __cpuinit smp_callin(void)
>  {
> @@ -224,6 +225,13 @@ static void __cpuinit smp_callin(void)
>  	smp_store_cpu_info(cpuid);
>  
>  	/*
> +	 * This function won't run on the BSP during boot time. It run
> +	 * on BSP only when BSP is offlined and onlined again.
> +	 */
> +	if (cpuid == 0)
> +		identify_boot_cpu_online();

Again, what's the point? numa_add_cpu() is called in identify_cpu()
already and you just added that crap here because you failed to fix
smp_store_cpu_info(). That whole patch set is just a sloppy hack which
leaves tons of cpu0 assumptions all over the place instead of cleaning
them up completely.

> +	if (cpu == 0) {
> +		int i;
> +		for (i = 0; i < NR_IRQS; i++) {
> +			struct irq_desc *desc = irq_to_desc(i);
> +
> +			if (!desc)
> +				continue;
> +
> +			if (irqd_irq_disabled(&desc->irq_data))
> +				continue;
> +
> +			if (!irq_can_set_affinity(i)) {
> +				pr_debug("irq%d can't move out of BSP\n", i);
> +				return -EBUSY;
> +			}

This is just disgusting.

Have you ever looked at other code which iterates over the interrupt
descriptors and if yes have you noticed that all that code uses
iterator functions for a fcking good reason? Hint SPARSE_IRQ

What gives you the guarantee that the interrupt is not going to be
enabled right after you offlined the cpu? Do you really think that
chekcing whether the interrupt is disabled is sufficient ????

Is there any guarantee, that an interrupt which is currently not
assigned is going to be requested after you offlined cpu0 ?

We know upfront whether we are using interrupt chips which are not
capable of irq affinity settings. So why the hell do you want to poke
in the interrupt descriptors?

Thanks,

	tglx

[PATCH 6/8] x86, topology.c: Enable CPU0 online/offline

[Fenghua Yu]

From: Fenghua Yu <fenghua.yu@intel.com>

Enable CPU0 online/offline by setting up its sysfs control file. By default,
this feature is disabled. You can enabled it by bsp_hotplug kernel option.

Resume from hibernate, standby, or suspend needs CPU0 online. According to
arch/x86/kernel/topology.c, some PCI quirks(??) need CPU0 online as well. Anyone
knows which PCI quirks depends on CPU0 online?

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 Documentation/kernel-parameters.txt |    7 +++++++
 arch/x86/kernel/topology.c          |   22 +++++++++++++++-------
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 854ed5c..bf011ee 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1805,6 +1805,13 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 	nox2apic	[X86-64,APIC] Do not enable x2APIC mode.
 
+	bsp_hotplug	[X86] BSP (aka CPU0) is hotpluggable.
+			Suspend/resume depends on BSP. It's said some PCI
+			quirks depend on BSP too. But not sure which quirks.
+			If don't care the dependencies, you can turn on
+			bsp_ hotplug. Suspend will fail if BSP is offlined and
+			you need to online BSP before suspend/resume.
+
 	nptcg=		[IA-64] Override max number of concurrent global TLB
 			purges which is reported from either PAL_VM_SUMMARY or
 			SAL PALO.
diff --git a/arch/x86/kernel/topology.c b/arch/x86/kernel/topology.c
index 8927486..ed2da5c 100644
--- a/arch/x86/kernel/topology.c
+++ b/arch/x86/kernel/topology.c
@@ -34,18 +34,26 @@
 static DEFINE_PER_CPU(struct x86_cpu, cpu_devices);
 
 #ifdef CONFIG_HOTPLUG_CPU
+
+static int __init enable_bsp_hotplug(char *str)
+{
+	bsp_hotpluggable = 1;
+	return 0;
+}
+
+__setup("bsp_hotplug", enable_bsp_hotplug);
+
 int __ref arch_register_cpu(int num)
 {
 	/*
-	 * CPU0 cannot be offlined due to several
-	 * restrictions and assumptions in kernel. This basically
-	 * doesn't add a control file, one cannot attempt to offline
-	 * BSP.
+	 * Suspend/resume depends on BSP. Certain PCI quirks (which PCI
+	 * quirks??) require not to enable hotplug control for all CPU's.
 	 *
-	 * Also certain PCI quirks require not to enable hotplug control
-	 * for all CPU's.
+	 * If one don't care those BSP depencies, tell kernel to
+	 * enable BSP hotplug. This basically adds a control file and
+	 * one can attempt to offline BSP.
 	 */
-	if (num)
+	if (num || bsp_hotpluggable)
 		per_cpu(cpu_devices, num).cpu.hotpluggable = 1;
 
 	return register_cpu(&per_cpu(cpu_devices, num).cpu, num);
-- 
1.6.0.3

Re: [PATCH 6/8] x86, topology.c: Enable CPU0 online/offline

[Thomas Gleixner]

On Wed, 5 Oct 2011, Fenghua Yu wrote:

> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> Enable CPU0 online/offline by setting up its sysfs control file. By default,
> this feature is disabled. You can enabled it by bsp_hotplug kernel option.
> 
> Resume from hibernate, standby, or suspend needs CPU0 online. According to
> arch/x86/kernel/topology.c, some PCI quirks(??) need CPU0 online as well. Anyone
> knows which PCI quirks depends on CPU0 online?
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  Documentation/kernel-parameters.txt |    7 +++++++
>  arch/x86/kernel/topology.c          |   22 +++++++++++++++-------
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 854ed5c..bf011ee 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1805,6 +1805,13 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  
>  	nox2apic	[X86-64,APIC] Do not enable x2APIC mode.
>  
> +	bsp_hotplug	[X86] BSP (aka CPU0) is hotpluggable.
> +			Suspend/resume depends on BSP. It's said some PCI
> +			quirks depend on BSP too. But not sure which quirks.

Right, not sure what breaks. Go ahead and watch your machine explode.

> +			If don't care the dependencies, you can turn on
> +			bsp_ hotplug. Suspend will fail if BSP is offlined and
> +			you need to online BSP before suspend/resume.

And you forgot poweroff and reboot, which have similar dependencies on
some machines. That whole low level ACPI stuff is sensitive.

> +
>  	nptcg=		[IA-64] Override max number of concurrent global TLB
>  			purges which is reported from either PAL_VM_SUMMARY or
>  			SAL PALO.
> diff --git a/arch/x86/kernel/topology.c b/arch/x86/kernel/topology.c
> index 8927486..ed2da5c 100644
> --- a/arch/x86/kernel/topology.c
> +++ b/arch/x86/kernel/topology.c
> @@ -34,18 +34,26 @@
>  static DEFINE_PER_CPU(struct x86_cpu, cpu_devices);
>  
>  #ifdef CONFIG_HOTPLUG_CPU
> +
> +static int __init enable_bsp_hotplug(char *str)
> +{
> +	bsp_hotpluggable = 1;

And that variable is defined where? I know where in the following
patch, so it's clear that series was neither compiled step per step
nor booted.

> +	 * Suspend/resume depends on BSP. Certain PCI quirks (which PCI
> +	 * quirks??) require not to enable hotplug control for all CPU's.

Again, you dammit better make shure which quirks break when cpu0 is
offlined.

Thanks,

	tglx

RE: [PATCH 6/8] x86, topology.c: Enable CPU0 online/offline

[Yu, Fenghua]

> 
> > +	 * Suspend/resume depends on BSP. Certain PCI quirks (which PCI
> > +	 * quirks??) require not to enable hotplug control for all CPU's.
> 
> Again, you dammit better make shure which quirks break when cpu0 is
> offlined.
> 

Maybe I misunderstood the comment in arch/x86/kernel/topology.c

+        * Also certain PCI quirks require not to enable hotplug control
+        * for all CPU's.
+        */

and commit b0d0a4ba45760b10ecee9035ed45b442c1a6cc84

So there is NO PCI quirks dependency on CPU0 hotplug.

I'm going to send out a new cpu0 online/offline patch set after collecting all the comments so far.

Thanks.

-Fenghua

RE: [PATCH 6/8] x86, topology.c: Enable CPU0 online/offline

[Yu, Fenghua]

> > +	bsp_hotplug	[X86] BSP (aka CPU0) is hotpluggable.
> > +			Suspend/resume depends on BSP. It's said some PCI
> > +			quirks depend on BSP too. But not sure which quirks.
> 
> Right, not sure what breaks. Go ahead and watch your machine explode.
> 
> > +			If don't care the dependencies, you can turn on
> > +			bsp_ hotplug. Suspend will fail if BSP is offlined
> and
> > +			you need to online BSP before suspend/resume.
> 
> And you forgot poweroff and reboot, which have similar dependencies on
> some machines. That whole low level ACPI stuff is sensitive.

I tested poweroff, shutdown, and reboot with various reboot_type on a few different platforms. I haven't seen poweroff and reboot issues after CPU0 is offline.

Do you have specific platforms that I can test poweroff and reboot dependency on CPU0? Or we just assume there are some platforms out there that depend on CPU0 for poweroff/reboot?

Thanks.

-Fenghua 

Re: [PATCH 6/8] x86, topology.c: Enable CPU0 online/offline

[Len Brown]

On 11/03/2011 06:47 PM, Yu, Fenghua wrote:

>>> +	bsp_hotplug	[X86] BSP (aka CPU0) is hotpluggable.
>>> +			Suspend/resume depends on BSP. It's said some PCI
>>> +			quirks depend on BSP too. But not sure which quirks.
>>
>> Right, not sure what breaks. Go ahead and watch your machine explode.
>>
>>> +			If don't care the dependencies, you can turn on
>>> +			bsp_ hotplug. Suspend will fail if BSP is offlined
>> and
>>> +			you need to online BSP before suspend/resume.
>>
>> And you forgot poweroff and reboot, which have similar dependencies on
>> some machines. That whole low level ACPI stuff is sensitive.
> 
> I tested poweroff, shutdown, and reboot with various reboot_type on a few different platforms. I haven't seen poweroff and reboot issues after CPU0 is offline.
> 
> Do you have specific platforms that I can test poweroff and reboot dependency on CPU0?

> Or we just assume there are some platforms out there that depend on CPU0 for poweroff/reboot?

A classic Linux/ACPI failure happened when HT first shipped.
Some platforms stopped powering off or rebooting 50% of the time.

It turned out that SMM on those machines assumed
it would be triggered from CPU0 and not CPU1.

the original code should have worked, of course,
and most of the time it did -- but some systems broke.
The same will probably happen here.

-Len

RE: [PATCH 6/8] x86, topology.c: Enable CPU0 online/offline

[Yu, Fenghua]

> >> And you forgot poweroff and reboot, which have similar dependencies
> on
> >> some machines. That whole low level ACPI stuff is sensitive.
> >
> > I tested poweroff, shutdown, and reboot with various reboot_type on a
> few different platforms. I haven't seen poweroff and reboot issues
> after CPU0 is offline.
> >
> > Do you have specific platforms that I can test poweroff and reboot
> dependency on CPU0?
> 
> > Or we just assume there are some platforms out there that depend on
> CPU0 for poweroff/reboot?
> 
> A classic Linux/ACPI failure happened when HT first shipped.
> Some platforms stopped powering off or rebooting 50% of the time.
> 
> It turned out that SMM on those machines assumed
> it would be triggered from CPU0 and not CPU1.
> 
> the original code should have worked, of course,
> and most of the time it did -- but some systems broke.
> The same will probably happen here.

Thanks for your info on poweroff/reboot and CPU0.

I wrote a statement in the cpu-hotplug.txt doc in newer patch set:

"It's said poweroff/reboot may depend on BSP on some machines although I haven't
seen any poweroff/reboot failure so far after BSP is offline on a few tested
machines."
"Please let me know if you know or see any other dependencies of BSP."

If we know a specific model of machine can't poweroff/reboot after CPU0 is offline, I can do something like put that machine in a black list in the poweroff/reboot path to let user online CPU0 before poweroff/reboot.

Hopefully this is a temp OK solution for the patch set to get in kernel and let people report any poweroff/reboot or any other dependencies on CPU0.

Thanks.

-Fenghua

[PATCH 7/8] kernel/power/main.c: Not suspend/resume if CPU0 is offlined

[Fenghua Yu]

From: Fenghua Yu <fenghua.yu@intel.com>

System resumes from CPU0 on today's x86 BIOS. Don't suspend/resume if CPU0 is
offlined and bsp_hotpluggable is 1.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 kernel/power/main.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/kernel/power/main.c b/kernel/power/main.c
index 6c601f8..33ffb6a 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -12,6 +12,7 @@
 #include <linux/string.h>
 #include <linux/resume-trace.h>
 #include <linux/workqueue.h>
+#include <linux/cpu.h>
 
 #include "power.h"
 
@@ -178,6 +179,14 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
 	int len;
 	int error = -EINVAL;
 
+#ifdef CONFIG_HOTPLUG_CPU
+	if (bsp_hotpluggable && cpumask_first(cpu_online_mask) != 0) {
+		printk(KERN_WARNING "Because CPU0 is offlined, system can't suspend/resume.\n");
+
+		return -ENODEV;
+	}
+#endif
+
 	p = memchr(buf, '\n', n);
 	len = p ? p - buf : n;
 
-- 
1.6.0.3

Re: [PATCH 7/8] kernel/power/main.c: Not suspend/resume if CPU0 is offlined

[Srivatsa S. Bhat]

Hi,

On 10/05/2011 10:09 PM, Fenghua Yu wrote:
> 
> @@ -178,6 +179,14 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  	int len;
>  	int error = -EINVAL;
> 
> +#ifdef CONFIG_HOTPLUG_CPU
> +	if (bsp_hotpluggable && cpumask_first(cpu_online_mask) != 0) {
> +		printk(KERN_WARNING "Because CPU0 is offlined, system can't suspend/resume.\n");
> +
> +		return -ENODEV;
> +	}
> +#endif

There is a possible race condition here. What if CPU0 gets offlined
AFTER this point(due to a CPU hotplug operation)?
We will probably have to prevent CPU0 from being taken offline from
this point onwards, and remove that restriction later on.

> +
>  	p = memchr(buf, '\n', n);
>  	len = p ? p - buf : n;
> 


-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@linux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab

Re: [PATCH 7/8] kernel/power/main.c: Not suspend/resume if CPU0 is offlined

[Thomas Gleixner]

On Wed, 5 Oct 2011, Fenghua Yu wrote:

> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> System resumes from CPU0 on today's x86 BIOS. Don't suspend/resume if CPU0 is
> offlined and bsp_hotpluggable is 1.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  kernel/power/main.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 6c601f8..33ffb6a 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -12,6 +12,7 @@
>  #include <linux/string.h>
>  #include <linux/resume-trace.h>
>  #include <linux/workqueue.h>
> +#include <linux/cpu.h>
>  
>  #include "power.h"
>  
> @@ -178,6 +179,14 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  	int len;
>  	int error = -EINVAL;
>  
> +#ifdef CONFIG_HOTPLUG_CPU
> +	if (bsp_hotpluggable && cpumask_first(cpu_online_mask) != 0) {
> +		printk(KERN_WARNING "Because CPU0 is offlined, system can't suspend/resume.\n");
> +
> +		return -ENODEV;
> +	}
> +#endif

Oh yes, we enforce that for all architectures no matter what.

This is a x86 restriction and the cpu hotplug architecture code can
veto unplugging.

Thanks,

	tglx

[PATCH 8/8] kernel/cpu.c: Define bsp_hotpluggable variable

[Fenghua Yu]

From: Fenghua Yu <fenghua.yu@intel.com>

This gloable variable controls BSP (aka CPU0) hotplug. If set, BSP is
hotpluggable. By default, it's 0. On X86, kernel option bsp_hotpluggable sets
the variable as 1.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 include/linux/cpu.h |    2 ++
 kernel/cpu.c        |    5 +++++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index b1a635a..1b0b1ad 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -172,6 +172,8 @@ extern void put_online_cpus(void);
 #define unregister_hotcpu_notifier(nb)	unregister_cpu_notifier(nb)
 int cpu_down(unsigned int cpu);
 
+extern int bsp_hotpluggable;
+
 #ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
 extern void cpu_hotplug_driver_lock(void);
 extern void cpu_hotplug_driver_unlock(void);
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 12b7458..65d1a13 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -43,6 +43,11 @@ static int cpu_hotplug_disabled;
 
 #ifdef CONFIG_HOTPLUG_CPU
 
+/*
+ * If set, BSP (aka CPU0) can be onlined/offlined. By default, it's not set.
+ */
+int bsp_hotpluggable;
+
 static struct {
 	struct task_struct *active_writer;
 	struct mutex lock; /* Synchronizes accesses to refcount, */
-- 
1.6.0.3

Re: [PATCH 8/8] kernel/cpu.c: Define bsp_hotpluggable variable

[Thomas Gleixner]

On Wed, 5 Oct 2011, Fenghua Yu wrote:

> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> This gloable variable controls BSP (aka CPU0) hotplug. If set, BSP is
> hotpluggable. By default, it's 0. On X86, kernel option bsp_hotpluggable sets
> the variable as 1.

.... and on !x86 its just pointless.
 
I have yet to see a justification for that whole cpu0 unplugging
business.

If there is a real reason that this is desireable, then ALL cpu0
assumptions in arch/x86 need to be cleaned up and fixed.

Thanks,

	tglx

RE: [PATCH 8/8] kernel/cpu.c: Define bsp_hotpluggable variable

[Yu, Fenghua]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Wednesday, October 05, 2011 12:25 PM
> To: Yu, Fenghua
> Cc: Ingo Molnar; H Peter Anvin; Zwane Mwaikambo; Luck, Tony; Mallick,
> Asit K; Siddha, Suresh B; Len Brown; linux-kernel
> Subject: Re: [PATCH 8/8] kernel/cpu.c: Define bsp_hotpluggable variable
> 
> On Wed, 5 Oct 2011, Fenghua Yu wrote:
> 
> > From: Fenghua Yu <fenghua.yu@intel.com>
> >
> > This gloable variable controls BSP (aka CPU0) hotplug. If set, BSP is
> > hotpluggable. By default, it's 0. On X86, kernel option
> bsp_hotpluggable sets
> > the variable as 1.
> 
> .... and on !x86 its just pointless.

I'll change the variable to an inline function which returns bsp_hotpluggable on x86 and is empty function on !x86.

> 
> I have yet to see a justification for that whole cpu0 unplugging
> business.
> 
> If there is a real reason that this is desireable, then ALL cpu0
> assumptions in arch/x86 need to be cleaned up and fixed.

I can think of two reasons for bsp offline/online:

1. RAS needs the feature. If socket0 needs to be hotplugged for any reason (any thread on socket0 is bad, shared cache issue, uncore issue, etc), CPU0 is required to be offlined.
2. CPU0 is symmetrical to other CPU's. There is no specific reason why it shouldn't be offlined except BIOS requirements.

Thanks.

-Fenghua

RE: [PATCH 8/8] kernel/cpu.c: Define bsp_hotpluggable variable

[Thomas Gleixner]

On Wed, 5 Oct 2011, Yu, Fenghua wrote:

> > -----Original Message-----
> > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > Sent: Wednesday, October 05, 2011 12:25 PM
> > To: Yu, Fenghua
> > Cc: Ingo Molnar; H Peter Anvin; Zwane Mwaikambo; Luck, Tony; Mallick,
> > Asit K; Siddha, Suresh B; Len Brown; linux-kernel
> > Subject: Re: [PATCH 8/8] kernel/cpu.c: Define bsp_hotpluggable variable
> > 
> > On Wed, 5 Oct 2011, Fenghua Yu wrote:
> > 
> > > From: Fenghua Yu <fenghua.yu@intel.com>
> > >
> > > This gloable variable controls BSP (aka CPU0) hotplug. If set, BSP is
> > > hotpluggable. By default, it's 0. On X86, kernel option
> > bsp_hotpluggable sets
> > > the variable as 1.
> > 
> > .... and on !x86 its just pointless.
> 
> I'll change the variable to an inline function which returns bsp_hotpluggable on x86 and is empty function on !x86.
> 

Can you finally fix your mail client, please ? It want's to look like
this:

> I'll change the variable to an inline function which returns
> bsp_hotpluggable on x86 and is empty function on !x86.

No, that's wrong again. This needs to be in the arch function not in
some random sysfs file op. Someone else pointed it out to you already,
that it's racy as well.

> > 
> > I have yet to see a justification for that whole cpu0 unplugging
> > business.
> > 
> > If there is a real reason that this is desireable, then ALL cpu0
> > assumptions in arch/x86 need to be cleaned up and fixed.
> 
> I can think of two reasons for bsp offline/online:
> 
> 1. RAS needs the feature. If socket0 needs to be hotplugged for any
>    reason (any thread on socket0 is bad, shared cache issue, uncore
>    issue, etc), CPU0 is required to be offlined.

That sounds like a reasonable requirement, which should have been
mentioned in the 0/N mail to a patch series to avoid such questions.

> 2. CPU0 is symmetrical to other CPU's. There is no specific reason
>    why it shouldn't be offlined except BIOS requirements.

That's not a reason at all. And "except BIOS requirements" might be
actually a reason NOT to do that.
 
Though as this needs to be runtime enabled, I have no general
objections against doing it, but it has to be done right.

To do that proper, it needs

   - to fixup _ALL_ cpu 0 assumptions in arch/x86 and not just hacking
     around some of them

   - a proper mechanism to deal with hardware which cannot handle it
     (i.e. no IOAPIC .....)

   - proper fixups for set up but unused irqs like irq0

   - ....

Thanks,

	tglx

Re: [PATCH 0/8] Online/offline BSP on x86

[Peter Zijlstra]

On Wed, 2011-10-05 at 09:39 -0700, Fenghua Yu wrote:
> 
> BSP or CPU0 has been the last obstacle to CPU hotplug on x86. This patch set
> implements BSP online and offline and removes this obstacle to CPU hotplug.

Are you talking about physical cpu hotplug on x86? And you need to
offline the BSP so you can replace socket0 ?

RE: [PATCH 0/8] Online/offline BSP on x86

[Yu, Fenghua]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 910 bytes --]



> -----Original Message-----
> From: Peter Zijlstra [mailto:peterz@infradead.org]
> Sent: Wednesday, October 05, 2011 12:17 PM
> To: Yu, Fenghua
> Cc: Ingo Molnar; Thomas Gleixner; H Peter Anvin; Zwane Mwaikambo; Luck,
> Tony; Mallick, Asit K; Siddha, Suresh B; Len Brown; linux-kernel
> Subject: Re: [PATCH 0/8] Online/offline BSP on x86
> 
> On Wed, 2011-10-05 at 09:39 -0700, Fenghua Yu wrote:
> >
> > BSP or CPU0 has been the last obstacle to CPU hotplug on x86. This
> patch set
> > implements BSP online and offline and removes this obstacle to CPU
> hotplug.
> 
> Are you talking about physical cpu hotplug on x86? And you need to
> offline the BSP so you can replace socket0 ?


This patch set is for BSP online/offline.

Thanks.

-Fenghua
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 0/8] Online/offline BSP on x86

[Peter Zijlstra]

On Wed, 2011-10-05 at 12:22 -0700, Yu, Fenghua wrote:
> 
> > -----Original Message-----
> > From: Peter Zijlstra [mailto:peterz@infradead.org]
> > Sent: Wednesday, October 05, 2011 12:17 PM
> > To: Yu, Fenghua
> > Cc: Ingo Molnar; Thomas Gleixner; H Peter Anvin; Zwane Mwaikambo; Luck,
> > Tony; Mallick, Asit K; Siddha, Suresh B; Len Brown; linux-kernel
> > Subject: Re: [PATCH 0/8] Online/offline BSP on x86
> > 
> > On Wed, 2011-10-05 at 09:39 -0700, Fenghua Yu wrote:
> > >
> > > BSP or CPU0 has been the last obstacle to CPU hotplug on x86. This
> > patch set
> > > implements BSP online and offline and removes this obstacle to CPU
> > hotplug.
> > 
> > Are you talking about physical cpu hotplug on x86? And you need to
> > offline the BSP so you can replace socket0 ?
> 
> 
> This patch set is for BSP online/offline.

Yes, I got that part. Why do you care?

RE: [PATCH 0/8] Online/offline BSP on x86

[Yu, Fenghua]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1777 bytes --]



> -----Original Message-----
> From: Peter Zijlstra [mailto:peterz@infradead.org]
> Sent: Wednesday, October 05, 2011 12:28 PM
> To: Yu, Fenghua
> Cc: Ingo Molnar; Thomas Gleixner; H Peter Anvin; Luck, Tony; Mallick,
> Asit K; Siddha, Suresh B; Brown, Len; linux-kernel
> Subject: RE: [PATCH 0/8] Online/offline BSP on x86
> 
> On Wed, 2011-10-05 at 12:22 -0700, Yu, Fenghua wrote:
> >
> > > -----Original Message-----
> > > From: Peter Zijlstra [mailto:peterz@infradead.org]
> > > Sent: Wednesday, October 05, 2011 12:17 PM
> > > To: Yu, Fenghua
> > > Cc: Ingo Molnar; Thomas Gleixner; H Peter Anvin; Zwane Mwaikambo;
> Luck,
> > > Tony; Mallick, Asit K; Siddha, Suresh B; Len Brown; linux-kernel
> > > Subject: Re: [PATCH 0/8] Online/offline BSP on x86
> > >
> > > On Wed, 2011-10-05 at 09:39 -0700, Fenghua Yu wrote:
> > > >
> > > > BSP or CPU0 has been the last obstacle to CPU hotplug on x86.
> This
> > > patch set
> > > > implements BSP online and offline and removes this obstacle to
> CPU
> > > hotplug.
> > >
> > > Are you talking about physical cpu hotplug on x86? And you need to
> > > offline the BSP so you can replace socket0 ?
> >
> >
> > This patch set is for BSP online/offline.
> 
> Yes, I got that part. Why do you care?

I can think of two reasons for bsp offline/online:

1. RAS needs the feature. If socket0 needs to be hotplugged for any reason (any thread on socket0 is bad, shared cache issue, uncore issue, etc), CPU0 is required to be offlined.

2. CPU0 is symmetrical to other CPU's. There is no specific reason why it shouldn't be offlined except BIOS requirements.

Thanks.

-Fenghua
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 0/8] Online/offline BSP on x86

[Peter Zijlstra]

On Wed, 2011-10-05 at 13:29 -0700, Yu, Fenghua wrote:
> > Yes, I got that part. Why do you care?
> 
> I can think of two reasons for bsp offline/online:
> 
> 1. RAS needs the feature. If socket0 needs to be hotplugged for any
> reason (any thread on socket0 is bad, shared cache issue, uncore
> issue, etc), CPU0 is required to be offlined.
> 
Right, this is what I asked.

> 2. CPU0 is symmetrical to other CPU's. There is no specific reason why
> it shouldn't be offlined except BIOS requirements. 

Right, so how are you going to deal with crappy BIOS stuffs? We all know
there's plenty of that around.