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

[PATCH v2 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.

v2: Interchanged the ordering of patches 1 and 2 and transferred a small
    comment between them. No code changes.
--
 Srivatsa S. Bhat (4):
      PM/Freezer: Use thaw_kernel_threads() in preparation for changes to thaw_processes()
      PM/Freezer: Make thaw_processes() thaw only userspace tasks
      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 v2 1/4] PM/Freezer: Use thaw_kernel_threads() in preparation for changes to thaw_processes()

[Srivatsa S. Bhat]

As of now, thaw_processes() is a superset of thaw_kernel_threads() in the
sense that thaw_processes() thaws both userspace tasks and kernel threads.
But that is going to change very soon. So, in preparation for that, add
some calls to thaw_kernel_threads() like this:

 * If only kernel threads need to be thawed, _replace_ thaw_processes()
   with thaw_kernel_threads().

 * If everything needs to be thawed, _add_ a call to thaw_kernel_threads()
   adjacent to the existing call to thaw_processes().

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   |    4 ++--
 kernel/power/user.c      |    8 ++++++++
 4 files changed, 28 insertions(+), 4 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 eeca003..3734fb9 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -143,7 +143,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 +159,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 v2 2/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 |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/power/process.c b/kernel/power/process.c
index 3734fb9..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)
 {
@@ -170,17 +171,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 v2 2/4] PM/Freezer: Make thaw_processes() thaw only userspace tasks

[Tejun Heo]

On Tue, Jan 31, 2012 at 04:44:48AM +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)

Umm... I don't really get this.  Why is this a problem?  The list is
not even correct.  freeze_kernel_threads() doesn't freeze "only"
kernel threads.  It freezes all threads "including" kernel threads and
that's only natural because you can't freeze kernel threads without
freezing userland threads and of course you can't thaw userland
threads without thawing kernel threads.

The system simply won't work if you do it otherwise and making them
disjoint operations increases the chance of bugs.  These operations
are naturally enclosed within each other and trying to break them
apart isn't a good idea.

What's the problem you're trying to solve here?  I don't really see
code clean up.  Code is different but not necessarily cleaner and FWIW
it seems more unnatural and brittle to me.

Thanks.

-- 
tejun

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

[Srivatsa S. Bhat]

On 01/31/2012 05:00 AM, Tejun Heo wrote:

> On Tue, Jan 31, 2012 at 04:44:48AM +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)
> 
> Umm... I don't really get this.  Why is this a problem?  The list is
> not even correct.  freeze_kernel_threads() doesn't freeze "only"
> kernel threads.  It freezes all threads "including" kernel threads and


Oh, you are are right - the list is incorrect. I guess I got carried away
thinking about thaw_kernel_threads().

> that's only natural because you can't freeze kernel threads without
> freezing userland threads and of course you can't thaw userland
> threads without thawing kernel threads.
>

> The system simply won't work if you do it otherwise and making them

> disjoint operations increases the chance of bugs.  These operations
> are naturally enclosed within each other and trying to break them
> apart isn't a good idea.
> 


Yeah, I get it now.. Thanks for the explanation!

> What's the problem you're trying to solve here?  I don't really see
> code clean up.  Code is different but not necessarily cleaner and FWIW
> it seems more unnatural and brittle to me.
> 


The thing is that, I wanted to avoid a bug in the patch posted at
https://lkml.org/lkml/2012/1/29/47 as explained in the link.

So I guess I should have simply done:

freeze_kernel_threads() calls thaw_kernel_threads() upon error.
The caller of freeze_kernel_threads() will call thaw_processes() if
necessary.

This way even the SNAPSHOT_CREATE_IMAGE ioctl would remain safe.

I'll think it through again and post an updated patch.
Thank you very much for the review!

Regards,
Srivatsa S. Bhat

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

[Tejun Heo]

Hello,

On Tue, Jan 31, 2012 at 05:39:00AM +0530, Srivatsa S. Bhat wrote:
> The thing is that, I wanted to avoid a bug in the patch posted at
> https://lkml.org/lkml/2012/1/29/47 as explained in the link.
> 
> So I guess I should have simply done:
> 
> freeze_kernel_threads() calls thaw_kernel_threads() upon error.
> The caller of freeze_kernel_threads() will call thaw_processes() if
> necessary.
> 
> This way even the SNAPSHOT_CREATE_IMAGE ioctl would remain safe.

Yeah, I'd prefer to avoid exporting "userland only" interface to
outside.  If it's some internal fail path thing, let's handle it
inside pm proper.

Thanks.

-- 
tejun

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

[Pavel Machek]

Hi!

> The thing is that, I wanted to avoid a bug in the patch posted at
> https://lkml.org/lkml/2012/1/29/47 as explained in the link.
> 
> So I guess I should have simply done:
> 
> freeze_kernel_threads() calls thaw_kernel_threads() upon error.

Seems like a good idea. When error is reported, call should be NOP.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH v2 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 v2 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;