2012-06-19 12:14:53

by Igor Mammedov

[permalink] [raw]
Subject: [PATCH 0/2 v2] x86: Improve secondary CPU bring-up process robustness

Reproduced mostly in virt. environments, where physical CPUs are shared
beetween many guests and on overcommited host guest's CPUs may stall
and lead to master CPU 'canceling' CPU bring-up. But in reality being
onlined CPU continued to boot and caused hangs when onlining another CPU.

Now, I'm trying do fix issue a bit earlier. I hope it's better than
the first attempt: https://lkml.org/lkml/2012/5/9/125
Please review.


2012-06-19 12:14:49

by Igor Mammedov

[permalink] [raw]
Subject: [PATCH 1/2] x86: abort secondary CPU bring-up gracefully if do_boot_cpu timed out on cpu_callin_mask

Master CPU may timeout before cpu_callin_mask is set and cancel
booting CPU, but being onlined CPU still continues to boot, sets
cpu_active_mask (CPU_STARTING notifiers) and spins in
check_tsc_sync_target() for master cpu to arrive. Following attempt
to online another cpu hangs in stop_machine, initiated from here:

smp_callin ->
smp_store_cpu_info ->
identify_secondary_cpu ->
mtrr_ap_init -> set_mtrr_from_inactive_cpu

stop_machine waits on completion of stop_work on all CPUs from
cpu_active_mask including a failed CPU that spins in check_tsc_sync_target().

Issue could be fixed if being onlined CPU continues to boot and calls
notify_cpu_starting(cpuid) only when master CPU waits for it to
come online. If master CPU times out on cpu_callin_mask and goes via
cancel path, the being onlined CPU should gracefully shutdown itself.

Patch introduces cpu_may_complete_boot_mask to notify a being onlined
CPU that it may call notify_cpu_starting(cpuid) and continue to boot
when master CPU goes via normal boot path and going to wait till the
being onlined CPU completes its initialization.

normal boot sequence will look like:
master CPU1 being onlined CPU2

* wait for CPU2 in cpu_callin_mask
---------------------------------------------------------------------
* set CPU2 in cpu_callin_mask
* wait till CPU1 set CPU2 bit
in cpu_may_complete_boot_mask
---------------------------------------------------------------------
* set CPU2 bit in
cpu_may_complete_boot_mask
* return from do_boot_cpu() and
wait in
- check_tsc_sync_source() or
- while (!cpu_online(CPU2))
---------------------------------------------------------------------
* call notify_cpu_starting()
and continue CPU2 initialization
* mark itself as ONLINE
---------------------------------------------------------------------
* return to _cpu_up and call
cpu_notify(CPU_ONLINE, ...)

cancel/error path will look like:
master CPU1 being onlined CPU2

* time out on cpu_callin_mask
---------------------------------------------------------------------
* set CPU2 in cpu_callin_mask
* wait till CPU2 is set in
cpu_may_complete_boot_mask or
cleared in cpu_callout_mask
---------------------------------------------------------------------
* clear CPU2 in cpu_callout_mask
and return with error
---------------------------------------------------------------------
* do cleanups and play_dead()

Note:
It's safe for being onlined CPU to set cpu_callin_mask before calling
notify_cpu_starting() because master CPU may first wait for being booted
CPU in check_tsc_sync_source() and then it waits in native_cpu_up() till
being booted CPU comes online and only when being booted CPU sets cpu_online_mask
it will exit native_cpu_up() and then call CPU_ONLINE notifiers.

v3:
- added missing remove_siblinginfo() on 'die' error path.
- added explanation why it's ok to set cpu_callin_mask before calling
CPU_STARTING notifiers
- being booted CPU will wait for master CPU without timeouts

Signed-off-by: Igor Mammedov <[email protected]>
---
arch/x86/include/asm/cpumask.h | 1 +
arch/x86/kernel/cpu/common.c | 2 ++
arch/x86/kernel/smpboot.c | 34 ++++++++++++++++++++++++++++++++--
3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/cpumask.h b/arch/x86/include/asm/cpumask.h
index 61c852f..eacd269 100644
--- a/arch/x86/include/asm/cpumask.h
+++ b/arch/x86/include/asm/cpumask.h
@@ -7,6 +7,7 @@ extern cpumask_var_t cpu_callin_mask;
extern cpumask_var_t cpu_callout_mask;
extern cpumask_var_t cpu_initialized_mask;
extern cpumask_var_t cpu_sibling_setup_mask;
+extern cpumask_var_t cpu_may_complete_boot_mask;

