[PATCH v2] PM/Memory-hotplug: Avoid task freezing failures

[PATCH v2] PM/Memory-hotplug: Avoid task freezing failures

[Srivatsa S. Bhat]

The lock_system_sleep() function is used in the memory hotplug code at
several places in order to implement mutual exclusion with hibernation.
However, this function tries to acquire the 'pm_mutex' lock using
mutex_lock() and hence blocks in TASK_UNINTERRUPTIBLE state if it doesn't
get the lock. This would lead to task freezing failures and hence
hibernation failure as a consequence, even though the hibernation call path
successfully acquired the lock.

This patch fixes this issue by modifying lock_system_sleep() to use
mutex_trylock() in a loop until the lock is acquired, instead of using
mutex_lock(), in order to avoid going to uninterruptible sleep.
Also, try_to_freeze() is called within the loop, so that we don't cause
freezing failures due to busy looping.

v2: Tejun pointed problems with using mutex_lock_interruptible() in a
    while loop, when signals not related to freezing are involved.
    So, replaced it with mutex_trylock().

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

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

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 57a6924..c2b5aab 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -5,6 +5,7 @@
 #include <linux/notifier.h>
 #include <linux/init.h>
 #include <linux/pm.h>
+#include <linux/freezer.h>
 #include <linux/mm.h>
 #include <asm/errno.h>
 
@@ -380,7 +381,18 @@ static inline void unlock_system_sleep(void) {}
 
 static inline void lock_system_sleep(void)
 {
-	mutex_lock(&pm_mutex);
+	/*
+	 * We should not use mutex_lock() here because, in case we fail to
+	 * acquire the lock, it would put us to sleep in TASK_UNINTERRUPTIBLE
+	 * state, which would lead to task freezing failures. As a
+	 * consequence, hibernation would fail (even though it had acquired
+	 * the 'pm_mutex' lock).
+	 *
+	 * We should use try_to_freeze() in the while loop so that we don't
+	 * cause freezing failures due to busy looping.
+	 */
+	while (!mutex_trylock(&pm_mutex))
+		try_to_freeze();
 }
 
 static inline void unlock_system_sleep(void)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] PM/Memory-hotplug: Avoid task freezing failures

[Tejun Heo]

Hello,

On Wed, Nov 16, 2011 at 05:25:23PM +0530, Srivatsa S. Bhat wrote:
> v2: Tejun pointed problems with using mutex_lock_interruptible() in a
>     while loop, when signals not related to freezing are involved.
>     So, replaced it with mutex_trylock().
> 
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> 
>  include/linux/suspend.h |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 57a6924..c2b5aab 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -5,6 +5,7 @@
>  #include <linux/notifier.h>
>  #include <linux/init.h>
>  #include <linux/pm.h>
> +#include <linux/freezer.h>
>  #include <linux/mm.h>
>  #include <asm/errno.h>
>  
> @@ -380,7 +381,18 @@ static inline void unlock_system_sleep(void) {}
>  
>  static inline void lock_system_sleep(void)
>  {
> -	mutex_lock(&pm_mutex);
> +	/*
> +	 * We should not use mutex_lock() here because, in case we fail to
> +	 * acquire the lock, it would put us to sleep in TASK_UNINTERRUPTIBLE
> +	 * state, which would lead to task freezing failures. As a
> +	 * consequence, hibernation would fail (even though it had acquired
> +	 * the 'pm_mutex' lock).
> +	 *
> +	 * We should use try_to_freeze() in the while loop so that we don't
> +	 * cause freezing failures due to busy looping.
> +	 */
> +	while (!mutex_trylock(&pm_mutex))
> +		try_to_freeze();

I'm kinda lost.  We now always busy-loop if the lock is held by
someone else.  I can't see how that is an improvement.  If this isn't
an immediate issue, wouldn't it be better to wait for proper solution?

Thank you.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] PM/Memory-hotplug: Avoid task freezing failures

[Srivatsa S. Bhat]

