[PATCH v3 0/3] CPU hotplug, Freezer: Fix bugs in CPU hotplug call path

[PATCH v3 0/3] CPU hotplug, Freezer: Fix bugs in CPU hotplug call path

[Srivatsa S. Bhat]

When using the CPU hotplug infrastructure to offline/online CPUs, the cpu_up()
and cpu_down() functions are used, which internally call _cpu_up() and
_cpu_down() with the second argument *always* set to 0. The second argument
is "tasks_frozen", which should be correctly set to 1 when tasks have been
frozen, even when the freezing of tasks may be due to an event unrelated
to CPU hotplug, such as a suspend operation in progress, in which case the
freezer subsystem freezes all the freezable tasks.

Not giving the correct value for the 'tasks_frozen' argument can potentially
lead to a lot of issues since this will send wrong notifications via the
notifier mechanism leading to the execution of inappropriate code by the
callbacks registered for the notifiers. That is, instead of CPU_ONLINE_FROZEN
and CPU_DEAD_FROZEN notifications, it results in CPU_ONLINE and CPU_DEAD
notifications to be sent all the time, irrespective of the actual state of
the system (i.e., whether tasks are frozen or not).

This patch introduces a flag to indicate whether the tasks are frozen are not
(by any event) and modifies cpu_up() and cpu_down() functions to check the
value of this flag and accordingly call _cpu_up() and _cpu_down() respectively
by supplying the correct value as the second argument based on the state of
the system. This in turn means that the correct notifications are sent, thus
ensuring that all the registered callbacks do the right thing.

Additionally, to ensure that the tasks are not frozen or thawed by the freezer
subsystem while the registered callbacks are running, this patch adds a few
notifications in the freezer which is then hooked onto by the CPU hotplug
code, to avoid this race.

v3: * Added synchronization between CPU hotplug and the freezer subsystem
      without introducing any new locks in the CPU hotplug call path.

v2: * Removed the atomic_t declaration of tasks_frozen flag and the
      atomic_[set|read] functions since they were unnecessary.
    * Updated the changelog to give an example scenario where things could go
      wrong due to the bug in the CPU hotplug call path.

References:
v1 -> http://thread.gmane.org/gmane.linux.kernel/1198312/
v2 -> http://thread.gmane.org/gmane.linux.kernel/1198312/focus=1199087

--
Srivatsa S. Bhat (3):
      CPU hotplug, Freezer: Don't hardcode 'tasks_frozen' argument in _cpu_*()
      CPU hotplug, Freezer: Synchronize CPU hotplug and freezer
      PM, Freezer, Docs: Update documentation about the new freezer notifiers


 Documentation/power/notifiers.txt |    8 ++++
 include/linux/freezer.h           |    2 +
 include/linux/suspend.h           |    6 ++-
 kernel/cpu.c                      |   76 ++++++++++++++++++++++++++++++++++++-
 kernel/power/process.c            |   32 +++++++++++++++-
 5 files changed, 120 insertions(+), 4 deletions(-)

[PATCH v3 1/3] CPU hotplug, Freezer: Don't hardcode 'tasks_frozen' argument in _cpu_*()

[Srivatsa S. Bhat]

This patch introduces a 'tasks_frozen' flag that is set whenever tasks are
frozen by the freezer and reset whenever the tasks are thawed. The value of
this flag is then checked by the CPU hotplug code in order to pass the
correct argument to the _cpu_up() and _cpu_down() functions, thereby
reflecting the correct state of the system.

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

 include/linux/freezer.h |    2 ++
 kernel/cpu.c            |    5 +++--
 kernel/power/process.c  |   22 +++++++++++++++++++++-
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index 1effc8b..a5283ce 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -50,6 +50,7 @@ extern int thaw_process(struct task_struct *p);
 extern void refrigerator(void);
 extern int freeze_processes(void);
 extern void thaw_processes(void);
