[PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Commit bcda53faf5814c0c6025a0bd47108adfcbe9f199, "PM / Sleep: Replace
mutex_[un]lock(&pm_mutex) with [un]lock_system_sleep()", modified
snapshot_read() and snapshot_write() in kernel/power/user.c, among
other things, by making them use lock_system_sleep() and
unlock_system_sleep() instead of just locking and unlocking pm_mutex.
Unfortunately, however, this was a mistake, because these routines
are supposed to be executed after processes have been frozen
(i.e. when system_freezing_cnt is nonzero), so when
unlock_system_sleep() is executed by one of them, this causes the
caller to execute try_to_freeze() and go into the refrigerator as
a result.  This, in turn, deadlocks the suspend process and locks up
the system.

Fix the problem by reverting the part of commit bcda53faf5814c0c6025a
that changed snapshot_read() and snapshot_write().  Additionally,
make these functions check system_freezing_cnt and return error codes
if it is zero to ensure that they won't do anything if tasks have not
been frozen.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/user.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Index: linux/kernel/power/user.c
===================================================================
--- linux.orig/kernel/power/user.c
+++ linux/kernel/power/user.c
@@ -137,7 +137,10 @@ static ssize_t snapshot_read(struct file
 	ssize_t res;
 	loff_t pg_offp = *offp & ~PAGE_MASK;
 
-	lock_system_sleep();
+	if (!atomic_read(&system_freezing_cnt))
+		return -EBUSY;
+
+	mutex_lock(&pm_mutex);
 
 	data = filp->private_data;
 	if (!data->ready) {
@@ -158,7 +161,7 @@ static ssize_t snapshot_read(struct file
 		*offp += res;
 
  Unlock:
-	unlock_system_sleep();
+	mutex_unlock(&pm_mutex);
 
 	return res;
 }
@@ -170,7 +173,10 @@ static ssize_t snapshot_write(struct fil
 	ssize_t res;
 	loff_t pg_offp = *offp & ~PAGE_MASK;
 
-	lock_system_sleep();
+	if (!atomic_read(&system_freezing_cnt))
+		return -EBUSY;
+
+	mutex_lock(&pm_mutex);
 
 	data = filp->private_data;
 
@@ -187,7 +193,7 @@ static ssize_t snapshot_write(struct fil
 	if (res > 0)
 		*offp += res;
 unlock:
-	unlock_system_sleep();
+	mutex_unlock(&pm_mutex);
 
 	return res;
 }

[Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

Commit bcda53faf5814c0c6025a0bd47108adfcbe9f199, "PM / Sleep: Replace
mutex_[un]lock(&pm_mutex) with [un]lock_system_sleep()", modified
snapshot_read() and snapshot_write() in kernel/power/user.c, among
other things, by making them use lock_system_sleep() and
unlock_system_sleep() instead of just locking and unlocking pm_mutex.
Unfortunately, however, this was a mistake, because these routines
are supposed to be executed after processes have been frozen
(i.e. when system_freezing_cnt is nonzero), so when
unlock_system_sleep() is executed by one of them, this causes the
caller to execute try_to_freeze() and go into the refrigerator as
a result.  This, in turn, deadlocks the suspend process and locks up
the system.

Fix the problem by reverting the part of commit bcda53faf5814c0c6025a
that changed snapshot_read() and snapshot_write().

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---

Well, it wasn't a good idea to make snapshot_write() check
system_freezing_cnt, because the resume process doesn't freeze tasks.
So, it's better to simply revert the changes that introduced the
original regression.

Thanks,
Rafael

---
 kernel/power/user.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux/kernel/power/user.c
===================================================================
--- linux.orig/kernel/power/user.c
+++ linux/kernel/power/user.c
@@ -137,7 +137,7 @@ static ssize_t snapshot_read(struct file
 	ssize_t res;
 	loff_t pg_offp = *offp & ~PAGE_MASK;
 
-	lock_system_sleep();
+	mutex_lock(&pm_mutex);
 
 	data = filp->private_data;
 	if (!data->ready) {
@@ -158,7 +158,7 @@ static ssize_t snapshot_read(struct file
 		*offp += res;
 
  Unlock:
-	unlock_system_sleep();
+	mutex_unlock(&pm_mutex);
 
 	return res;
 }
@@ -170,7 +170,7 @@ static ssize_t snapshot_write(struct fil
 	ssize_t res;
 	loff_t pg_offp = *offp & ~PAGE_MASK;
 
-	lock_system_sleep();
+	mutex_lock(&pm_mutex);
 
 	data = filp->private_data;
 
@@ -187,7 +187,7 @@ static ssize_t snapshot_write(struct fil
 	if (res > 0)
 		*offp += res;
 unlock:
-	unlock_system_sleep();
+	mutex_unlock(&pm_mutex);
 
 	return res;
 }

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Srivatsa S. Bhat]

On 01/18/2012 04:45 AM, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()
> 
> Commit bcda53faf5814c0c6025a0bd47108adfcbe9f199, "PM / Sleep: Replace
> mutex_[un]lock(&pm_mutex) with [un]lock_system_sleep()", modified
> snapshot_read() and snapshot_write() in kernel/power/user.c, among
> other things, by making them use lock_system_sleep() and
> unlock_system_sleep() instead of just locking and unlocking pm_mutex.
> Unfortunately, however, this was a mistake, because these routines
> are supposed to be executed after processes have been frozen
> (i.e. when system_freezing_cnt is nonzero), so when
> unlock_system_sleep() is executed by one of them, this causes the
> caller to execute try_to_freeze() and go into the refrigerator as
> a result.  This, in turn, deadlocks the suspend process and locks up
> the system.
> 
> Fix the problem by reverting the part of commit bcda53faf5814c0c6025a
> that changed snapshot_read() and snapshot_write().
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---


This quick fix is good to fix the regression, but I am wondering if going
further, it would be better to rewrite unlock_system_sleep() by open-coding
a part of freezer_count() and excluding try_to_freeze() in that. 

When freezer_count() is used as it is (in other parts of the kernel), it
makes sense (that is, the try_to_freeze() embedded within it is justified).

However, in the case of unlock_system_sleep(), I don't quite see why
try_to_freeze() would be useful in any case at all... And moreover, it is
also semantically misleading because, unlock_system_sleep() tries to freeze
us behind our back, when we only intended to grab and release the pm_mutex
safely (as advertised by these APIs). IOW, commit bcda53f, while promising
to make stuff freezer-safe and nothing else, introduced a fundamental,
unneeded functionality change in all call paths that these APIs are invoked
in, by having that try_to_freeze() hidden inside. And now we just realized
that this change is not just unneeded, but it is harmful too!

So how about something like the following patch?
(This is not on top of your patch. Rather, this patch is an alternative to
your fix. Please let me know your thoughts.)

----
From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Subject: [PATCH] PM / Hibernate: Rewrite unlock_system_sleep() to fix s2disk regression

Commit 33e638b, "PM / Sleep: Use the freezer_count() functions in
[un]lock_system_sleep() APIs" introduced an undesirable change in the
behaviour of unlock_system_sleep() since freezer_count() internally calls
try_to_freeze() - which we don't need in unlock_system_sleep().

And commit bcda53f, "PM / Sleep: Replace mutex_[un]lock(&pm_mutex) with
[un]lock_system_sleep()" made these APIs wide-spread. This caused a
regression in suspend-to-disk where snapshot_read() and snapshot_write()
were getting frozen due to the try_to_freeze embedded in
unlock_system_sleep(), since these functions were invoked when the freezing
condition was still in effect.

Fix this by rewriting unlock_system_sleep() by open-coding freezer_count()
and dropping the try_to_freeze() part. Not only will this fix the
regression but this will also ensure that the API only does what it is
intended to do, and nothing more, under the hood.

Reported-by: Rafael J. Wysocki <rjw@sisk.pl>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 include/linux/suspend.h |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 95040cc..a1db01f 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -364,7 +364,11 @@ static inline void lock_system_sleep(void)
 static inline void unlock_system_sleep(void)
 {
 	mutex_unlock(&pm_mutex);
-	freezer_count();
+	/*
+	 * Don't use freezer_count() because we don't want the
+	 * call to try_to_freeze() here.
+	 */
+	current->flags &= ~PF_FREEZER_SKIP;
 }
 
 #else /* !CONFIG_PM_SLEEP */

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Tejun Heo]

Hello,

On Wed, Jan 18, 2012 at 4:59 AM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> +       /*
> +        * Don't use freezer_count() because we don't want the
> +        * call to try_to_freeze() here.
> +        */

Please explain "why" here.

Thanks.

-- 
tejun

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Srivatsa S. Bhat]

Hello Tejun,

On 01/18/2012 09:12 PM, Tejun Heo wrote:

> Hello,
> 
> On Wed, Jan 18, 2012 at 4:59 AM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> +       /*
>> +        * Don't use freezer_count() because we don't want the
>> +        * call to try_to_freeze() here.
>> +        */
> 
> Please explain "why" here.
>


Ok, how about the following patch?
----
From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Subject: [PATCH] PM / Hibernate: Rewrite unlock_system_sleep() to fix s2disk regression

Commit 33e638b, "PM / Sleep: Use the freezer_count() functions in
[un]lock_system_sleep() APIs" introduced an undesirable change in the
behaviour of unlock_system_sleep() since freezer_count() internally calls
try_to_freeze() - which we don't need in unlock_system_sleep().

And commit bcda53f, "PM / Sleep: Replace mutex_[un]lock(&pm_mutex) with
[un]lock_system_sleep()" made these APIs wide-spread. This caused a
regression in suspend-to-disk where snapshot_read() and snapshot_write()
were getting frozen due to the try_to_freeze embedded in
unlock_system_sleep(), since these functions were invoked when the freezing
condition was still in effect.

Fix this by rewriting unlock_system_sleep() by open-coding freezer_count()
and dropping the try_to_freeze() part. Not only will this fix the
regression but this will also ensure that the API only does what it is
intended to do, and nothing more, under the hood.

Reported-by: Rafael J. Wysocki <rjw@sisk.pl>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 include/linux/suspend.h |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 95040cc..6e76380 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -364,7 +364,18 @@ static inline void lock_system_sleep(void)
 static inline void unlock_system_sleep(void)
 {
 	mutex_unlock(&pm_mutex);
-	freezer_count();
+
+	/*
+	 * Don't use freezer_count() because we don't want the call to
+	 * try_to_freeze() here.
+	 *
+	 * Reason:
+	 * unlock_system_sleep() gets called in snapshot_read() and
+	 * snapshot_write() when the freezing condition is still in effect.
+	 * Which means, if we use try_to_freeze() here, it would make them
+	 * enter the refrigerator, thus causing suspend-to-disk to lockup.
+	 */
+	current->flags &= ~PF_FREEZER_SKIP;
 }
 
 #else /* !CONFIG_PM_SLEEP */

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Tejun Heo]

Hello,

On Wed, Jan 18, 2012 at 8:54 AM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> +       /*
> +        * Don't use freezer_count() because we don't want the call to
> +        * try_to_freeze() here.
> +        *
> +        * Reason:
> +        * unlock_system_sleep() gets called in snapshot_read() and
> +        * snapshot_write() when the freezing condition is still in effect.
> +        * Which means, if we use try_to_freeze() here, it would make them
> +        * enter the refrigerator, thus causing suspend-to-disk to lockup.
> +        */

Yeap, much better but wouldn't it be better if the description was
higher level?  ie. Explain why try_to_freeze() doesn't make sense for
this operation semantically and then, optionally, give an example of
breakage.  It usually is lack of proper rationale / reasoning that
confuses people reading the code.

Thank you.

-- 
tejun

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Srivatsa S. Bhat]

Hello Tejun,

On 01/18/2012 10:28 PM, Tejun Heo wrote:

> Hello,
> 
> On Wed, Jan 18, 2012 at 8:54 AM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> +       /*
>> +        * Don't use freezer_count() because we don't want the call to
>> +        * try_to_freeze() here.
>> +        *
>> +        * Reason:
>> +        * unlock_system_sleep() gets called in snapshot_read() and
>> +        * snapshot_write() when the freezing condition is still in effect.
>> +        * Which means, if we use try_to_freeze() here, it would make them
>> +        * enter the refrigerator, thus causing suspend-to-disk to lockup.
>> +        */
> 
> Yeap, much better but wouldn't it be better if the description was
> higher level?  ie. Explain why try_to_freeze() doesn't make sense for
> this operation semantically and then, optionally, give an example of
> breakage.  It usually is lack of proper rationale / reasoning that
> confuses people reading the code.
> 


I agree, but I was trying to keep the comment from growing too long ;)

In fact, before updating that patch, I wrote up the following description
but then didn't post it. I know you are asking me to update the comment
in the code, but let me post my original description as well, atleast just
because I took the effort to write it up ;)

Explanation as to why try_to_freeze() must not be used in
unlock_system_sleep():

We don't want try_to_freeze() here because of 2 reasons:

a) It is unnecessary at this point:
   Freezing always begins and ends in a piece of code which is protected by
   pm_mutex. 

   Some code piece, say "X":             PM/Freezer call path:

				         grab pm_mutex
				         start freezing tasks
   [-- lock_system_sleep() --]
				         skip freezing "X" since
				        it called freezer_do_not_count()

				         freezing succeeded.
				         Other things get done (suspend
				         or hibernate etc).

				         release pm_mutex

   lock_system_sleep()
   (it is only *now* that this
   function returned since pm_mutex
   had been grabbed all this while
   by the PM path.)

				          Let us say another PM operation
				          like suspend/hibernate begins.
					   So try to grab pm_mutex.

   /* do something */

   unlock_system_sleep()
   //Point 1

                		          It is only now that we acquire
					  pm_mutex.
					  So freezing for example, can
					  start only now.
					  //Point 2