extern void setup_cpu_local_masks(void);

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 6b9333b..16984f1 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -48,6 +48,7 @@
cpumask_var_t cpu_initialized_mask;
cpumask_var_t cpu_callout_mask;
cpumask_var_t cpu_callin_mask;
+cpumask_var_t cpu_may_complete_boot_mask;

/* representing cpus for which sibling maps can be computed */
cpumask_var_t cpu_sibling_setup_mask;
@@ -59,6 +60,7 @@ void __init setup_cpu_local_masks(void)
alloc_bootmem_cpumask_var(&cpu_callin_mask);
alloc_bootmem_cpumask_var(&cpu_callout_mask);
alloc_bootmem_cpumask_var(&cpu_sibling_setup_mask);
+ alloc_bootmem_cpumask_var(&cpu_may_complete_boot_mask);
}

static void __cpuinit default_init(struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 7bd8a08..95948b9 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -122,6 +122,8 @@ EXPORT_PER_CPU_SYMBOL(cpu_info);

atomic_t init_deasserted;

+static void remove_siblinginfo(int cpu);
+
/*
* Report back to the Boot Processor.
* Running on AP.
@@ -218,12 +220,37 @@ static void __cpuinit smp_callin(void)
set_cpu_sibling_map(raw_smp_processor_id());
wmb();

- notify_cpu_starting(cpuid);
-
/*
* Allow the master to continue.
*/
cpumask_set_cpu(cpuid, cpu_callin_mask);
+
+ /*
+ * Wait for signal from master CPU to continue or abort.
+ */
+ while (!cpumask_test_cpu(cpuid, cpu_may_complete_boot_mask) &&
+ cpumask_test_cpu(cpuid, cpu_callout_mask)) {
+ cpu_relax();
+ }
+
+ /* die if master cancelled cpu_up */
+ if (!cpumask_test_cpu(cpuid, cpu_may_complete_boot_mask))
+ goto die;
+
+ notify_cpu_starting(cpuid);
+ return;
+
+die:
+#ifdef CONFIG_HOTPLUG_CPU
+ numa_remove_cpu(cpuid);
+ remove_siblinginfo(cpuid);
+ clear_local_APIC();
+ /* was set by cpu_init() */
+ cpumask_clear_cpu(cpuid, cpu_initialized_mask);
+ cpumask_clear_cpu(cpuid, cpu_callin_mask);
+ play_dead();
+#endif
+ panic("%s: Failed to online CPU%d!\n", __func__, cpuid);
}

/*
@@ -752,6 +779,8 @@ static int __cpuinit do_boot_cpu(int apicid, int cpu, struct task_struct *idle)
}

if (cpumask_test_cpu(cpu, cpu_callin_mask)) {
+ /* Signal AP that it may continue to boot */
+ cpumask_set_cpu(cpu, cpu_may_complete_boot_mask);
print_cpu_msr(&cpu_data(cpu));
pr_debug("CPU%d: has booted.\n", cpu);
} else {
@@ -1225,6 +1254,7 @@ static void __ref remove_cpu_from_maps(int cpu)
cpumask_clear_cpu(cpu, cpu_callin_mask);
/* was set by cpu_init() */
cpumask_clear_cpu(cpu, cpu_initialized_mask);
+ cpumask_clear_cpu(cpu, cpu_may_complete_boot_mask);
numa_remove_cpu(cpu);
}

--
1.7.1

2012-06-19 12:15:21

by Igor Mammedov

[permalink] [raw]
Subject: [PATCH 2/2] x86: don't panic if master CPU haven't set cpu_callout_mask

Gracefully cancel CPU initialization instead of panic when master
CPU haven't managed to set cpu_callout_mask in time.

Signed-off-by: Igor Mammedov <[email protected]>
---
arch/x86/kernel/smpboot.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 95948b9..6470470 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -175,8 +175,9 @@ static void __cpuinit smp_callin(void)
}