+extern int tasks_are_frozen(void);
 
 static inline int try_to_freeze(void)
 {
@@ -173,6 +174,7 @@ static inline int thaw_process(struct task_struct *p) { return 1; }
 static inline void refrigerator(void) {}
 static inline int freeze_processes(void) { BUG(); return 0; }
 static inline void thaw_processes(void) {}
+static inline int tasks_are_frozen(void) { return 0; }
 
 static inline int try_to_freeze(void) { return 0; }
 
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 12b7458..e889ffd 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -15,6 +15,7 @@
 #include <linux/stop_machine.h>
 #include <linux/mutex.h>
 #include <linux/gfp.h>
+#include <linux/freezer.h>
 
 #ifdef CONFIG_SMP
 /* Serializes the updates to cpu_online_mask, cpu_present_mask */
@@ -280,7 +281,7 @@ int __ref cpu_down(unsigned int cpu)
 		goto out;
 	}
 
-	err = _cpu_down(cpu, 0);
+	err = _cpu_down(cpu, tasks_are_frozen());
 
 out:
 	cpu_maps_update_done();
@@ -373,7 +374,7 @@ int __cpuinit cpu_up(unsigned int cpu)
 		goto out;
 	}
 
-	err = _cpu_up(cpu, 0);
+	err = _cpu_up(cpu, tasks_are_frozen());
 
 out:
 	cpu_maps_update_done();
diff --git a/kernel/power/process.c b/kernel/power/process.c
index 0cf3a27..230bf96 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -17,11 +17,28 @@
 #include <linux/delay.h>
 #include <linux/workqueue.h>
 
-/* 
+/*
  * Timeout for stopping processes
  */
 #define TIMEOUT	(20 * HZ)
 
