[PATCH 0/4] PM / Freezer: Fix the semantics of thaw_processes() and thaw_kernel_threads()

[PATCH 0/4] PM / Freezer: Fix the semantics of thaw_processes() and thaw_kernel_threads()

[Srivatsa S. Bhat]

With the introduction of thaw_kernel_threads(), we can now finally fix the
semantics of thawing tasks, in such a way as to be just the opposite of
how tasks are frozen. This opens up some opportunities for code cleanup
as well. This patchset implements both of these improvements.

This patchset applies on top of the patch posted at
https://lkml.org/lkml/2012/1/30/439

--
 Srivatsa S. Bhat (4):
      PM/Freezer: Make thaw_processes() thaw only userspace tasks
      PM/Freezer: Use thaw_processes() and thaw_kernel_threads() correctly
      PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path
      PM/Hibernate: Refactor and simplify freezer_test_done


  kernel/power/hibernate.c |   22 +++++++++++++++-------
 kernel/power/power.h     |   15 +++++++++++++--
 kernel/power/process.c   |   13 ++++++-------
 kernel/power/user.c      |   20 ++++++++++++--------
 4 files changed, 46 insertions(+), 24 deletions(-)


Regards,
Srivatsa S. Bhat
IBM Linux Technology Center

[PATCH 1/4] PM/Freezer: Make thaw_processes() thaw only userspace tasks

[Srivatsa S. Bhat]

Currently the situation is:

freeze_processes() - freezes only userspace tasks
freeze_kernel_threads() - freezes only kernel threads
thaw_kernel_threads() - thaws only kernel threads
thaw_processes() - thaws *everything* (both userspace tasks and kernel threads)

The point that thaw_processes() thaws everything is rather unintuitive
and can lead to bugs. So, modify thaw_processes() so that it thaws only
userspace processes. This way we can also have more control over what
exactly gets thawed in different situations.

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

 kernel/power/process.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/power/process.c b/kernel/power/process.c
index eeca003..def6b1b 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -170,17 +170,15 @@ void thaw_processes(void)
 	if (pm_freezing)
 		atomic_dec(&system_freezing_cnt);
 	pm_freezing = false;
-	pm_nosig_freezing = false;
 
 	oom_killer_enable();
 
 	printk("Restarting tasks ... ");
 
-	thaw_workqueues();
-
 	read_lock(&tasklist_lock);
 	do_each_thread(g, p) {
-		__thaw_task(p);
+		if (!(p->flags & (PF_KTHREAD | PF_WQ_WORKER)))
+			__thaw_task(p);
 	} while_each_thread(g, p);
 	read_unlock(&tasklist_lock);
 

Re: [PATCH 1/4] PM/Freezer: Make thaw_processes() thaw only userspace tasks

[Tejun Heo]

On Tue, Jan 31, 2012 at 03:34:57AM +0530, Srivatsa S. Bhat wrote:
> Currently the situation is:
> 
> freeze_processes() - freezes only userspace tasks
> freeze_kernel_threads() - freezes only kernel threads
> thaw_kernel_threads() - thaws only kernel threads
> thaw_processes() - thaws *everything* (both userspace tasks and kernel threads)
> 
> The point that thaw_processes() thaws everything is rather unintuitive
> and can lead to bugs. So, modify thaw_processes() so that it thaws only
> userspace processes. This way we can also have more control over what
> exactly gets thawed in different situations.
> 
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Maybe I'm misreading it but doesn't this introduce window where kernel
tasks aren't thawed between this patch and the following ones?  It
looks like this one should come later.

Thanks.

-- 
tejun

Re: [PATCH 1/4] PM/Freezer: Make thaw_processes() thaw only userspace tasks

[Srivatsa S. Bhat]

On 01/31/2012 03:39 AM, Tejun Heo wrote:

> On Tue, Jan 31, 2012 at 03:34:57AM +0530, Srivatsa S. Bhat wrote:
>> Currently the situation is:
>>
>> freeze_processes() - freezes only userspace tasks
>> freeze_kernel_threads() - freezes only kernel threads
>> thaw_kernel_threads() - thaws only kernel threads
>> thaw_processes() - thaws *everything* (both userspace tasks and kernel threads)
>>
>> The point that thaw_processes() thaws everything is rather unintuitive
>> and can lead to bugs. So, modify thaw_processes() so that it thaws only
>> userspace processes. This way we can also have more control over what
>> exactly gets thawed in different situations.
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> 
> Maybe I'm misreading it but doesn't this introduce window where kernel
> tasks aren't thawed between this patch and the following ones?  It
> looks like this one should come later.
> 