if (!time_before(jiffies, timeout)) {
- panic("%s: CPU%d started up but did not get a callout!\n",
+ pr_debug("%s: CPU%d started up but did not get a callout!\n",
__func__, cpuid);
+ goto die;
}

/*
--
1.7.1

2012-08-02 09:35:12

by Igor Mammedov

[permalink] [raw]
Subject: Re: [PATCH 1/2] x86: abort secondary CPU bring-up gracefully if do_boot_cpu timed out on cpu_callin_mask

Hi Toshi,

I'm sorry for delayed response, I was on vacation. Thanks for looking at
the patch, my comments are below.

PS:
I'm not happy with introducing one more sync point and bitmat. Well,
it's necessary to somehow notify being on-lined CPU that master CPU will
wait for it, but perhaps it could be done even earlier than in this
patch and less stuff should be backed-out.

Currently master CPU spins on cpu_callin_mask till secondary CPU sets it
in smp_callin(). Then master CPU leaves do_boot_cpu() and does nothing
in native_cpu_up() first waiting for secondary CPU in
check_tsc_sync_source() or if that is skipped then immediately spinning
on 'while (!cpu_online(cpu))'.
Master CPU will do nothing and will not call any CPU notifiers until
secondary CPU reports that it is ONLINE by setting bit in
cpu_online_mask at the end of start_secondary().

So questions to experts are:

1. what is purpose of cpu_callin_mask

2. why master CPU spins on cpu_callin_mask but not on cpu_initialized_mask

In current state of code, it looks like cpu_callin_mask is not necessary
and we could remove it completely and spin on cpu_initialized_mask in
do_boot_cpu() instead. Then when master CPU
sees secondary CPU in cpu_initialized_mask it could set cpu_callout_mask
to ack its intention not to cancel and wait until
secondary CPU is booted.


PS2:
I wish x86 maintainers were more responsive to the topic and in
discussion we could find a way to fix problem. With their expertise,
It's surely easier to spot potential issues and see correct approach for
the fix.

----- Original Message -----
> From: "Toshi Kani" <[email protected]>
> To: [email protected]
> Cc: [email protected], [email protected], [email protected], [email protected], [email protected],
> [email protected], [email protected], [email protected], [email protected], "suresh b siddha" <[email protected]>,
> [email protected], "a p zijlstra" <[email protected]>, [email protected], "toshi kani" <[email protected]>
> Sent: Wednesday, July 11, 2012 11:49:19 PM
> Subject: Re: [PATCH 1/2] x86: abort secondary CPU bring-up gracefully if do_boot_cpu timed out on cpu_callin_mask
>
> Hi Igor,
>
> This is a nice work to handle CPU bring-up error properly. My
> comments
> are in-line below.
>
> On Wed, 2012-07-11 at 14:12 -0600, Toshi Kani wrote:
> > Master CPU may timeout before cpu_callin_mask is set and cancel
> > booting CPU, but being onlined CPU still continues to boot, sets
> > cpu_active_mask (CPU_STARTING notifiers) and spins in
> > check_tsc_sync_target() for master cpu to arrive. Following attempt
> > to online another cpu hangs in stop_machine, initiated from here:
> >
> > smp_callin ->
> > smp_store_cpu_info ->
> > identify_secondary_cpu ->
> > mtrr_ap_init -> set_mtrr_from_inactive_cpu
> >
> > stop_machine waits on completion of stop_work on all CPUs from
> > cpu_active_mask including a failed CPU that spins in
> > check_tsc_sync_target().
> >
> > Issue could be fixed if being onlined CPU continues to boot and
> > calls
> > notify_cpu_starting(cpuid) only when master CPU waits for it to
> > come online. If master CPU times out on cpu_callin_mask and goes
> > via
> > cancel path, the being onlined CPU should gracefully shutdown
> > itself.
> >
> > Patch introduces cpu_may_complete_boot_mask to notify a being
> > onlined
> > CPU that it may call notify_cpu_starting(cpuid) and continue to
> > boot
> > when master CPU goes via normal boot path and going to wait till
> > the
> > being onlined CPU completes its initialization.
> >
> > normal boot sequence will look like:
> > master CPU1 being onlined CPU2
> >
> > * wait for CPU2 in cpu_callin_mask
> > ---------------------------------------------------------------------
> > * set CPU2 in
> > cpu_callin_mask
> > * wait till CPU1 set CPU2
> > bit
> > in
> > cpu_may_complete_boot_mask
> > ---------------------------------------------------------------------
> > * set CPU2 bit in
> > cpu_may_complete_boot_mask
> > * return from do_boot_cpu() and
> > wait in
> > - check_tsc_sync_source() or
> > - while (!cpu_online(CPU2))
> > ---------------------------------------------------------------------
> > * call
> > notify_cpu_starting()
> > and continue CPU2 initialization
> > * mark itself as ONLINE
> > ---------------------------------------------------------------------
> > * return to _cpu_up and call
> > cpu_notify(CPU_ONLINE, ...)
> >
> > cancel/error path will look like:
> > master CPU1 being onlined CPU2
> >
> > * time out on cpu_callin_mask
> > ---------------------------------------------------------------------
> > * set CPU2 in
> > cpu_callin_mask
> > * wait till CPU2 is set in
> > cpu_may_complete_boot_mask
> > or
> > cleared in
> > cpu_callout_mask
> > ---------------------------------------------------------------------
> > * clear CPU2 in cpu_callout_mask
> > and return with error
> > ---------------------------------------------------------------------
> > * do cleanups and
> > play_dead()
> >
> > Note:
> > It's safe for being onlined CPU to set cpu_callin_mask before
> > calling
> > notify_cpu_starting() because master CPU may first wait for being
> > booted
> > CPU in check_tsc_sync_source() and then it waits in native_cpu_up()
> > till
> > being booted CPU comes online and only when being booted CPU sets
> > cpu_online_mask
> > it will exit native_cpu_up() and then call CPU_ONLINE notifiers.
> >
> > v3:
> > - added missing remove_siblinginfo() on 'die' error path.
> > - added explanation why it's ok to set cpu_callin_mask before
> > calling
> > CPU_STARTING notifiers
> > - being booted CPU will wait for master CPU without timeouts
> >
> > Signed-off-by: Igor Mammedov <[email protected]>
> > ---
> > arch/x86/include/asm/cpumask.h | 1 +
> > arch/x86/kernel/cpu/common.c | 2 ++
> > arch/x86/kernel/smpboot.c | 34
> > ++++++++++++++++++++++++++++++++--
> > 3 files changed, 35 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/cpumask.h
> > b/arch/x86/include/asm/cpumask.h
> > index 61c852f..eacd269 100644
> > --- a/arch/x86/include/asm/cpumask.h
> > +++ b/arch/x86/include/asm/cpumask.h
> > @@ -7,6 +7,7 @@ extern cpumask_var_t cpu_callin_mask;
> > extern cpumask_var_t cpu_callout_mask;
> > extern cpumask_var_t cpu_initialized_mask;
> > extern cpumask_var_t cpu_sibling_setup_mask;
> > +extern cpumask_var_t cpu_may_complete_boot_mask;
> >
> > extern void setup_cpu_local_masks(void);
> >
> > diff --git a/arch/x86/kernel/cpu/common.c
> > b/arch/x86/kernel/cpu/common.c
> > index 6b9333b..16984f1 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -48,6 +48,7 @@
> > cpumask_var_t cpu_initialized_mask;
> > cpumask_var_t cpu_callout_mask;
> > cpumask_var_t cpu_callin_mask;
> > +cpumask_var_t cpu_may_complete_boot_mask;
> >
> > /* representing cpus for which sibling maps can be computed */
> > cpumask_var_t cpu_sibling_setup_mask;
> > @@ -59,6 +60,7 @@ void __init setup_cpu_local_masks(void)
> > alloc_bootmem_cpumask_var(&cpu_callin_mask);
> > alloc_bootmem_cpumask_var(&cpu_callout_mask);
> > alloc_bootmem_cpumask_var(&cpu_sibling_setup_mask);
> > + alloc_bootmem_cpumask_var(&cpu_may_complete_boot_mask);
> > }
> >
> > static void __cpuinit default_init(struct cpuinfo_x86 *c)
> > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> > index 7bd8a08..95948b9 100644
> > --- a/arch/x86/kernel/smpboot.c
> > +++ b/arch/x86/kernel/smpboot.c
> > @@ -122,6 +122,8 @@ EXPORT_PER_CPU_SYMBOL(cpu_info);
> >
> > atomic_t init_deasserted;
> >
> > +static void remove_siblinginfo(int cpu);
> > +
> > /*
> > * Report back to the Boot Processor.
> > * Running on AP.
> > @@ -218,12 +220,37 @@ static void __cpuinit smp_callin(void)
> > set_cpu_sibling_map(raw_smp_processor_id());
> > wmb();
> >
> > - notify_cpu_starting(cpuid);
> > -
> > /*
> > * Allow the master to continue.
> > */
> > cpumask_set_cpu(cpuid, cpu_callin_mask);
> > +
> > + /*
> > + * Wait for signal from master CPU to continue or abort.
> > + */
> > + while (!cpumask_test_cpu(cpuid, cpu_may_complete_boot_mask) &&
> > + cpumask_test_cpu(cpuid, cpu_callout_mask)) {
> > + cpu_relax();
> > + }
> > +
> > + /* die if master cancelled cpu_up */
> > + if (!cpumask_test_cpu(cpuid, cpu_may_complete_boot_mask))
> > + goto die;
> > +
> > + notify_cpu_starting(cpuid);
> > + return;
> > +
> > +die:
> > +#ifdef CONFIG_HOTPLUG_CPU
> > + numa_remove_cpu(cpuid);
>
> smp_callin() calls numa_add_cpu(), so it makes sense to perform this
> back-out from here. However, do_boot_cpu() also calls this function
> in
> its error path. I think we should change do_boot_cpu() to perform
> its
> back-out to the master CPU's initialization code only. This would
> keep
> their responsibility clear and avoid any race condition.
Reason to keep numa_remove_cpu() in do_boot_cpu() is for the case where
being onlined CPU is permanently stuck on boot. In this case
numa_remove_cpu() would not be called from smp_callin().
Anyway race is still there:
master CPU: numa_remove_cpu()
... window with incorrect numa state
secondary CPU: numa_add_cpu()
secondary CPU: numa_remove_cpu()