On 11/16/2011 09:56 PM, Tejun Heo wrote:
> Hello,
> 
> On Wed, Nov 16, 2011 at 05:25:23PM +0530, Srivatsa S. Bhat wrote:
>> v2: Tejun pointed problems with using mutex_lock_interruptible() in a
>>     while loop, when signals not related to freezing are involved.
>>     So, replaced it with mutex_trylock().
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> ---
>>
>>  include/linux/suspend.h |   14 +++++++++++++-
>>  1 files changed, 13 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>> index 57a6924..c2b5aab 100644
>> --- a/include/linux/suspend.h
>> +++ b/include/linux/suspend.h
>> @@ -5,6 +5,7 @@
>>  #include <linux/notifier.h>
>>  #include <linux/init.h>
>>  #include <linux/pm.h>
>> +#include <linux/freezer.h>
>>  #include <linux/mm.h>
>>  #include <asm/errno.h>
>>  
>> @@ -380,7 +381,18 @@ static inline void unlock_system_sleep(void) {}
>>  
>>  static inline void lock_system_sleep(void)
>>  {
>> -	mutex_lock(&pm_mutex);
>> +	/*
>> +	 * We should not use mutex_lock() here because, in case we fail to
>> +	 * acquire the lock, it would put us to sleep in TASK_UNINTERRUPTIBLE
>> +	 * state, which would lead to task freezing failures. As a
>> +	 * consequence, hibernation would fail (even though it had acquired
>> +	 * the 'pm_mutex' lock).
>> +	 *
>> +	 * We should use try_to_freeze() in the while loop so that we don't
>> +	 * cause freezing failures due to busy looping.
>> +	 */
>> +	while (!mutex_trylock(&pm_mutex))
>> +		try_to_freeze();
> 
> I'm kinda lost.  We now always busy-loop if the lock is held by
> someone else.  I can't see how that is an improvement.  If this isn't
> an immediate issue, wouldn't it be better to wait for proper solution?
> 

lock_system_sleep() is used by memory hotplug to mutually exclude itself
from hibernation. Which means if memory hotplug didn't get the lock, then
the "someone else" is going to be the hibernation code path. 
And in that case, how can this busy-loop for long? The try_to_freeze() in
the loop will co-operate with the freezing of tasks (which is carried out
during hibernation) and this task will get frozen pretty soon.
And thus, this prevents task freezing failures. However, if we had slept
uninterruptibly like the original code does, then it is a sure-shot
candidate for freezing failure.
In fact I tried using this API as a test, in another scenario and as very
much expected, I encountered freezing failures with the original code.
And with this patch applied, those failures vanished, again as expected.

So, honestly I didn't understand what is wrong with the approach of this
patch. And as a consequence, I don't see why we should wait to fix this
issue. 

[And by the way recently I happened to see yet another proposed patch
trying to make use of this API. So wouldn't it be better to fix this
ASAP, especially when we have a fix readily available?]

Thanks,
Srivatsa S. Bhat

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] PM/Memory-hotplug: Avoid task freezing failures

[Tejun Heo]

Hello,

On Wed, Nov 16, 2011 at 10:52:14PM +0530, Srivatsa S. Bhat wrote:
> So, honestly I didn't understand what is wrong with the approach of this
> patch. And as a consequence, I don't see why we should wait to fix this
> issue. 
> 
> [And by the way recently I happened to see yet another proposed patch
> trying to make use of this API. So wouldn't it be better to fix this
> ASAP, especially when we have a fix readily available?]

It just doesn't look like a proper solution.  Nothing guarantees
freezing will happen soonish.  Not all pm operations involve freezer.
The exclusion is against mem hotplug at this point, right?  I don't
think it's a good idea to add such hack to fix a mostly theoretical
problem and it's actually quite likely someone would be scratching
head trying to figure out why the hell the CPU was hot spinning while
the system is trying to enter one of powersaving mode (we had similar
behavior in freezer code a while back and it was ugly).

So, let's either fix it properly or leave it alone.

Thank you.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] PM/Memory-hotplug: Avoid task freezing failures

[Srivatsa S. Bhat]

On 11/16/2011 11:13 PM, Tejun Heo wrote:
> Hello,
> 
> On Wed, Nov 16, 2011 at 10:52:14PM +0530, Srivatsa S. Bhat wrote:
>> So, honestly I didn't understand what is wrong with the approach of this
>> patch. And as a consequence, I don't see why we should wait to fix this
>> issue. 
>>
>> [And by the way recently I happened to see yet another proposed patch
>> trying to make use of this API. So wouldn't it be better to fix this
>> ASAP, especially when we have a fix readily available?]
> 
> It just doesn't look like a proper solution.  Nothing guarantees
> freezing will happen soonish.  Not all pm operations involve freezer.
> The exclusion is against mem hotplug at this point, right?  I don't
> think it's a good idea to add such hack to fix a mostly theoretical
> problem and it's actually quite likely someone would be scratching
> head trying to figure out why the hell the CPU was hot spinning while
> the system is trying to enter one of powersaving mode (we had similar
> behavior in freezer code a while back and it was ugly).
> 
> So, let's either fix it properly or leave it alone.
> 

Ok, so by "proper solution", are you referring to a totally different
method (than grabbing pm_mutex) to implement mutual exclusion between
subsystems and suspend/hibernation, something like the suspend blockers
stuff and friends? 
Or are you hinting at just the existing code itself being fixed more
properly than what this patch does, to avoid having side effects like
you pointed out?

