x86: Arbitrary CPU hot(un)plug support

From: Fenghua Yu <[email protected]>

If CONFIG_BOOTPARAM_HOTPLUG_CPU is turned on, CPU0 hotplug feature is enabled by default.

If CONFIG_BOOTPARAM_HOTPLUG_CPU is not turned on, CPU0 hotplug feature is not enabled by default. The kernel parameter cpu0_hotplug can enable CPU0 hotplug feature at boot.

Currently the feature is supported on Intel platforms only.

Signed-off-by Fenghua Yu <[email protected]>

---
 arch/x86/kernel/topology.c |   50 +++++++++++++++++++++++++++++++++++++------

Quoted 3 lines

From: Fenghua Yu <[email protected]>

If CONFIG_BOOTPARAM_HOTPLUG_CPU is turned on, CPU0 hotplug feature is

enabled

Quoted 3 lines

by default.

If CONFIG_BOOTPARAM_HOTPLUG_CPU is not turned on, CPU0 hotplug

feature is not

Quoted 1 line

enabled by default. The kernel parameter cpu0_hotplug can enable CPU0

hotplug

Quoted 7 lines

feature at boot.

Currently the feature is supported on Intel platforms only.

Signed-off-by Fenghua Yu <[email protected]>

---
 arch/x86/kernel/topology.c |   50

+++++++++++++++++++++++++++++++++++++------

It is just me or is this whole change better suited for

arch/x86/kernel/cpu/topology.c

where cpu-specific stuff lives anyway?

arch/x86/kernel/cpu/topology.c doesn't contain CPU hotplug hooks. To place the BSP patchset in this file, other existing CPU hotplug code should be moved to this file as well. Apparently that's not this patchset's task.

From: Fenghua Yu <[email protected]>

Because x86 BIOS requires CPU0 to resume from sleep, suspend or hibernate can't be executed if CPU0 is detected offline. To make suspend or hibernate and further resume succeed, CPU0 must be online.

Signed-off-by Fenghua Yu <[email protected]>

---
 arch/x86/power/cpu.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)

arch/x86/power/cpu.c +22 -0

index 218cdb1..adde775 100644
@@ -237,3 +237,47 @@ void restore_processor_state(void)
 #ifdef CONFIG_X86_32
 EXPORT_SYMBOL(restore_processor_state);
 #endif