>From the above depiction, it is clear that try_to_freeze() at Point 1 is
pointless because freezing can only begin much later, at Point 2.
When freezer_count() is called in some generic code, the try_to_freeze() is
justified because, we had asked the freezer to skip us earlier, and now we
want the freezer to consider us again. And hence, just in case the freezer
is about to give up based on its 20 second timeout, we are extra-cooperative
- we immediately call try_to_freeze() ourselves.

However, in the unlock_system_sleep() case, the freezer won't be anywhere
near its 20 second timeout, because freezing wouldn't have begun at all!
Freezing would start only after we release the pm_mutex in
unlock_system_sleep.

So, considering all this, try_to_freeze() is not needed in
unlock_system_sleep() path.

b) A more important reason why try_to_freeze() should not be used here is that
   it causes suspend-to-disk to lock up, as pointed out by Rafael.
   So its no longer "pointless/unnecessary", it is now "known to make things
   fail and hence not allowed".

//End of my description :)

I'll see how best I can summarize this in a short comment and post an updated
patch when I'm done.

Regards,
Srivatsa S. Bhat

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Tejun Heo]

Hello, Srivatsa.

On Wed, Jan 18, 2012 at 10:49:09PM +0530, Srivatsa S. Bhat wrote:
> I agree, but I was trying to keep the comment from growing too long ;)

