[PATCH v4 0/2] CPU hotplug, Freezer: Fix race between CPU hotplug and freezer

[PATCH v4 0/2] CPU hotplug, Freezer: Fix race between CPU hotplug and freezer

[Srivatsa S. Bhat]

The CPU hotplug notifications sent out by the _cpu_up() and _cpu_down()
functions depends on the value of the 'tasks_frozen' argument passed to them.
(Examples: CPU_ONLINE, CPU_ONLINE_FROZEN, CPU_DEAD, CPU_DEAD_FROZEN).

Thus, it is essential that while the callbacks for those notifications are
running, the state of the system with respect to the tasks being frozen or
not remains unchanged, *throughout that duration*. Hence there is a need for
synchronizing the CPU hotplug code with the freezer subsystem.

This patchset introduces two freezer notifications PM_FREEZE_PREPARE and
PM_POST_THAW to announce the freezer's activities to other interested
subsystems. The CPU hotplug code hooks onto these notifications and prevents
the race between CPU hotplug and freezer, thus ensuring that CPU hotplug
notifications will always be run with the state of the system really being
what the notifications mean.


v4: * Retained the value 0 for the 'tasks_frozen' argument, while calling
      _cpu_up() and _cpu_down().
      Removed the unnecessary PM_POST_FREEZE and PM_THAW_PREPARE notifications.

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
v3 -> http://thread.gmane.org/gmane.linux.documentation/3472

--
Srivatsa S. Bhat (2):
      PM / Freezer: Introduce PM_FREEZE_PREPARE and PM_POST_THAW notifications
      CPU hotplug, Freezer: Synchronize CPU hotplug and Freezer


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

[PATCH v4 1/2] PM / Freezer: Introduce PM_FREEZE_PREPARE and PM_POST_THAW notifications

[Srivatsa S. Bhat]

There are several subsystems and code paths (like CPU hotplug) that would
like to sync up with the activities of the freezer subsystem. So, this patch
introduces two notifications in the freezer, namely PM_FREEZE_PREPARE and
PM_POST_THAW, so as to make other subsystems aware of the freezer's activity.
Thus whichever code wants to avoid racing with the freezer or take any
specific action when the freezer is active, can hook onto these notifications
and implement the necessary functionality in their callbacks.

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

 Documentation/power/notifiers.txt |    4 ++++
 include/linux/suspend.h           |    4 +++-
 kernel/power/process.c            |    8 +++++++-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/Documentation/power/notifiers.txt b/Documentation/power/notifiers.txt
index c2a4a34..3234306 100644
--- a/Documentation/power/notifiers.txt
+++ b/Documentation/power/notifiers.txt
@@ -37,6 +37,10 @@ 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_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
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 57a6924..9530832 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -323,13 +323,15 @@ 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_THAW		0x0008 /* Thawing of tasks finished */
 
 #ifdef CONFIG_PM_SLEEP
 void save_processor_state(void);
diff --git a/kernel/power/process.c b/kernel/power/process.c
index addbbe5..17a31a2 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -17,7 +17,9 @@
 #include <linux/delay.h>
 #include <linux/workqueue.h>
 
-/* 
+#include "power.h"
+
+/*
  * Timeout for stopping processes
  */
 #define TIMEOUT	(20 * HZ)
@@ -141,6 +143,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) {
@@ -201,5 +205,7 @@ void thaw_processes(void)
 	thaw_tasks(false);
 	schedule();
 	printk("done.\n");
+
+	pm_notifier_call_chain(PM_POST_THAW);
 }
 

[PATCH v4 2/2] CPU hotplug, Freezer: Synchronize CPU hotplug and Freezer

[Srivatsa S. Bhat]

Prevent CPU hotplug and the freezer from racing with each other, to ensure
that during the *entire duration* for which the callbacks for CPU hotplug
notifications such as CPU_ONLINE[_FROZEN], CPU_DEAD[_FROZEN] etc are being
executed, the state of the system (with respect to the tasks being frozen
or not) remains constant.

This patches hooks the CPU hotplug infrastructure onto the freezer
notifications (PM_FREEZE_PREPARE and PM_POST_THAW) and thus synchronizes
with the freezer.

Specifically,

* Upon the PM_FREEZE_PREPARE notification, the CPU hotplug callback disables
  future (regular) CPU hotplugging and also ensures that any currently running
  CPU hotplug operation is completed before allowing the freezer to continue
  any further.