Yes, I was aware that it introduces such a window. But I ignored it in the
interest of making the patch series sensible (as in, for example, patch
2/4 wouldn't make much sense without patch 1/4).
Maybe I will interchange patch 1 and patch 2 and just reword the patch
descriptions suitably so that they still make sense..

Thanks a lot!

Regards,
Srivatsa S. Bhat

Re: [PATCH 1/4] PM/Freezer: Make thaw_processes() thaw only userspace tasks

[Tejun Heo]

On Tue, Jan 31, 2012 at 03:53:05AM +0530, Srivatsa S. Bhat wrote:
> Yes, I was aware that it introduces such a window. But I ignored it in the
> interest of making the patch series sensible (as in, for example, patch
> 2/4 wouldn't make much sense without patch 1/4).
> Maybe I will interchange patch 1 and patch 2 and just reword the patch
> descriptions suitably so that they still make sense..

Yeah, I think saying sth like "Make extra calls to thaw_kthread in
preparation for blah blah.." should do.

Thanks.

-- 
tejun

[PATCH 2/4] PM/Freezer: Use thaw_processes() and thaw_kernel_threads() correctly

[Srivatsa S. Bhat]

Now thaw_processes() thaws only userspace tasks (instead of thawing
everything, unlike what it used to do before). So fix the uses of
thaw_processes() by using thaw_kernel_threads() where appropriate.

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

 kernel/power/hibernate.c |    5 +++++
 kernel/power/power.h     |   15 +++++++++++++--
 kernel/power/process.c   |    7 ++++---
 kernel/power/user.c      |    8 ++++++++
 4 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index a5d4cf0..1670387 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -656,6 +656,11 @@ int hibernate(void)
 	}
 
  Thaw:
+	/*
+	 * Kernel threads had been frozen inside hibernation_snapshot().
+	 * So thaw them here, on the success path.
+	 */
+	thaw_kernel_threads();
 	thaw_processes();
  Finish:
 	free_basic_memory_bitmaps();
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 0c4defe..e37a1e0 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -231,12 +231,23 @@ extern int pm_test_level;
 #ifdef CONFIG_SUSPEND_FREEZER
 static inline int suspend_freeze_processes(void)
 {
-	int error = freeze_processes();
-	return error ? : freeze_kernel_threads();
+	int error;
+
+	error = freeze_processes();
+	if (error)
+		goto Finish;
+
+	error = freeze_kernel_threads();
+	if (error)
+		thaw_processes();
+
+Finish:
+	return error;
 }
 
 static inline void suspend_thaw_processes(void)
 {
+	thaw_kernel_threads();
 	thaw_processes();
 }
 #else
diff --git a/kernel/power/process.c b/kernel/power/process.c
index def6b1b..8ae6c4b 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -116,7 +116,8 @@ static int try_to_freeze_tasks(bool user_only)
 /**
  * freeze_processes - Signal user space processes to enter the refrigerator.
  *
- * On success, returns 0.  On failure, -errno and system is fully thawed.
+ * On success, returns 0.  On failure, -errno and all user space processes
+ * are thawed.
  */
 int freeze_processes(void)
 {
@@ -143,7 +144,7 @@ int freeze_processes(void)
 /**
  * freeze_kernel_threads - Make freezable kernel threads go to the refrigerator.
  *
- * On success, returns 0.  On failure, -errno and system is fully thawed.
+ * On success, returns 0. On failure, -errno and all kernel threads are thawed.
  */
 int freeze_kernel_threads(void)
 {
@@ -159,7 +160,7 @@ int freeze_kernel_threads(void)
 	BUG_ON(in_atomic());
 
 	if (error)
-		thaw_processes();
+		thaw_kernel_threads();
 	return error;
 }
 
diff --git a/kernel/power/user.c b/kernel/power/user.c
index 3e10007..eaf5c97 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -119,6 +119,7 @@ static int snapshot_release(struct inode *inode, struct file *filp)
 	free_all_swap_pages(data->swap);
 	if (data->frozen) {
 		pm_restore_gfp_mask();
+		thaw_kernel_threads();
 		thaw_processes();
 	}
 	pm_notifier_call_chain(data->mode == O_RDONLY ?
@@ -237,6 +238,13 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 		if (!data->frozen || data->ready)
 			break;
 		pm_restore_gfp_mask();
+
+		/*
+		 * Be paranoid and thaw kernel threads as well, in order to
+		 * make sure that the system gets thawed fully, irrespective
+		 * of which (error) path we took to come here.
+		 */
+		thaw_kernel_threads();
 		thaw_processes();
 		usermodehelper_enable();
 		data->frozen = 0;

[PATCH 3/4] PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path

[Srivatsa S. Bhat]

In the hibernation call path, the kernel threads are frozen inside
hibernation_snapshot(). If we happen to encounter an error further down
the road or if we are exiting early due to a successful freezer test,
then thaw kernel threads before returning to the caller.

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

 kernel/power/hibernate.c |    9 ++++++---
 kernel/power/user.c      |    8 ++------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 1670387..0ce1220 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -343,13 +343,13 @@ int hibernation_snapshot(int platform_mode)
 		 * successful freezer test.
 		 */
 		freezer_test_done = true;
-		goto Cleanup;
+		goto Thaw;
 	}
 
 	error = dpm_prepare(PMSG_FREEZE);
 	if (error) {
 		dpm_complete(PMSG_RECOVER);
-		goto Cleanup;
+		goto Thaw;
 	}
 
 	suspend_console();
@@ -385,6 +385,8 @@ int hibernation_snapshot(int platform_mode)
 	platform_end(platform_mode);
 	return error;
 
+ Thaw:
+	thaw_kernel_threads();
  Cleanup:
 	swsusp_free();
 	goto Close;
@@ -655,12 +657,13 @@ int hibernate(void)
 		pr_debug("PM: Image restored successfully.\n");
 	}
 
- Thaw:
 	/*
 	 * Kernel threads had been frozen inside hibernation_snapshot().
 	 * So thaw them here, on the success path.
 	 */
 	thaw_kernel_threads();
+
+ Thaw:
 	thaw_processes();
  Finish:
 	free_basic_memory_bitmaps();
diff --git a/kernel/power/user.c b/kernel/power/user.c
index eaf5c97..62dbe93 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -257,16 +257,12 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 		}
 		pm_restore_gfp_mask();
 		error = hibernation_snapshot(data->platform_support);