It doesn't have to be long.  It just has to give some meaning to the
decision.  AFAICS, it is correct to call try_to_freeze() on
unlock_system_sleep() regardless of 20sec window.  There's no
guarantee the unlocking task is gonna hit try_to_freeze() elsewhere
and not calling it actually makes the interface buggy.

That said, it causes a problem because unlock_system_sleep() is called
in a special context during later stage of hibernation where the usual
expectation - that a freezable task which sees a freezing condition
should freeze - doesn't hold.

The correct solution would be somehow marking that condition so that
either try_to_freeze() doesn't get invoked or gets nullified -
e.g. making the SKIP thing a counter and ensure the hibernating task
has it elevated throughout the whole process.  Alternatively, if the
code path is limited enough, using a different version of the unlock
function, unlock_system_sleep_nofreeze() or whatever, would work too -
this is a popular approach for synchronization functions which
interacts with scheduler and preemption.

For now, as a quick fix, maybe not calling try_to_freeze()
unconditionally is okay, I don't know, but it's a hack.

Thanks.

-- 
tejun

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Tejun Heo]

On Wed, Jan 18, 2012 at 09:30:37AM -0800, Tejun Heo wrote:
> Hello, Srivatsa.
> 
> On Wed, Jan 18, 2012 at 10:49:09PM +0530, Srivatsa S. Bhat wrote:
> > I agree, but I was trying to keep the comment from growing too long ;)
> 
> It doesn't have to be long.  It just has to give some meaning to the
> decision.  AFAICS, it is correct to call try_to_freeze() on
> unlock_system_sleep() regardless of 20sec window.  There's no
> guarantee the unlocking task is gonna hit try_to_freeze() elsewhere
> and not calling it actually makes the interface buggy.

To give an example,

	/*
	 * XXX: Open code SKIP clearing for now to avoid invoking
	 * try_to_freeze().  This isn't correct but this function is
	 * called from deep inside hibernation path and calling
	 * try_to_freeze() leads to hang during hibernation.  This
	 * will be properly fixed soon.  See commit message for
	 * more details.
	 */

Thanks.

-- 
tejun

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Srivatsa S. Bhat]

On 01/18/2012 11:00 PM, Tejun Heo wrote:

> Hello, Srivatsa.
> 
> On Wed, Jan 18, 2012 at 10:49:09PM +0530, Srivatsa S. Bhat wrote:
>> I agree, but I was trying to keep the comment from growing too long ;)
> 
> It doesn't have to be long.  It just has to give some meaning to the
> decision.  AFAICS, it is correct to call try_to_freeze() on
> unlock_system_sleep() regardless of 20sec window.  There's no
> guarantee the unlocking task is gonna hit try_to_freeze() elsewhere
> and not calling it actually makes the interface buggy.
> 


Not really -

In short, there is a *key* difference between:

freezer_do_not_count() - freezer_count()

vs

lock_system_sleep() - unlock_system_sleep()

And that is, the latter pair acquire and release pm_mutex -
and the freezer doesn't do *anything* without first acquiring the same
pm_mutex.

Which is convincing enough to start believing that try_to_freeze()
might be unnecessary in the latter pair.

Now, more explanation follows:
Consider the 2 cases below:

Some random code:

Case 1:

/* Was doing something */

freezer_do_not_count();

/* Important work */

freezer_count();

Case 2:

/* Was doing something */

lock_system_sleep();

/* Important work */

unlock_system_sleep();

In case 1, we asked the freezer to ignore us until we are done with some
"important" work. And precisely because we interfered with the freezer
trying to freeze us, it is our duty to cooperate with the freezer when
our "important" work is finished. Which is why we call try_to_freeze()
inside freezer_count().

However, in case 2, the same story as above applies, but with a catch:
After we asked the freezer to ignore us, we could not even *start* doing our
"important" work! Because, we were waiting for pm_mutex, since the freezer
had got hold of it!

So we get to do our "important work" only when the freezer and all other
PM operations after that is done. And here is another catch: Once we start
doing our "important work", no PM operation (freezing in particular) can begin,
because now *we* have acquired the pm_mutex :-)

So to summarize, calling try_to_freeze() inside some generic freezer_count()
is justified and _needed_. But in case of unlock_system_sleep(), as I depicted
in my previous post, it wouldn't buy us anything at all! (because freezing
wouldn't have even begun by then).

[ And in the case of freezer_count(), your point is right that calling
try_to_freeze() is not to help the freezer meet its 20sec deadline - the
freezer could have finished its work and gone away long before we completed
our "important" work. So the real reason why we call try_to_freeze in
freezer_count is that when some PM operation is running which requires tasks
to be frozen, we want to respect that condition and get frozen - but we
probably ended up totally bypassing the freezer by calling freezer_do_not_count,
but now that we are done with our important work, we want to manually enter
the refrigerator, to satisfy the freezing condition requested by the PM
operation in progress and hence avoid any havoc by running any further.]

> That said, it causes a problem because unlock_system_sleep() is called
> in a special context during later stage of hibernation where the usual
> expectation - that a freezable task which sees a freezing condition
> should freeze - doesn't hold.
> 
> The correct solution would be somehow marking that condition so that
> either try_to_freeze() doesn't get invoked or gets nullified -


Even I thought this would be the right solution at first, but then realized
that having try_to_freeze() inside unlock_system_sleep() is not at all
essential, to begin with.

And not having try_to_freeze() within unlock_system_sleep() automatically
solves the hibernation problem. In fact, it does even better:
Later stages of hibernation would no longer be a "special context" to watch
out for!

> e.g. making the SKIP thing a counter and ensure the hibernating task
> has it elevated throughout the whole process.  Alternatively, if the
> code path is limited enough, using a different version of the unlock
> function, unlock_system_sleep_nofreeze() or whatever, would work too -
> this is a popular approach for synchronization functions which
> interacts with scheduler and preemption.
> 
> For now, as a quick fix, maybe not calling try_to_freeze()
> unconditionally is okay, I don't know, but it's a hack.
> 


