[PATCH] CPU hotplug, freezer: Fix bugs in CPU hotplug call path

[PATCH] 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 a wrong notification 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_OFFLINE_FROZEN notifications, it results in
CPU_ONLINE and CPU_OFFLINE 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 adds a check in cpu_up() and cpu_down() to see whether
the tasks have been frozen (by any event) so that they can 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.

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

 include/linux/freezer.h |    4 ++++
 kernel/cpu.c            |    9 +++++++--
 kernel/power/process.c  |   24 +++++++++++++++++++++++-
 3 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index 1effc8b..3a37d8b 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -51,6 +51,10 @@ extern void refrigerator(void);
 extern int freeze_processes(void);
 extern void thaw_processes(void);
 
+extern void set_tasks_frozen_flag(void);
+extern void clear_tasks_frozen_flag(void);
+extern int tasks_are_frozen(void);
+
 static inline int try_to_freeze(void)
 {
 	if (freezing(current)) {
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 12b7458..e4530ea 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 */
@@ -272,6 +273,7 @@ out_release:
 int __ref cpu_down(unsigned int cpu)
 {
 	int err;
+	int tasks_frozen;
 
 	cpu_maps_update_begin();
 
@@ -280,7 +282,8 @@ int __ref cpu_down(unsigned int cpu)
 		goto out;
 	}
 
-	err = _cpu_down(cpu, 0);
+	tasks_frozen = tasks_are_frozen();
+	err = _cpu_down(cpu, tasks_frozen);
 
 out:
 	cpu_maps_update_done();
@@ -328,6 +331,7 @@ out_notify:
 int __cpuinit cpu_up(unsigned int cpu)
 {
 	int err = 0;
+	int tasks_frozen;
 
 #ifdef	CONFIG_MEMORY_HOTPLUG
 	int nid;
@@ -373,7 +377,8 @@ int __cpuinit cpu_up(unsigned int cpu)
 		goto out;
 	}
 
-	err = _cpu_up(cpu, 0);
+	tasks_frozen = tasks_are_frozen();
+	err = _cpu_up(cpu, tasks_frozen);
 
 out:
 	cpu_maps_update_done();
diff --git a/kernel/power/process.c b/kernel/power/process.c
index 0cf3a27..7642314 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -17,11 +17,30 @@
 #include <linux/delay.h>
 #include <linux/workqueue.h>
 
-/* 
+/*
  * Timeout for stopping processes
  */
 #define TIMEOUT	(20 * HZ)
 
+static atomic_t tasks_frozen = ATOMIC_INIT(0);
+
+void set_tasks_frozen_flag(void)
+{
+	atomic_set(&tasks_frozen, 1);
+}
+
+void clear_tasks_frozen_flag(void)
+{
+	atomic_set(&tasks_frozen, 0);
+}
+
+int tasks_are_frozen(void)
+{
+	int ret;
+	ret = atomic_read(&tasks_frozen);
+	return ret;
+}
+
 static inline int freezable(struct task_struct * p)
 {
 	if ((p == current) ||
@@ -153,6 +172,8 @@ int freeze_processes(void)
 		goto Exit;
 	printk("done.");
 
+	set_tasks_frozen_flag();
+
 	oom_killer_disable();
  Exit:
 	BUG_ON(in_atomic());
@@ -191,5 +212,6 @@ void thaw_processes(void)
 	thaw_tasks(false);
 	schedule();
 	printk("done.\n");
+	clear_tasks_frozen_flag();
 }
 

Re: [PATCH] CPU hotplug, freezer: Fix bugs in CPU hotplug call path

[Peter Zijlstra]

On Mon, 2011-10-03 at 01:14 +0530, Srivatsa S. Bhat wrote:
> +static atomic_t tasks_frozen = ATOMIC_INIT(0);
> +
> +void set_tasks_frozen_flag(void)
> +{
> +       atomic_set(&tasks_frozen, 1);
> +}
> +
> +void clear_tasks_frozen_flag(void)
> +{
> +       atomic_set(&tasks_frozen, 0);
> +}
> +
> +int tasks_are_frozen(void)
> +{
> +       int ret;
> +       ret = atomic_read(&tasks_frozen);
> +       return ret;
> +} 

What's the point of using atomic_t here? Neither set nor read are
actually atomic ops.

Re: [PATCH] CPU hotplug, freezer: Fix bugs in CPU hotplug call path

[Srivatsa S. Bhat]

On 10/03/2011 03:33 PM, Peter Zijlstra wrote:
> On Mon, 2011-10-03 at 01:14 +0530, Srivatsa S. Bhat wrote:
>> +static atomic_t tasks_frozen = ATOMIC_INIT(0);
>> +
>> +void set_tasks_frozen_flag(void)
>> +{
>> +       atomic_set(&tasks_frozen, 1);
>> +}
>> +
>> +void clear_tasks_frozen_flag(void)
>> +{
>> +       atomic_set(&tasks_frozen, 0);
>> +}
>> +
>> +int tasks_are_frozen(void)
>> +{
>> +       int ret;
>> +       ret = atomic_read(&tasks_frozen);
>> +       return ret;
>> +} 
> 
> What's the point of using atomic_t here? Neither set nor read are
> actually atomic ops.
> 

Hi Peter,
Thank you for the review. I'll send version 2 of the patch with
the changes incorporated.

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

Re: [PATCH v2] 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).
To give an example that CPU hotplug notifications that only differ with
respect to the task frozen status might actually trigger different actions
in the callbacks, the x86 microcode update driver has a callback registered
for CPU hotplug events, which takes different actions based on whether
a CPU_DEAD or a CPU_DEAD_FROZEN notification is received.

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.

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.

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

 kernel/cpu.c           |    5 +++--
 kernel/power/process.c |   23 ++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index 1effc8b..3a37d8b 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -51,6 +51,10 @@ extern void refrigerator(void);
 extern int freeze_processes(void);
 extern void thaw_processes(void);
 
+extern void set_tasks_frozen_flag(void);
+extern void clear_tasks_frozen_flag(void);
+extern int tasks_are_frozen(void);
+
 static inline int try_to_freeze(void)
 {
 	if (freezing(current)) {
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..687f6fa 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -17,11 +17,30 @@
 #include <linux/delay.h>
 #include <linux/workqueue.h>
 
-/* 
+/*
  * Timeout for stopping processes
  */
 #define TIMEOUT	(20 * HZ)
 
+static int tasks_frozen;
+
+void set_tasks_frozen_flag(void)
+{
+	tasks_frozen = 1;
+	smp_mb();
+}
+
+void clear_tasks_frozen_flag(void)
+{
+	tasks_frozen = 0;
+	smp_mb();
+}
+
+int tasks_are_frozen(void)
+{
+	return tasks_frozen;
+}
+
 static inline int freezable(struct task_struct * p)
 {
 	if ((p == current) ||
@@ -151,6 +170,7 @@ int freeze_processes(void)
 	error = try_to_freeze_tasks(false);
 	if (error)
 		goto Exit;
+	set_tasks_frozen_flag();
 	printk("done.");
 
 	oom_killer_disable();
@@ -190,6 +210,7 @@ void thaw_processes(void)
 	thaw_tasks(true);
 	thaw_tasks(false);
 	schedule();
+	clear_tasks_frozen_flag();
 	printk("done.\n");
 }
 

Re: [PATCH v2] CPU hotplug, freezer: Fix bugs in CPU hotplug call path

[Peter Zijlstra]

On Tue, 2011-10-04 at 18:58 +0530, Srivatsa S. Bhat wrote:
> +static int tasks_frozen;
> +
> +void set_tasks_frozen_flag(void)
> +{
> +       tasks_frozen = 1;
> +       smp_mb();
> +}
> +
> +void clear_tasks_frozen_flag(void)
> +{
> +       tasks_frozen = 0;
> +       smp_mb();
> +}
> +
> +int tasks_are_frozen(void)
> +{
> +       return tasks_frozen;
> +} 

See Documentation/memory-barriers.txt, memory barriers always come in
pairs, furthermore memory barriers always should have a comment
explaining the ordering and referring to the pair match.

I think you want at least an smp_rmb() before reading tasks_frozen,
possible you also want to use ACCESS_ONCE() to force the compiler to
emit the read.

Furthermore, do you really need this? isn't it both set and read from
the same task context, all under pm_mutex?

Re: [PATCH v2] CPU hotplug, freezer: Fix bugs in CPU hotplug call path

[Srivatsa S. Bhat]

On 10/04/2011 07:07 PM, Peter Zijlstra wrote:
> On Tue, 2011-10-04 at 18:58 +0530, Srivatsa S. Bhat wrote:
>> +static int tasks_frozen;
>> +
>> +void set_tasks_frozen_flag(void)
>> +{
>> +       tasks_frozen = 1;
>> +       smp_mb();
>> +}
>> +
>> +void clear_tasks_frozen_flag(void)
>> +{
>> +       tasks_frozen = 0;
>> +       smp_mb();
>> +}
>> +
>> +int tasks_are_frozen(void)
>> +{
>> +       return tasks_frozen;
>> +} 
> 
> See Documentation/memory-barriers.txt, memory barriers always come in
> pairs, furthermore memory barriers always should have a comment
> explaining the ordering and referring to the pair match.
> 
Thank you for the pointer. I'll go through it (though now I think we
wouldn't need any of this, see below).

> I think you want at least an smp_rmb() before reading tasks_frozen,
> possible you also want to use ACCESS_ONCE() to force the compiler to
> emit the read.
> 
> Furthermore, do you really need this? isn't it both set and read from
> the same task context, all under pm_mutex?

I found that the cpu hotplug path that is triggered by writing to sysfs file
is not under pm_mutex. [The cpu hotplug path via disable/enable_nonboot_cpus()
is under pm_mutex, but in this patch we are not touching that path at all.]

By the way, this patch needs much more than just a flag set/reset by the
freezer and then the cpu hotplug code looking it up before calling _cpu_*()
functions. Because, we will have to ensure that while the registered
callbacks are being executed for the event notified, the state of the system
doesn't change (with respect to the tasks being frozen or not).
For example, while a callback for CPU_ONLINE is running, suddenly the tasks
must not get frozen (in which state it should have rather run callback for
CPU_ONLINE_FROZEN).

So, we should mutually exclude cpu hotplugging and freezing/thawing to
ensure that during the entire duration that the callbacks for the notified
event are running, the state introduced by that event holds good.

So the following patch tries to achieve this goal. It has not been tested.
I have posted it here just to generate discussion about the approach used.
I'll test it with lockdep enabled and find out if there are any problems.


---

 include/linux/freezer.h |    7 +++++++
 kernel/cpu.c            |    9 +++++++--
 kernel/power/process.c  |   27 ++++++++++++++++++++++++++-
 3 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index 1effc8b..75c65d6 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -7,6 +7,9 @@
 #include <linux/wait.h>
 
 #ifdef CONFIG_FREEZER
+
+extern struct mutex freezer_lock;
+
 /*
  * Check if a process has been frozen
  */
@@ -51,6 +54,10 @@ extern void refrigerator(void);
 extern int freeze_processes(void);
 extern void thaw_processes(void);
 
+extern void set_tasks_frozen_flag(void);
+extern void clear_tasks_frozen_flag(void);
+extern int tasks_are_frozen(void);
+
 static inline int try_to_freeze(void)
 {
 	if (freezing(current)) {
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 12b7458..b94c8f6 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,9 @@ int __ref cpu_down(unsigned int cpu)
 		goto out;
 	}
 
-	err = _cpu_down(cpu, 0);
+	mutex_lock(&freezer_lock);
+	err = _cpu_down(cpu, tasks_are_frozen());
+	mutex_unlock(&freezer_lock);
 
 out:
 	cpu_maps_update_done();
@@ -373,7 +376,9 @@ int __cpuinit cpu_up(unsigned int cpu)
 		goto out;
 	}
 
-	err = _cpu_up(cpu, 0);
+	mutex_lock(&freezer_lock);
+	err = _cpu_up(cpu, tasks_are_frozen());
+	mutex_unlock(&freezer_lock);
 
 out:
 	cpu_maps_update_done();
diff --git a/kernel/power/process.c b/kernel/power/process.c
index 0cf3a27..1285da2 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -17,11 +17,30 @@
 #include <linux/delay.h>
 #include <linux/workqueue.h>
 
-/* 
+/*
  * Timeout for stopping processes
  */
 #define TIMEOUT	(20 * HZ)
 
+DEFINE_MUTEX(freezer_lock);
+
+static int tasks_frozen;
+
+void set_tasks_frozen_flag(void)
+{
+	tasks_frozen = 1;
+}
+
+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) ||
@@ -141,6 +160,7 @@ int freeze_processes(void)
 {
 	int error;
 
+	mutex_lock(&freezer_lock);
 	printk("Freezing user space processes ... ");
 	error = try_to_freeze_tasks(true);
 	if (error)
@@ -151,10 +171,12 @@ int freeze_processes(void)
 	error = try_to_freeze_tasks(false);
 	if (error)
 		goto Exit;
+	set_tasks_frozen_flag();
 	printk("done.");
 
 	oom_killer_disable();
  Exit:
+	mutex_unlock(&freezer_lock);
 	BUG_ON(in_atomic());
 	printk("\n");
 
@@ -183,6 +205,7 @@ static void thaw_tasks(bool nosig_only)
 
 void thaw_processes(void)
 {
+	mutex_lock(&freezer_lock);
 	oom_killer_enable();
 
 	printk("Restarting tasks ... ");
@@ -190,6 +213,8 @@ void thaw_processes(void)
 	thaw_tasks(true);
 	thaw_tasks(false);
 	schedule();
+	clear_tasks_frozen_flag();
+	mutex_unlock(&freezer_lock);
 	printk("done.\n");
 }

Re: [PATCH v2] CPU hotplug, freezer: Fix bugs in CPU hotplug call path

[Américo Wang]

On Sat, Oct 8, 2011 at 4:56 AM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index 1effc8b..75c65d6 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -7,6 +7,9 @@
>  #include <linux/wait.h>
>
>  #ifdef CONFIG_FREEZER
> +
> +extern struct mutex freezer_lock;
> +
[...]
>  static inline int try_to_freeze(void)
>  {
>        if (freezing(current)) {
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 12b7458..b94c8f6 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,9 @@ int __ref cpu_down(unsigned int cpu)
>                goto out;
>        }
>
> -       err = _cpu_down(cpu, 0);
> +       mutex_lock(&freezer_lock);
> +       err = _cpu_down(cpu, tasks_are_frozen());
> +       mutex_unlock(&freezer_lock);
>

Can this even be compiled when CONFIG_FREEZER=n?

Re: [PATCH v2] CPU hotplug, freezer: Fix bugs in CPU hotplug call path

[Srivatsa S. Bhat]

On 10/08/2011 06:22 PM, Américo Wang wrote:
> On Sat, Oct 8, 2011 at 4:56 AM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
>> index 1effc8b..75c65d6 100644
>> --- a/include/linux/freezer.h
>> +++ b/include/linux/freezer.h
>> @@ -7,6 +7,9 @@
>>  #include <linux/wait.h>
>>
>>  #ifdef CONFIG_FREEZER
>> +
>> +extern struct mutex freezer_lock;
>> +
> [...]
>>  static inline int try_to_freeze(void)
>>  {
>>        if (freezing(current)) {
>> diff --git a/kernel/cpu.c b/kernel/cpu.c
>> index 12b7458..b94c8f6 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,9 @@ int __ref cpu_down(unsigned int cpu)
>>                goto out;
>>        }
>>
>> -       err = _cpu_down(cpu, 0);
>> +       mutex_lock(&freezer_lock);
>> +       err = _cpu_down(cpu, tasks_are_frozen());
>> +       mutex_unlock(&freezer_lock);
>>
> 
> Can this even be compiled when CONFIG_FREEZER=n?
> 
Oh! That's a mistake.. Thank you for pointing it out.

This one should fix it:

---

 include/linux/freezer.h |   22 ++++++++++++++++++++++
 kernel/cpu.c            |    9 +++++++--
 kernel/power/process.c  |   27 ++++++++++++++++++++++++++-
 3 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index 1effc8b..3d80ddb 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -7,6 +7,19 @@
 #include <linux/wait.h>
 
 #ifdef CONFIG_FREEZER
+
+extern struct mutex freezer_lock;
+
+static inline void lock_freezer(void)
+{
+	mutex_lock(&freezer_lock);
+}
+
+static inline void unlock_freezer(void)
+{
+	mutex_unlock(&freezer_lock);
+}
+
 /*
  * Check if a process has been frozen
  */
@@ -51,6 +64,10 @@ extern void refrigerator(void);
 extern int freeze_processes(void);
 extern void thaw_processes(void);
 
+extern void set_tasks_frozen_flag(void);
+extern void clear_tasks_frozen_flag(void);
+extern int tasks_are_frozen(void);
+
 static inline int try_to_freeze(void)
 {
 	if (freezing(current)) {
@@ -164,6 +181,8 @@ static inline void set_freezable_with_signal(void)
 	__retval;							\
 })
 #else /* !CONFIG_FREEZER */
+static inline void lock_freezer(void) {}
+static inline void unlock_freezer(void) {}
 static inline int frozen(struct task_struct *p) { return 0; }
 static inline int freezing(struct task_struct *p) { return 0; }
 static inline void set_freeze_flag(struct task_struct *p) {}
@@ -173,6 +192,9 @@ 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 void set_tasks_frozen_flag(void) {}
+static inline void clear_tasks_frozen_flag(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..d81ceea 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,9 @@ int __ref cpu_down(unsigned int cpu)
 		goto out;
 	}
 
-	err = _cpu_down(cpu, 0);
+	lock_freezer();
+	err = _cpu_down(cpu, tasks_are_frozen());
+	unlock_freezer();
 
 out:
 	cpu_maps_update_done();
@@ -373,7 +376,9 @@ int __cpuinit cpu_up(unsigned int cpu)
 		goto out;
 	}
 
-	err = _cpu_up(cpu, 0);
+	lock_freezer();
+	err = _cpu_up(cpu, tasks_are_frozen());
+	unlock_freezer();
 
 out:
 	cpu_maps_update_done();
diff --git a/kernel/power/process.c b/kernel/power/process.c
index 0cf3a27..9da3369 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -17,11 +17,30 @@
 #include <linux/delay.h>
 #include <linux/workqueue.h>
 
-/* 
+/*
  * Timeout for stopping processes
  */
 #define TIMEOUT	(20 * HZ)
 
+DEFINE_MUTEX(freezer_lock);
+
+static int tasks_frozen;
+
+void set_tasks_frozen_flag(void)
+{
+	tasks_frozen = 1;
+}
+
+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) ||
@@ -141,6 +160,7 @@ int freeze_processes(void)
 {
 	int error;
 
+	lock_freezer();
 	printk("Freezing user space processes ... ");
 	error = try_to_freeze_tasks(true);
 	if (error)
@@ -151,10 +171,12 @@ int freeze_processes(void)
 	error = try_to_freeze_tasks(false);
 	if (error)
 		goto Exit;
+	set_tasks_frozen_flag();
 	printk("done.");
 
 	oom_killer_disable();
  Exit:
+	unlock_freezer();
 	BUG_ON(in_atomic());
 	printk("\n");
 
@@ -183,6 +205,7 @@ static void thaw_tasks(bool nosig_only)
 
 void thaw_processes(void)
 {
+	lock_freezer();
 	oom_killer_enable();
 
 	printk("Restarting tasks ... ");
@@ -190,6 +213,8 @@ void thaw_processes(void)
 	thaw_tasks(true);
 	thaw_tasks(false);
 	schedule();
+	clear_tasks_frozen_flag();
+	unlock_freezer();
 	printk("done.\n");
 }
 

Re: [PATCH v2] CPU hotplug, freezer: Fix bugs in CPU hotplug call path

[Peter Zijlstra]

On Sat, 2011-10-08 at 02:26 +0530, Srivatsa S. Bhat wrote:
> 
> So, we should mutually exclude cpu hotplugging and freezing/thawing to
> ensure that during the entire duration that the callbacks for the notified
> event are running, the state introduced by that event holds good. 

Yes we should, but I'd rather see that done on a more fundamental level,
and not frob yet another mutex in the already complex hotplug system.

Re: [PATCH v2] CPU hotplug, freezer: Fix bugs in CPU hotplug call path

[Srivatsa S. Bhat]

On 10/10/2011 04:18 PM, Peter Zijlstra wrote:
> On Sat, 2011-10-08 at 02:26 +0530, Srivatsa S. Bhat wrote:
>>
>> So, we should mutually exclude cpu hotplugging and freezing/thawing to
>> ensure that during the entire duration that the callbacks for the notified
>> event are running, the state introduced by that event holds good. 
> 
> Yes we should, but I'd rather see that done on a more fundamental level,
> and not frob yet another mutex in the already complex hotplug system.
> 

Sure, thank you for the feedback. I have now come up with another version
that doesn't introduce any new mutex. I'll post it out in a while.

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