-		if (error) {
-			thaw_kernel_threads();
-		} else {
+		if (!error) {
 			error = put_user(in_suspend, (int __user *)arg);
 			if (!error && !freezer_test_done)
 				data->ready = 1;
-			if (freezer_test_done) {
+			if (freezer_test_done)
 				freezer_test_done = false;
-				thaw_kernel_threads();
-			}
 		}
 		break;
 

[PATCH 4/4] PM/Hibernate: Refactor and simplify freezer_test_done

[Srivatsa S. Bhat]

The code related to 'freezer_test_done' is needlessly convoluted.
Refactor the code and simplify the implementation.

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

 kernel/power/hibernate.c |   10 +++++-----
 kernel/power/user.c      |    4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 0ce1220..30b315a 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -629,12 +629,8 @@ int hibernate(void)
 		goto Finish;
 
 	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
-	if (error)
-		goto Thaw;
-	if (freezer_test_done) {
-		freezer_test_done = false;
+	if (error || freezer_test_done)
 		goto Thaw;
-	}
 
 	if (in_suspend) {
 		unsigned int flags = 0;
@@ -665,6 +661,10 @@ int hibernate(void)
 
  Thaw:
 	thaw_processes();
+
+	/* Don't bother checking whether freezer_test_done is true */
+	freezer_test_done = false;
+
  Finish:
 	free_basic_memory_bitmaps();
 	usermodehelper_enable();
diff --git a/kernel/power/user.c b/kernel/power/user.c
index 62dbe93..056088e 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -259,10 +259,10 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 		error = hibernation_snapshot(data->platform_support);
 		if (!error) {
 			error = put_user(in_suspend, (int __user *)arg);
-			if (!error && !freezer_test_done)
-				data->ready = 1;
 			if (freezer_test_done)
 				freezer_test_done = false;
+			else if (!error)
+				data->ready = 1;
 		}
 		break;