Somehow I don't think its a hack, based on my perception as described
above. But feel free to prove me wrong :-)
 
Regards,
Srivatsa S. Bhat

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Tejun Heo]

Hello,

On Thu, Jan 19, 2012 at 12:52:32AM +0530, Srivatsa S. Bhat wrote:
> Somehow I don't think its a hack, based on my perception as described
> above. But feel free to prove me wrong :-)

Thanks for the explanation.  Yeah, I agree and it's much simpler this
way, which is nice.  So, in short, because freezing state can't change
across lock_system_sleep(), there's no reason to check for freezing
state on unlock and this nicely resolves the freezer problem together.

The only thing to be careful is, then, we need to set and clear SKIP
inside pm_mutex.

Thanks.

-- 
tejun

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Srivatsa S. Bhat]

On 01/19/2012 01:00 AM, Tejun Heo wrote:

> Hello,
> 
> On Thu, Jan 19, 2012 at 12:52:32AM +0530, Srivatsa S. Bhat wrote:
>> Somehow I don't think its a hack, based on my perception as described
>> above. But feel free to prove me wrong :-)
> 
> Thanks for the explanation.  Yeah, I agree and it's much simpler this
> way, which is nice.  So, in short, because freezing state can't change
> across lock_system_sleep(), there's no reason to check for freezing
> state on unlock and this nicely resolves the freezer problem together.
> 


Absolutely!

> The only thing to be careful is, then, we need to set and clear SKIP
> inside pm_mutex.
> 


Not exactly. We need to set SKIP before grabbing pm_mutex and clear it
inside pm_mutex. The reason is that we decided to set SKIP in the first
place just to avoid the freezer from declaring failure when we are
blocked on pm_mutex. If we move it to *after* mutex_lock(&pm_mutex), that
original intention itself is not satisfied, and we will hit freezing
failures - IOW making the set and clear exercise useless!

So, something like this should work perfectly:

lock_system_sleep()
{
	freezer_do_not_count();
	mutex_lock(&pm_mutex);
	current->flags &= ~PF_FREEZER_SKIP;
}

But in the interest of making the code look a bit symmetric, we can do:

lock_system_sleep()
{
	freezer_do_not_count();
	mutex_lock(&pm_mutex);
}

unlock_system_sleep()
{
	current->flags &= ~PF_FREEZER_SKIP;
	mutex_unlock(&pm_mutex);
}
 
Regards,
Srivatsa S. Bhat
IBM Linux Technology Center

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Srivatsa S. Bhat]

On 01/19/2012 01:16 AM, Srivatsa S. Bhat wrote:

> On 01/19/2012 01:00 AM, Tejun Heo wrote:
> 
>> Hello,
>>
>> On Thu, Jan 19, 2012 at 12:52:32AM +0530, Srivatsa S. Bhat wrote:
>>> Somehow I don't think its a hack, based on my perception as described
>>> above. But feel free to prove me wrong :-)
>>
>> Thanks for the explanation.  Yeah, I agree and it's much simpler this
>> way, which is nice.  So, in short, because freezing state can't change
>> across lock_system_sleep(), there's no reason to check for freezing
>> state on unlock and this nicely resolves the freezer problem together.
>>
> 
> 
> Absolutely!
> 
>> The only thing to be careful is, then, we need to set and clear SKIP
>> inside pm_mutex.
>>
> 
> 
> Not exactly. We need to set SKIP before grabbing pm_mutex and clear it
> inside pm_mutex. The reason is that we decided to set SKIP in the first
> place just to avoid the freezer from declaring failure when we are
> blocked on pm_mutex. If we move it to *after* mutex_lock(&pm_mutex), that
> original intention itself is not satisfied, and we will hit freezing
> failures - IOW making the set and clear exercise useless!
> 
> So, something like this should work perfectly:
> 
> lock_system_sleep()
> {
> 	freezer_do_not_count();
> 	mutex_lock(&pm_mutex);
> 	current->flags &= ~PF_FREEZER_SKIP;
> }
> 
> But in the interest of making the code look a bit symmetric, we can do:
> 
> lock_system_sleep()
> {
> 	freezer_do_not_count();
> 	mutex_lock(&pm_mutex);
> }
> 
> unlock_system_sleep()
> {
> 	current->flags &= ~PF_FREEZER_SKIP;
> 	mutex_unlock(&pm_mutex);
> }
>  


So how about this patch, with the comment updated?
(and the changelog updated as well to reflect the movement of code)

---
From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Subject: [PATCH] PM / Hibernate: Rewrite unlock_system_sleep() to fix s2disk regression

Commit 33e638b, "PM / Sleep: Use the freezer_count() functions in
[un]lock_system_sleep() APIs" introduced an undesirable change in the
behaviour of unlock_system_sleep() since freezer_count() internally calls
try_to_freeze() - which we don't need in unlock_system_sleep().

And commit bcda53f, "PM / Sleep: Replace mutex_[un]lock(&pm_mutex) with
[un]lock_system_sleep()" made these APIs wide-spread. This caused a
regression in suspend-to-disk where snapshot_read() and snapshot_write()
were getting frozen due to the try_to_freeze embedded in
unlock_system_sleep(), since these functions were invoked when the freezing
condition was still in effect.

Fix this by rewriting unlock_system_sleep() by open-coding freezer_count()
and dropping the try_to_freeze() part. Not only will this fix the
regression but this will also ensure that the API only does what it is
intended to do, and nothing more, under the hood.

While at it, make the code more correct and robust by ensuring that the
PF_FREEZER_SKIP flag gets cleared with pm_mutex held, to avoid a race with
the freezer.

Reported-by: Rafael J. Wysocki <rjw@sisk.pl>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 include/linux/suspend.h |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 95040cc..cb9d3f4 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -363,8 +363,23 @@ static inline void lock_system_sleep(void)
 
 static inline void unlock_system_sleep(void)
 {
+	/*
+	 * Don't use freezer_count() because we don't want the call to
+	 * try_to_freeze() here.
+	 *
+	 * Reason:
+	 * Fundamentally, we just don't need it, because freezing condition
+	 * doesn't come into effect until we release the pm_mutex lock,
+	 * since the freezer always works with pm_mutex held.
+	 *
+	 * More importantly, in the case of hibernation,
+	 * unlock_system_sleep() gets called in snapshot_read() and
+	 * snapshot_write() when the freezing condition is still in effect.
+	 * Which means, if we use try_to_freeze() here, it would make them
+	 * enter the refrigerator, thus causing hibernation to lockup.
+	 */
+	current->flags &= ~PF_FREEZER_SKIP;
 	mutex_unlock(&pm_mutex);
-	freezer_count();
 }
 
 #else /* !CONFIG_PM_SLEEP */

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Tejun Heo]