I am just trying to figure out what would be the best way to proceed here.
By the way I consider it lucky that we have spotted this bug before we
actually hit it.. So I would really love to get this fixed before it
comes back to haunt us in the future ;-)

Thanks,
Srivatsa S. Bhat

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] PM/Memory-hotplug: Avoid task freezing failures

[Tejun Heo]

Hello,

On Wed, Nov 16, 2011 at 11:54:04PM +0530, Srivatsa S. Bhat wrote:
> Ok, so by "proper solution", are you referring to a totally different
> method (than grabbing pm_mutex) to implement mutual exclusion between
> subsystems and suspend/hibernation, something like the suspend blockers
> stuff and friends?
> Or are you hinting at just the existing code itself being fixed more
> properly than what this patch does, to avoid having side effects like
> you pointed out?

Oh, nothing fancy.  Just something w/o busy looping would be fine.
The stinking thing is we don't have mutex_lock_freezable().  Lack of
proper freezable interface seems to be a continuing problem and I'm
not sure what the proper solution should be at this point.  Maybe we
should promote freezable to a proper task state.  Maybe freezable
kthread is a bad idea to begin with.  Maybe instead of removing
freezable_with_signal() we should make that default, that way,
freezable can hitch on the pending signal handling (this creates
another set of problems tho - ie. who's responsible for clearing
TIF_SIGPENDING?).  I don't know.

Maybe just throw in msleep(10) there with fat ugly comment explaining
why the hack is necessary?

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] PM/Memory-hotplug: Avoid task freezing failures

[Srivatsa S. Bhat]

On 11/17/2011 12:11 AM, Tejun Heo wrote:
> Hello,
> 
> On Wed, Nov 16, 2011 at 11:54:04PM +0530, Srivatsa S. Bhat wrote:
>> Ok, so by "proper solution", are you referring to a totally different
>> method (than grabbing pm_mutex) to implement mutual exclusion between
>> subsystems and suspend/hibernation, something like the suspend blockers
>> stuff and friends?
>> Or are you hinting at just the existing code itself being fixed more
>> properly than what this patch does, to avoid having side effects like
>> you pointed out?
> 
> Oh, nothing fancy.  Just something w/o busy looping would be fine.
> The stinking thing is we don't have mutex_lock_freezable().  Lack of
> proper freezable interface seems to be a continuing problem and I'm
> not sure what the proper solution should be at this point.  Maybe we
> should promote freezable to a proper task state.  Maybe freezable
> kthread is a bad idea to begin with.  Maybe instead of removing
> freezable_with_signal() we should make that default, that way,
> freezable can hitch on the pending signal handling (this creates
> another set of problems tho - ie. who's responsible for clearing
> TIF_SIGPENDING?).  I don't know.
> 

Thanks a lot for the explanation! I now get an idea about your thoughts
on the fundamental issues with the freezer that are causing a broad range
of problems... Hmm, definitely something to ponder over...

> Maybe just throw in msleep(10) there with fat ugly comment explaining
> why the hack is necessary?
> 

Hehe, that surely sounds like the simplest of all the approaches you
suggested ;-) I'll add this to the while loop in the patch and repost
it, hoping we can solve the fundamental issues effectively at a later time.

Thanks,
Srivatsa S. Bhat

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] PM/Memory-hotplug: Avoid task freezing failures

[Rafael J. Wysocki]

On Wednesday, November 16, 2011, Tejun Heo wrote:
> Hello,
> 
> On Wed, Nov 16, 2011 at 11:54:04PM +0530, Srivatsa S. Bhat wrote:
> > Ok, so by "proper solution", are you referring to a totally different
> > method (than grabbing pm_mutex) to implement mutual exclusion between
> > subsystems and suspend/hibernation, something like the suspend blockers
> > stuff and friends?
> > Or are you hinting at just the existing code itself being fixed more
> > properly than what this patch does, to avoid having side effects like
> > you pointed out?
> 
> Oh, nothing fancy.  Just something w/o busy looping would be fine.
> The stinking thing is we don't have mutex_lock_freezable().  Lack of
> proper freezable interface seems to be a continuing problem and I'm
> not sure what the proper solution should be at this point.  Maybe we
> should promote freezable to a proper task state.  Maybe freezable
> kthread is a bad idea to begin with.

It generally is, but some of them really want to be freezable.

> Maybe instead of removing
> freezable_with_signal() we should make that default, that way,
> freezable can hitch on the pending signal handling (this creates
> another set of problems tho - ie. who's responsible for clearing
> TIF_SIGPENDING?).  I don't know.
> 
> Maybe just throw in msleep(10) there with fat ugly comment explaining
> why the hack is necessary?

Perhaps.

Thanks,
Rafael

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>