>
> Also, it would be helpful to have a comment like /* was set by
> smp_store_cpu_info() */ to state this responsibility clearly.
I'll fix it in next version.

>
> > + remove_siblinginfo(cpuid);
> > + clear_local_APIC();
> > + /* was set by cpu_init() */
> > + cpumask_clear_cpu(cpuid, cpu_initialized_mask);
>
> This is also called by do_boot_cpu(). Same comment as above.
>
> > + cpumask_clear_cpu(cpuid, cpu_callin_mask);
> > + play_dead();
> > +#endif
> > + panic("%s: Failed to online CPU%d!\n", __func__, cpuid);
>
> Why does it panic in case of !CONFIG_HOTPLUG_CPU? Is this because
> user
> cannot online this CPU later on, so it might be better off rebooting
> with a panic? Can I also assume that user can try to on-line this
> failed CPU if CONFIG_HOTPLUG_CPU is set? Some comment would be
> helpful
> to clarify this behavior.
User isn't able to online/offline CPUs if kernel is built without
CONFIG_HOTPLUG_CPU.
Define is here to cover failed on boot CPU for non hotplug capable
kernel. A bunch of code to stop CPU is just not built for non hotplug
kernel so what else to do except of panicking?

>
> Thanks,
> -Toshi
>
>
> > }
> >
> > /*
> > @@ -752,6 +779,8 @@ static int __cpuinit do_boot_cpu(int apicid,
> > int cpu, struct task_struct *idle)
> > }
> >
> > if (cpumask_test_cpu(cpu, cpu_callin_mask)) {
> > + /* Signal AP that it may continue to boot */
> > + cpumask_set_cpu(cpu, cpu_may_complete_boot_mask);
> > print_cpu_msr(&cpu_data(cpu));
> > pr_debug("CPU%d: has booted.\n", cpu);
> > } else {
> > @@ -1225,6 +1254,7 @@ static void __ref remove_cpu_from_maps(int
> > cpu)
> > cpumask_clear_cpu(cpu, cpu_callin_mask);
> > /* was set by cpu_init() */
> > cpumask_clear_cpu(cpu, cpu_initialized_mask);
> > + cpumask_clear_cpu(cpu, cpu_may_complete_boot_mask);
> > numa_remove_cpu(cpu);
> > }
> >
>
>
>