On Thu, Jan 19, 2012 at 01:59:01AM +0530, Srivatsa S. Bhat wrote:
> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Subject: [PATCH] PM / Hibernate: Rewrite unlock_system_sleep() to fix s2disk regression
> 
> Commit 33e638b, "PM / Sleep: Use the freezer_count() functions in
> [un]lock_system_sleep() APIs" introduced an undesirable change in the
> behaviour of unlock_system_sleep() since freezer_count() internally calls
> try_to_freeze() - which we don't need in unlock_system_sleep().
> 
> And commit bcda53f, "PM / Sleep: Replace mutex_[un]lock(&pm_mutex) with
> [un]lock_system_sleep()" made these APIs wide-spread. This caused a
> regression in suspend-to-disk where snapshot_read() and snapshot_write()
> were getting frozen due to the try_to_freeze embedded in
> unlock_system_sleep(), since these functions were invoked when the freezing
> condition was still in effect.
> 
> Fix this by rewriting unlock_system_sleep() by open-coding freezer_count()
> and dropping the try_to_freeze() part. Not only will this fix the
> regression but this will also ensure that the API only does what it is
> intended to do, and nothing more, under the hood.
> 
> While at it, make the code more correct and robust by ensuring that the
> PF_FREEZER_SKIP flag gets cleared with pm_mutex held, to avoid a race with
> the freezer.
> 
> Reported-by: Rafael J. Wysocki <rjw@sisk.pl>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks!

-- 
tejun

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Rafael J. Wysocki]

On Wednesday, January 18, 2012, Srivatsa S. Bhat wrote:
> On 01/19/2012 01:16 AM, Srivatsa S. Bhat wrote:
> 
> > On 01/19/2012 01:00 AM, Tejun Heo wrote:
> > 
> >> Hello,
> >>
> >> On Thu, Jan 19, 2012 at 12:52:32AM +0530, Srivatsa S. Bhat wrote:
> >>> Somehow I don't think its a hack, based on my perception as described
> >>> above. But feel free to prove me wrong :-)
> >>
> >> Thanks for the explanation.  Yeah, I agree and it's much simpler this
> >> way, which is nice.  So, in short, because freezing state can't change
> >> across lock_system_sleep(), there's no reason to check for freezing
> >> state on unlock and this nicely resolves the freezer problem together.
> >>
> > 
> > 
> > Absolutely!
> > 
> >> The only thing to be careful is, then, we need to set and clear SKIP
> >> inside pm_mutex.
> >>
> > 
> > 
> > Not exactly. We need to set SKIP before grabbing pm_mutex and clear it
> > inside pm_mutex. The reason is that we decided to set SKIP in the first
> > place just to avoid the freezer from declaring failure when we are
> > blocked on pm_mutex. If we move it to *after* mutex_lock(&pm_mutex), that
> > original intention itself is not satisfied, and we will hit freezing
> > failures - IOW making the set and clear exercise useless!
> > 
> > So, something like this should work perfectly:
> > 
> > lock_system_sleep()
> > {
> > 	freezer_do_not_count();
> > 	mutex_lock(&pm_mutex);
> > 	current->flags &= ~PF_FREEZER_SKIP;
> > }
> > 
> > But in the interest of making the code look a bit symmetric, we can do:
> > 
> > lock_system_sleep()
> > {
> > 	freezer_do_not_count();
> > 	mutex_lock(&pm_mutex);
> > }
> > 
> > unlock_system_sleep()
> > {
> > 	current->flags &= ~PF_FREEZER_SKIP;
> > 	mutex_unlock(&pm_mutex);
> > }
> >  
> 
> 
> So how about this patch, with the comment updated?
> (and the changelog updated as well to reflect the movement of code)
> 
> ---
> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Subject: [PATCH] PM / Hibernate: Rewrite unlock_system_sleep() to fix s2disk regression
> 
> Commit 33e638b, "PM / Sleep: Use the freezer_count() functions in
> [un]lock_system_sleep() APIs" introduced an undesirable change in the
> behaviour of unlock_system_sleep() since freezer_count() internally calls
> try_to_freeze() - which we don't need in unlock_system_sleep().
> 
> And commit bcda53f, "PM / Sleep: Replace mutex_[un]lock(&pm_mutex) with
> [un]lock_system_sleep()" made these APIs wide-spread. This caused a
> regression in suspend-to-disk where snapshot_read() and snapshot_write()
> were getting frozen due to the try_to_freeze embedded in
> unlock_system_sleep(), since these functions were invoked when the freezing
> condition was still in effect.
> 
> Fix this by rewriting unlock_system_sleep() by open-coding freezer_count()
> and dropping the try_to_freeze() part. Not only will this fix the
> regression but this will also ensure that the API only does what it is
> intended to do, and nothing more, under the hood.
> 
> While at it, make the code more correct and robust by ensuring that the
> PF_FREEZER_SKIP flag gets cleared with pm_mutex held, to avoid a race with
> the freezer.
> 
> Reported-by: Rafael J. Wysocki <rjw@sisk.pl>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

So, I'd prefer lock_system_sleep() to be open-coded too, to
avoid situations in which someone changes freezer_do_not_count()
and freezer_count() leaving unmodified unlock_system_sleep() behind.

Also, have you actually tested this?

Rafael


> ---
> 
>  include/linux/suspend.h |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 95040cc..cb9d3f4 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -363,8 +363,23 @@ static inline void lock_system_sleep(void)
>  
>  static inline void unlock_system_sleep(void)
>  {
> +	/*
> +	 * Don't use freezer_count() because we don't want the call to
> +	 * try_to_freeze() here.
> +	 *
> +	 * Reason:
> +	 * Fundamentally, we just don't need it, because freezing condition
> +	 * doesn't come into effect until we release the pm_mutex lock,
> +	 * since the freezer always works with pm_mutex held.
> +	 *
> +	 * More importantly, in the case of hibernation,
> +	 * unlock_system_sleep() gets called in snapshot_read() and
> +	 * snapshot_write() when the freezing condition is still in effect.
> +	 * Which means, if we use try_to_freeze() here, it would make them
> +	 * enter the refrigerator, thus causing hibernation to lockup.
> +	 */
> +	current->flags &= ~PF_FREEZER_SKIP;
>  	mutex_unlock(&pm_mutex);
> -	freezer_count();
>  }
>  
>  #else /* !CONFIG_PM_SLEEP */
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Pavel Machek]

Hi!

> > > I agree, but I was trying to keep the comment from growing too long ;)
> > 
> > It doesn't have to be long.  It just has to give some meaning to the
> > decision.  AFAICS, it is correct to call try_to_freeze() on
> > unlock_system_sleep() regardless of 20sec window.  There's no
> > guarantee the unlocking task is gonna hit try_to_freeze() elsewhere
> > and not calling it actually makes the interface buggy.
> 
> To give an example,
> 
> 	/*
> 	 * XXX: Open code SKIP clearing for now to avoid invoking
> 	 * try_to_freeze().  This isn't correct but this function is
> 	 * called from deep inside hibernation path and calling
> 	 * try_to_freeze() leads to hang during hibernation.  This
> 	 * will be properly fixed soon.  See commit message for
> 	 * more details.
> 	 */

Which paths are affected?

With uswsusp, we have userland in control of hibernation process; I'd
say almost anything can be called...
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Srivatsa S. Bhat]

On 01/19/2012 04:07 PM, Pavel Machek wrote:

