CPU hotplug, freezer: Fix bugs in CPU hotplug call path

PATCH Superseded Srivatsa S. Bhat 3 participants 10 messages Oct 2, 2011 – Oct 21, 2011

Subsystems (4)

Suspend to RAM Hibernation CPU hotplug Freezer

lore

Share & tools

lore

Srivatsa S. Bhat

author

2011-10-02 19:45 UTC #0 lore raw

PATCH 1/1 CPU hotplug, freezer: Fix bugs in CPU hotplug call path

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 <[email protected]>

---

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

include/linux/freezer.h +4 -0

index 1effc8b..3a37d8b 100644
@@ -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)) {

kernel/cpu.c +7 -2

index 12b7458..e4530ea 100644
@@ -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();

kernel/power/process.c +23 -1

index 0cf3a27..7642314 100644
@@ -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();
 }

Peter Zijlstra

2011-10-03 10:05 UTC #1 lore raw

Quoted 8 lines · Srivatsa S. Bhat

kernel/power/process.c +8 -0

+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)

kernel/power/process.c line 42

Quoted 10 lines

kernel/power/process.c +10 -0

+{
+       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.

Srivatsa S. Bhat

author

2011-10-04 13:19 UTC #2 lore raw

Quoted 14 lines · Peter Zijlstra

Quoted 13 lines · Srivatsa S. Bhat

kernel/power/process.c +13 -0

+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)

Quoted 9 lines

kernel/power/process.c line 42
Quoted 5 lines
kernel/power/process.c +5 -0
+{
+       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.

Signature · 4 lines

Regards,
Srivatsa S. Bhat  <[email protected]>
Linux Technology Center,
IBM India Systems and Technology Lab

Srivatsa S. Bhat

author

2011-10-04 13:28 UTC #3 lore raw

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 <[email protected]>

---

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

include/linux/freezer.h +4 -0

index 1effc8b..3a37d8b 100644
@@ -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)) {

kernel/cpu.c +3 -2

index 12b7458..e889ffd 100644
@@ -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();

kernel/power/process.c +22 -1

index 0cf3a27..687f6fa 100644
@@ -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");
 }

Peter Zijlstra

2011-10-04 13:38 UTC #4 lore raw

Quoted 8 lines · Srivatsa S. Bhat

kernel/power/process.c +8 -0

+static int tasks_frozen;
+
+void set_tasks_frozen_flag(void)
+{
+       tasks_frozen = 1;
+       smp_mb();
+}
+

kernel/power/process.c line 42

Quoted 10 lines

kernel/power/process.c +10 -0

+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?

Srivatsa S. Bhat

author

2011-10-07 20:56 UTC #5 lore raw

Quoted 15 lines · Peter Zijlstra

Quoted 14 lines · Srivatsa S. Bhat

(quoted diff) +14 -0

+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();
+}
+

Quoted 9 lines

Review
Quoted 4 lines
(quoted diff) +4 -0
+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).

Quoted 6 lines

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(-)

include/linux/freezer.h +7 -0

index 1effc8b..75c65d6 100644
@@ -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)) {

kernel/cpu.c +7 -2

index 12b7458..b94c8f6 100644
@@ -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();

kernel/power/process.c +26 -1

index 0cf3a27..1285da2 100644
@@ -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");
 }

Américo Wang

2011-10-08 12:52 UTC #6 lore raw

Quoted 11 lines · Srivatsa S. Bhat

include/linux/freezer.h +3 -0

index 1effc8b..75c65d6 100644
@@ -7,6 +7,9 @@
  #include <linux/wait.h>
 
  #ifdef CONFIG_FREEZER
+
+extern struct mutex freezer_lock;
+

[...]

Quoted 16 lines

static inline int try_to_freeze(void)
{
if (freezing(current)) {

kernel/cpu.c +1 -0

index 12b7458..b94c8f6 100644
@@ -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 */

kernel/cpu.c int __ref cpu_down(unsigned int cpu) · line 287

Quoted 8 lines

kernel/cpu.c +3 -1

@@ -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?

Srivatsa S. Bhat

author

2011-10-08 20:28 UTC #7 lore raw

Quoted 33 lines · Américo Wang

Quoted 11 lines · Srivatsa S. Bhat

include/linux/freezer.h +3 -0

index 1effc8b..75c65d6 100644
@@ -7,6 +7,9 @@
 #include <linux/wait.h>
 
 #ifdef CONFIG_FREEZER
+
+extern struct mutex freezer_lock;
+

[...]

Quoted 19 lines

static inline int try_to_freeze(void)

 {
       if (freezing(current)) {

kernel/cpu.c +1 -0

index 12b7458..b94c8f6 100644
@@ -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;
       }

Quoted 8 lines

kernel/cpu.c int __ref cpu_down(unsigned int cpu) · line 287
Quoted 5 lines
kernel/cpu.c +3 -1
-       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(-)

include/linux/freezer.h +22 -0

index 1effc8b..3d80ddb 100644
@@ -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; }

kernel/cpu.c +7 -2

index 12b7458..d81ceea 100644
@@ -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();

kernel/power/process.c +26 -1

index 0cf3a27..9da3369 100644
@@ -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");
 }

Peter Zijlstra

2011-10-10 10:43 UTC #8 lore raw

In reply to Srivatsa S. Bhat

Quoted 4 lines · Srivatsa S. Bhat

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.

Srivatsa S. Bhat

author

2011-10-21 11:30 UTC #9 lore raw

Quoted 9 lines · Peter Zijlstra

Quoted 4 lines · Srivatsa S. Bhat
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.

Signature · 4 lines

Regards,
Srivatsa S. Bhat  <[email protected]>
Linux Technology Center,
IBM India Systems and Technology Lab