2012-08-02 17:53:54

by Toshi Kani

[permalink] [raw]
Subject: Re: [PATCH 1/2] x86: abort secondary CPU bring-up gracefully if do_boot_cpu timed out on cpu_callin_mask

On Thu, 2012-08-02 at 11:34 +0200, Igor Mammedov wrote:
> Hi Toshi,
>
> I'm sorry for delayed response, I was on vacation. Thanks for looking at
> the patch, my comments are below.

Hi Igor,

Welcome back!

> PS:
> I'm not happy with introducing one more sync point and bitmat. Well,
> it's necessary to somehow notify being on-lined CPU that master CPU will
> wait for it, but perhaps it could be done even earlier than in this
> patch and less stuff should be backed-out.
>
> Currently master CPU spins on cpu_callin_mask till secondary CPU sets it
> in smp_callin(). Then master CPU leaves do_boot_cpu() and does nothing
> in native_cpu_up() first waiting for secondary CPU in
> check_tsc_sync_source() or if that is skipped then immediately spinning
> on 'while (!cpu_online(cpu))'.
> Master CPU will do nothing and will not call any CPU notifiers until
> secondary CPU reports that it is ONLINE by setting bit in
> cpu_online_mask at the end of start_secondary().
>
> So questions to experts are:
>
> 1. what is purpose of cpu_callin_mask
>
> 2. why master CPU spins on cpu_callin_mask but not on cpu_initialized_mask
>
> In current state of code, it looks like cpu_callin_mask is not necessary
> and we could remove it completely and spin on cpu_initialized_mask in
> do_boot_cpu() instead. Then when master CPU
> sees secondary CPU in cpu_initialized_mask it could set cpu_callout_mask
> to ack its intention not to cancel and wait until
> secondary CPU is booted.