> Hi!
> 
>>>> I agree, but I was trying to keep the comment from growing too long ;)
>>>
>>> It doesn't have to be long.  It just has to give some meaning to the
>>> decision.  AFAICS, it is correct to call try_to_freeze() on
>>> unlock_system_sleep() regardless of 20sec window.  There's no
>>> guarantee the unlocking task is gonna hit try_to_freeze() elsewhere
>>> and not calling it actually makes the interface buggy.
>>
>> To give an example,
>>
>> 	/*
>> 	 * XXX: Open code SKIP clearing for now to avoid invoking
>> 	 * try_to_freeze().  This isn't correct but this function is
>> 	 * called from deep inside hibernation path and calling
>> 	 * try_to_freeze() leads to hang during hibernation.  This
>> 	 * will be properly fixed soon.  See commit message for
>> 	 * more details.
>> 	 */
> 
> Which paths are affected?
> 
> With uswsusp, we have userland in control of hibernation process; I'd
> say almost anything can be called...


Hi Pavel,
I am afraid you are looking at a stale comment. We finalized on a
different comment altogether.

Here is the latest patch:
http://thread.gmane.org/gmane.linux.kernel/1240493/focus=1240976

Rafael made some suggestions on this one here
http://thread.gmane.org/gmane.linux.kernel/1240493/focus=1241028

I will address that and send out a patch soon.

Do let me know if you have any comments, thanks!

Regards,
Srivatsa S. Bhat

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Srivatsa S. Bhat]

On 01/19/2012 03:52 AM, Rafael J. Wysocki wrote:

> On Wednesday, January 18, 2012, Srivatsa S. Bhat wrote:
>> On 01/19/2012 01:16 AM, Srivatsa S. Bhat wrote:
>>
>>> On 01/19/2012 01:00 AM, Tejun Heo wrote:
>>>
>>>> Hello,
>>>>
>>>> On Thu, Jan 19, 2012 at 12:52:32AM +0530, Srivatsa S. Bhat wrote:
>>>>> Somehow I don't think its a hack, based on my perception as described
>>>>> above. But feel free to prove me wrong :-)
>>>>
>>>> Thanks for the explanation.  Yeah, I agree and it's much simpler this
>>>> way, which is nice.  So, in short, because freezing state can't change
>>>> across lock_system_sleep(), there's no reason to check for freezing
>>>> state on unlock and this nicely resolves the freezer problem together.
>>>>
>>>
>>>
>>> Absolutely!
>>>
>>>> The only thing to be careful is, then, we need to set and clear SKIP
>>>> inside pm_mutex.
>>>>
>>>
>>>
>>> Not exactly. We need to set SKIP before grabbing pm_mutex and clear it
>>> inside pm_mutex. The reason is that we decided to set SKIP in the first
>>> place just to avoid the freezer from declaring failure when we are
>>> blocked on pm_mutex. If we move it to *after* mutex_lock(&pm_mutex), that
>>> original intention itself is not satisfied, and we will hit freezing
>>> failures - IOW making the set and clear exercise useless!
>>>
>>> So, something like this should work perfectly:
>>>
>>> lock_system_sleep()
>>> {
>>> 	freezer_do_not_count();
>>> 	mutex_lock(&pm_mutex);
>>> 	current->flags &= ~PF_FREEZER_SKIP;
>>> }
>>>
>>> But in the interest of making the code look a bit symmetric, we can do:
>>>
>>> lock_system_sleep()
>>> {
>>> 	freezer_do_not_count();
>>> 	mutex_lock(&pm_mutex);
>>> }
>>>
>>> unlock_system_sleep()
>>> {
>>> 	current->flags &= ~PF_FREEZER_SKIP;
>>> 	mutex_unlock(&pm_mutex);
>>> }
>>>  
>>
>>
>> So how about this patch, with the comment updated?
>> (and the changelog updated as well to reflect the movement of code)
>>
>> ---
>> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> Subject: [PATCH] PM / Hibernate: Rewrite unlock_system_sleep() to fix s2disk regression
>>
>> Commit 33e638b, "PM / Sleep: Use the freezer_count() functions in
>> [un]lock_system_sleep() APIs" introduced an undesirable change in the
>> behaviour of unlock_system_sleep() since freezer_count() internally calls
>> try_to_freeze() - which we don't need in unlock_system_sleep().
>>
>> And commit bcda53f, "PM / Sleep: Replace mutex_[un]lock(&pm_mutex) with
>> [un]lock_system_sleep()" made these APIs wide-spread. This caused a
>> regression in suspend-to-disk where snapshot_read() and snapshot_write()
>> were getting frozen due to the try_to_freeze embedded in
>> unlock_system_sleep(), since these functions were invoked when the freezing
>> condition was still in effect.
>>
>> Fix this by rewriting unlock_system_sleep() by open-coding freezer_count()
>> and dropping the try_to_freeze() part. Not only will this fix the
>> regression but this will also ensure that the API only does what it is
>> intended to do, and nothing more, under the hood.
>>
>> While at it, make the code more correct and robust by ensuring that the
>> PF_FREEZER_SKIP flag gets cleared with pm_mutex held, to avoid a race with
>> the freezer.
>>
>> Reported-by: Rafael J. Wysocki <rjw@sisk.pl>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> 
> So, I'd prefer lock_system_sleep() to be open-coded too, to
> avoid situations in which someone changes freezer_do_not_count()
> and freezer_count() leaving unmodified unlock_system_sleep() behind.
>


Hehe, actually, I had left lock_system_sleep() as it is, intentionally, due to
a far-fetched idea to keep the code sane: I felt that people who go through
this code would naturally feel tempted to replace any open-coded stuff (if
possible) with the appropriate function call. And my rationale was that when
they see that lock_system_sleep() has a function call but
unlock_system_sleep() has been open-coded, they would immediately know that
this was not yet another case of "oh I wasn't aware of this function,
hence I open-coded it", but it was rather "I was very much aware of the
available functions, but I chose to open-code it intentionally - so touch it
only if you really know what you are doing" :-)

And I guess, now that job of guarding the code from "obvious code cleanups"
like that would be done by the comment I added as per Tejun's suggestion.
And moreover, your point to open-code in lock_system_sleep() sounds more
compelling than my far-fetched idea :-) So okay, I will open-code it.. 

> Also, have you actually tested this?


Oh I almost forgot to mention: I have not specifically tested this with the
userspace s2disk utility (part of uswsusp/suspend-utils package) as such.
I have only tested it in general (by invoking hibernate by writing to
/sys/power/disk, and testing different levels of pm_test framework etc) to
ensure that this patch doesn't break anything else.
So, any help with testing the userspace utilities would be great!

So Rafael, could you please check if the following patch solves the
regression for you? Thanks a lot!

---
From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Subject: [PATCH] PM / Hibernate: Rewrite unlock_system_sleep() to fix s2disk regression

Commit 33e638b, "PM / Sleep: Use the freezer_count() functions in
[un]lock_system_sleep() APIs" introduced an undesirable change in the
behaviour of unlock_system_sleep() since freezer_count() internally calls
try_to_freeze() - which we don't need in unlock_system_sleep().