* Upon the PM_POST_THAW notification, the CPU hotplug callback re-enables
  regular CPU hotplug.

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

 kernel/cpu.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 76 insertions(+), 0 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 12b7458..61985ce 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/suspend.h>
 
 #ifdef CONFIG_SMP
 /* Serializes the updates to cpu_online_mask, cpu_present_mask */
@@ -478,6 +479,81 @@ 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.
+ *
+ * Also, don't allow the freezer subsystem to continue until any currently
+ * running CPU hotplug operation gets completed.
+ * To modify the 'cpu_hotplug_disabled' flag, we need to acquire the
+ * 'cpu_add_remove_lock'. And this same lock is also taken by the regular
+ * CPU hotplug path and released only after it is complete. Thus, we
+ * (and hence the freezer) will block here until any currently running CPU
+ * hotplug operation is completed.
+ */
+static void cpu_hotplug_freezer_block_begin(void)
+{
+	cpu_maps_update_begin();
+	cpu_hotplug_disabled = 1;
+	cpu_maps_update_done();
+}
+
+
+/*
+ * When thawing of tasks is complete, re-enable CPU hotplug (which had been
+ * disabled while beginning to freeze tasks).
+ */
+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:
+		cpu_hotplug_freezer_block_begin();
+		break;
+
+	case PM_POST_THAW:
+		cpu_hotplug_freezer_block_done();
+		break;
+
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
+
+int cpu_hotplug_freezer_sync_init(void)
+{
+	pm_notifier(cpu_hotplug_freezer_callback, 0);
+	return 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

Re: [PATCH v4 2/2] CPU hotplug, Freezer: Synchronize CPU hotplug and Freezer

[Rafael J. Wysocki]

Hi,

On Thursday, October 27, 2011, Srivatsa S. Bhat wrote:
> Prevent CPU hotplug and the freezer from racing with each other, to ensure
> that during the *entire duration* for which the callbacks for CPU hotplug
> notifications such as CPU_ONLINE[_FROZEN], CPU_DEAD[_FROZEN] etc are being
> executed, the state of the system (with respect to the tasks being frozen
> or not) remains constant.
> 
> This patches hooks the CPU hotplug infrastructure onto the freezer
> notifications (PM_FREEZE_PREPARE and PM_POST_THAW) and thus synchronizes
> with the freezer.
> 
> Specifically,
> 
> * Upon the PM_FREEZE_PREPARE notification, the CPU hotplug callback disables
>   future (regular) CPU hotplugging and also ensures that any currently running
>   CPU hotplug operation is completed before allowing the freezer to continue
>   any further.
> 
> * Upon the PM_POST_THAW notification, the CPU hotplug callback re-enables
>   regular CPU hotplug.
> 
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> 
>  kernel/cpu.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 76 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 12b7458..61985ce 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/suspend.h>
>  
>  #ifdef CONFIG_SMP
>  /* Serializes the updates to cpu_online_mask, cpu_present_mask */
> @@ -478,6 +479,81 @@ 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.
> + *
> + * Also, don't allow the freezer subsystem to continue until any currently
> + * running CPU hotplug operation gets completed.
> + * To modify the 'cpu_hotplug_disabled' flag, we need to acquire the
> + * 'cpu_add_remove_lock'. And this same lock is also taken by the regular
> + * CPU hotplug path and released only after it is complete. Thus, we
> + * (and hence the freezer) will block here until any currently running CPU
> + * hotplug operation is completed.
> + */
> +static void cpu_hotplug_freezer_block_begin(void)
> +{
> +	cpu_maps_update_begin();
> +	cpu_hotplug_disabled = 1;
> +	cpu_maps_update_done();
> +}
> +
> +
> +/*
> + * When thawing of tasks is complete, re-enable CPU hotplug (which had been
> + * disabled while beginning to freeze tasks).
> + */
> +static void cpu_hotplug_freezer_block_done(void)
> +{
> +	cpu_maps_update_begin();
> +	cpu_hotplug_disabled = 0;
> +	cpu_maps_update_done();
> +}
> +

I wonder if the new PM notifier events are really necessary?

Why don't you just call cpu_hotplug_freezer_block_begin() (perhaps
with a better name?) directly from freeze_processes()?  And analogously
for cpu_hotplug_freezer_block_done() and thaw_processes()?