I agree and I'd like to know the answers as well. This way, the master
does not have to deal with secondary's back-back out procedure.

> PS2:
> I wish x86 maintainers were more responsive to the topic and in
> discussion we could find a way to fix problem. With their expertise,
> It's surely easier to spot potential issues and see correct approach for
> the fix.
>

(snip)

> > > +
> > > +die:
> > > +#ifdef CONFIG_HOTPLUG_CPU
> > > + numa_remove_cpu(cpuid);
> >
> > smp_callin() calls numa_add_cpu(), so it makes sense to perform this
> > back-out from here. However, do_boot_cpu() also calls this function
> > in
> > its error path. I think we should change do_boot_cpu() to perform its
> > back-out to the master CPU's initialization code only. This would keep
> > their responsibility clear and avoid any race condition.
> Reason to keep numa_remove_cpu() in do_boot_cpu() is for the case where
> being onlined CPU is permanently stuck on boot. In this case
> numa_remove_cpu() would not be called from smp_callin().
> Anyway race is still there:
> master CPU: numa_remove_cpu()
> ... window with incorrect numa state
> secondary CPU: numa_add_cpu()
> secondary CPU: numa_remove_cpu()

Right. Another example is that the master can call numa_remove_cpu()
after a secondary called numa_add_cpu(). If the secondary's next
procedure relies on numa_add_cpu() be done, it causes a problem.

Anyway, I like your idea of the master to wait for cpu_initialized_mask.
This should eliminate the need of the master to perform secondary's
back-out procedure.

> > Also, it would be helpful to have a comment like /* was set by
> > smp_store_cpu_info() */ to state this responsibility clearly.
> I'll fix it in next version.
>
> >
> > > + remove_siblinginfo(cpuid);
> > > + clear_local_APIC();
> > > + /* was set by cpu_init() */
> > > + cpumask_clear_cpu(cpuid, cpu_initialized_mask);
> >
> > This is also called by do_boot_cpu(). Same comment as above.
> >
> > > + cpumask_clear_cpu(cpuid, cpu_callin_mask);
> > > + play_dead();
> > > +#endif
> > > + panic("%s: Failed to online CPU%d!\n", __func__, cpuid);
> >
> > Why does it panic in case of !CONFIG_HOTPLUG_CPU? Is this because user
> > cannot online this CPU later on, so it might be better off rebooting
> > with a panic? Can I also assume that user can try to on-line this
> > failed CPU if CONFIG_HOTPLUG_CPU is set? Some comment would be helpful
> > to clarify this behavior.
> User isn't able to online/offline CPUs if kernel is built without
> CONFIG_HOTPLUG_CPU.
> Define is here to cover failed on boot CPU for non hotplug capable
> kernel. A bunch of code to stop CPU is just not built for non hotplug
> kernel so what else to do except of panicking?

Another option is to simply boot-up the system with a reduced number of
CPUs for all cases. This way has advantage when:

- If resume/suspend works without CONFIG_HOTPLUG_CPU, it avoids
crashing the system at resume.
- User can boot-up and uses the system with a reduced number of CPUs
even if the error persists after a reboot.


Thanks,
-Toshi