+static int tasks_frozen;
+
+static void set_tasks_frozen_flag(void)
+{
+	tasks_frozen = 1;
+}
+
+static void clear_tasks_frozen_flag(void)
+{
+	tasks_frozen = 0;
+}
+
+int tasks_are_frozen(void)
+{
+	return tasks_frozen;
+}
+
 static inline int freezable(struct task_struct * p)
 {
 	if ((p == current) ||
@@ -151,6 +168,8 @@ int freeze_processes(void)
 	error = try_to_freeze_tasks(false);
 	if (error)
 		goto Exit;
+
+	set_tasks_frozen_flag();
 	printk("done.");
 
 	oom_killer_disable();
@@ -189,6 +208,7 @@ void thaw_processes(void)
 	thaw_workqueues();
 	thaw_tasks(true);
 	thaw_tasks(false);
+	clear_tasks_frozen_flag();
 	schedule();
 	printk("done.\n");
 }

[PATCH v3 2/3] CPU hotplug, Freezer: Synchronize CPU hotplug and freezer

[Srivatsa S. Bhat]

Don't allow the freezer subsystem to race with CPU hotplug to ensure that
during the entire duration for which the CPU hotplug notifications are run,
the state of the system (with respect to the tasks being frozen or not)
remains constant.

This patch introduces four notifications in the freezer subsystem,
PM_FREEZE_PREPARE, PM_POST_FREEZE, PM_THAW_PREPARE and PM_POST_THAW.
These help in making other subsystems aware of the freezer's activity.
The CPU hotplug infrastructure hooks on to these notifications and
synchronizes with the freezer.

Specifically:

* Upon any freezer notification that indicates a change about to happen to
  the tasks' frozen/unfrozen state, such as PM_FREEZE_PREPARE or
  PM_THAW_PREPARE, the CPU hotplug callback for these events disables future
  (regular) CPU hotplugging and also ensures that any currently running CPU
  hotplug operation is completed before allowing the freezer to continue.

* Upon freezer notifications that indicate a completion of an action such as
  freezing or thawing of processes, the CPU hotplug callback handler
  re-enables regular CPU hotplug.

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

 include/linux/suspend.h |    6 +++-
 kernel/cpu.c            |   71 +++++++++++++++++++++++++++++++++++++++++++++++
 kernel/power/process.c  |   10 +++++++
 3 files changed, 86 insertions(+), 1 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 6bbcef2..e526543 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -268,13 +268,17 @@ static inline int hibernate(void) { return -ENOSYS; }
 static inline bool system_entering_hibernation(void) { return false; }
 #endif /* CONFIG_HIBERNATION */
 
-/* Hibernation and suspend events */
+/* Hibernation, suspend and freezer events */
 #define PM_HIBERNATION_PREPARE	0x0001 /* Going to hibernate */
 #define PM_POST_HIBERNATION	0x0002 /* Hibernation finished */
 #define PM_SUSPEND_PREPARE	0x0003 /* Going to suspend the system */
 #define PM_POST_SUSPEND		0x0004 /* Suspend finished */
 #define PM_RESTORE_PREPARE	0x0005 /* Going to restore a saved image */
 #define PM_POST_RESTORE		0x0006 /* Restore failed */
+#define PM_FREEZE_PREPARE	0x0007 /* Going to freeze tasks */
+#define PM_POST_FREEZE  	0x0008 /* Freezing of tasks finished */
+#define PM_THAW_PREPARE  	0x0009 /* Going to thaw tasks */
+#define PM_POST_THAW    	0x000A /* Thawing of tasks finished */
 
 #ifdef CONFIG_PM_SLEEP
 void save_processor_state(void);
diff --git a/kernel/cpu.c b/kernel/cpu.c
index e889ffd..3d14385 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -16,6 +16,7 @@
 #include <linux/mutex.h>
 #include <linux/gfp.h>
 #include <linux/freezer.h>
+#include <linux/suspend.h>
 
 #ifdef CONFIG_SMP
 /* Serializes the updates to cpu_online_mask, cpu_present_mask */
@@ -479,6 +480,76 @@ static int alloc_frozen_cpus(void)
 core_initcall(alloc_frozen_cpus);
 #endif /* CONFIG_PM_SLEEP_SMP */
 
+
+#ifdef CONFIG_FREEZER
+
+/*
+ * Avoid CPU hotplug racing with the freezer subsystem, by disabling CPU
+ * hotplug when tasks are about to be frozen or thawed.
+ * Don't allow the freezer subsystem to continue until any currently running
+ * CPU hotplug operation gets completed.
+ */
+static void cpu_hotplug_freezer_block_begin(void)
+{
+	cpu_maps_update_begin();
+	cpu_hotplug_disabled = 1;
+	cpu_maps_update_done();
+}
+
+
+/*
+ * When freezing or thawing of tasks is complete, re-enable CPU hotplug (which
+ * was disabled when freezing/thawing had begun).
+ */
+static void cpu_hotplug_freezer_block_done(void)
+{
+	cpu_maps_update_begin();
+	cpu_hotplug_disabled = 0;
+	cpu_maps_update_done();
+}
+
+
+/*
+ * Avoid CPU hotplug and the freezer subsystem from racing with each other,
+ * so that when CPU hotplug notifications are being sent (i.e., the
+ * registered callbacks being executed), the state of the system reported
+ * by the notifier (with respect to the tasks being frozen or not) is
+ * consistent with the actual state of the system, *throughout the duration*
+ * during which the CPU hotplug notifications are active.
+ */
+static int
+cpu_hotplug_freezer_callback(struct notifier_block *nb,
+				unsigned long action, void *ptr)
+{
+	switch (action) {
+
+	case PM_FREEZE_PREPARE:
+	case PM_THAW_PREPARE:
+		cpu_hotplug_freezer_block_begin();
+		break;
+
+	case PM_POST_FREEZE:
+	case PM_POST_THAW:
+		cpu_hotplug_freezer_block_done();
+		break;
+
+	default:
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
+
+static void cpu_hotplug_freezer_sync_init(void)
+{
+	pm_notifier(cpu_hotplug_freezer_callback, 0);
+}
+core_initcall(cpu_hotplug_freezer_sync_init);
+
+#endif /* CONFIG_FREEZER */
+
+
 /**
  * notify_cpu_starting(cpu) - call the CPU_STARTING notifiers
  * @cpu: cpu that just started
diff --git a/kernel/power/process.c b/kernel/power/process.c
index 230bf96..9b7e07a 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -17,6 +17,8 @@
 #include <linux/delay.h>
 #include <linux/workqueue.h>
 
+#include "power.h"
+
 /*
  * Timeout for stopping processes
  */
@@ -158,6 +160,8 @@ int freeze_processes(void)
 {
 	int error;
 
+	pm_notifier_call_chain(PM_FREEZE_PREPARE);
+
 	printk("Freezing user space processes ... ");
 	error = try_to_freeze_tasks(true);
 	if (error)
@@ -177,6 +181,8 @@ int freeze_processes(void)
 	BUG_ON(in_atomic());
 	printk("\n");
 
+	pm_notifier_call_chain(PM_POST_FREEZE);
+
 	return error;
 }
 
@@ -202,6 +208,8 @@ static void thaw_tasks(bool nosig_only)
 
 void thaw_processes(void)
 {
+	pm_notifier_call_chain(PM_THAW_PREPARE);
+
 	oom_killer_enable();
 
 	printk("Restarting tasks ... ");
@@ -211,5 +219,7 @@ void thaw_processes(void)
 	clear_tasks_frozen_flag();
 	schedule();
 	printk("done.\n");
+
+	pm_notifier_call_chain(PM_POST_THAW);
 }
 

Re: [PATCH v3 2/3] CPU hotplug, Freezer: Synchronize CPU hotplug and freezer

[Alan Stern]

On Fri, 21 Oct 2011, Srivatsa S. Bhat wrote:

> Don't allow the freezer subsystem to race with CPU hotplug to ensure that
> during the entire duration for which the CPU hotplug notifications are run,
> the state of the system (with respect to the tasks being frozen or not)
> remains constant.
> 
> This patch introduces four notifications in the freezer subsystem,
> PM_FREEZE_PREPARE, PM_POST_FREEZE, PM_THAW_PREPARE and PM_POST_THAW.
> These help in making other subsystems aware of the freezer's activity.
> The CPU hotplug infrastructure hooks on to these notifications and
> synchronizes with the freezer.

It seems pointless to send out PM_POST_FREEZE and PM_THAW_PREPARE 
notifications.  No task will be running to be affected by them.

Alan Stern

[PATCH v3 3/3] PM, Freezer, Docs: Update documentation about the new freezer notifiers

[Srivatsa S. Bhat]

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

 Documentation/power/notifiers.txt |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/Documentation/power/notifiers.txt b/Documentation/power/notifiers.txt
index c2a4a34..f47fe30 100644
--- a/Documentation/power/notifiers.txt
+++ b/Documentation/power/notifiers.txt
@@ -37,6 +37,14 @@ PM_POST_SUSPEND		The system has just resumed or an error occurred during
 			suspend.  Device drivers' resume callbacks have been
 			executed and tasks have been thawed.
 
+PM_FREEZE_PREPARE	Freezing of tasks is about to begin.
+
+PM_POST_FREEZE		Freezing of tasks has been completed.
+
+PM_THAW_PREPARE   	Thawing of tasks is about to begin.
+
+PM_POST_THAW     	Thawing of tasks has been completed.
+
 It is generally assumed that whatever the notifiers do for
 PM_HIBERNATION_PREPARE, should be undone for PM_POST_HIBERNATION.  Analogously,
 operations performed for PM_SUSPEND_PREPARE should be reversed for

Re: [PATCH v3 0/3] CPU hotplug, Freezer: Fix bugs in CPU hotplug call path

[Alan Stern]

On Fri, 21 Oct 2011, Srivatsa S. Bhat wrote:

> When using the CPU hotplug infrastructure to offline/online CPUs, the cpu_up()
> and cpu_down() functions are used, which internally call _cpu_up() and
> _cpu_down() with the second argument *always* set to 0. The second argument
> is "tasks_frozen", which should be correctly set to 1 when tasks have been
> frozen, even when the freezing of tasks may be due to an event unrelated
> to CPU hotplug, such as a suspend operation in progress, in which case the
> freezer subsystem freezes all the freezable tasks.
> 
> Not giving the correct value for the 'tasks_frozen' argument can potentially
> lead to a lot of issues since this will send wrong notifications via the
> notifier mechanism leading to the execution of inappropriate code by the
> callbacks registered for the notifiers. That is, instead of CPU_ONLINE_FROZEN
> and CPU_DEAD_FROZEN notifications, it results in CPU_ONLINE and CPU_DEAD
> notifications to be sent all the time, irrespective of the actual state of
> the system (i.e., whether tasks are frozen or not).
> 
> This patch introduces a flag to indicate whether the tasks are frozen are not
> (by any event) and modifies cpu_up() and cpu_down() functions to check the
> value of this flag and accordingly call _cpu_up() and _cpu_down() respectively
> by supplying the correct value as the second argument based on the state of
> the system. This in turn means that the correct notifications are sent, thus
> ensuring that all the registered callbacks do the right thing.

That doesn't make any sense.  If tasks were frozen then the task 
calling _cpu_up() or _cpu_down() would be frozen too, and therefore 
wouldn't be able to make the call.

> Additionally, to ensure that the tasks are not frozen or thawed by the freezer
> subsystem while the registered callbacks are running, this patch adds a few
> notifications in the freezer which is then hooked onto by the CPU hotplug
> code, to avoid this race.

That's more sensible.  Freezing or thawing tasks isn't instantaneous.  
It's possible that _cpu_up() or _cpu_down() could be called while some 
tasks were frozen and others were still running.

If you're careful to prevent this from happening then there's no reason 
to change the tasks_frozen argument.  It should never be anything but 
0 in this situation.

Alan Stern

Re: [PATCH v3 0/3] CPU hotplug, Freezer: Fix bugs in CPU hotplug call path

[Srivatsa S. Bhat]

Hi Alan,

On 10/21/2011 08:12 PM, Alan Stern wrote:
> On Fri, 21 Oct 2011, Srivatsa S. Bhat wrote:
> 
>> When using the CPU hotplug infrastructure to offline/online CPUs, the cpu_up()
>> and cpu_down() functions are used, which internally call _cpu_up() and
>> _cpu_down() with the second argument *always* set to 0. The second argument
>> is "tasks_frozen", which should be correctly set to 1 when tasks have been
>> frozen, even when the freezing of tasks may be due to an event unrelated
>> to CPU hotplug, such as a suspend operation in progress, in which case the
>> freezer subsystem freezes all the freezable tasks.
>>
>> Not giving the correct value for the 'tasks_frozen' argument can potentially
>> lead to a lot of issues since this will send wrong notifications via the
>> notifier mechanism leading to the execution of inappropriate code by the
>> callbacks registered for the notifiers. That is, instead of CPU_ONLINE_FROZEN
>> and CPU_DEAD_FROZEN notifications, it results in CPU_ONLINE and CPU_DEAD
>> notifications to be sent all the time, irrespective of the actual state of
>> the system (i.e., whether tasks are frozen or not).
>>
>> This patch introduces a flag to indicate whether the tasks are frozen are not
>> (by any event) and modifies cpu_up() and cpu_down() functions to check the
>> value of this flag and accordingly call _cpu_up() and _cpu_down() respectively
>> by supplying the correct value as the second argument based on the state of
>> the system. This in turn means that the correct notifications are sent, thus
>> ensuring that all the registered callbacks do the right thing.
> 
> That doesn't make any sense.  If tasks were frozen then the task 
> calling _cpu_up() or _cpu_down() would be frozen too, and therefore 
> wouldn't be able to make the call.
>

I can't believe I missed such an obvious thing!
I must have been carried away while thinking about the race between freezer
and CPU hotplug. Thanks for pointing this out.
 
>> Additionally, to ensure that the tasks are not frozen or thawed by the freezer
>> subsystem while the registered callbacks are running, this patch adds a few
>> notifications in the freezer which is then hooked onto by the CPU hotplug
>> code, to avoid this race.
> 
> That's more sensible.  Freezing or thawing tasks isn't instantaneous.  
> It's possible that _cpu_up() or _cpu_down() could be called while some 
> tasks were frozen and others were still running.
> 
> If you're careful to prevent this from happening then there's no reason 
> to change the tasks_frozen argument.  It should never be anything but 
> 0 in this situation.
> 

Agreed. So patch 1 becomes unnecessary. Patch 2 implements the needed
synchronization.
I'll fix the code (also incorporating your point that PM_POST_FREEZE and
PM_THAW_PREPARE are unnecessary) and repost.

Thank you for the review.

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