Rafael


> +
> +/*
> + * 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:
> +		cpu_hotplug_freezer_block_begin();
> +		break;
> +
> +	case PM_POST_THAW:
> +		cpu_hotplug_freezer_block_done();
> +		break;
> +
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +
> +int cpu_hotplug_freezer_sync_init(void)
> +{
> +	pm_notifier(cpu_hotplug_freezer_callback, 0);
> +	return 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
> 
> 
> 

Re: [PATCH v4 2/2] CPU hotplug, Freezer: Synchronize CPU hotplug and Freezer

[Srivatsa S. Bhat]

On 10/28/2011 01:43 AM, Rafael J. Wysocki wrote:
> Hi,
> 
> On Thursday, October 27, 2011, Srivatsa S. Bhat wrote:
>> Prevent CPU hotplug and the freezer from racing with each other, to ensure
>> that during the *entire duration* for which the callbacks for CPU hotplug
>> notifications such as CPU_ONLINE[_FROZEN], CPU_DEAD[_FROZEN] etc are being
>> executed, the state of the system (with respect to the tasks being frozen
>> or not) remains constant.
>>
>> This patches hooks the CPU hotplug infrastructure onto the freezer
>> notifications (PM_FREEZE_PREPARE and PM_POST_THAW) and thus synchronizes
>> with the freezer.
>>
>> Specifically,
>>
>> * Upon the PM_FREEZE_PREPARE notification, the CPU hotplug callback disables
>>   future (regular) CPU hotplugging and also ensures that any currently running
>>   CPU hotplug operation is completed before allowing the freezer to continue
>>   any further.
>>
>> * Upon the PM_POST_THAW notification, the CPU hotplug callback re-enables
>>   regular CPU hotplug.
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> ---
>>
>>  kernel/cpu.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 76 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/cpu.c b/kernel/cpu.c
>> index 12b7458..61985ce 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/suspend.h>
>>  
>>  #ifdef CONFIG_SMP
>>  /* Serializes the updates to cpu_online_mask, cpu_present_mask */
>> @@ -478,6 +479,81 @@ 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.
>> + *
>> + * Also, don't allow the freezer subsystem to continue until any currently
>> + * running CPU hotplug operation gets completed.
>> + * To modify the 'cpu_hotplug_disabled' flag, we need to acquire the
>> + * 'cpu_add_remove_lock'. And this same lock is also taken by the regular
>> + * CPU hotplug path and released only after it is complete. Thus, we
>> + * (and hence the freezer) will block here until any currently running CPU
>> + * hotplug operation is completed.
>> + */
>> +static void cpu_hotplug_freezer_block_begin(void)
>> +{
>> +	cpu_maps_update_begin();
>> +	cpu_hotplug_disabled = 1;
>> +	cpu_maps_update_done();
>> +}
>> +
>> +
>> +/*
>> + * When thawing of tasks is complete, re-enable CPU hotplug (which had been
>> + * disabled while beginning to freeze tasks).
>> + */
>> +static void cpu_hotplug_freezer_block_done(void)
>> +{
>> +	cpu_maps_update_begin();
>> +	cpu_hotplug_disabled = 0;
>> +	cpu_maps_update_done();
>> +}
>> +
> 
> I wonder if the new PM notifier events are really necessary?
> 
> Why don't you just call cpu_hotplug_freezer_block_begin() (perhaps
> with a better name?) directly from freeze_processes()?  And analogously
> for cpu_hotplug_freezer_block_done() and thaw_processes()?
> 

Yes, we can definitely do that. 

But the reason why I chose to introduce new notifiers was to make this
more extensible (because we know that at least 2 subsystems would benefit
from mutually excluding themselves from the freezer, namely CPU hotplug
and x86 microcode).
http://thread.gmane.org/gmane.linux.kernel/1198291/focus=1200591