+
+/*
+ * When bsp_check() is called in hibernate and suspend, cpu hotplug
+ * is disabled already. So it's unnessary to handle race condition between
+ * cpumask query and cpu hotplug.
+ */
+static int bsp_check(void)
+{
+	if (cpumask_first(cpu_online_mask) != 0) {
+		pr_warn("CPU0 is offline.\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int bsp_pm_callback(struct notifier_block *nb, unsigned long action,
+			   void *ptr)
+{
+	int ret = 0;
+
+	switch (action) {

arch/x86/power/cpu.c +12 -0

+static int __init bsp_pm_check_init(void)
+{
+	/*
+	 * Set this bsp_pm_callback as lower priority than
+	 * cpu_hotplug_pm_callback. So cpu_hotplug_pm_callback will be called
+	 * earlier to disable cpu hotplug before bsp online check.
+	 */
+	pm_notifier(bsp_pm_callback, -INT_MAX);
+	return 0;
+}
+
+core_initcall(bsp_pm_check_init);

Quoted 46 lines · On Friday 12 of October 2012 09:09:42 Fenghua Yu wrote:

From: Fenghua Yu <[email protected]>

Because x86 BIOS requires CPU0 to resume from sleep, suspend or hibernate can't be executed if CPU0 is detected offline. To make suspend or hibernate and further resume succeed, CPU0 must be online.

Signed-off-by Fenghua Yu <[email protected]>

---
 arch/x86/power/cpu.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)

arch/x86/power/cpu.c +27 -0

index 218cdb1..adde775 100644
@@ -237,3 +237,47 @@ void restore_processor_state(void)
 #ifdef CONFIG_X86_32
 EXPORT_SYMBOL(restore_processor_state);
 #endif
+
+/*
+ * When bsp_check() is called in hibernate and suspend, cpu hotplug
+ * is disabled already. So it's unnessary to handle race condition between
+ * cpumask query and cpu hotplug.
+ */
+static int bsp_check(void)
+{
+	if (cpumask_first(cpu_online_mask) != 0) {
+		pr_warn("CPU0 is offline.\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int bsp_pm_callback(struct notifier_block *nb, unsigned long action,
+			   void *ptr)
+{
+	int ret = 0;
+
+	switch (action) {
+	case PM_SUSPEND_PREPARE:
+	case PM_HIBERNATION_PREPARE:
+		ret = bsp_check();
+		break;
+	default:

arch/x86/power/cpu.c void restore_processor_state(void) · line 271

Quoted 5 lines

arch/x86/power/cpu.c +5 -0

+		break;
+	}
+	return notifier_from_errno(ret);
+}
+

I wonder if there's anything preventing CPU0 from becoming offline after you've done this check and before user space is frozen?

Quoted 15 lines

arch/x86/power/cpu.c +12 -0

+static int __init bsp_pm_check_init(void)
+{
+	/*
+	 * Set this bsp_pm_callback as lower priority than
+	 * cpu_hotplug_pm_callback. So cpu_hotplug_pm_callback will be called
+	 * earlier to disable cpu hotplug before bsp online check.
+	 */
+	pm_notifier(bsp_pm_callback, -INT_MAX);
+	return 0;
+}
+
+core_initcall(bsp_pm_check_init);

Quoted 47 lines · Rafael J. Wysocki

Quoted 46 lines · On Friday 12 of October 2012 09:09:42 Fenghua Yu wrote:

From: Fenghua Yu <[email protected]>

Because x86 BIOS requires CPU0 to resume from sleep, suspend or hibernate can't be executed if CPU0 is detected offline. To make suspend or hibernate and further resume succeed, CPU0 must be online.

Signed-off-by Fenghua Yu <[email protected]>

---
 arch/x86/power/cpu.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)

arch/x86/power/cpu.c +27 -0

index 218cdb1..adde775 100644
@@ -237,3 +237,47 @@ void restore_processor_state(void)
 #ifdef CONFIG_X86_32
 EXPORT_SYMBOL(restore_processor_state);
 #endif
+
+/*
+ * When bsp_check() is called in hibernate and suspend, cpu hotplug
+ * is disabled already. So it's unnessary to handle race condition between
+ * cpumask query and cpu hotplug.
+ */
+static int bsp_check(void)
+{
+	if (cpumask_first(cpu_online_mask) != 0) {
+		pr_warn("CPU0 is offline.\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int bsp_pm_callback(struct notifier_block *nb, unsigned long action,
+			   void *ptr)
+{
+	int ret = 0;
+
+	switch (action) {
+	case PM_SUSPEND_PREPARE:
+	case PM_HIBERNATION_PREPARE:
+		ret = bsp_check();
+		break;
+	default:

Quoted 9 lines

arch/x86/power/cpu.c void restore_processor_state(void) · line 271

Quoted 5 lines

arch/x86/power/cpu.c +5 -0

+		break;
+	}
+	return notifier_from_errno(ret);
+}
+

I wonder if there's anything preventing CPU0 from becoming offline after you've done this check and before user space is frozen?

Hi Rafael,

bsp_pm_callback runs as a low priority notifier callback, specifically with lower priority than the cpu_hotplug_pm_callback (as mentioned in the comment below). And cpu_hotplug_pm_callback disables regular CPU hotplug (till the suspend/resume sequence is complete).. So there is no chance for CPU0 to become offline after that.

Or, are you thinking of some other scenario where CPU0 can go offline?

Quoted 4 lines

Quoted 4 lines

arch/x86/power/cpu.c +4 -0

+static int __init bsp_pm_check_init(void)
+{
+	/*
+	 * Set this bsp_pm_callback as lower priority than

Quoted 8 lines

arch/x86/power/cpu.c void restore_processor_state(void) · line 283

Quoted 8 lines

arch/x86/power/cpu.c +8 -0

+	 * cpu_hotplug_pm_callback. So cpu_hotplug_pm_callback will be called
+	 * earlier to disable cpu hotplug before bsp online check.
+	 */
+	pm_notifier(bsp_pm_callback, -INT_MAX);
+	return 0;
+}
+
+core_initcall(bsp_pm_check_init);

Quoted 48 lines · On Tuesday 16 of October 2012 11:05:18 Srivatsa S. Bhat wrote:

Quoted 30 lines · Rafael J. Wysocki

Quoted 29 lines · On Friday 12 of October 2012 09:09:42 Fenghua Yu wrote:

From: Fenghua Yu <[email protected]>

arch/x86/power/cpu.c +27 -0

+
+/*
+ * When bsp_check() is called in hibernate and suspend, cpu hotplug
+ * is disabled already. So it's unnessary to handle race condition between
+ * cpumask query and cpu hotplug.
+ */
+static int bsp_check(void)
+{
+	if (cpumask_first(cpu_online_mask) != 0) {
+		pr_warn("CPU0 is offline.\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int bsp_pm_callback(struct notifier_block *nb, unsigned long action,
+			   void *ptr)
+{
+	int ret = 0;
+
+	switch (action) {
+	case PM_SUSPEND_PREPARE:
+	case PM_HIBERNATION_PREPARE:
+		ret = bsp_check();
+		break;
+	default:

Quoted 9 lines

arch/x86/power/cpu.c void restore_processor_state(void) · line 271

Quoted 5 lines

arch/x86/power/cpu.c +5 -0

+		break;
+	}
+	return notifier_from_errno(ret);
+}
+

I wonder if there's anything preventing CPU0 from becoming offline after you've done this check and before user space is frozen?

Hi Rafael,

bsp_pm_callback runs as a low priority notifier callback, specifically with lower priority than the cpu_hotplug_pm_callback (as mentioned in the comment below). And cpu_hotplug_pm_callback disables regular CPU hotplug (till the suspend/resume sequence is complete).. So there is no chance for CPU0 to become offline after that.

Quoted 1 line

Or, are you thinking of some other scenario where CPU0 can go offline?

No, that should be fine technically, but designs relying on notifier priority for correctness are kind of fragile.

Would it be possible to make cpu_hotplug_pm_callback() do the BSP online check?

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

arch/x86/power/cpu.c +12 -0

+static int __init bsp_pm_check_init(void)
+{
+	/*
+	 * Set this bsp_pm_callback as lower priority than
+	 * cpu_hotplug_pm_callback. So cpu_hotplug_pm_callback will be called
+	 * earlier to disable cpu hotplug before bsp online check.
+	 */
+	pm_notifier(bsp_pm_callback, -INT_MAX);
+	return 0;
+}
+
+core_initcall(bsp_pm_check_init);

Quoted 8 lines · On Tuesday 16 of October 2012 11:05:18 Srivatsa S. Bhat wrote:

Quoted 7 lines · Rafael J. Wysocki

Quoted 6 lines · On Friday 12 of October 2012 09:09:42 Fenghua Yu wrote:

arch/x86/power/cpu.c void restore_processor_state(void)

Quoted 5 lines

From: Fenghua Yu <[email protected]>

arch/x86/power/cpu.c +3 -0

+
+/*
+ * When bsp_check() is called in hibernate and suspend, cpu

hotplug

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ * is disabled already. So it's unnessary to handle race

condition between

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

+ * cpumask query and cpu hotplug.
+ */

arch/x86/power/cpu.c +3 -0

+static int bsp_check(void)
+{
+	if (cpumask_first(cpu_online_mask) != 0) {

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/x86/power/cpu.c void restore_processor_state(void)

Quoted 8 lines

arch/x86/power/cpu.c +8 -0

+		pr_warn("CPU0 is offline.\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int bsp_pm_callback(struct notifier_block *nb, unsigned

long action,

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

arch/x86/power/cpu.c +9 -0

+			   void *ptr)
+{
+	int ret = 0;
+
+	switch (action) {
+	case PM_SUSPEND_PREPARE:
+	case PM_HIBERNATION_PREPARE:
+		ret = bsp_check();
+		break;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/x86/power/cpu.c void restore_processor_state(void) · line 271

Quoted 6 lines

arch/x86/power/cpu.c +6 -0

+	default:
+		break;
+	}
+	return notifier_from_errno(ret);
+}
+

I wonder if there's anything preventing CPU0 from becoming offline

after you've

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

done this check and before user space is frozen?

Hi Rafael,

bsp_pm_callback runs as a low priority notifier callback,

specifically with lower

Quoted 1 line

Quoted 1 line

priority than the cpu_hotplug_pm_callback (as mentioned in the

comment below).

Quoted 1 line

Quoted 1 line

And cpu_hotplug_pm_callback disables regular CPU hotplug (till the

suspend/resume

Quoted 1 line

Quoted 1 line

sequence is complete).. So there is no chance for CPU0 to become

offline after that.

Quoted 2 lines

Quoted 2 lines

Or, are you thinking of some other scenario where CPU0 can go

offline?

Quoted 2 lines

No, that should be fine technically, but designs relying on notifier

priority

Quoted 2 lines

for correctness are kind of fragile.

Hmm.. I agree.

Quoted 1 line

Would it be possible to make cpu_hotplug_pm_callback() do the BSP

online check?

Quoted 1 line

Good idea! I think that could be done quite easily. And while doing that, it would
be good to rethink what to do in patch 12/12 (Debug CPU0 hotplug) to fix the bug I
pointed out in my other mail.

Quoted 72 lines · Rafael J. Wysocki

Quoted 8 lines · On Tuesday 16 of October 2012 11:05:18 Srivatsa S. Bhat wrote:

Quoted 7 lines · Rafael J. Wysocki

Quoted 6 lines · On Friday 12 of October 2012 09:09:42 Fenghua Yu wrote:

arch/x86/power/cpu.c void restore_processor_state(void)

Quoted 5 lines

From: Fenghua Yu <[email protected]>

arch/x86/power/cpu.c +3 -0

+
+/*
+ * When bsp_check() is called in hibernate and suspend, cpu

hotplug

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ * is disabled already. So it's unnessary to handle race

condition between

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

+ * cpumask query and cpu hotplug.
+ */

arch/x86/power/cpu.c +3 -0

+static int bsp_check(void)
+{
+	if (cpumask_first(cpu_online_mask) != 0) {

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/x86/power/cpu.c void restore_processor_state(void)

Quoted 8 lines

arch/x86/power/cpu.c +8 -0

+		pr_warn("CPU0 is offline.\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int bsp_pm_callback(struct notifier_block *nb, unsigned

long action,

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

arch/x86/power/cpu.c +9 -0

+			   void *ptr)
+{
+	int ret = 0;
+
+	switch (action) {
+	case PM_SUSPEND_PREPARE:
+	case PM_HIBERNATION_PREPARE:
+		ret = bsp_check();
+		break;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/x86/power/cpu.c void restore_processor_state(void) · line 271

Quoted 6 lines

arch/x86/power/cpu.c +6 -0

+	default:
+		break;
+	}
+	return notifier_from_errno(ret);
+}
+

I wonder if there's anything preventing CPU0 from becoming offline

after you've

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

done this check and before user space is frozen?

Hi Rafael,

bsp_pm_callback runs as a low priority notifier callback,

specifically with lower

Quoted 1 line

Quoted 1 line

priority than the cpu_hotplug_pm_callback (as mentioned in the

comment below).

Quoted 1 line

Quoted 1 line

And cpu_hotplug_pm_callback disables regular CPU hotplug (till the

suspend/resume

Quoted 1 line

Quoted 1 line

sequence is complete).. So there is no chance for CPU0 to become

offline after that.

Quoted 2 lines

Quoted 2 lines

Or, are you thinking of some other scenario where CPU0 can go

offline?

Quoted 2 lines

No, that should be fine technically, but designs relying on notifier

priority

Quoted 2 lines

for correctness are kind of fragile.

Hmm.. I agree.

Quoted 1 line

Would it be possible to make cpu_hotplug_pm_callback() do the BSP

online check?

Quoted 1 line

Quoted 5 lines

Good idea! I think that could be done quite easily. And while doing that, it would
be good to rethink what to do in patch 12/12 (Debug CPU0 hotplug) to fix the bug I
pointed out in my other mail.

Why is this design relying on notifier priority fragile? I don't get it.

The priority in pm_notifier() is in a well designed API. This code just follows the API to assign lower priority for bsp_pm_callback than cpu_hotplug_pm_callback. Unless your justification is that the API of pm_notifier() is fragile, I think this patch should be ok.

It's not a good idea to move bsp_pm_callback() into cpu_hotplug_pm_callback(). There is no architecture specific hook to call x86 bsp specific hotplug in cpu_hotplug_pm_callback(). Moving bsp_pm_callback() into cpu_hotplug_pm_callback() is hacking while this patch follows well defined API and has better coding.

Quoted 1 line

Quoted 1 line · Rafael J. Wysocki

Quoted 4 lines · Fenghua

Quoted 4 lines · Rafael J. Wysocki

Quoted 3 lines · On Tuesday 16 of October 2012 11:05:18 Srivatsa S. Bhat wrote:

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · On Friday 12 of October 2012 09:09:42 Fenghua Yu wrote:

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

From: Fenghua Yu <[email protected]>

fix the bug I
pointed out in my other mail.

Because things like this are often overlooked by people trying to optimize
stuff and the fact that you have to add a comment explaining that dependency
only means that it is not exactly straightforward.

Moreover, you should also add a corresponding comment in the other notifier
indicating that its priority should be higher than the priority of the new thing and explaining why.

I really think that notifiers are fragile in general and this particular
one is no exception.

Quoted 6 lines

It's not a good idea to move bsp_pm_callback() into cpu_hotplug_pm_callback(). There is no architecture specific hook to call x86 bsp specific hotplug in cpu_hotplug_pm_callback(). Moving bsp_pm_callback() into cpu_hotplug_pm_callback() is hacking while this patch follows well defined API and has better coding.

I'm not sure it is better coding. You really want one piece of code to always follow another piece of code and the best way to ensure that is to execute them both explicitly in sequence.

Quoted 6 lines

Quoted 1 line

Quoted 1 line · Rafael J. Wysocki

Quoted 4 lines · Fenghua

Quoted 4 lines · Rafael J. Wysocki

Quoted 3 lines · On Tuesday 16 of October 2012 11:05:18 Srivatsa S. Bhat wrote:

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · On Friday 12 of October 2012 09:09:42 Fenghua Yu wrote:

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

From: Fenghua Yu <[email protected]>

fix the bug I
pointed out in my other mail.

Because things like this are often overlooked by people trying to optimize
stuff and the fact that you have to add a comment explaining that dependency
only means that it is not exactly straightforward.

If people try to optimize pm notifier, they really need to know pm_notifier() API and its priority. If they ignore the priority, they will screw up kernel no matter how many comments in it.

Quoted 5 lines

Moreover, you should also add a corresponding comment in the other notifier
indicating that its priority should be higher than the priority of the new thing and explaining why.

The comment in the patch explains this very clearly. I don't think it's necessary to add comments in other notifiers.

If adding comments in other notifiers each time when you add a new notifier, will you add 10 more comments in all other notifiers if you add 10 new notifiers?

I would think when people try to change notifier priority, they should know what they are going to do and search the notifiers and see the comments.

BTW, the other notifier is in generic code. Adding a paranoid comment in it doesn't seem to be worth. The comment in this patch code is very clear already.

Quoted 3 lines

I really think that notifiers are fragile in general and this particular
one is no exception.

If we think pm_notifier API is fragile, we need to fix the API instead of leaving it there and not allowing people to use it because it's suspected fragile.

It's simply not realistic to tell people not to use the API each time in code review while in the meantime the API and its priority are staying in kernel to allow people to use it.

Quoted 28 lines · Fenghua

Quoted 6 lines

Quoted 1 line

Quoted 1 line · Rafael J. Wysocki

Quoted 4 lines · Fenghua

Quoted 4 lines · Rafael J. Wysocki

Quoted 3 lines · On Tuesday 16 of October 2012 11:05:18 Srivatsa S. Bhat wrote:

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · On Friday 12 of October 2012 09:09:42 Fenghua Yu wrote:

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

From: Fenghua Yu <[email protected]>

fix the bug I
pointed out in my other mail.

Because things like this are often overlooked by people trying to optimize
stuff and the fact that you have to add a comment explaining that dependency
only means that it is not exactly straightforward.

If people try to optimize pm notifier, they really need to know pm_notifier() API and its priority. If they ignore the priority, they will screw up kernel no matter how many comments in it.

Quoted 5 lines

Moreover, you should also add a corresponding comment in the other notifier
indicating that its priority should be higher than the priority of the new thing and explaining why.

The comment in the patch explains this very clearly. I don't think it's necessary to add comments in other notifiers.

Quoted 2 lines

If adding comments in other notifiers each time when you add a new notifier, will you add 10 more comments in all other notifiers if you add 10 new notifiers?

Well, if there are strict ordering requirements regarding them, then those requirements should be documented in both places. Otherwise, if somebody looks at cpu_hotplug_pm_callback() alone, for example, he/she may not even realize that it has to be strictly ordered with respect to bsp_pm_callback().

Of course, you can argue that people should know what they are doing, but then reality is that it's quite easy to overlook things like that.

Quoted 6 lines

I would think when people try to change notifier priority, they should know what they are going to do and search the notifiers and see the comments.

BTW, the other notifier is in generic code. Adding a paranoid comment in it doesn't seem to be worth. The comment in this patch code is very clear already.

The problem is that it is generally difficult to find all subsystems that have registered notifiers in a given chain and figuring out dependencies between them is highly problematic. That's why I'm saying this is a fragile design - because it is so easy to break accidentally (and that's already happened in CPU hotplug, so I'm not making that up).

Quoted 8 lines

Quoted 4 lines

I really think that notifiers are fragile in general and this particular
one is no exception.

If we think pm_notifier API is fragile, we need to fix the API instead of leaving it there and not allowing people to use it because it's suspected fragile.

Because it is use by the existing code which generally works and may be broken while attempting to "fix" the API.

Quoted 3 lines

It's simply not realistic to tell people not to use the API each time in code review while in the meantime the API and its priority are staying in kernel to allow people to use it.

I don't quite agree. It sometimes is not worth changing code that's been around for a while already, because it's been tested in multiple configurations and changing it may cause things to break, although we may not like the APIs used by that code. That doesn't necessarily mean, however, that adding new code using those APIs is always a good idea.

In this particular case I just wondered if we could avoid adding more code that would use an API having known problems. The answer seems to be that it would cost some additional complexity that might not be worth it. This is a good answer, but you might have given it to me directly. :-)

From: Srivatsa S. Bhat [mailto:[email protected]]

Quoted 4 lines · Rafael J. Wysocki

Quoted 3 lines · On Friday 12 of October 2012 09:09:42 Fenghua Yu wrote:

From: Fenghua Yu <[email protected]>

Because x86 BIOS requires CPU0 to resume from sleep, suspend or

hibernate can't

Quoted 1 line

Quoted 1 line

be executed if CPU0 is detected offline. To make suspend or

hibernate and

Quoted 5 lines

Quoted 5 lines

further resume succeed, CPU0 must be online.

Signed-off-by Fenghua Yu <[email protected]>

---
 arch/x86/power/cpu.c |   44

++++++++++++++++++++++++++++++++++++++++++++

Quoted 14 lines

Quoted 7 lines

 1 files changed, 44 insertions(+), 0 deletions(-)

arch/x86/power/cpu.c +0 -0

index 218cdb1..adde775 100644

arch/x86/power/cpu.c void restore_processor_state(void) · line 243

Quoted 7 lines

arch/x86/power/cpu.c +4 -0

@@ -237,3 +237,47 @@ void restore_processor_state(void)
 #ifdef CONFIG_X86_32
 EXPORT_SYMBOL(restore_processor_state);
 #endif
+
+/*
+ * When bsp_check() is called in hibernate and suspend, cpu hotplug
+ * is disabled already. So it's unnessary to handle race condition

between

Quoted 5 lines

Quoted 5 lines

arch/x86/power/cpu.c +5 -0

+ * cpumask query and cpu hotplug.
+ */
+static int bsp_check(void)
+{
+	if (cpumask_first(cpu_online_mask) != 0) {

Quoted 8 lines

arch/x86/power/cpu.c void restore_processor_state(void)

Quoted 8 lines

arch/x86/power/cpu.c +8 -0

+		pr_warn("CPU0 is offline.\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int bsp_pm_callback(struct notifier_block *nb, unsigned long

action,

Quoted 9 lines

Quoted 9 lines

arch/x86/power/cpu.c +9 -0

+			   void *ptr)
+{
+	int ret = 0;
+
+	switch (action) {
+	case PM_SUSPEND_PREPARE:
+	case PM_HIBERNATION_PREPARE:
+		ret = bsp_check();
+		break;

Quoted 8 lines

arch/x86/power/cpu.c void restore_processor_state(void) · line 271

Quoted 6 lines

arch/x86/power/cpu.c +6 -0

+	default:
+		break;
+	}
+	return notifier_from_errno(ret);
+}
+

I wonder if there's anything preventing CPU0 from becoming offline

after you've

Quoted 2 lines

done this check and before user space is frozen?

Hi Rafael,

bsp_pm_callback runs as a low priority notifier callback, specifically with lower
priority than the cpu_hotplug_pm_callback (as mentioned in the comment below).
And cpu_hotplug_pm_callback disables regular CPU hotplug (till the suspend/resume
sequence is complete).. So there is no chance for CPU0 to become offline after that.

From: Fenghua Yu <[email protected]>

start_cpu0() is defined in head_64.S for 64-bit. The function sets up stack and jumps to start_secondary() for CPU0 wake up.

Signed-off-by Fenghua Yu <[email protected]>

---
 arch/x86/kernel/head_64.S |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

arch/x86/kernel/head_64.S +3 -0

index 94bf9cc..3faac8a 100644
@@ -252,6 +252,21 @@ ENTRY(secondary_startup_64)
 	pushq	%rax		# target address in negative space
 	lretq
 
+#ifdef CONFIG_HOTPLUG_CPU
+/*
+ * Boot CPU0 entry point. It's called from play_dead(). Everything has been set

From: Fenghua Yu <[email protected]>

Instead of waiting for STARTUP after INITs, BSP will execute the BIOS boot-strap code which is not a desired behavior for waking up BSP. To avoid the boot-strap code, wake up CPU0 by NMI instead.

This works to wake up soft offlined CPU0 only. If CPU0 is hard offlined (i.e. physically hot removed and then hot added), NMI won't wake it up. We'll change this code in the future to wake up hard offlined CPU0 if real platform and request are available.

AP is still waken up as before by INIT, SIPI, SIPI sequence.

Signed-off-by Fenghua Yu <[email protected]>

---
 arch/x86/include/asm/cpu.h |    1 +
 arch/x86/kernel/smpboot.c  |  112 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 107 insertions(+), 6 deletions(-)

arch/x86/kernel/smpboot.c +29 -2

@@ -727,8 +745,47 @@ static int __cpuinit do_boot_cpu(int apicid, int cpu, struct task_struct *idle)
 	 */
 	if (apic->wakeup_secondary_cpu)
 		boot_error = apic->wakeup_secondary_cpu(apicid, start_ip);
-	else
-		boot_error = wakeup_secondary_cpu_via_init(apicid, start_ip);
+	else {
+		if (cpu)
+			/*
+			 * Wake up AP by INIT, INIT, STARTUP sequence.
+			 */
+			boot_error = wakeup_secondary_cpu_via_init(apicid,
+								   start_ip);
+		else {
+			/*
+			 * Instead of waiting for STARTUP after INITs, BSP will
+			 * execute the BIOS boot-strap code which is not a
+			 * desired behavior for waking up BSP. To avoid the
+			 * boot-strap code, wake up CPU0 by NMI instead.
+			 *
+			 * This works to wake up soft offlined CPU0 only. If
+			 * CPU0 is hard offlined (i.e. physically hot removed
+			 * and then hot added), NMI won't wake it up. We'll
+			 * change this code in the future to wake up hard
+			 * offlined CPU0 if real platform and request are
+			 * available.
+			 */
+			int id;
+
+			enable_start_cpu0 = 1;
+			/*
+			 * Register a NMI handler to help wake up CPU0.
+			 */
+			boot_error = register_nmi_handler(NMI_LOCAL,
+					 wakeup_cpu0_nmi, 0, "wake_cpu0");

From: Fenghua Yu <[email protected]>

arch/x86/Kconfig +4 -0

+config DEBUG_HOTPLUG_CPU0
+	def_bool n
+	prompt "Debug CPU0 hotplug"
+	depends on HOTPLUG_CPU && EXPERIMENTAL

arch/x86/kernel/topology.c +51 -0

+#ifdef CONFIG_DEBUG_HOTPLUG_CPU0
+/*
+ * This function offlines a CPU as early as possible and allows userspace to
+ * boot up without the CPU. The CPU can be onlined back by user after boot.
+ *
+ * This is only called for debugging CPU offline/online feature.
+ */
+int __ref _debug_hotplug_cpu(int cpu, int action)
+{
+	struct device *dev = get_cpu_device(cpu);
+	int ret;
+
+	if (!cpu_is_hotpluggable(cpu))
+		return -EINVAL;
+
+	cpu_hotplug_driver_lock();
+
+	switch (action) {
+	case 0:
+		ret = cpu_down(cpu);
+		if (!ret) {
+			pr_info("CPU %u is now offline\n", cpu);
+			kobject_uevent(&dev->kobj, KOBJ_OFFLINE);
+		} else
+			pr_debug("Can't offline CPU%d.\n", cpu);
+		break;
+	case 1:
+		ret = cpu_up(cpu);
+		if (!ret)
+			kobject_uevent(&dev->kobj, KOBJ_ONLINE);
+		else
+			pr_debug("Can't online CPU%d.\n", cpu);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	cpu_hotplug_driver_unlock();
+
+	return ret;
+}
+
+static int __init debug_hotplug_cpu(void)
+{
+	_debug_hotplug_cpu(0, 0);
+	return 0;
+}
+
+late_initcall_sync(debug_hotplug_cpu);
+#endif /* CONFIG_DEBUG_HOTPLUG_CPU0 */
+
 int __ref arch_register_cpu(int num)
 {
 	struct cpuinfo_x86 *c = &cpu_data(num);

arch/x86/power/cpu.c +1 -0

index adde775..120cee1 100644
@@ -21,6 +21,7 @@
 #include <asm/suspend.h>
 #include <asm/debugreg.h>
 #include <asm/fpu-internal.h> /* pcntxt_mask */
+#include <asm/cpu.h>
 
 #ifdef CONFIG_X86_32
 static struct saved_context saved_context;

@@ -263,6 +264,43 @@ static int bsp_pm_callback(struct notifier_block *nb, unsigned long action,
 	case PM_HIBERNATION_PREPARE:
 		ret = bsp_check();

arch/x86/power/cpu.c +28 -0

+		break;
+	case PM_POST_RESTORE:
+		/*
+		 * When a resume really happens, this code won't be called.
+		 *
+		 * This code is called only when user space hibernation software
+		 * prepares for snapshot device during boot time. So we just
+		 * call _debug_hotplug_cpu() to restore to CPU0's state prior to
+		 * preparing the snapshot device.
+		 *
+		 * This works for normal boot case in our CPU0 hotplug debug
+		 * mode, i.e. CPU0 is offline and user mode hibernation
+		 * software initializes during boot time.
+		 *
+		 * If CPU0 is online and user application accesses snapshot
+		 * device after boot time, this will offline CPU0 and user may
+		 * see different CPU0 state before and after accessing
+		 * the snapshot device. But hopefully this is not a case when
+		 * user debugging CPU0 hotplug. Even if users hit this case,
+		 * they can easily online CPU0 back.
+		 *
+		 * To simplify this debug code, we only consider normal boot
+		 * case. Otherwise we need to remember CPU0's state and restore
+		 * to that state and resolve racy conditions etc.
+		 */
+		_debug_hotplug_cpu(0, 0);
+		break;
+#endif
 	default:
 		break;
 	}

Quoted 1 line

+#ifdef CONFIG_DEBUG_HOTPLUG_CPU0

arch/x86/power/cpu.c +1 -0

+	case PM_RESTORE_PREPARE:

Review

Quoted 2 lines

(quoted diff) +2 -0

+		/*
+		 * When system resumes from hibernation, online CPU0

because

arch/x86/power/cpu.c static int bsp_pm_callback(struct notifier_block *nb, unsigned long action, · line 275

Quoted 5 lines

arch/x86/power/cpu.c +5 -0

+		 * 1. it's required for resume and
+		 * 2. the CPU was online before hibernation
+		 */
+		if (!cpu_online(0))
+			_debug_hotplug_cpu(0, 1);

This won't work. CPU hotplug is disabled by cpu_hotplug_pm_callback() during
PM_HIBERNATION_PREPARE and is enabled back only during PM_POST_HIBERNATION.

So calls to cpu_up() or cpu_down() will fail in the meantime.

Quoted 16 lines

Quoted 1 line

+#ifdef CONFIG_DEBUG_HOTPLUG_CPU0

arch/x86/power/cpu.c +1 -0

+	case PM_RESTORE_PREPARE:

Review

Quoted 2 lines

(quoted diff) +2 -0

+		/*
+		 * When system resumes from hibernation, online CPU0

because

arch/x86/power/cpu.c static int bsp_pm_callback(struct notifier_block *nb, unsigned long action, · line 275

Quoted 5 lines

arch/x86/power/cpu.c +5 -0

+		 * 1. it's required for resume and
+		 * 2. the CPU was online before hibernation
+		 */
+		if (!cpu_online(0))
+			_debug_hotplug_cpu(0, 1);

This won't work. CPU hotplug is disabled by cpu_hotplug_pm_callback() during
PM_HIBERNATION_PREPARE and is enabled back only during PM_POST_HIBERNATION.

Quoted 1 line

So calls to cpu_up() or cpu_down() will fail in the meantime.

The code works just fine.

You are talking about hibernation/suspend phase. This piece of code is all about restore phase. Of course you can call cpu_up() and cpu_down() during restore phase.

So there is no problem here.

From: Srivatsa S. Bhat [mailto:[email protected]]
Sent: Wednesday, October 17, 2012 11:43 PM

Quoted 18 lines · Fenghua

Quoted 16 lines

Quoted 1 line

+#ifdef CONFIG_DEBUG_HOTPLUG_CPU0

arch/x86/power/cpu.c +1 -0

+	case PM_RESTORE_PREPARE:

Review

Quoted 2 lines

(quoted diff) +2 -0

+		/*
+		 * When system resumes from hibernation, online CPU0

because

arch/x86/power/cpu.c static int bsp_pm_callback(struct notifier_block *nb, unsigned long action, · line 275

Quoted 5 lines

arch/x86/power/cpu.c +5 -0

+		 * 1. it's required for resume and
+		 * 2. the CPU was online before hibernation
+		 */
+		if (!cpu_online(0))
+			_debug_hotplug_cpu(0, 1);

This won't work. CPU hotplug is disabled by cpu_hotplug_pm_callback() during
PM_HIBERNATION_PREPARE and is enabled back only during PM_POST_HIBERNATION.

Quoted 1 line

So calls to cpu_up() or cpu_down() will fail in the meantime.

Quoted 8 lines

The code works just fine.

You are talking about hibernation/suspend phase. This piece of code is all about restore phase. Of course you can call cpu_up() and cpu_down() during restore phase.

So there is no problem here.

I looked into it again, and actually you are right, it'll work fine; but the reason *why* it will work fine is quite subtle.

Note that there is a difference between calling cpu_up() or cpu_down() vs calling _cpu_up() or _cpu_down(). The former set of calls will return
-EBUSY if the 'cpu_hotplug_disabled' flag is set. The latter set of calls
don't care about that flag and hence proceed irrespective of the state of
that flag. The former set of calls are used when you echo 0/1 to the online
file from userspace. Whereas the latter set of calls are used by the disable/enable_nonboot_cpus() functions. This is how you can disable cpu hotplug by userspace, but continue to do cpu hotplug yourself from within the kernel in the suspend/hibernation/restore phases.

Now, coming to the notifications part, earlier, I was referring to the entire hibernate->restore phase, not just the hibernate phase. The relevant
notifications that are sent out during this entire (successful) sequence are:

-> PM_HIBERNATION_PREPARE
-> PM_RESTORE_PREPARE
-> PM_POST_HIBERNATION

cpu_hotplug_pm_callback() will set the cpu_hotplug_disabled flag during PM_HIBERNATION_PREPARE and clears it only during PM_POST_HIBERNATION. So in-between, if you invoke cpu_up() or cpu_down(), it should return - EBUSY.

Then how is it that this patch can work fine? The answer is, during the restore phase, you are not yet in the kernel-to-be-restored. IOW, that cpu_hotplug_disabled flag is clear in that kernel. That's why cpu_up() and
cpu_down() will continue to work at that point. But once you resume execution
from the saved hibernation image, you won't be able to do cpu_up() and cpu_down() until that flag is cleared by cpu_hotplug_pm_callback() during
PM_POST_HIBERNATION.

All in all, its tricky, and luckily this patch uses cpu_up()/cpu_down() at the right moment...

I don't actually see why cpu_hotplug_pm_callback() should not clear the cpu_hotplug_disabled flag during PM_RESTORE_PREPARE itself.. if we do that,
we don't have to be *this* careful about -when- we can invoke -which- function..