And commit bcda53f, "PM / Sleep: Replace mutex_[un]lock(&pm_mutex) with
[un]lock_system_sleep()" made these APIs wide-spread. This caused a
regression in suspend-to-disk where snapshot_read() and snapshot_write()
were getting frozen due to the try_to_freeze embedded in
unlock_system_sleep(), since these functions were invoked when the freezing
condition was still in effect.

Fix this by rewriting unlock_system_sleep() by open-coding freezer_count()
and dropping the try_to_freeze() part. Not only will this fix the
regression but this will also ensure that the API only does what it is
intended to do, and nothing more, under the hood.

While at it, make the code more correct and robust by ensuring that the
PF_FREEZER_SKIP flag gets cleared with pm_mutex held, to avoid a race with
the freezer.

Also, to be on the safer side, open-code freezer_do_not_count() as well
(inside lock_system_sleep()), to ensure that any unrelated modification to
freezer[_do_not]_count() does not break things again!

Reported-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 include/linux/suspend.h |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 95040cc..91784a4 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -357,14 +357,29 @@ extern bool pm_save_wakeup_count(unsigned int count);
 
 static inline void lock_system_sleep(void)
 {
-	freezer_do_not_count();
+	current->flags |= PF_FREEZER_SKIP;
 	mutex_lock(&pm_mutex);
 }
 
 static inline void unlock_system_sleep(void)
 {
+	/*
+	 * Don't use freezer_count() because we don't want the call to
+	 * try_to_freeze() here.
+	 *
+	 * Reason:
+	 * Fundamentally, we just don't need it, because freezing condition
+	 * doesn't come into effect until we release the pm_mutex lock,
+	 * since the freezer always works with pm_mutex held.
+	 *
+	 * More importantly, in the case of hibernation,
+	 * unlock_system_sleep() gets called in snapshot_read() and
+	 * snapshot_write() when the freezing condition is still in effect.
+	 * Which means, if we use try_to_freeze() here, it would make them
+	 * enter the refrigerator, thus causing hibernation to lockup.
+	 */
+	current->flags &= ~PF_FREEZER_SKIP;
 	mutex_unlock(&pm_mutex);
-	freezer_count();
 }
 
 #else /* !CONFIG_PM_SLEEP */

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Pavel Machek]

Hi!

> >> To give an example,
> >>
> >> 	/*
> >> 	 * XXX: Open code SKIP clearing for now to avoid invoking
> >> 	 * try_to_freeze().  This isn't correct but this function is
> >> 	 * called from deep inside hibernation path and calling
> >> 	 * try_to_freeze() leads to hang during hibernation.  This
> >> 	 * will be properly fixed soon.  See commit message for
> >> 	 * more details.
> >> 	 */
> > 
> > Which paths are affected?
> > 
> > With uswsusp, we have userland in control of hibernation process; I'd
> > say almost anything can be called...
> 
> 
> Hi Pavel,
> I am afraid you are looking at a stale comment. We finalized on a
> different comment altogether.

I know. But the underlying problem (uswsusp code is userland, and can
do anything) is still there, right? I guess limitations for
applications using uswsusp interface should be documented somewhere.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Srivatsa S. Bhat]

On 01/19/2012 11:10 PM, Pavel Machek wrote:

> Hi!
> 
>>>> To give an example,
>>>>
>>>> 	/*
>>>> 	 * XXX: Open code SKIP clearing for now to avoid invoking
>>>> 	 * try_to_freeze().  This isn't correct but this function is
>>>> 	 * called from deep inside hibernation path and calling
>>>> 	 * try_to_freeze() leads to hang during hibernation.  This
>>>> 	 * will be properly fixed soon.  See commit message for
>>>> 	 * more details.
>>>> 	 */
>>>
>>> Which paths are affected?
>>>
>>> With uswsusp, we have userland in control of hibernation process; I'd
>>> say almost anything can be called...
>>
>>
>> Hi Pavel,
>> I am afraid you are looking at a stale comment. We finalized on a
>> different comment altogether.
> 
> I know. But the underlying problem (uswsusp code is userland, and can
> do anything) is still there, right? I guess limitations for
> applications using uswsusp interface should be documented somewhere.

Sorry, but I think you missed the conclusion of the discussion in this
thread: The comment mentioned above is completely wrong! Rewriting
unlock_system_sleep() by open coding the SKIP clearing and omitting the
try_to_freeze() part is not a hack - instead, it is a clean and sane
design. Hence, with the latest version of my patch applied, it would
no longer be "hey, please watch out for this, this and this for now;
we know things are messy but we will fix it up later".
Instead it is like "Don't worry, we have fixed up the buggy
unlock_system_sleep() function. Go ahead and do what you want!"

On a more serious note, here is a (hopefully) convincing explanation
as to why the patch makes perfect sense:

http://thread.gmane.org/gmane.linux.kernel/1240493/focus=1240892
and
http://thread.gmane.org/gmane.linux.kernel/1240493/focus=1240940

Moreover, we have already advised people to use the lock_system_sleep()
and unlock_system_sleep() APIs instead of directly using mutex_lock and
mutex_unlock on pm_mutex, in Documentation/power/freezing-of-tasks.txt

But unfortunately, unlock_system_sleep() was actually not quite correct
and hence this patch fixes it. So things should be fine now...

But, in case you were hinting at some totally different problem, please
let me know..

Regards,
Srivatsa S. Bhat

Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

[Rafael J. Wysocki]