But now that I think of it, hooking onto the freezer notifiers wouldn't
solve the microcode cases since usermodehelper_disable() is called
_before_ freezing tasks... :(

So we should probably call the functions directly like you suggested.. 

But I really didn't want to clutter the freezer call path because of problems
elsewhere. So I felt freezer notifiers would be a cleaner way of dealing with
such things. Also, since freezer is a generic subsystem that could be used
for purposes other than S3/S4 as well (I have heard of attempts to use freezer
during tracing), wouldn't it be better to introduce new notifiers to
announce the begin and end of freezer activity to interested subsystems?
(and then use them to solve the CPU hotplug issue like this patch does...)

Please let me know your suggestions.

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

Re: [PATCH v4 2/2] CPU hotplug, Freezer: Synchronize CPU hotplug and Freezer

[Rafael J. Wysocki]

On Friday, October 28, 2011, Srivatsa S. Bhat wrote:
> On 10/28/2011 01:43 AM, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > On Thursday, October 27, 2011, Srivatsa S. Bhat wrote:
> >> Prevent CPU hotplug and the freezer from racing with each other, to ensure
> >> that during the *entire duration* for which the callbacks for CPU hotplug
> >> notifications such as CPU_ONLINE[_FROZEN], CPU_DEAD[_FROZEN] etc are being
> >> executed, the state of the system (with respect to the tasks being frozen
> >> or not) remains constant.
> >>
> >> This patches hooks the CPU hotplug infrastructure onto the freezer
> >> notifications (PM_FREEZE_PREPARE and PM_POST_THAW) and thus synchronizes
> >> with the freezer.
> >>
> >> Specifically,
> >>
> >> * Upon the PM_FREEZE_PREPARE notification, the CPU hotplug callback disables
> >>   future (regular) CPU hotplugging and also ensures that any currently running
> >>   CPU hotplug operation is completed before allowing the freezer to continue
> >>   any further.
> >>
> >> * Upon the PM_POST_THAW notification, the CPU hotplug callback re-enables
> >>   regular CPU hotplug.
> >>
> >> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> >> ---
> >>
> >>  kernel/cpu.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 files changed, 76 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/kernel/cpu.c b/kernel/cpu.c
> >> index 12b7458..61985ce 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/suspend.h>
> >>  
> >>  #ifdef CONFIG_SMP
> >>  /* Serializes the updates to cpu_online_mask, cpu_present_mask */
> >> @@ -478,6 +479,81 @@ 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.
> >> + *
> >> + * Also, don't allow the freezer subsystem to continue until any currently
> >> + * running CPU hotplug operation gets completed.
> >> + * To modify the 'cpu_hotplug_disabled' flag, we need to acquire the
> >> + * 'cpu_add_remove_lock'. And this same lock is also taken by the regular
> >> + * CPU hotplug path and released only after it is complete. Thus, we
> >> + * (and hence the freezer) will block here until any currently running CPU
> >> + * hotplug operation is completed.
> >> + */
> >> +static void cpu_hotplug_freezer_block_begin(void)
> >> +{
> >> +	cpu_maps_update_begin();
> >> +	cpu_hotplug_disabled = 1;
> >> +	cpu_maps_update_done();
> >> +}
> >> +
> >> +
> >> +/*
> >> + * When thawing of tasks is complete, re-enable CPU hotplug (which had been
> >> + * disabled while beginning to freeze tasks).
> >> + */
> >> +static void cpu_hotplug_freezer_block_done(void)
> >> +{
> >> +	cpu_maps_update_begin();
> >> +	cpu_hotplug_disabled = 0;
> >> +	cpu_maps_update_done();
> >> +}
> >> +
> > 
> > I wonder if the new PM notifier events are really necessary?
> > 
> > Why don't you just call cpu_hotplug_freezer_block_begin() (perhaps
> > with a better name?) directly from freeze_processes()?  And analogously
> > for cpu_hotplug_freezer_block_done() and thaw_processes()?
> > 
> 
> Yes, we can definitely do that. 
> 
> But the reason why I chose to introduce new notifiers was to make this
> more extensible (because we know that at least 2 subsystems would benefit
> from mutually excluding themselves from the freezer, namely CPU hotplug
> and x86 microcode).
> http://thread.gmane.org/gmane.linux.kernel/1198291/focus=1200591
> 
> But now that I think of it, hooking onto the freezer notifiers wouldn't
> solve the microcode cases since usermodehelper_disable() is called
> _before_ freezing tasks... :(
> 
> So we should probably call the functions directly like you suggested.. 
> 
> But I really didn't want to clutter the freezer call path because of problems
> elsewhere. So I felt freezer notifiers would be a cleaner way of dealing with
> such things. Also, since freezer is a generic subsystem that could be used
> for purposes other than S3/S4 as well (I have heard of attempts to use freezer
> during tracing), wouldn't it be better to introduce new notifiers to
> announce the begin and end of freezer activity to interested subsystems?
> (and then use them to solve the CPU hotplug issue like this patch does...)
> 
> Please let me know your suggestions.

The freeze_processes() and thaw_processes() functions are only used for
system suspend and hibernation, as far as I can tell, and I don't think there
will be any other users in predictable future.

Also, adding the calls directly to those functions will show exactly what
the dependecies are, while doing that through a notifier kind of obfuscates
things.  So, please make direct calls from there.

Thanks,
Rafael

Re: [PATCH v4 2/2] CPU hotplug, Freezer: Synchronize CPU hotplug and Freezer

[Srivatsa S. Bhat]

On 10/28/2011 05:27 PM, Rafael J. Wysocki wrote:
> On Friday, October 28, 2011, Srivatsa S. Bhat wrote:
>> On 10/28/2011 01:43 AM, Rafael J. Wysocki wrote:
>>> Hi,
>>>
>>> On Thursday, October 27, 2011, Srivatsa S. Bhat wrote:
>>>> Prevent CPU hotplug and the freezer from racing with each other, to ensure
>>>> that during the *entire duration* for which the callbacks for CPU hotplug
>>>> notifications such as CPU_ONLINE[_FROZEN], CPU_DEAD[_FROZEN] etc are being
>>>> executed, the state of the system (with respect to the tasks being frozen
>>>> or not) remains constant.
>>>>
>>>> This patches hooks the CPU hotplug infrastructure onto the freezer
>>>> notifications (PM_FREEZE_PREPARE and PM_POST_THAW) and thus synchronizes
>>>> with the freezer.
>>>>
>>>> Specifically,
>>>>
>>>> * Upon the PM_FREEZE_PREPARE notification, the CPU hotplug callback disables
>>>>   future (regular) CPU hotplugging and also ensures that any currently running
>>>>   CPU hotplug operation is completed before allowing the freezer to continue
>>>>   any further.
>>>>
>>>> * Upon the PM_POST_THAW notification, the CPU hotplug callback re-enables
>>>>   regular CPU hotplug.
>>>>
>>>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>>>> ---
>>>>
>>>>  kernel/cpu.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 files changed, 76 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/kernel/cpu.c b/kernel/cpu.c
>>>> index 12b7458..61985ce 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/suspend.h>
>>>>  
>>>>  #ifdef CONFIG_SMP
>>>>  /* Serializes the updates to cpu_online_mask, cpu_present_mask */
>>>> @@ -478,6 +479,81 @@ 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.
>>>> + *
>>>> + * Also, don't allow the freezer subsystem to continue until any currently
>>>> + * running CPU hotplug operation gets completed.
>>>> + * To modify the 'cpu_hotplug_disabled' flag, we need to acquire the
>>>> + * 'cpu_add_remove_lock'. And this same lock is also taken by the regular
>>>> + * CPU hotplug path and released only after it is complete. Thus, we
>>>> + * (and hence the freezer) will block here until any currently running CPU
>>>> + * hotplug operation is completed.
>>>> + */
>>>> +static void cpu_hotplug_freezer_block_begin(void)
>>>> +{
>>>> +	cpu_maps_update_begin();
>>>> +	cpu_hotplug_disabled = 1;
>>>> +	cpu_maps_update_done();
>>>> +}
>>>> +
>>>> +
>>>> +/*
>>>> + * When thawing of tasks is complete, re-enable CPU hotplug (which had been
>>>> + * disabled while beginning to freeze tasks).
>>>> + */
>>>> +static void cpu_hotplug_freezer_block_done(void)
>>>> +{
>>>> +	cpu_maps_update_begin();
>>>> +	cpu_hotplug_disabled = 0;
>>>> +	cpu_maps_update_done();
>>>> +}
>>>> +
>>>
>>> I wonder if the new PM notifier events are really necessary?
>>>
>>> Why don't you just call cpu_hotplug_freezer_block_begin() (perhaps
>>> with a better name?) directly from freeze_processes()?  And analogously
>>> for cpu_hotplug_freezer_block_done() and thaw_processes()?
>>>
>>
>> Yes, we can definitely do that. 
>>
>> But the reason why I chose to introduce new notifiers was to make this
>> more extensible (because we know that at least 2 subsystems would benefit
>> from mutually excluding themselves from the freezer, namely CPU hotplug
>> and x86 microcode).
>> http://thread.gmane.org/gmane.linux.kernel/1198291/focus=1200591
>>
>> But now that I think of it, hooking onto the freezer notifiers wouldn't
>> solve the microcode cases since usermodehelper_disable() is called
>> _before_ freezing tasks... :(
>>
>> So we should probably call the functions directly like you suggested.. 
>>
>> But I really didn't want to clutter the freezer call path because of problems
>> elsewhere. So I felt freezer notifiers would be a cleaner way of dealing with
>> such things. Also, since freezer is a generic subsystem that could be used
>> for purposes other than S3/S4 as well (I have heard of attempts to use freezer
>> during tracing), wouldn't it be better to introduce new notifiers to
>> announce the begin and end of freezer activity to interested subsystems?
>> (and then use them to solve the CPU hotplug issue like this patch does...)
>>
>> Please let me know your suggestions.
> 
> The freeze_processes() and thaw_processes() functions are only used for
> system suspend and hibernation, as far as I can tell, and I don't think there
> will be any other users in predictable future.
> 
> Also, adding the calls directly to those functions will show exactly what
> the dependecies are, while doing that through a notifier kind of obfuscates
> things.  So, please make direct calls from there.
> 

Ok, thank you for the clarification. I'll post the next version of the patch
with direct function calls.

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

Re: [PATCH v4 2/2] CPU hotplug, Freezer: Synchronize CPU hotplug and Freezer

[Rafael J. Wysocki]

On Friday, October 28, 2011, Rafael J. Wysocki wrote:
> On Friday, October 28, 2011, Srivatsa S. Bhat wrote:
> > On 10/28/2011 01:43 AM, Rafael J. Wysocki wrote:
> > > Hi,
> > > 
> > > On Thursday, October 27, 2011, Srivatsa S. Bhat wrote:
> > >> Prevent CPU hotplug and the freezer from racing with each other, to ensure
> > >> that during the *entire duration* for which the callbacks for CPU hotplug
> > >> notifications such as CPU_ONLINE[_FROZEN], CPU_DEAD[_FROZEN] etc are being
> > >> executed, the state of the system (with respect to the tasks being frozen
> > >> or not) remains constant.
> > >>
> > >> This patches hooks the CPU hotplug infrastructure onto the freezer
> > >> notifications (PM_FREEZE_PREPARE and PM_POST_THAW) and thus synchronizes
> > >> with the freezer.
> > >>
> > >> Specifically,
> > >>
> > >> * Upon the PM_FREEZE_PREPARE notification, the CPU hotplug callback disables
> > >>   future (regular) CPU hotplugging and also ensures that any currently running
> > >>   CPU hotplug operation is completed before allowing the freezer to continue
> > >>   any further.
> > >>
> > >> * Upon the PM_POST_THAW notification, the CPU hotplug callback re-enables
> > >>   regular CPU hotplug.
> > >>
> > >> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> > >> ---
> > >>
> > >>  kernel/cpu.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >>  1 files changed, 76 insertions(+), 0 deletions(-)
> > >>
> > >> diff --git a/kernel/cpu.c b/kernel/cpu.c
> > >> index 12b7458..61985ce 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/suspend.h>
> > >>  
> > >>  #ifdef CONFIG_SMP
> > >>  /* Serializes the updates to cpu_online_mask, cpu_present_mask */
> > >> @@ -478,6 +479,81 @@ 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.
> > >> + *
> > >> + * Also, don't allow the freezer subsystem to continue until any currently
> > >> + * running CPU hotplug operation gets completed.
> > >> + * To modify the 'cpu_hotplug_disabled' flag, we need to acquire the
> > >> + * 'cpu_add_remove_lock'. And this same lock is also taken by the regular
> > >> + * CPU hotplug path and released only after it is complete. Thus, we
> > >> + * (and hence the freezer) will block here until any currently running CPU
> > >> + * hotplug operation is completed.
> > >> + */
> > >> +static void cpu_hotplug_freezer_block_begin(void)
> > >> +{
> > >> +	cpu_maps_update_begin();
> > >> +	cpu_hotplug_disabled = 1;
> > >> +	cpu_maps_update_done();
> > >> +}
> > >> +
> > >> +
> > >> +/*
> > >> + * When thawing of tasks is complete, re-enable CPU hotplug (which had been
> > >> + * disabled while beginning to freeze tasks).
> > >> + */
> > >> +static void cpu_hotplug_freezer_block_done(void)
> > >> +{
> > >> +	cpu_maps_update_begin();
> > >> +	cpu_hotplug_disabled = 0;
> > >> +	cpu_maps_update_done();
> > >> +}
> > >> +
> > > 
> > > I wonder if the new PM notifier events are really necessary?
> > > 
> > > Why don't you just call cpu_hotplug_freezer_block_begin() (perhaps
> > > with a better name?) directly from freeze_processes()?  And analogously
> > > for cpu_hotplug_freezer_block_done() and thaw_processes()?
> > > 
> > 
> > Yes, we can definitely do that. 
> > 
> > But the reason why I chose to introduce new notifiers was to make this
> > more extensible (because we know that at least 2 subsystems would benefit
> > from mutually excluding themselves from the freezer, namely CPU hotplug
> > and x86 microcode).
> > http://thread.gmane.org/gmane.linux.kernel/1198291/focus=1200591
> > 
> > But now that I think of it, hooking onto the freezer notifiers wouldn't
> > solve the microcode cases since usermodehelper_disable() is called
> > _before_ freezing tasks... :(
> > 
> > So we should probably call the functions directly like you suggested.. 
> > 
> > But I really didn't want to clutter the freezer call path because of problems
> > elsewhere. So I felt freezer notifiers would be a cleaner way of dealing with
> > such things. Also, since freezer is a generic subsystem that could be used
> > for purposes other than S3/S4 as well (I have heard of attempts to use freezer
> > during tracing), wouldn't it be better to introduce new notifiers to
> > announce the begin and end of freezer activity to interested subsystems?
> > (and then use them to solve the CPU hotplug issue like this patch does...)
> > 
> > Please let me know your suggestions.
> 
> The freeze_processes() and thaw_processes() functions are only used for
> system suspend and hibernation, as far as I can tell, and I don't think there
> will be any other users in predictable future.
> 
> Also, adding the calls directly to those functions will show exactly what
> the dependecies are, while doing that through a notifier kind of obfuscates
> things.  So, please make direct calls from there.

Alternatively, which I'd even prefer in fact, you can simply use the
PM_SUSPEND_PREPARE and PM_POST_SUSPEND notifier events (and analogously
for hibernation) to run that code.  Which also might be useful for solving
the microcode case.

Thanks,
Rafael

Re: [PATCH v4 2/2] CPU hotplug, Freezer: Synchronize CPU hotplug and Freezer

[Srivatsa S. Bhat]

On 10/28/2011 05:32 PM, Rafael J. Wysocki wrote:
> On Friday, October 28, 2011, Rafael J. Wysocki wrote:
>> On Friday, October 28, 2011, Srivatsa S. Bhat wrote:
>>> On 10/28/2011 01:43 AM, Rafael J. Wysocki wrote:
>>>> Hi,
>>>>
>>>> On Thursday, October 27, 2011, Srivatsa S. Bhat wrote:
>>>>> Prevent CPU hotplug and the freezer from racing with each other, to ensure
>>>>> that during the *entire duration* for which the callbacks for CPU hotplug
>>>>> notifications such as CPU_ONLINE[_FROZEN], CPU_DEAD[_FROZEN] etc are being
>>>>> executed, the state of the system (with respect to the tasks being frozen
>>>>> or not) remains constant.
>>>>>
>>>>> This patches hooks the CPU hotplug infrastructure onto the freezer
>>>>> notifications (PM_FREEZE_PREPARE and PM_POST_THAW) and thus synchronizes
>>>>> with the freezer.
>>>>>
>>>>> Specifically,
>>>>>
>>>>> * Upon the PM_FREEZE_PREPARE notification, the CPU hotplug callback disables
>>>>>   future (regular) CPU hotplugging and also ensures that any currently running
>>>>>   CPU hotplug operation is completed before allowing the freezer to continue
>>>>>   any further.
>>>>>
>>>>> * Upon the PM_POST_THAW notification, the CPU hotplug callback re-enables
>>>>>   regular CPU hotplug.
>>>>>
>>>>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>>>>> ---
>>>>>
>>>>>  kernel/cpu.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  1 files changed, 76 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/kernel/cpu.c b/kernel/cpu.c
>>>>> index 12b7458..61985ce 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/suspend.h>
>>>>>  
>>>>>  #ifdef CONFIG_SMP
>>>>>  /* Serializes the updates to cpu_online_mask, cpu_present_mask */
>>>>> @@ -478,6 +479,81 @@ 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.
>>>>> + *
>>>>> + * Also, don't allow the freezer subsystem to continue until any currently
>>>>> + * running CPU hotplug operation gets completed.
>>>>> + * To modify the 'cpu_hotplug_disabled' flag, we need to acquire the
>>>>> + * 'cpu_add_remove_lock'. And this same lock is also taken by the regular
>>>>> + * CPU hotplug path and released only after it is complete. Thus, we
>>>>> + * (and hence the freezer) will block here until any currently running CPU
>>>>> + * hotplug operation is completed.
>>>>> + */
>>>>> +static void cpu_hotplug_freezer_block_begin(void)
>>>>> +{
>>>>> +	cpu_maps_update_begin();
>>>>> +	cpu_hotplug_disabled = 1;
>>>>> +	cpu_maps_update_done();
>>>>> +}
>>>>> +
>>>>> +
>>>>> +/*
>>>>> + * When thawing of tasks is complete, re-enable CPU hotplug (which had been
>>>>> + * disabled while beginning to freeze tasks).
>>>>> + */
>>>>> +static void cpu_hotplug_freezer_block_done(void)
>>>>> +{
>>>>> +	cpu_maps_update_begin();
>>>>> +	cpu_hotplug_disabled = 0;
>>>>> +	cpu_maps_update_done();
>>>>> +}
>>>>> +
>>>>
>>>> I wonder if the new PM notifier events are really necessary?
>>>>
>>>> Why don't you just call cpu_hotplug_freezer_block_begin() (perhaps
>>>> with a better name?) directly from freeze_processes()?  And analogously
>>>> for cpu_hotplug_freezer_block_done() and thaw_processes()?
>>>>
>>>
>>> Yes, we can definitely do that. 
>>>
>>> But the reason why I chose to introduce new notifiers was to make this
>>> more extensible (because we know that at least 2 subsystems would benefit
>>> from mutually excluding themselves from the freezer, namely CPU hotplug
>>> and x86 microcode).
>>> http://thread.gmane.org/gmane.linux.kernel/1198291/focus=1200591
>>>
>>> But now that I think of it, hooking onto the freezer notifiers wouldn't
>>> solve the microcode cases since usermodehelper_disable() is called
>>> _before_ freezing tasks... :(
>>>
>>> So we should probably call the functions directly like you suggested.. 
>>>
>>> But I really didn't want to clutter the freezer call path because of problems
>>> elsewhere. So I felt freezer notifiers would be a cleaner way of dealing with
>>> such things. Also, since freezer is a generic subsystem that could be used
>>> for purposes other than S3/S4 as well (I have heard of attempts to use freezer
>>> during tracing), wouldn't it be better to introduce new notifiers to
>>> announce the begin and end of freezer activity to interested subsystems?
>>> (and then use them to solve the CPU hotplug issue like this patch does...)
>>>
>>> Please let me know your suggestions.
>>
>> The freeze_processes() and thaw_processes() functions are only used for
>> system suspend and hibernation, as far as I can tell, and I don't think there
>> will be any other users in predictable future.
>>
>> Also, adding the calls directly to those functions will show exactly what
>> the dependecies are, while doing that through a notifier kind of obfuscates
>> things.  So, please make direct calls from there.
> 
> Alternatively, which I'd even prefer in fact, you can simply use the
> PM_SUSPEND_PREPARE and PM_POST_SUSPEND notifier events (and analogously
> for hibernation) to run that code.  Which also might be useful for solving
> the microcode case.
> 

The primary reason I hadn't used those notifications for this purpose till now was
because I thought freezer might have usecases other than S3/S4 and hence we needed
synchronization at the freezer level. But now that you clarified that part, I'll
go ahead and use the SUSPEND/HIBERNATE notifiers for this.
Thank you very much.

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