On Thursday, January 19, 2012, Srivatsa S. Bhat wrote:
> On 01/19/2012 03:52 AM, Rafael J. Wysocki wrote:
> 
> > On Wednesday, January 18, 2012, Srivatsa S. Bhat wrote:
> >> On 01/19/2012 01:16 AM, Srivatsa S. Bhat wrote:
> >>
> >>> On 01/19/2012 01:00 AM, Tejun Heo wrote:
> >>>
> >>>> Hello,
> >>>>
> >>>> On Thu, Jan 19, 2012 at 12:52:32AM +0530, Srivatsa S. Bhat wrote:
> >>>>> Somehow I don't think its a hack, based on my perception as described
> >>>>> above. But feel free to prove me wrong :-)
> >>>>
> >>>> Thanks for the explanation.  Yeah, I agree and it's much simpler this
> >>>> way, which is nice.  So, in short, because freezing state can't change
> >>>> across lock_system_sleep(), there's no reason to check for freezing
> >>>> state on unlock and this nicely resolves the freezer problem together.
> >>>>
> >>>
> >>>
> >>> Absolutely!
> >>>
> >>>> The only thing to be careful is, then, we need to set and clear SKIP
> >>>> inside pm_mutex.
> >>>>
> >>>
> >>>
> >>> Not exactly. We need to set SKIP before grabbing pm_mutex and clear it
> >>> inside pm_mutex. The reason is that we decided to set SKIP in the first
> >>> place just to avoid the freezer from declaring failure when we are
> >>> blocked on pm_mutex. If we move it to *after* mutex_lock(&pm_mutex), that
> >>> original intention itself is not satisfied, and we will hit freezing
> >>> failures - IOW making the set and clear exercise useless!
> >>>
> >>> So, something like this should work perfectly:
> >>>
> >>> lock_system_sleep()
> >>> {
> >>> 	freezer_do_not_count();
> >>> 	mutex_lock(&pm_mutex);
> >>> 	current->flags &= ~PF_FREEZER_SKIP;
> >>> }
> >>>
> >>> But in the interest of making the code look a bit symmetric, we can do:
> >>>
> >>> lock_system_sleep()
> >>> {
> >>> 	freezer_do_not_count();
> >>> 	mutex_lock(&pm_mutex);
> >>> }
> >>>
> >>> unlock_system_sleep()
> >>> {
> >>> 	current->flags &= ~PF_FREEZER_SKIP;
> >>> 	mutex_unlock(&pm_mutex);
> >>> }
> >>>  
> >>
> >>
> >> So how about this patch, with the comment updated?
> >> (and the changelog updated as well to reflect the movement of code)
> >>
> >> ---
> >> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> >> Subject: [PATCH] PM / Hibernate: Rewrite unlock_system_sleep() to fix s2disk regression
> >>
> >> Commit 33e638b, "PM / Sleep: Use the freezer_count() functions in
> >> [un]lock_system_sleep() APIs" introduced an undesirable change in the
> >> behaviour of unlock_system_sleep() since freezer_count() internally calls
> >> try_to_freeze() - which we don't need in unlock_system_sleep().
> >>
> >> And commit bcda53f, "PM / Sleep: Replace mutex_[un]lock(&pm_mutex) with
> >> [un]lock_system_sleep()" made these APIs wide-spread. This caused a
> >> regression in suspend-to-disk where snapshot_read() and snapshot_write()
> >> were getting frozen due to the try_to_freeze embedded in
> >> unlock_system_sleep(), since these functions were invoked when the freezing
> >> condition was still in effect.
> >>
> >> Fix this by rewriting unlock_system_sleep() by open-coding freezer_count()
> >> and dropping the try_to_freeze() part. Not only will this fix the
> >> regression but this will also ensure that the API only does what it is
> >> intended to do, and nothing more, under the hood.
> >>
> >> While at it, make the code more correct and robust by ensuring that the
> >> PF_FREEZER_SKIP flag gets cleared with pm_mutex held, to avoid a race with
> >> the freezer.
> >>
> >> Reported-by: Rafael J. Wysocki <rjw@sisk.pl>
> >> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> > 
> > So, I'd prefer lock_system_sleep() to be open-coded too, to
> > avoid situations in which someone changes freezer_do_not_count()
> > and freezer_count() leaving unmodified unlock_system_sleep() behind.
> >
> 
> 
> Hehe, actually, I had left lock_system_sleep() as it is, intentionally, due to
> a far-fetched idea to keep the code sane: I felt that people who go through
> this code would naturally feel tempted to replace any open-coded stuff (if
> possible) with the appropriate function call. And my rationale was that when
> they see that lock_system_sleep() has a function call but
> unlock_system_sleep() has been open-coded, they would immediately know that
> this was not yet another case of "oh I wasn't aware of this function,
> hence I open-coded it", but it was rather "I was very much aware of the
> available functions, but I chose to open-code it intentionally - so touch it
> only if you really know what you are doing" :-)
> 
> And I guess, now that job of guarding the code from "obvious code cleanups"
> like that would be done by the comment I added as per Tejun's suggestion.
> And moreover, your point to open-code in lock_system_sleep() sounds more
> compelling than my far-fetched idea :-) So okay, I will open-code it.. 
> 
> > Also, have you actually tested this?
> 
> 
> Oh I almost forgot to mention: I have not specifically tested this with the
> userspace s2disk utility (part of uswsusp/suspend-utils package) as such.
> I have only tested it in general (by invoking hibernate by writing to
> /sys/power/disk, and testing different levels of pm_test framework etc) to
> ensure that this patch doesn't break anything else.
> So, any help with testing the userspace utilities would be great!
> 
> So Rafael, could you please check if the following patch solves the
> regression for you? Thanks a lot!

Yes, it does, thanks!

Applied to linux-pm/linux-next.

Thanks,
Rafael


> ---
> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Subject: [PATCH] PM / Hibernate: Rewrite unlock_system_sleep() to fix s2disk regression
> 
> Commit 33e638b, "PM / Sleep: Use the freezer_count() functions in
> [un]lock_system_sleep() APIs" introduced an undesirable change in the
> behaviour of unlock_system_sleep() since freezer_count() internally calls
> try_to_freeze() - which we don't need in unlock_system_sleep().
> 
> And commit bcda53f, "PM / Sleep: Replace mutex_[un]lock(&pm_mutex) with
> [un]lock_system_sleep()" made these APIs wide-spread. This caused a
> regression in suspend-to-disk where snapshot_read() and snapshot_write()
> were getting frozen due to the try_to_freeze embedded in
> unlock_system_sleep(), since these functions were invoked when the freezing
> condition was still in effect.
> 
> Fix this by rewriting unlock_system_sleep() by open-coding freezer_count()
> and dropping the try_to_freeze() part. Not only will this fix the
> regression but this will also ensure that the API only does what it is
> intended to do, and nothing more, under the hood.
> 
> While at it, make the code more correct and robust by ensuring that the
> PF_FREEZER_SKIP flag gets cleared with pm_mutex held, to avoid a race with
> the freezer.
> 
> Also, to be on the safer side, open-code freezer_do_not_count() as well
> (inside lock_system_sleep()), to ensure that any unrelated modification to
> freezer[_do_not]_count() does not break things again!
> 
> Reported-by: Rafael J. Wysocki <rjw@sisk.pl>
> Acked-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> 
>  include/linux/suspend.h |   19 +++++++++++++++++--
>  1 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 95040cc..91784a4 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -357,14 +357,29 @@ extern bool pm_save_wakeup_count(unsigned int count);
>  
>  static inline void lock_system_sleep(void)
>  {
> -	freezer_do_not_count();
> +	current->flags |= PF_FREEZER_SKIP;
>  	mutex_lock(&pm_mutex);
>  }
>  
>  static inline void unlock_system_sleep(void)
>  {
> +	/*
> +	 * Don't use freezer_count() because we don't want the call to
> +	 * try_to_freeze() here.
> +	 *
> +	 * Reason:
> +	 * Fundamentally, we just don't need it, because freezing condition
> +	 * doesn't come into effect until we release the pm_mutex lock,
> +	 * since the freezer always works with pm_mutex held.
> +	 *
> +	 * More importantly, in the case of hibernation,
> +	 * unlock_system_sleep() gets called in snapshot_read() and
> +	 * snapshot_write() when the freezing condition is still in effect.
> +	 * Which means, if we use try_to_freeze() here, it would make them
> +	 * enter the refrigerator, thus causing hibernation to lockup.
> +	 */
> +	current->flags &= ~PF_FREEZER_SKIP;
>  	mutex_unlock(&pm_mutex);
> -	freezer_count();
>  }
>  
>  #else /* !CONFIG_PM_SLEEP */
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>