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

[PATCH v3] 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, we use msleep() to avoid busy looping and breaking expectations
that we go to sleep when we fail to acquire the lock.
And we also call try_to_freeze() in order to cooperate with the freezer,
without which we would end up in almost the same situation as mutex_lock(),
due to uninterruptible sleep caused by msleep().

v3: Tejun suggested avoiding busy-looping by adding an msleep() since
    it is not guaranteed that we will get frozen immediately.

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 |   37 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 36 insertions(+), 1 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 57a6924..0af3048 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -5,6 +5,8 @@
 #include <linux/notifier.h>
 #include <linux/init.h>
 #include <linux/pm.h>
+#include <linux/freezer.h>
+#include <linux/delay.h>
 #include <linux/mm.h>
 #include <asm/errno.h>
 
@@ -380,7 +382,40 @@ static inline void unlock_system_sleep(void) {}
 
 static inline void lock_system_sleep(void)
 {
-	mutex_lock(&pm_mutex);
+	/*
+	 * "To sleep, or not to sleep, that is the question!"
+	 *
+	 * 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).
+	 * Using mutex_lock_interruptible() in a loop is not a good idea,
+	 * because we could end up treating non-freezing signals badly.
+	 * So we use mutex_trylock() in a loop instead.
+	 *
+	 * Also, we add try_to_freeze() to the loop, to co-operate with the
+	 * freezer, to avoid task freezing failures due to busy-looping.
+	 *
+	 * But then, since it is not guaranteed that we will get frozen
+	 * rightaway, we could keep spinning for some time, breaking the
+	 * expectation that we go to sleep when we fail to acquire the lock.
+	 * So we add an msleep() to the loop, to dampen the spin (but we are
+	 * careful enough not to sleep for too long at a stretch, lest the
+	 * freezer whine and give up again!).
+	 *
+	 * Now that we no longer busy-loop, try_to_freeze() becomes all the
+	 * more important, due to a subtle reason: if we don't cooperate with
+	 * the freezer at this point, we could end up in a situation very
+	 * similar to mutex_lock() due to the usage of msleep() (which sleeps
+	 * uninterruptibly).
+	 *
+	 * Phew! What a delicate balance!
+	 */
+	while (!mutex_trylock(&pm_mutex)) {
+		try_to_freeze();
+		msleep(10);
+	}
 }
 
 static inline void unlock_system_sleep(void)

[PATCH v3] 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, we use msleep() to avoid busy looping and breaking expectations
that we go to sleep when we fail to acquire the lock.
And we also call try_to_freeze() in order to cooperate with the freezer,
without which we would end up in almost the same situation as mutex_lock(),
due to uninterruptible sleep caused by msleep().

v3: Tejun suggested avoiding busy-looping by adding an msleep() since
    it is not guaranteed that we will get frozen immediately.

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 |   37 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 36 insertions(+), 1 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 57a6924..0af3048 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -5,6 +5,8 @@
 #include <linux/notifier.h>
 #include <linux/init.h>
 #include <linux/pm.h>
+#include <linux/freezer.h>
+#include <linux/delay.h>
 #include <linux/mm.h>
 #include <asm/errno.h>
 
@@ -380,7 +382,40 @@ static inline void unlock_system_sleep(void) {}
 
 static inline void lock_system_sleep(void)
 {
-	mutex_lock(&pm_mutex);
+	/*
+	 * "To sleep, or not to sleep, that is the question!"
+	 *
+	 * 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).
+	 * Using mutex_lock_interruptible() in a loop is not a good idea,
+	 * because we could end up treating non-freezing signals badly.
+	 * So we use mutex_trylock() in a loop instead.
+	 *
+	 * Also, we add try_to_freeze() to the loop, to co-operate with the
+	 * freezer, to avoid task freezing failures due to busy-looping.
+	 *
+	 * But then, since it is not guaranteed that we will get frozen
+	 * rightaway, we could keep spinning for some time, breaking the
+	 * expectation that we go to sleep when we fail to acquire the lock.
+	 * So we add an msleep() to the loop, to dampen the spin (but we are
+	 * careful enough not to sleep for too long at a stretch, lest the
+	 * freezer whine and give up again!).
+	 *
+	 * Now that we no longer busy-loop, try_to_freeze() becomes all the
+	 * more important, due to a subtle reason: if we don't cooperate with
+	 * the freezer at this point, we could end up in a situation very
+	 * similar to mutex_lock() due to the usage of msleep() (which sleeps
+	 * uninterruptibly).
+	 *
+	 * Phew! What a delicate balance!
+	 */
+	while (!mutex_trylock(&pm_mutex)) {
+		try_to_freeze();
+		msleep(10);
+	}
 }
 
 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 v3] PM/Memory-hotplug: Avoid task freezing failures

[Tejun Heo]

Hello, Srivatsa.

On Thu, Nov 17, 2011 at 02:00:50PM +0530, Srivatsa S. Bhat wrote:
> @@ -380,7 +382,40 @@ static inline void unlock_system_sleep(void) {}
>  
>  static inline void lock_system_sleep(void)
>  {
> -	mutex_lock(&pm_mutex);
> +	/*
> +	 * "To sleep, or not to sleep, that is the question!"
> +	 *
> +	 * 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).
> +	 * Using mutex_lock_interruptible() in a loop is not a good idea,
> +	 * because we could end up treating non-freezing signals badly.
> +	 * So we use mutex_trylock() in a loop instead.
> +	 *
> +	 * Also, we add try_to_freeze() to the loop, to co-operate with the
> +	 * freezer, to avoid task freezing failures due to busy-looping.
> +	 *
> +	 * But then, since it is not guaranteed that we will get frozen
> +	 * rightaway, we could keep spinning for some time, breaking the
> +	 * expectation that we go to sleep when we fail to acquire the lock.
> +	 * So we add an msleep() to the loop, to dampen the spin (but we are
> +	 * careful enough not to sleep for too long at a stretch, lest the
> +	 * freezer whine and give up again!).
> +	 *
> +	 * Now that we no longer busy-loop, try_to_freeze() becomes all the
> +	 * more important, due to a subtle reason: if we don't cooperate with
> +	 * the freezer at this point, we could end up in a situation very
> +	 * similar to mutex_lock() due to the usage of msleep() (which sleeps
> +	 * uninterruptibly).
> +	 *
> +	 * Phew! What a delicate balance!
> +	 */
> +	while (!mutex_trylock(&pm_mutex)) {
> +		try_to_freeze();
> +		msleep(10);
> +	}

I tried to think about a better way to do it but couldn't, so I
suppose this is what we should go with for now.  That said, I think
the comment is a bit too umm.... verbose.  What we want here is
freezable but !interruptible mutex_lock() and while I do appreciate
the detailed comment, I think it makes it look a lot more complex than
it actually is.  Other than that,

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

Thank you very much.

-- 
tejun

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

[Tejun Heo]

Hello, Srivatsa.

On Thu, Nov 17, 2011 at 02:00:50PM +0530, Srivatsa S. Bhat wrote:
> @@ -380,7 +382,40 @@ static inline void unlock_system_sleep(void) {}
>  
>  static inline void lock_system_sleep(void)
>  {
> -	mutex_lock(&pm_mutex);
> +	/*
> +	 * "To sleep, or not to sleep, that is the question!"
> +	 *
> +	 * 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).
> +	 * Using mutex_lock_interruptible() in a loop is not a good idea,
> +	 * because we could end up treating non-freezing signals badly.
> +	 * So we use mutex_trylock() in a loop instead.
> +	 *
> +	 * Also, we add try_to_freeze() to the loop, to co-operate with the
> +	 * freezer, to avoid task freezing failures due to busy-looping.
> +	 *
> +	 * But then, since it is not guaranteed that we will get frozen
> +	 * rightaway, we could keep spinning for some time, breaking the
> +	 * expectation that we go to sleep when we fail to acquire the lock.
> +	 * So we add an msleep() to the loop, to dampen the spin (but we are
> +	 * careful enough not to sleep for too long at a stretch, lest the
> +	 * freezer whine and give up again!).
> +	 *
> +	 * Now that we no longer busy-loop, try_to_freeze() becomes all the
> +	 * more important, due to a subtle reason: if we don't cooperate with
> +	 * the freezer at this point, we could end up in a situation very
> +	 * similar to mutex_lock() due to the usage of msleep() (which sleeps
> +	 * uninterruptibly).
> +	 *
> +	 * Phew! What a delicate balance!
> +	 */
> +	while (!mutex_trylock(&pm_mutex)) {
> +		try_to_freeze();
> +		msleep(10);
> +	}

I tried to think about a better way to do it but couldn't, so I
suppose this is what we should go with for now.  That said, I think
the comment is a bit too umm.... verbose.  What we want here is
freezable but !interruptible mutex_lock() and while I do appreciate
the detailed comment, I think it makes it look a lot more complex than
it actually is.  Other than that,

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

Thank you very much.

-- 
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 v3] PM/Memory-hotplug: Avoid task freezing failures

[Srivatsa S. Bhat]

Hi Tejun,

On 11/20/2011 12:02 AM, Tejun Heo wrote:
> Hello, Srivatsa.
> 
> On Thu, Nov 17, 2011 at 02:00:50PM +0530, Srivatsa S. Bhat wrote:
>> @@ -380,7 +382,40 @@ static inline void unlock_system_sleep(void) {}
>>  
>>  static inline void lock_system_sleep(void)
>>  {
>> -	mutex_lock(&pm_mutex);
>> +	/*
>> +	 * "To sleep, or not to sleep, that is the question!"
>> +	 *
>> +	 * 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).
>> +	 * Using mutex_lock_interruptible() in a loop is not a good idea,
>> +	 * because we could end up treating non-freezing signals badly.
>> +	 * So we use mutex_trylock() in a loop instead.
>> +	 *
>> +	 * Also, we add try_to_freeze() to the loop, to co-operate with the
>> +	 * freezer, to avoid task freezing failures due to busy-looping.
>> +	 *
>> +	 * But then, since it is not guaranteed that we will get frozen
>> +	 * rightaway, we could keep spinning for some time, breaking the
>> +	 * expectation that we go to sleep when we fail to acquire the lock.
>> +	 * So we add an msleep() to the loop, to dampen the spin (but we are
>> +	 * careful enough not to sleep for too long at a stretch, lest the
>> +	 * freezer whine and give up again!).
>> +	 *
>> +	 * Now that we no longer busy-loop, try_to_freeze() becomes all the
>> +	 * more important, due to a subtle reason: if we don't cooperate with
>> +	 * the freezer at this point, we could end up in a situation very
>> +	 * similar to mutex_lock() due to the usage of msleep() (which sleeps
>> +	 * uninterruptibly).
>> +	 *
>> +	 * Phew! What a delicate balance!
>> +	 */
>> +	while (!mutex_trylock(&pm_mutex)) {
>> +		try_to_freeze();
>> +		msleep(10);
>> +	}
> 
> I tried to think about a better way to do it but couldn't, so I
> suppose this is what we should go with for now.  That said, I think
> the comment is a bit too umm.... verbose.  

Hehe, I grabbed the opportunity to add some custom-modified Shakespearean
quotes and stuff to make the long story interesting ;-)

> What we want here is
> freezable but !interruptible mutex_lock() and while I do appreciate
> the detailed comment, I think it makes it look a lot more complex than
> it actually is.  

Yes, the overall requirement we have here could be explained in those simple
words, as you said, but that is only because we already know the background
and the problems involved (due to the discussions on this thread and our
knowledge of how the freezer works).

But for someone reading it from purely a memory-hotplug point of view, IMHO,
all the constraints we are handling here might not be that intuitive..
So I felt I needed to document the workaround very well (including the
thought process used to develop it), so that nobody messes with it without
knowing what each line of that workaround does, and why it does it, since
it is already quite delicate and hacky.

So, I would rather prefer retaining that comment verbosity until we have
a real, better fix...

At the same time, I wouldn't want to make it feel so scary that nobody
even dares to contribute a real fix! ;-) So, considering all this, could
you let me know if you feel it is better to cut it short and make it look
simpler or just retain it as it is?

> Other than that,
> 
>  Acked-by: Tejun Heo <tj@kernel.org>
> 

Thanks a lot for your review and Ack!

Thanks,
Srivatsa S. Bhat

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

[Srivatsa S. Bhat]

Hi Tejun,

On 11/20/2011 12:02 AM, Tejun Heo wrote:
> Hello, Srivatsa.
> 
> On Thu, Nov 17, 2011 at 02:00:50PM +0530, Srivatsa S. Bhat wrote:
>> @@ -380,7 +382,40 @@ static inline void unlock_system_sleep(void) {}
>>  
>>  static inline void lock_system_sleep(void)
>>  {
>> -	mutex_lock(&pm_mutex);
>> +	/*
>> +	 * "To sleep, or not to sleep, that is the question!"
>> +	 *
>> +	 * 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).
>> +	 * Using mutex_lock_interruptible() in a loop is not a good idea,
>> +	 * because we could end up treating non-freezing signals badly.
>> +	 * So we use mutex_trylock() in a loop instead.
>> +	 *
>> +	 * Also, we add try_to_freeze() to the loop, to co-operate with the
>> +	 * freezer, to avoid task freezing failures due to busy-looping.
>> +	 *
>> +	 * But then, since it is not guaranteed that we will get frozen
>> +	 * rightaway, we could keep spinning for some time, breaking the
>> +	 * expectation that we go to sleep when we fail to acquire the lock.
>> +	 * So we add an msleep() to the loop, to dampen the spin (but we are
>> +	 * careful enough not to sleep for too long at a stretch, lest the
>> +	 * freezer whine and give up again!).
>> +	 *
>> +	 * Now that we no longer busy-loop, try_to_freeze() becomes all the
>> +	 * more important, due to a subtle reason: if we don't cooperate with
>> +	 * the freezer at this point, we could end up in a situation very
>> +	 * similar to mutex_lock() due to the usage of msleep() (which sleeps
>> +	 * uninterruptibly).
>> +	 *
>> +	 * Phew! What a delicate balance!
>> +	 */
>> +	while (!mutex_trylock(&pm_mutex)) {
>> +		try_to_freeze();
>> +		msleep(10);
>> +	}
> 
> I tried to think about a better way to do it but couldn't, so I
> suppose this is what we should go with for now.  That said, I think
> the comment is a bit too umm.... verbose.  

Hehe, I grabbed the opportunity to add some custom-modified Shakespearean
quotes and stuff to make the long story interesting ;-)

> What we want here is
> freezable but !interruptible mutex_lock() and while I do appreciate
> the detailed comment, I think it makes it look a lot more complex than
> it actually is.  

Yes, the overall requirement we have here could be explained in those simple
words, as you said, but that is only because we already know the background
and the problems involved (due to the discussions on this thread and our
knowledge of how the freezer works).

But for someone reading it from purely a memory-hotplug point of view, IMHO,
all the constraints we are handling here might not be that intuitive..
So I felt I needed to document the workaround very well (including the
thought process used to develop it), so that nobody messes with it without
knowing what each line of that workaround does, and why it does it, since
it is already quite delicate and hacky.

So, I would rather prefer retaining that comment verbosity until we have
a real, better fix...

At the same time, I wouldn't want to make it feel so scary that nobody
even dares to contribute a real fix! ;-) So, considering all this, could
you let me know if you feel it is better to cut it short and make it look
simpler or just retain it as it is?

> Other than that,
> 
>  Acked-by: Tejun Heo <tj@kernel.org>
> 

Thanks a lot for your review and Ack!

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 v3] PM/Memory-hotplug: Avoid task freezing failures

[Rafael J. Wysocki]

On Thursday, November 17, 2011, Srivatsa S. Bhat wrote:
> 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, we use msleep() to avoid busy looping and breaking expectations
> that we go to sleep when we fail to acquire the lock.
> And we also call try_to_freeze() in order to cooperate with the freezer,
> without which we would end up in almost the same situation as mutex_lock(),
> due to uninterruptible sleep caused by msleep().
> 
> v3: Tejun suggested avoiding busy-looping by adding an msleep() since
>     it is not guaranteed that we will get frozen immediately.
> 
> 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 |   37 ++++++++++++++++++++++++++++++++++++-
>  1 files changed, 36 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 57a6924..0af3048 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -5,6 +5,8 @@
>  #include <linux/notifier.h>
>  #include <linux/init.h>
>  #include <linux/pm.h>
> +#include <linux/freezer.h>
> +#include <linux/delay.h>
>  #include <linux/mm.h>
>  #include <asm/errno.h>
>  
> @@ -380,7 +382,40 @@ static inline void unlock_system_sleep(void) {}
>  
>  static inline void lock_system_sleep(void)
>  {
> -	mutex_lock(&pm_mutex);
> +	/*
> +	 * "To sleep, or not to sleep, that is the question!"
> +	 *
> +	 * 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).
> +	 * Using mutex_lock_interruptible() in a loop is not a good idea,
> +	 * because we could end up treating non-freezing signals badly.
> +	 * So we use mutex_trylock() in a loop instead.
> +	 *
> +	 * Also, we add try_to_freeze() to the loop, to co-operate with the
> +	 * freezer, to avoid task freezing failures due to busy-looping.
> +	 *
> +	 * But then, since it is not guaranteed that we will get frozen
> +	 * rightaway, we could keep spinning for some time, breaking the
> +	 * expectation that we go to sleep when we fail to acquire the lock.
> +	 * So we add an msleep() to the loop, to dampen the spin (but we are
> +	 * careful enough not to sleep for too long at a stretch, lest the
> +	 * freezer whine and give up again!).
> +	 *
> +	 * Now that we no longer busy-loop, try_to_freeze() becomes all the
> +	 * more important, due to a subtle reason: if we don't cooperate with
> +	 * the freezer at this point, we could end up in a situation very
> +	 * similar to mutex_lock() due to the usage of msleep() (which sleeps
> +	 * uninterruptibly).
> +	 *
> +	 * Phew! What a delicate balance!
> +	 */
> +	while (!mutex_trylock(&pm_mutex)) {
> +		try_to_freeze();
> +		msleep(10);

The number here seems to be somewhat arbitrary.  Is there any reason not to
use 100 or any other number?

> +	}
>  }
>  
>  static inline void unlock_system_sleep(void)

Rafael

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

[Rafael J. Wysocki]

On Thursday, November 17, 2011, Srivatsa S. Bhat wrote:
> 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, we use msleep() to avoid busy looping and breaking expectations
> that we go to sleep when we fail to acquire the lock.
> And we also call try_to_freeze() in order to cooperate with the freezer,
> without which we would end up in almost the same situation as mutex_lock(),
> due to uninterruptible sleep caused by msleep().
> 
> v3: Tejun suggested avoiding busy-looping by adding an msleep() since
>     it is not guaranteed that we will get frozen immediately.
> 
> 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 |   37 ++++++++++++++++++++++++++++++++++++-
>  1 files changed, 36 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 57a6924..0af3048 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -5,6 +5,8 @@
>  #include <linux/notifier.h>
>  #include <linux/init.h>
>  #include <linux/pm.h>
> +#include <linux/freezer.h>
> +#include <linux/delay.h>
>  #include <linux/mm.h>
>  #include <asm/errno.h>
>  
> @@ -380,7 +382,40 @@ static inline void unlock_system_sleep(void) {}
>  
>  static inline void lock_system_sleep(void)
>  {
> -	mutex_lock(&pm_mutex);
> +	/*
> +	 * "To sleep, or not to sleep, that is the question!"
> +	 *
> +	 * 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).
> +	 * Using mutex_lock_interruptible() in a loop is not a good idea,
> +	 * because we could end up treating non-freezing signals badly.
> +	 * So we use mutex_trylock() in a loop instead.
> +	 *
> +	 * Also, we add try_to_freeze() to the loop, to co-operate with the
> +	 * freezer, to avoid task freezing failures due to busy-looping.
> +	 *
> +	 * But then, since it is not guaranteed that we will get frozen
> +	 * rightaway, we could keep spinning for some time, breaking the
> +	 * expectation that we go to sleep when we fail to acquire the lock.
> +	 * So we add an msleep() to the loop, to dampen the spin (but we are
> +	 * careful enough not to sleep for too long at a stretch, lest the
> +	 * freezer whine and give up again!).
> +	 *
> +	 * Now that we no longer busy-loop, try_to_freeze() becomes all the
> +	 * more important, due to a subtle reason: if we don't cooperate with
> +	 * the freezer at this point, we could end up in a situation very
> +	 * similar to mutex_lock() due to the usage of msleep() (which sleeps
> +	 * uninterruptibly).
> +	 *
> +	 * Phew! What a delicate balance!
> +	 */
> +	while (!mutex_trylock(&pm_mutex)) {
> +		try_to_freeze();
> +		msleep(10);

The number here seems to be somewhat arbitrary.  Is there any reason not to
use 100 or any other number?

> +	}
>  }
>  
>  static inline void unlock_system_sleep(void)

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>

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

[Srivatsa S. Bhat]

On 11/20/2011 03:27 AM, Rafael J. Wysocki wrote:
> On Thursday, November 17, 2011, Srivatsa S. Bhat wrote:
>> 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, we use msleep() to avoid busy looping and breaking expectations
>> that we go to sleep when we fail to acquire the lock.
>> And we also call try_to_freeze() in order to cooperate with the freezer,
>> without which we would end up in almost the same situation as mutex_lock(),
>> due to uninterruptible sleep caused by msleep().
>>
>> v3: Tejun suggested avoiding busy-looping by adding an msleep() since
>>     it is not guaranteed that we will get frozen immediately.
>>
>> 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 |   37 ++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 36 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>> index 57a6924..0af3048 100644
>> --- a/include/linux/suspend.h
>> +++ b/include/linux/suspend.h
>> @@ -5,6 +5,8 @@
>>  #include <linux/notifier.h>
>>  #include <linux/init.h>
>>  #include <linux/pm.h>
>> +#include <linux/freezer.h>
>> +#include <linux/delay.h>
>>  #include <linux/mm.h>
>>  #include <asm/errno.h>
>>  
>> @@ -380,7 +382,40 @@ static inline void unlock_system_sleep(void) {}
>>  
>>  static inline void lock_system_sleep(void)
>>  {
>> -	mutex_lock(&pm_mutex);
>> +	/*
>> +	 * "To sleep, or not to sleep, that is the question!"
>> +	 *
>> +	 * 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).
>> +	 * Using mutex_lock_interruptible() in a loop is not a good idea,
>> +	 * because we could end up treating non-freezing signals badly.
>> +	 * So we use mutex_trylock() in a loop instead.
>> +	 *
>> +	 * Also, we add try_to_freeze() to the loop, to co-operate with the
>> +	 * freezer, to avoid task freezing failures due to busy-looping.
>> +	 *
>> +	 * But then, since it is not guaranteed that we will get frozen
>> +	 * rightaway, we could keep spinning for some time, breaking the
>> +	 * expectation that we go to sleep when we fail to acquire the lock.
>> +	 * So we add an msleep() to the loop, to dampen the spin (but we are
>> +	 * careful enough not to sleep for too long at a stretch, lest the
>> +	 * freezer whine and give up again!).
>> +	 *
>> +	 * Now that we no longer busy-loop, try_to_freeze() becomes all the
>> +	 * more important, due to a subtle reason: if we don't cooperate with
>> +	 * the freezer at this point, we could end up in a situation very
>> +	 * similar to mutex_lock() due to the usage of msleep() (which sleeps
>> +	 * uninterruptibly).
>> +	 *
>> +	 * Phew! What a delicate balance!
>> +	 */
>> +	while (!mutex_trylock(&pm_mutex)) {
>> +		try_to_freeze();
>> +		msleep(10);
> 
> The number here seems to be somewhat arbitrary.  Is there any reason not to
> use 100 or any other number?
> 

Short answer:

The number is not arbitrary. It is designed to match the frequency at which
the freezer re-tries to freeze tasks in a loop for 20 seconds (after which
it gives up).

Long answer:

Let us define 'time-to-freeze-this-task' as the duration of time between the
setting of TIF_FREEZE flag for this task (after the task enters the while
loop in this patch) and the time at which this task is considered frozen
by the freezer.

There are 2 constraints we are trying to handle here:

[And let us see extreme case solutions for these constraints, to start with]

1. We want task freezing to be fast, to make hibernation fast.
Hence, we don't want to use msleep() here at all, just the
try_to_freeze() within the while loop would fit well.

2. As Tejun suggested, considering that we might not get frozen immediately,
we don't want to hurt cpu power management during that time. So, we
want to sleep when possible. Which means we can sleep for ~20 seconds at a
stretch and still manage to prevent freezing failures.

But obviously we need to strike a balance between these 2 contradictions.
Hence, we observe that the freezer goes in a loop and tries to freeze
tasks, and waits for 10ms before retrying (and continues this procedure
for 20 seconds).

Since we want time-to-freeze-this-task as small as possible, we have to
minimize the number of iterations the freezer does waiting for us.
Hence we choose to sleep for 10ms, which means, in the worst case,
our time-to-freeze-task will be one iteration of the freezer, IOW 10ms.
[That way, actually sleeping for 9ms would do best, but we probably don't
want to get that specific here, or should we?]

I think I have given a slight hint about this issue in the comment as well...

I prefer not to #define 10 and use it in freezer's loop and in this above
msleep() because, good design means, "Keep the freezer internals internal
to the freezer!". But all of us agree that this patch is only a temporary
hack (which unfortunately needs to know about freezer's internal working)..
and also that, we need to fix this whole issue at a design level sooner
or later.
So having 10ms msleep(), as well as hard-coding this value here, are both
justified, IMHO.

As for the comment, I don't know if I should be expanding that "slight hint"
into a full-fledged explanation, since Tejun is already complaining about
its verbosity ;-)

By the way, for somebody who is looking from a purely memory-hotplug point
of view and is not that familiar with the freezer, the "slight hint" in
the comment "careful enough not to sleep for too long at a stretch...
freezing failure..." is supposed to be interpreted as : "Oh when does
freezing fail? Let me look up the freezer code.. ah, 20 seconds.
By the way, I spot a 10ms sleep in the freezer loop as well..
Oh yes, *now* it all makes sense!" :-)

Or perhaps, adding the same justification I gave above (about the 10ms
sleep) to the changelog should do, right?

>> +	}
>>  }
>>  
>>  static inline void unlock_system_sleep(void)
> 

Thanks,
Srivatsa S. Bhat

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

[Srivatsa S. Bhat]

On 11/20/2011 03:27 AM, Rafael J. Wysocki wrote:
> On Thursday, November 17, 2011, Srivatsa S. Bhat wrote:
>> 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, we use msleep() to avoid busy looping and breaking expectations
>> that we go to sleep when we fail to acquire the lock.
>> And we also call try_to_freeze() in order to cooperate with the freezer,
>> without which we would end up in almost the same situation as mutex_lock(),
>> due to uninterruptible sleep caused by msleep().
>>
>> v3: Tejun suggested avoiding busy-looping by adding an msleep() since
>>     it is not guaranteed that we will get frozen immediately.
>>
>> 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 |   37 ++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 36 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>> index 57a6924..0af3048 100644
>> --- a/include/linux/suspend.h
>> +++ b/include/linux/suspend.h
>> @@ -5,6 +5,8 @@
>>  #include <linux/notifier.h>
>>  #include <linux/init.h>
>>  #include <linux/pm.h>
>> +#include <linux/freezer.h>
>> +#include <linux/delay.h>
>>  #include <linux/mm.h>
>>  #include <asm/errno.h>
>>  
>> @@ -380,7 +382,40 @@ static inline void unlock_system_sleep(void) {}
>>  
>>  static inline void lock_system_sleep(void)
>>  {
>> -	mutex_lock(&pm_mutex);
>> +	/*
>> +	 * "To sleep, or not to sleep, that is the question!"
>> +	 *
>> +	 * 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).
>> +	 * Using mutex_lock_interruptible() in a loop is not a good idea,
>> +	 * because we could end up treating non-freezing signals badly.
>> +	 * So we use mutex_trylock() in a loop instead.
>> +	 *
>> +	 * Also, we add try_to_freeze() to the loop, to co-operate with the
>> +	 * freezer, to avoid task freezing failures due to busy-looping.
>> +	 *
>> +	 * But then, since it is not guaranteed that we will get frozen
>> +	 * rightaway, we could keep spinning for some time, breaking the
>> +	 * expectation that we go to sleep when we fail to acquire the lock.
>> +	 * So we add an msleep() to the loop, to dampen the spin (but we are
>> +	 * careful enough not to sleep for too long at a stretch, lest the
>> +	 * freezer whine and give up again!).
>> +	 *
>> +	 * Now that we no longer busy-loop, try_to_freeze() becomes all the
>> +	 * more important, due to a subtle reason: if we don't cooperate with
>> +	 * the freezer at this point, we could end up in a situation very
>> +	 * similar to mutex_lock() due to the usage of msleep() (which sleeps
>> +	 * uninterruptibly).
>> +	 *
>> +	 * Phew! What a delicate balance!
>> +	 */
>> +	while (!mutex_trylock(&pm_mutex)) {
>> +		try_to_freeze();
>> +		msleep(10);
> 
> The number here seems to be somewhat arbitrary.  Is there any reason not to
> use 100 or any other number?
> 

Short answer:

The number is not arbitrary. It is designed to match the frequency at which
the freezer re-tries to freeze tasks in a loop for 20 seconds (after which
it gives up).

Long answer:

Let us define 'time-to-freeze-this-task' as the duration of time between the
setting of TIF_FREEZE flag for this task (after the task enters the while
loop in this patch) and the time at which this task is considered frozen
by the freezer.

There are 2 constraints we are trying to handle here:

[And let us see extreme case solutions for these constraints, to start with]

1. We want task freezing to be fast, to make hibernation fast.
Hence, we don't want to use msleep() here at all, just the
try_to_freeze() within the while loop would fit well.

2. As Tejun suggested, considering that we might not get frozen immediately,
we don't want to hurt cpu power management during that time. So, we
want to sleep when possible. Which means we can sleep for ~20 seconds at a
stretch and still manage to prevent freezing failures.

But obviously we need to strike a balance between these 2 contradictions.
Hence, we observe that the freezer goes in a loop and tries to freeze
tasks, and waits for 10ms before retrying (and continues this procedure
for 20 seconds).

Since we want time-to-freeze-this-task as small as possible, we have to
minimize the number of iterations the freezer does waiting for us.
Hence we choose to sleep for 10ms, which means, in the worst case,
our time-to-freeze-task will be one iteration of the freezer, IOW 10ms.
[That way, actually sleeping for 9ms would do best, but we probably don't
want to get that specific here, or should we?]

I think I have given a slight hint about this issue in the comment as well...

I prefer not to #define 10 and use it in freezer's loop and in this above
msleep() because, good design means, "Keep the freezer internals internal
to the freezer!". But all of us agree that this patch is only a temporary
hack (which unfortunately needs to know about freezer's internal working)..
and also that, we need to fix this whole issue at a design level sooner
or later.
So having 10ms msleep(), as well as hard-coding this value here, are both
justified, IMHO.

As for the comment, I don't know if I should be expanding that "slight hint"
into a full-fledged explanation, since Tejun is already complaining about
its verbosity ;-)

By the way, for somebody who is looking from a purely memory-hotplug point
of view and is not that familiar with the freezer, the "slight hint" in
the comment "careful enough not to sleep for too long at a stretch...
freezing failure..." is supposed to be interpreted as : "Oh when does
freezing fail? Let me look up the freezer code.. ah, 20 seconds.
By the way, I spot a 10ms sleep in the freezer loop as well..
Oh yes, *now* it all makes sense!" :-)

Or perhaps, adding the same justification I gave above (about the 10ms
sleep) to the changelog should do, right?

>> +	}
>>  }
>>  
>>  static inline void unlock_system_sleep(void)
> 

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 v3] PM/Memory-hotplug: Avoid task freezing failures

[Rafael J. Wysocki]

On Sunday, November 20, 2011, Srivatsa S. Bhat wrote:
> On 11/20/2011 03:27 AM, Rafael J. Wysocki wrote:
> > On Thursday, November 17, 2011, Srivatsa S. Bhat wrote:
> >> 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, we use msleep() to avoid busy looping and breaking expectations
> >> that we go to sleep when we fail to acquire the lock.
> >> And we also call try_to_freeze() in order to cooperate with the freezer,
> >> without which we would end up in almost the same situation as mutex_lock(),
> >> due to uninterruptible sleep caused by msleep().
> >>
> >> v3: Tejun suggested avoiding busy-looping by adding an msleep() since
> >>     it is not guaranteed that we will get frozen immediately.
> >>
> >> 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 |   37 ++++++++++++++++++++++++++++++++++++-
> >>  1 files changed, 36 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> >> index 57a6924..0af3048 100644
> >> --- a/include/linux/suspend.h
> >> +++ b/include/linux/suspend.h
> >> @@ -5,6 +5,8 @@
> >>  #include <linux/notifier.h>
> >>  #include <linux/init.h>
> >>  #include <linux/pm.h>
> >> +#include <linux/freezer.h>
> >> +#include <linux/delay.h>
> >>  #include <linux/mm.h>
> >>  #include <asm/errno.h>
> >>  
> >> @@ -380,7 +382,40 @@ static inline void unlock_system_sleep(void) {}
> >>  
> >>  static inline void lock_system_sleep(void)
> >>  {
> >> -	mutex_lock(&pm_mutex);
> >> +	/*
> >> +	 * "To sleep, or not to sleep, that is the question!"
> >> +	 *
> >> +	 * 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).
> >> +	 * Using mutex_lock_interruptible() in a loop is not a good idea,
> >> +	 * because we could end up treating non-freezing signals badly.
> >> +	 * So we use mutex_trylock() in a loop instead.
> >> +	 *
> >> +	 * Also, we add try_to_freeze() to the loop, to co-operate with the
> >> +	 * freezer, to avoid task freezing failures due to busy-looping.
> >> +	 *
> >> +	 * But then, since it is not guaranteed that we will get frozen
> >> +	 * rightaway, we could keep spinning for some time, breaking the
> >> +	 * expectation that we go to sleep when we fail to acquire the lock.
> >> +	 * So we add an msleep() to the loop, to dampen the spin (but we are
> >> +	 * careful enough not to sleep for too long at a stretch, lest the
> >> +	 * freezer whine and give up again!).
> >> +	 *
> >> +	 * Now that we no longer busy-loop, try_to_freeze() becomes all the
> >> +	 * more important, due to a subtle reason: if we don't cooperate with
> >> +	 * the freezer at this point, we could end up in a situation very
> >> +	 * similar to mutex_lock() due to the usage of msleep() (which sleeps
> >> +	 * uninterruptibly).
> >> +	 *
> >> +	 * Phew! What a delicate balance!
> >> +	 */
> >> +	while (!mutex_trylock(&pm_mutex)) {
> >> +		try_to_freeze();
> >> +		msleep(10);
> > 
> > The number here seems to be somewhat arbitrary.  Is there any reason not to
> > use 100 or any other number?
> > 
> 
> Short answer:
> 
> The number is not arbitrary. It is designed to match the frequency at which
> the freezer re-tries to freeze tasks in a loop for 20 seconds (after which
> it gives up).

So that should be documented too, right?  Perhaps there should be a #define
for that number.

> Long answer:
> 
> Let us define 'time-to-freeze-this-task' as the duration of time between the
> setting of TIF_FREEZE flag for this task (after the task enters the while
> loop in this patch) and the time at which this task is considered frozen
> by the freezer.
> 
> There are 2 constraints we are trying to handle here:
> 
> [And let us see extreme case solutions for these constraints, to start with]
> 
> 1. We want task freezing to be fast, to make hibernation fast.
> Hence, we don't want to use msleep() here at all, just the
> try_to_freeze() within the while loop would fit well.
> 
> 2. As Tejun suggested, considering that we might not get frozen immediately,
> we don't want to hurt cpu power management during that time. So, we
> want to sleep when possible. Which means we can sleep for ~20 seconds at a
> stretch and still manage to prevent freezing failures.
> 
> But obviously we need to strike a balance between these 2 contradictions.
> Hence, we observe that the freezer goes in a loop and tries to freeze
> tasks, and waits for 10ms before retrying (and continues this procedure
> for 20 seconds).
> 
> Since we want time-to-freeze-this-task as small as possible, we have to
> minimize the number of iterations the freezer does waiting for us.
> Hence we choose to sleep for 10ms, which means, in the worst case,
> our time-to-freeze-task will be one iteration of the freezer, IOW 10ms.
> [That way, actually sleeping for 9ms would do best, but we probably don't
> want to get that specific here, or should we?]
> 
> I think I have given a slight hint about this issue in the comment as well...
> 
> I prefer not to #define 10 and use it in freezer's loop and in this above
> msleep() because, good design means, "Keep the freezer internals internal
> to the freezer!". But all of us agree that this patch is only a temporary
> hack (which unfortunately needs to know about freezer's internal working)..
> and also that, we need to fix this whole issue at a design level sooner
> or later.
> So having 10ms msleep(), as well as hard-coding this value here, are both
> justified, IMHO.
> 
> As for the comment, I don't know if I should be expanding that "slight hint"
> into a full-fledged explanation, since Tejun is already complaining about
> its verbosity ;-)
> 
> By the way, for somebody who is looking from a purely memory-hotplug point
> of view and is not that familiar with the freezer, the "slight hint" in
> the comment "careful enough not to sleep for too long at a stretch...
> freezing failure..." is supposed to be interpreted as : "Oh when does
> freezing fail? Let me look up the freezer code.. ah, 20 seconds.
> By the way, I spot a 10ms sleep in the freezer loop as well..
> Oh yes, *now* it all makes sense!" :-)
> 
> Or perhaps, adding the same justification I gave above (about the 10ms
> sleep) to the changelog should do, right?
> 
> >> +	}
> >>  }
> >>  
> >>  static inline void unlock_system_sleep(void)
> > 

Well, let's not add temporary hacks, especially as fragile as this one.

Why don't we simply open code what we need using a proper wait queue
and waiting for an event?

So, have something like transition_in_progress (I believe there already is
something like that somewhere), set it to 'true' under pm_mutex in
lock_system_sleep(), possibly waiting for it to become 'false' properly
(using interruptible sleep with try_to_freeze()) and reset it to 'false' (under pm_mutex) in unlock_system_sleep()?

Rafael

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

[Rafael J. Wysocki]

On Sunday, November 20, 2011, Srivatsa S. Bhat wrote:
> On 11/20/2011 03:27 AM, Rafael J. Wysocki wrote:
> > On Thursday, November 17, 2011, Srivatsa S. Bhat wrote:
> >> 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, we use msleep() to avoid busy looping and breaking expectations
> >> that we go to sleep when we fail to acquire the lock.
> >> And we also call try_to_freeze() in order to cooperate with the freezer,
> >> without which we would end up in almost the same situation as mutex_lock(),
> >> due to uninterruptible sleep caused by msleep().
> >>
> >> v3: Tejun suggested avoiding busy-looping by adding an msleep() since
> >>     it is not guaranteed that we will get frozen immediately.
> >>
> >> 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 |   37 ++++++++++++++++++++++++++++++++++++-
> >>  1 files changed, 36 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> >> index 57a6924..0af3048 100644
> >> --- a/include/linux/suspend.h
> >> +++ b/include/linux/suspend.h
> >> @@ -5,6 +5,8 @@
> >>  #include <linux/notifier.h>
> >>  #include <linux/init.h>
> >>  #include <linux/pm.h>
> >> +#include <linux/freezer.h>
> >> +#include <linux/delay.h>
> >>  #include <linux/mm.h>
> >>  #include <asm/errno.h>
> >>  
> >> @@ -380,7 +382,40 @@ static inline void unlock_system_sleep(void) {}
> >>  
> >>  static inline void lock_system_sleep(void)
> >>  {
> >> -	mutex_lock(&pm_mutex);
> >> +	/*
> >> +	 * "To sleep, or not to sleep, that is the question!"
> >> +	 *
> >> +	 * 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).
> >> +	 * Using mutex_lock_interruptible() in a loop is not a good idea,
> >> +	 * because we could end up treating non-freezing signals badly.
> >> +	 * So we use mutex_trylock() in a loop instead.
> >> +	 *
> >> +	 * Also, we add try_to_freeze() to the loop, to co-operate with the
> >> +	 * freezer, to avoid task freezing failures due to busy-looping.
> >> +	 *
> >> +	 * But then, since it is not guaranteed that we will get frozen
> >> +	 * rightaway, we could keep spinning for some time, breaking the
> >> +	 * expectation that we go to sleep when we fail to acquire the lock.
> >> +	 * So we add an msleep() to the loop, to dampen the spin (but we are
> >> +	 * careful enough not to sleep for too long at a stretch, lest the
> >> +	 * freezer whine and give up again!).
> >> +	 *
> >> +	 * Now that we no longer busy-loop, try_to_freeze() becomes all the
> >> +	 * more important, due to a subtle reason: if we don't cooperate with
> >> +	 * the freezer at this point, we could end up in a situation very
> >> +	 * similar to mutex_lock() due to the usage of msleep() (which sleeps
> >> +	 * uninterruptibly).
> >> +	 *
> >> +	 * Phew! What a delicate balance!
> >> +	 */
> >> +	while (!mutex_trylock(&pm_mutex)) {
> >> +		try_to_freeze();
> >> +		msleep(10);
> > 
> > The number here seems to be somewhat arbitrary.  Is there any reason not to
> > use 100 or any other number?
> > 
> 
> Short answer:
> 
> The number is not arbitrary. It is designed to match the frequency at which
> the freezer re-tries to freeze tasks in a loop for 20 seconds (after which
> it gives up).

So that should be documented too, right?  Perhaps there should be a #define
for that number.

> Long answer:
> 
> Let us define 'time-to-freeze-this-task' as the duration of time between the
> setting of TIF_FREEZE flag for this task (after the task enters the while
> loop in this patch) and the time at which this task is considered frozen
> by the freezer.
> 
> There are 2 constraints we are trying to handle here:
> 
> [And let us see extreme case solutions for these constraints, to start with]
> 
> 1. We want task freezing to be fast, to make hibernation fast.
> Hence, we don't want to use msleep() here at all, just the
> try_to_freeze() within the while loop would fit well.
> 
> 2. As Tejun suggested, considering that we might not get frozen immediately,
> we don't want to hurt cpu power management during that time. So, we
> want to sleep when possible. Which means we can sleep for ~20 seconds at a
> stretch and still manage to prevent freezing failures.
> 
> But obviously we need to strike a balance between these 2 contradictions.
> Hence, we observe that the freezer goes in a loop and tries to freeze
> tasks, and waits for 10ms before retrying (and continues this procedure
> for 20 seconds).
> 
> Since we want time-to-freeze-this-task as small as possible, we have to
> minimize the number of iterations the freezer does waiting for us.
> Hence we choose to sleep for 10ms, which means, in the worst case,
> our time-to-freeze-task will be one iteration of the freezer, IOW 10ms.
> [That way, actually sleeping for 9ms would do best, but we probably don't
> want to get that specific here, or should we?]
> 
> I think I have given a slight hint about this issue in the comment as well...
> 
> I prefer not to #define 10 and use it in freezer's loop and in this above
> msleep() because, good design means, "Keep the freezer internals internal
> to the freezer!". But all of us agree that this patch is only a temporary
> hack (which unfortunately needs to know about freezer's internal working)..
> and also that, we need to fix this whole issue at a design level sooner
> or later.
> So having 10ms msleep(), as well as hard-coding this value here, are both
> justified, IMHO.
> 
> As for the comment, I don't know if I should be expanding that "slight hint"
> into a full-fledged explanation, since Tejun is already complaining about
> its verbosity ;-)
> 
> By the way, for somebody who is looking from a purely memory-hotplug point
> of view and is not that familiar with the freezer, the "slight hint" in
> the comment "careful enough not to sleep for too long at a stretch...
> freezing failure..." is supposed to be interpreted as : "Oh when does
> freezing fail? Let me look up the freezer code.. ah, 20 seconds.
> By the way, I spot a 10ms sleep in the freezer loop as well..
> Oh yes, *now* it all makes sense!" :-)
> 
> Or perhaps, adding the same justification I gave above (about the 10ms
> sleep) to the changelog should do, right?
> 
> >> +	}
> >>  }
> >>  
> >>  static inline void unlock_system_sleep(void)
> > 

Well, let's not add temporary hacks, especially as fragile as this one.

Why don't we simply open code what we need using a proper wait queue
and waiting for an event?

So, have something like transition_in_progress (I believe there already is
something like that somewhere), set it to 'true' under pm_mutex in
lock_system_sleep(), possibly waiting for it to become 'false' properly
(using interruptible sleep with try_to_freeze()) and reset it to 'false' (under pm_mutex) in unlock_system_sleep()?

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>

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

[Srivatsa S. Bhat]

On 11/20/2011 03:54 PM, Rafael J. Wysocki wrote:
> On Sunday, November 20, 2011, Srivatsa S. Bhat wrote:
>> On 11/20/2011 03:27 AM, Rafael J. Wysocki wrote:
>>> On Thursday, November 17, 2011, Srivatsa S. Bhat wrote:
>>>> 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, we use msleep() to avoid busy looping and breaking expectations
>>>> that we go to sleep when we fail to acquire the lock.
>>>> And we also call try_to_freeze() in order to cooperate with the freezer,
>>>> without which we would end up in almost the same situation as mutex_lock(),
>>>> due to uninterruptible sleep caused by msleep().
>>>>
>>>> v3: Tejun suggested avoiding busy-looping by adding an msleep() since
>>>>     it is not guaranteed that we will get frozen immediately.
>>>>
>>>> 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 |   37 ++++++++++++++++++++++++++++++++++++-
>>>>  1 files changed, 36 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>>>> index 57a6924..0af3048 100644
>>>> --- a/include/linux/suspend.h
>>>> +++ b/include/linux/suspend.h
>>>> @@ -5,6 +5,8 @@
>>>>  #include <linux/notifier.h>
>>>>  #include <linux/init.h>
>>>>  #include <linux/pm.h>
>>>> +#include <linux/freezer.h>
>>>> +#include <linux/delay.h>
>>>>  #include <linux/mm.h>
>>>>  #include <asm/errno.h>
>>>>  
>>>> @@ -380,7 +382,40 @@ static inline void unlock_system_sleep(void) {}
>>>>  
>>>>  static inline void lock_system_sleep(void)
>>>>  {
>>>> -	mutex_lock(&pm_mutex);
>>>> +	/*
>>>> +	 * "To sleep, or not to sleep, that is the question!"
>>>> +	 *
>>>> +	 * 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).
>>>> +	 * Using mutex_lock_interruptible() in a loop is not a good idea,
>>>> +	 * because we could end up treating non-freezing signals badly.
>>>> +	 * So we use mutex_trylock() in a loop instead.
>>>> +	 *
>>>> +	 * Also, we add try_to_freeze() to the loop, to co-operate with the
>>>> +	 * freezer, to avoid task freezing failures due to busy-looping.
>>>> +	 *
>>>> +	 * But then, since it is not guaranteed that we will get frozen
>>>> +	 * rightaway, we could keep spinning for some time, breaking the
>>>> +	 * expectation that we go to sleep when we fail to acquire the lock.
>>>> +	 * So we add an msleep() to the loop, to dampen the spin (but we are
>>>> +	 * careful enough not to sleep for too long at a stretch, lest the
>>>> +	 * freezer whine and give up again!).
>>>> +	 *
>>>> +	 * Now that we no longer busy-loop, try_to_freeze() becomes all the
>>>> +	 * more important, due to a subtle reason: if we don't cooperate with
>>>> +	 * the freezer at this point, we could end up in a situation very
>>>> +	 * similar to mutex_lock() due to the usage of msleep() (which sleeps
>>>> +	 * uninterruptibly).
>>>> +	 *
>>>> +	 * Phew! What a delicate balance!
>>>> +	 */
>>>> +	while (!mutex_trylock(&pm_mutex)) {
>>>> +		try_to_freeze();
>>>> +		msleep(10);
>>>
>>> The number here seems to be somewhat arbitrary.  Is there any reason not to
>>> use 100 or any other number?
>>>
>>
>> Short answer:
>>
>> The number is not arbitrary. It is designed to match the frequency at which
>> the freezer re-tries to freeze tasks in a loop for 20 seconds (after which
>> it gives up).
> 
> So that should be documented too, right?  Perhaps there should be a #define
> for that number.
> 
>> Long answer:
>>
>> Let us define 'time-to-freeze-this-task' as the duration of time between the
>> setting of TIF_FREEZE flag for this task (after the task enters the while
>> loop in this patch) and the time at which this task is considered frozen
>> by the freezer.
>>
>> There are 2 constraints we are trying to handle here:
>>
>> [And let us see extreme case solutions for these constraints, to start with]
>>
>> 1. We want task freezing to be fast, to make hibernation fast.
>> Hence, we don't want to use msleep() here at all, just the
>> try_to_freeze() within the while loop would fit well.
>>
>> 2. As Tejun suggested, considering that we might not get frozen immediately,
>> we don't want to hurt cpu power management during that time. So, we
>> want to sleep when possible. Which means we can sleep for ~20 seconds at a
>> stretch and still manage to prevent freezing failures.
>>
>> But obviously we need to strike a balance between these 2 contradictions.
>> Hence, we observe that the freezer goes in a loop and tries to freeze
>> tasks, and waits for 10ms before retrying (and continues this procedure
>> for 20 seconds).
>>
>> Since we want time-to-freeze-this-task as small as possible, we have to
>> minimize the number of iterations the freezer does waiting for us.
>> Hence we choose to sleep for 10ms, which means, in the worst case,
>> our time-to-freeze-task will be one iteration of the freezer, IOW 10ms.
>> [That way, actually sleeping for 9ms would do best, but we probably don't
>> want to get that specific here, or should we?]
>>
>> I think I have given a slight hint about this issue in the comment as well...
>>
>> I prefer not to #define 10 and use it in freezer's loop and in this above
>> msleep() because, good design means, "Keep the freezer internals internal
>> to the freezer!". But all of us agree that this patch is only a temporary
>> hack (which unfortunately needs to know about freezer's internal working)..
>> and also that, we need to fix this whole issue at a design level sooner
>> or later.
>> So having 10ms msleep(), as well as hard-coding this value here, are both
>> justified, IMHO.
>>
>> As for the comment, I don't know if I should be expanding that "slight hint"
>> into a full-fledged explanation, since Tejun is already complaining about
>> its verbosity ;-)
>>
>> By the way, for somebody who is looking from a purely memory-hotplug point
>> of view and is not that familiar with the freezer, the "slight hint" in
>> the comment "careful enough not to sleep for too long at a stretch...
>> freezing failure..." is supposed to be interpreted as : "Oh when does
>> freezing fail? Let me look up the freezer code.. ah, 20 seconds.
>> By the way, I spot a 10ms sleep in the freezer loop as well..
>> Oh yes, *now* it all makes sense!" :-)
>>
>> Or perhaps, adding the same justification I gave above (about the 10ms
>> sleep) to the changelog should do, right?
>>
>>>> +	}
>>>>  }
>>>>  
>>>>  static inline void unlock_system_sleep(void)
>>>
> 
> Well, let's not add temporary hacks, especially as fragile as this one.
> 
> Why don't we simply open code what we need using a proper wait queue
> and waiting for an event?
> 
> So, have something like transition_in_progress (I believe there already is
> something like that somewhere), set it to 'true' under pm_mutex in
> lock_system_sleep(), possibly waiting for it to become 'false' properly
> (using interruptible sleep with try_to_freeze()) and reset it to 'false' (under pm_mutex) in unlock_system_sleep()?
> 

Actually, I think I have a better idea based on a key observation:
We are trying to acquire pm_mutex here. And if we block due to this,
we are *100% sure* that we are not going to run as long as hibernation
sequence is running, since hibernation releases pm_mutex only at the
very end, when everything is done.
And this means, this task is going to be blocked for much more longer
than what the freezer intends to achieve. Which means, freezing and
thawing doesn't really make a difference to this task!

So, let's just ask the freezer to skip freezing us!! And everything
will be just fine!

Something like:

void lock_system_sleep(void)
{
	/* simplified freezer_do_not_count() */
	current->flags |= PF_FREEZER_SKIP;

	mutex_lock(&pm_mutex);

}

void unlock_system_sleep(void)
{
	mutex_unlock(&pm_mutex);

	/* simplified freezer_count() */
	current->flags &= ~PF_FREEZER_SKIP;

}

We probably don't want the restriction that freezer_do_not_count() and
freezer_count() work only for userspace tasks. So I have open coded
the relevant parts of those functions here.

I haven't tested this solution yet. Let me know if this solution looks
good and I'll send it out as a patch after testing and analyzing some
corner cases, if any.

Thanks,
Srivatsa S. Bhat
 

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

[Srivatsa S. Bhat]

On 11/20/2011 03:54 PM, Rafael J. Wysocki wrote:
> On Sunday, November 20, 2011, Srivatsa S. Bhat wrote:
>> On 11/20/2011 03:27 AM, Rafael J. Wysocki wrote:
>>> On Thursday, November 17, 2011, Srivatsa S. Bhat wrote:
>>>> 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, we use msleep() to avoid busy looping and breaking expectations
>>>> that we go to sleep when we fail to acquire the lock.
>>>> And we also call try_to_freeze() in order to cooperate with the freezer,
>>>> without which we would end up in almost the same situation as mutex_lock(),
>>>> due to uninterruptible sleep caused by msleep().
>>>>
>>>> v3: Tejun suggested avoiding busy-looping by adding an msleep() since
>>>>     it is not guaranteed that we will get frozen immediately.
>>>>
>>>> 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 |   37 ++++++++++++++++++++++++++++++++++++-
>>>>  1 files changed, 36 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>>>> index 57a6924..0af3048 100644
>>>> --- a/include/linux/suspend.h
>>>> +++ b/include/linux/suspend.h
>>>> @@ -5,6 +5,8 @@
>>>>  #include <linux/notifier.h>
>>>>  #include <linux/init.h>
>>>>  #include <linux/pm.h>
>>>> +#include <linux/freezer.h>
>>>> +#include <linux/delay.h>
>>>>  #include <linux/mm.h>
>>>>  #include <asm/errno.h>
>>>>  
>>>> @@ -380,7 +382,40 @@ static inline void unlock_system_sleep(void) {}
>>>>  
>>>>  static inline void lock_system_sleep(void)
>>>>  {
>>>> -	mutex_lock(&pm_mutex);
>>>> +	/*
>>>> +	 * "To sleep, or not to sleep, that is the question!"
>>>> +	 *
>>>> +	 * 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).
>>>> +	 * Using mutex_lock_interruptible() in a loop is not a good idea,
>>>> +	 * because we could end up treating non-freezing signals badly.
>>>> +	 * So we use mutex_trylock() in a loop instead.
>>>> +	 *
>>>> +	 * Also, we add try_to_freeze() to the loop, to co-operate with the
>>>> +	 * freezer, to avoid task freezing failures due to busy-looping.
>>>> +	 *
>>>> +	 * But then, since it is not guaranteed that we will get frozen
>>>> +	 * rightaway, we could keep spinning for some time, breaking the
>>>> +	 * expectation that we go to sleep when we fail to acquire the lock.
>>>> +	 * So we add an msleep() to the loop, to dampen the spin (but we are
>>>> +	 * careful enough not to sleep for too long at a stretch, lest the
>>>> +	 * freezer whine and give up again!).
>>>> +	 *
>>>> +	 * Now that we no longer busy-loop, try_to_freeze() becomes all the
>>>> +	 * more important, due to a subtle reason: if we don't cooperate with
>>>> +	 * the freezer at this point, we could end up in a situation very
>>>> +	 * similar to mutex_lock() due to the usage of msleep() (which sleeps
>>>> +	 * uninterruptibly).
>>>> +	 *
>>>> +	 * Phew! What a delicate balance!
>>>> +	 */
>>>> +	while (!mutex_trylock(&pm_mutex)) {
>>>> +		try_to_freeze();
>>>> +		msleep(10);
>>>
>>> The number here seems to be somewhat arbitrary.  Is there any reason not to
>>> use 100 or any other number?
>>>
>>
>> Short answer:
>>
>> The number is not arbitrary. It is designed to match the frequency at which
>> the freezer re-tries to freeze tasks in a loop for 20 seconds (after which
>> it gives up).
> 
> So that should be documented too, right?  Perhaps there should be a #define
> for that number.
> 
>> Long answer:
>>
>> Let us define 'time-to-freeze-this-task' as the duration of time between the
>> setting of TIF_FREEZE flag for this task (after the task enters the while
>> loop in this patch) and the time at which this task is considered frozen
>> by the freezer.
>>
>> There are 2 constraints we are trying to handle here:
>>
>> [And let us see extreme case solutions for these constraints, to start with]
>>
>> 1. We want task freezing to be fast, to make hibernation fast.
>> Hence, we don't want to use msleep() here at all, just the
>> try_to_freeze() within the while loop would fit well.
>>
>> 2. As Tejun suggested, considering that we might not get frozen immediately,
>> we don't want to hurt cpu power management during that time. So, we
>> want to sleep when possible. Which means we can sleep for ~20 seconds at a
>> stretch and still manage to prevent freezing failures.
>>
>> But obviously we need to strike a balance between these 2 contradictions.
>> Hence, we observe that the freezer goes in a loop and tries to freeze
>> tasks, and waits for 10ms before retrying (and continues this procedure
>> for 20 seconds).
>>
>> Since we want time-to-freeze-this-task as small as possible, we have to
>> minimize the number of iterations the freezer does waiting for us.
>> Hence we choose to sleep for 10ms, which means, in the worst case,
>> our time-to-freeze-task will be one iteration of the freezer, IOW 10ms.
>> [That way, actually sleeping for 9ms would do best, but we probably don't
>> want to get that specific here, or should we?]
>>
>> I think I have given a slight hint about this issue in the comment as well...
>>
>> I prefer not to #define 10 and use it in freezer's loop and in this above
>> msleep() because, good design means, "Keep the freezer internals internal
>> to the freezer!". But all of us agree that this patch is only a temporary
>> hack (which unfortunately needs to know about freezer's internal working)..
>> and also that, we need to fix this whole issue at a design level sooner
>> or later.
>> So having 10ms msleep(), as well as hard-coding this value here, are both
>> justified, IMHO.
>>
>> As for the comment, I don't know if I should be expanding that "slight hint"
>> into a full-fledged explanation, since Tejun is already complaining about
>> its verbosity ;-)
>>
>> By the way, for somebody who is looking from a purely memory-hotplug point
>> of view and is not that familiar with the freezer, the "slight hint" in
>> the comment "careful enough not to sleep for too long at a stretch...
>> freezing failure..." is supposed to be interpreted as : "Oh when does
>> freezing fail? Let me look up the freezer code.. ah, 20 seconds.
>> By the way, I spot a 10ms sleep in the freezer loop as well..
>> Oh yes, *now* it all makes sense!" :-)
>>
>> Or perhaps, adding the same justification I gave above (about the 10ms
>> sleep) to the changelog should do, right?
>>
>>>> +	}
>>>>  }
>>>>  
>>>>  static inline void unlock_system_sleep(void)
>>>
> 
> Well, let's not add temporary hacks, especially as fragile as this one.
> 
> Why don't we simply open code what we need using a proper wait queue
> and waiting for an event?
> 
> So, have something like transition_in_progress (I believe there already is
> something like that somewhere), set it to 'true' under pm_mutex in
> lock_system_sleep(), possibly waiting for it to become 'false' properly
> (using interruptible sleep with try_to_freeze()) and reset it to 'false' (under pm_mutex) in unlock_system_sleep()?
> 

Actually, I think I have a better idea based on a key observation:
We are trying to acquire pm_mutex here. And if we block due to this,
we are *100% sure* that we are not going to run as long as hibernation
sequence is running, since hibernation releases pm_mutex only at the
very end, when everything is done.
And this means, this task is going to be blocked for much more longer
than what the freezer intends to achieve. Which means, freezing and
thawing doesn't really make a difference to this task!

So, let's just ask the freezer to skip freezing us!! And everything
will be just fine!

Something like:

void lock_system_sleep(void)
{
	/* simplified freezer_do_not_count() */
	current->flags |= PF_FREEZER_SKIP;

	mutex_lock(&pm_mutex);

}

void unlock_system_sleep(void)
{
	mutex_unlock(&pm_mutex);

	/* simplified freezer_count() */
	current->flags &= ~PF_FREEZER_SKIP;

}

We probably don't want the restriction that freezer_do_not_count() and
freezer_count() work only for userspace tasks. So I have open coded
the relevant parts of those functions here.

I haven't tested this solution yet. Let me know if this solution looks
good and I'll send it out as a patch after testing and analyzing some
corner cases, if any.

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 v3] PM/Memory-hotplug: Avoid task freezing failures

[Chen Gong]

[...]
>
> Actually, I think I have a better idea based on a key observation:
> We are trying to acquire pm_mutex here. And if we block due to this,
> we are *100% sure* that we are not going to run as long as hibernation
> sequence is running, since hibernation releases pm_mutex only at the
> very end, when everything is done.
> And this means, this task is going to be blocked for much more longer
> than what the freezer intends to achieve. Which means, freezing and
> thawing doesn't really make a difference to this task!
>
> So, let's just ask the freezer to skip freezing us!! And everything
> will be just fine!
>
> Something like:
>
> void lock_system_sleep(void)
> {
> 	/* simplified freezer_do_not_count() */
> 	current->flags |= PF_FREEZER_SKIP;
>
> 	mutex_lock(&pm_mutex);
>
> }
>
> void unlock_system_sleep(void)
> {
> 	mutex_unlock(&pm_mutex);
>
> 	/* simplified freezer_count() */
> 	current->flags&= ~PF_FREEZER_SKIP;
>
> }
>
> We probably don't want the restriction that freezer_do_not_count() and
> freezer_count() work only for userspace tasks. So I have open coded
> the relevant parts of those functions here.
>

This new design looks clean and better than old one. I just curious how do
you design your test environment? e.g. when hibernating is in progress,
try to online some memories and wait for hibernation fails or succeeds?

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

[Chen Gong]

[...]
>
> Actually, I think I have a better idea based on a key observation:
> We are trying to acquire pm_mutex here. And if we block due to this,
> we are *100% sure* that we are not going to run as long as hibernation
> sequence is running, since hibernation releases pm_mutex only at the
> very end, when everything is done.
> And this means, this task is going to be blocked for much more longer
> than what the freezer intends to achieve. Which means, freezing and
> thawing doesn't really make a difference to this task!
>
> So, let's just ask the freezer to skip freezing us!! And everything
> will be just fine!
>
> Something like:
>
> void lock_system_sleep(void)
> {
> 	/* simplified freezer_do_not_count() */
> 	current->flags |= PF_FREEZER_SKIP;
>
> 	mutex_lock(&pm_mutex);
>
> }
>
> void unlock_system_sleep(void)
> {
> 	mutex_unlock(&pm_mutex);
>
> 	/* simplified freezer_count() */
> 	current->flags&= ~PF_FREEZER_SKIP;
>
> }
>
> We probably don't want the restriction that freezer_do_not_count() and
> freezer_count() work only for userspace tasks. So I have open coded
> the relevant parts of those functions here.
>

This new design looks clean and better than old one. I just curious how do
you design your test environment? e.g. when hibernating is in progress,
try to online some memories and wait for hibernation fails or succeeds?

--
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 v3] PM/Memory-hotplug: Avoid task freezing failures

[Srivatsa S. Bhat]

On 11/21/2011 01:25 PM, Chen Gong wrote:
> [...]
>>
>> Actually, I think I have a better idea based on a key observation:
>> We are trying to acquire pm_mutex here. And if we block due to this,
>> we are *100% sure* that we are not going to run as long as hibernation
>> sequence is running, since hibernation releases pm_mutex only at the
>> very end, when everything is done.
>> And this means, this task is going to be blocked for much more longer
>> than what the freezer intends to achieve. Which means, freezing and
>> thawing doesn't really make a difference to this task!
>>
>> So, let's just ask the freezer to skip freezing us!! And everything
>> will be just fine!
>>
>> Something like:
>>
>> void lock_system_sleep(void)
>> {
>>     /* simplified freezer_do_not_count() */
>>     current->flags |= PF_FREEZER_SKIP;
>>
>>     mutex_lock(&pm_mutex);
>>
>> }
>>
>> void unlock_system_sleep(void)
>> {
>>     mutex_unlock(&pm_mutex);
>>
>>     /* simplified freezer_count() */
>>     current->flags&= ~PF_FREEZER_SKIP;
>>
>> }
>>
>> We probably don't want the restriction that freezer_do_not_count() and
>> freezer_count() work only for userspace tasks. So I have open coded
>> the relevant parts of those functions here.
>>
> 
> This new design looks clean and better than old one. I just curious how do
> you design your test environment? e.g. when hibernating is in progress,
> try to online some memories and wait for hibernation fails or succeeds?
> 

Hi Chen,

Thanks a lot for taking a look!

As I have indicated earlier in some of my mails, I am more concerned about
the API lock_system_sleep() than memory hotplug, because it is this *API*
that is buggy, not memory-hotplug. And going further, any other code planning
to use this API will be problematic. So our focus here, is to fix this *API*.

So, to test this API, I have written a kernel module that calls
lock_system_sleep() in its init code. Then I load/unload that module wildly
in a loop and simultaneously run hibernation tests using the 'pm_test'
framework. It is to be also noted that, the issue here is only with the initial
steps of hibernation, namely, related to freezer. Hence, pm_test framework
fits pretty well to debug these freezer issues. (And in fact, I have found that
this method is quite effective to test whether my patch fixes the issue or not.)

Thanks,
Srivatsa S. Bhat

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

[Srivatsa S. Bhat]

On 11/21/2011 01:25 PM, Chen Gong wrote:
> [...]
>>
>> Actually, I think I have a better idea based on a key observation:
>> We are trying to acquire pm_mutex here. And if we block due to this,
>> we are *100% sure* that we are not going to run as long as hibernation
>> sequence is running, since hibernation releases pm_mutex only at the
>> very end, when everything is done.
>> And this means, this task is going to be blocked for much more longer
>> than what the freezer intends to achieve. Which means, freezing and
>> thawing doesn't really make a difference to this task!
>>
>> So, let's just ask the freezer to skip freezing us!! And everything
>> will be just fine!
>>
>> Something like:
>>
>> void lock_system_sleep(void)
>> {
>>     /* simplified freezer_do_not_count() */
>>     current->flags |= PF_FREEZER_SKIP;
>>
>>     mutex_lock(&pm_mutex);
>>
>> }
>>
>> void unlock_system_sleep(void)
>> {
>>     mutex_unlock(&pm_mutex);
>>
>>     /* simplified freezer_count() */
>>     current->flags&= ~PF_FREEZER_SKIP;
>>
>> }
>>
>> We probably don't want the restriction that freezer_do_not_count() and
>> freezer_count() work only for userspace tasks. So I have open coded
>> the relevant parts of those functions here.
>>
> 
> This new design looks clean and better than old one. I just curious how do
> you design your test environment? e.g. when hibernating is in progress,
> try to online some memories and wait for hibernation fails or succeeds?
> 

Hi Chen,

Thanks a lot for taking a look!

As I have indicated earlier in some of my mails, I am more concerned about
the API lock_system_sleep() than memory hotplug, because it is this *API*
that is buggy, not memory-hotplug. And going further, any other code planning
to use this API will be problematic. So our focus here, is to fix this *API*.

So, to test this API, I have written a kernel module that calls
lock_system_sleep() in its init code. Then I load/unload that module wildly
in a loop and simultaneously run hibernation tests using the 'pm_test'
framework. It is to be also noted that, the issue here is only with the initial
steps of hibernation, namely, related to freezer. Hence, pm_test framework
fits pretty well to debug these freezer issues. (And in fact, I have found that
this method is quite effective to test whether my patch fixes the issue or not.)

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 v3] PM/Memory-hotplug: Avoid task freezing failures

[Tejun Heo]

Hello, Srivatsa.

On Mon, Nov 21, 2011 at 10:06:39AM +0530, Srivatsa S. Bhat wrote:
> void lock_system_sleep(void)
> {
> 	/* simplified freezer_do_not_count() */
> 	current->flags |= PF_FREEZER_SKIP;
> 
> 	mutex_lock(&pm_mutex);
> 
> }
> 
> void unlock_system_sleep(void)
> {
> 	mutex_unlock(&pm_mutex);
> 
> 	/* simplified freezer_count() */
> 	current->flags &= ~PF_FREEZER_SKIP;
> 
> }
> 
> We probably don't want the restriction that freezer_do_not_count() and
> freezer_count() work only for userspace tasks. So I have open coded
> the relevant parts of those functions here.
> 
> I haven't tested this solution yet. Let me know if this solution looks
> good and I'll send it out as a patch after testing and analyzing some
> corner cases, if any.

Ooh ooh, I definitely like this one much better.  Oleg did something
similar w/ wait_event_freezekillable() too.  On related notes,

* I think it would be better to remove direct access to pm_mutex and
  use [un]lock_system_sleep() universally.  I don't think hinging it
  on CONFIG_HIBERNATE_CALLBACKS buys us anything.

* In the longer term, we should be able to toggle PF_NOFREEZE instead
  as SKIP doesn't mean anything different.  We'll probably need a
  better API tho.  But for now SKIP should work fine.

Thank you.

-- 
tejun

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

[Tejun Heo]

Hello, Srivatsa.

On Mon, Nov 21, 2011 at 10:06:39AM +0530, Srivatsa S. Bhat wrote:
> void lock_system_sleep(void)
> {
> 	/* simplified freezer_do_not_count() */
> 	current->flags |= PF_FREEZER_SKIP;
> 
> 	mutex_lock(&pm_mutex);
> 
> }
> 
> void unlock_system_sleep(void)
> {
> 	mutex_unlock(&pm_mutex);
> 
> 	/* simplified freezer_count() */
> 	current->flags &= ~PF_FREEZER_SKIP;
> 
> }
> 
> We probably don't want the restriction that freezer_do_not_count() and
> freezer_count() work only for userspace tasks. So I have open coded
> the relevant parts of those functions here.
> 
> I haven't tested this solution yet. Let me know if this solution looks
> good and I'll send it out as a patch after testing and analyzing some
> corner cases, if any.

Ooh ooh, I definitely like this one much better.  Oleg did something
similar w/ wait_event_freezekillable() too.  On related notes,

* I think it would be better to remove direct access to pm_mutex and
  use [un]lock_system_sleep() universally.  I don't think hinging it
  on CONFIG_HIBERNATE_CALLBACKS buys us anything.

* In the longer term, we should be able to toggle PF_NOFREEZE instead
  as SKIP doesn't mean anything different.  We'll probably need a
  better API tho.  But for now SKIP should work fine.

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 v3] PM/Memory-hotplug: Avoid task freezing failures

[Tejun Heo]

Hello, Rafael.

On Sat, Nov 19, 2011 at 10:57:19PM +0100, Rafael J. Wysocki wrote:
> > +	while (!mutex_trylock(&pm_mutex)) {
> > +		try_to_freeze();
> > +		msleep(10);
> 
> The number here seems to be somewhat arbitrary.  Is there any reason not to
> use 100 or any other number?

This is a bit moot at this point but, at least for me, yeah, it's a
number I pulled out of my ass.  That said, I think it's a good number
to pull out of ass for userland visible retry delays for the following
reasons.

* It's a good number - 10! which happens to match the number of
  fingers I have!  Isn't that just weird? @.@

* For modern hardware of most classes, repeating not-so-complex stuff
  every 10ms for a while isn't taxing (or even noticeable) at all.

* Sub 10ms delays usually aren't noticeable to human beings even when
  several of them are staggered.  This is very different when you get
  to 100ms range.

ie. going from 1ms to 10ms doesn't cost you too much in terms of human
noticeable latency (for this type of situations anyway) but going from
10ms to 100ms does.  In terms of computational cost, the reverse is
somewhat true too.  So, yeah, I think 10ms is a good out-of-ass number
for this type of delays.

Thanks.

-- 
tejun

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

[Tejun Heo]

Hello, Rafael.

On Sat, Nov 19, 2011 at 10:57:19PM +0100, Rafael J. Wysocki wrote:
> > +	while (!mutex_trylock(&pm_mutex)) {
> > +		try_to_freeze();
> > +		msleep(10);
> 
> The number here seems to be somewhat arbitrary.  Is there any reason not to
> use 100 or any other number?

This is a bit moot at this point but, at least for me, yeah, it's a
number I pulled out of my ass.  That said, I think it's a good number
to pull out of ass for userland visible retry delays for the following
reasons.

* It's a good number - 10! which happens to match the number of
  fingers I have!  Isn't that just weird? @.@

* For modern hardware of most classes, repeating not-so-complex stuff
  every 10ms for a while isn't taxing (or even noticeable) at all.

* Sub 10ms delays usually aren't noticeable to human beings even when
  several of them are staggered.  This is very different when you get
  to 100ms range.

ie. going from 1ms to 10ms doesn't cost you too much in terms of human
noticeable latency (for this type of situations anyway) but going from
10ms to 100ms does.  In terms of computational cost, the reverse is
somewhat true too.  So, yeah, I think 10ms is a good out-of-ass number
for this type of delays.

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 v3] PM/Memory-hotplug: Avoid task freezing failures

[Srivatsa S. Bhat]

On 11/21/2011 10:10 PM, Tejun Heo wrote:
> Hello, Srivatsa.
> 
> On Mon, Nov 21, 2011 at 10:06:39AM +0530, Srivatsa S. Bhat wrote:
>> void lock_system_sleep(void)
>> {
>> 	/* simplified freezer_do_not_count() */
>> 	current->flags |= PF_FREEZER_SKIP;
>>
>> 	mutex_lock(&pm_mutex);
>>
>> }
>>
>> void unlock_system_sleep(void)
>> {
>> 	mutex_unlock(&pm_mutex);
>>
>> 	/* simplified freezer_count() */
>> 	current->flags &= ~PF_FREEZER_SKIP;
>>
>> }
>>
>> We probably don't want the restriction that freezer_do_not_count() and
>> freezer_count() work only for userspace tasks. So I have open coded
>> the relevant parts of those functions here.
>>
>> I haven't tested this solution yet. Let me know if this solution looks
>> good and I'll send it out as a patch after testing and analyzing some
>> corner cases, if any.

I tested this, and it works great! I'll send the patch in some time.

> 
> Ooh ooh, I definitely like this one much better. 

Thanks :-) Even I like it far better than all those ugly hacks I proposed
earlier ;-)

> Oleg did something
> similar w/ wait_event_freezekillable() too.  On related notes,
> 
> * I think it would be better to remove direct access to pm_mutex and
>   use [un]lock_system_sleep() universally.  I don't think hinging it
>   on CONFIG_HIBERNATE_CALLBACKS buys us anything.
> 

Which direct access to pm_mutex are you referring to?
Other than suspend/hibernation call paths, I think mem-hotplug is the only
subsystem trying to access pm_mutex. I haven't checked thoroughly though. 

But yes, using lock_system_sleep() for mutually excluding some code path
from suspend/hibernation is good, and that is one reason why I wanted
to fix this API ASAP. But as long as memory hotplug is the only direct user
of pm_mutex, is it justified to remove the CONFIG_HIBERNATE_CALLBACKS
restriction and make it generic? I don't know...

Or, are you saying that we should use these APIs even in suspend/hibernate
call paths? That's not such a bad idea either...

[ On a totally different note, I was wondering:- if mem-hotplug wants to
exclude itself from hibernation alone, CONFIG_HIBERNATE_CALLBACKS is not
the right way to do it, because, it would still unintentionally exclude
itself from suspend also! (if suspend and hibernation are both enabled).
I don't think we should worry about this too much, because we don't get
much benefit trying to make mem-hotplug co-exist with suspend.. In fact,
I would say, its even better to let it be this way and exclude suspend
as well, since running exotic stuff like memory hotplug during suspend
or hibernation is best avoided ;-) ]

> * In the longer term, we should be able to toggle PF_NOFREEZE instead
>   as SKIP doesn't mean anything different.  We'll probably need a
>   better API tho.  But for now SKIP should work fine.
> 

Yep, I agree.

Thanks,
Srivatsa S. Bhat

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

[Srivatsa S. Bhat]

On 11/21/2011 10:10 PM, Tejun Heo wrote:
> Hello, Srivatsa.
> 
> On Mon, Nov 21, 2011 at 10:06:39AM +0530, Srivatsa S. Bhat wrote:
>> void lock_system_sleep(void)
>> {
>> 	/* simplified freezer_do_not_count() */
>> 	current->flags |= PF_FREEZER_SKIP;
>>
>> 	mutex_lock(&pm_mutex);
>>
>> }
>>
>> void unlock_system_sleep(void)
>> {
>> 	mutex_unlock(&pm_mutex);
>>
>> 	/* simplified freezer_count() */
>> 	current->flags &= ~PF_FREEZER_SKIP;
>>
>> }
>>
>> We probably don't want the restriction that freezer_do_not_count() and
>> freezer_count() work only for userspace tasks. So I have open coded
>> the relevant parts of those functions here.
>>
>> I haven't tested this solution yet. Let me know if this solution looks
>> good and I'll send it out as a patch after testing and analyzing some
>> corner cases, if any.

I tested this, and it works great! I'll send the patch in some time.

> 
> Ooh ooh, I definitely like this one much better. 

Thanks :-) Even I like it far better than all those ugly hacks I proposed
earlier ;-)

> Oleg did something
> similar w/ wait_event_freezekillable() too.  On related notes,
> 
> * I think it would be better to remove direct access to pm_mutex and
>   use [un]lock_system_sleep() universally.  I don't think hinging it
>   on CONFIG_HIBERNATE_CALLBACKS buys us anything.
> 

Which direct access to pm_mutex are you referring to?
Other than suspend/hibernation call paths, I think mem-hotplug is the only
subsystem trying to access pm_mutex. I haven't checked thoroughly though. 

But yes, using lock_system_sleep() for mutually excluding some code path
from suspend/hibernation is good, and that is one reason why I wanted
to fix this API ASAP. But as long as memory hotplug is the only direct user
of pm_mutex, is it justified to remove the CONFIG_HIBERNATE_CALLBACKS
restriction and make it generic? I don't know...

Or, are you saying that we should use these APIs even in suspend/hibernate
call paths? That's not such a bad idea either...

[ On a totally different note, I was wondering:- if mem-hotplug wants to
exclude itself from hibernation alone, CONFIG_HIBERNATE_CALLBACKS is not
the right way to do it, because, it would still unintentionally exclude
itself from suspend also! (if suspend and hibernation are both enabled).
I don't think we should worry about this too much, because we don't get
much benefit trying to make mem-hotplug co-exist with suspend.. In fact,
I would say, its even better to let it be this way and exclude suspend
as well, since running exotic stuff like memory hotplug during suspend
or hibernation is best avoided ;-) ]

> * In the longer term, we should be able to toggle PF_NOFREEZE instead
>   as SKIP doesn't mean anything different.  We'll probably need a
>   better API tho.  But for now SKIP should work fine.
> 

Yep, I agree.

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 v3] PM/Memory-hotplug: Avoid task freezing failures

[Srivatsa S. Bhat]

On 11/21/2011 10:17 PM, Tejun Heo wrote:
> Hello, Rafael.
> 
> On Sat, Nov 19, 2011 at 10:57:19PM +0100, Rafael J. Wysocki wrote:
>>> +	while (!mutex_trylock(&pm_mutex)) {
>>> +		try_to_freeze();
>>> +		msleep(10);
>>
>> The number here seems to be somewhat arbitrary.  Is there any reason not to
>> use 100 or any other number?
> 
> This is a bit moot at this point but, at least for me, yeah, it's a
> number I pulled out of my ass.  That said, I think it's a good number
> to pull out of ass for userland visible retry delays for the following
> reasons.
> 
> * It's a good number - 10! which happens to match the number of
>   fingers I have!  Isn't that just weird? @.@
> 
> * For modern hardware of most classes, repeating not-so-complex stuff
>   every 10ms for a while isn't taxing (or even noticeable) at all.
> 
> * Sub 10ms delays usually aren't noticeable to human beings even when
>   several of them are staggered.  This is very different when you get
>   to 100ms range.
> 
> ie. going from 1ms to 10ms doesn't cost you too much in terms of human
> noticeable latency (for this type of situations anyway) but going from
> 10ms to 100ms does.  In terms of computational cost, the reverse is
> somewhat true too.  So, yeah, I think 10ms is a good out-of-ass number
> for this type of delays.
> 

My God! I had absolutely no idea you had cooked up that number just like
that ;-) Look at how creative I was when defending that number :P
Your justification is not bad either ;-)

[ Well, seriously, I had given a fair amount of thought before incorporating
that number in my patch, by looking at the freezer re-try latency and so on,
which I explained in my reply earlier.]

Anyways, nice one :-)

Thanks,
Srivatsa S. Bhat

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

[Srivatsa S. Bhat]

On 11/21/2011 10:17 PM, Tejun Heo wrote:
> Hello, Rafael.
> 
> On Sat, Nov 19, 2011 at 10:57:19PM +0100, Rafael J. Wysocki wrote:
>>> +	while (!mutex_trylock(&pm_mutex)) {
>>> +		try_to_freeze();
>>> +		msleep(10);
>>
>> The number here seems to be somewhat arbitrary.  Is there any reason not to
>> use 100 or any other number?
> 
> This is a bit moot at this point but, at least for me, yeah, it's a
> number I pulled out of my ass.  That said, I think it's a good number
> to pull out of ass for userland visible retry delays for the following
> reasons.
> 
> * It's a good number - 10! which happens to match the number of
>   fingers I have!  Isn't that just weird? @.@
> 
> * For modern hardware of most classes, repeating not-so-complex stuff
>   every 10ms for a while isn't taxing (or even noticeable) at all.
> 
> * Sub 10ms delays usually aren't noticeable to human beings even when
>   several of them are staggered.  This is very different when you get
>   to 100ms range.
> 
> ie. going from 1ms to 10ms doesn't cost you too much in terms of human
> noticeable latency (for this type of situations anyway) but going from
> 10ms to 100ms does.  In terms of computational cost, the reverse is
> somewhat true too.  So, yeah, I think 10ms is a good out-of-ass number
> for this type of delays.
> 

My God! I had absolutely no idea you had cooked up that number just like
that ;-) Look at how creative I was when defending that number :P
Your justification is not bad either ;-)

[ Well, seriously, I had given a fair amount of thought before incorporating
that number in my patch, by looking at the freezer re-try latency and so on,
which I explained in my reply earlier.]

Anyways, nice one :-)

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 v3] PM/Memory-hotplug: Avoid task freezing failures

[Tejun Heo]

Hello,

On Mon, Nov 21, 2011 at 10:34:40PM +0530, Srivatsa S. Bhat wrote:
> >> I haven't tested this solution yet. Let me know if this solution looks
> >> good and I'll send it out as a patch after testing and analyzing some
> >> corner cases, if any.
> 
> I tested this, and it works great! I'll send the patch in some time.

Awesome.

> > * I think it would be better to remove direct access to pm_mutex and
> >   use [un]lock_system_sleep() universally.  I don't think hinging it
> >   on CONFIG_HIBERNATE_CALLBACKS buys us anything.
> > 
> 
> Which direct access to pm_mutex are you referring to?
> Other than suspend/hibernation call paths, I think mem-hotplug is the only
> subsystem trying to access pm_mutex. I haven't checked thoroughly though. 
> 
> But yes, using lock_system_sleep() for mutually excluding some code path
> from suspend/hibernation is good, and that is one reason why I wanted
> to fix this API ASAP. But as long as memory hotplug is the only direct user
> of pm_mutex, is it justified to remove the CONFIG_HIBERNATE_CALLBACKS
> restriction and make it generic? I don't know...
> 
> Or, are you saying that we should use these APIs even in suspend/hibernate
> call paths? That's not such a bad idea either...

Yeap, all.  It's just confusing to have two different types of access
to a single lock and I don't believe CONFIG_HIBERNATE_CALLBACKS is a
meaningful optimization in this case.

Thank you.

-- 
tejun

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

[Tejun Heo]

Hello,

On Mon, Nov 21, 2011 at 10:34:40PM +0530, Srivatsa S. Bhat wrote:
> >> I haven't tested this solution yet. Let me know if this solution looks
> >> good and I'll send it out as a patch after testing and analyzing some
> >> corner cases, if any.
> 
> I tested this, and it works great! I'll send the patch in some time.

Awesome.

> > * I think it would be better to remove direct access to pm_mutex and
> >   use [un]lock_system_sleep() universally.  I don't think hinging it
> >   on CONFIG_HIBERNATE_CALLBACKS buys us anything.
> > 
> 
> Which direct access to pm_mutex are you referring to?
> Other than suspend/hibernation call paths, I think mem-hotplug is the only
> subsystem trying to access pm_mutex. I haven't checked thoroughly though. 
> 
> But yes, using lock_system_sleep() for mutually excluding some code path
> from suspend/hibernation is good, and that is one reason why I wanted
> to fix this API ASAP. But as long as memory hotplug is the only direct user
> of pm_mutex, is it justified to remove the CONFIG_HIBERNATE_CALLBACKS
> restriction and make it generic? I don't know...
> 
> Or, are you saying that we should use these APIs even in suspend/hibernate
> call paths? That's not such a bad idea either...

Yeap, all.  It's just confusing to have two different types of access
to a single lock and I don't believe CONFIG_HIBERNATE_CALLBACKS is a
meaningful optimization in this case.

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 v3] PM/Memory-hotplug: Avoid task freezing failures

[Srivatsa S. Bhat]

On 11/21/2011 11:22 PM, Tejun Heo wrote:
> Hello,
> 
> On Mon, Nov 21, 2011 at 10:34:40PM +0530, Srivatsa S. Bhat wrote:
>>>> I haven't tested this solution yet. Let me know if this solution looks
>>>> good and I'll send it out as a patch after testing and analyzing some
>>>> corner cases, if any.
>>
>> I tested this, and it works great! I'll send the patch in some time.
> 
> Awesome.
> 
>>> * I think it would be better to remove direct access to pm_mutex and
>>>   use [un]lock_system_sleep() universally.  I don't think hinging it
>>>   on CONFIG_HIBERNATE_CALLBACKS buys us anything.
>>>
>>
>> Which direct access to pm_mutex are you referring to?
>> Other than suspend/hibernation call paths, I think mem-hotplug is the only
>> subsystem trying to access pm_mutex. I haven't checked thoroughly though. 
>>
>> But yes, using lock_system_sleep() for mutually excluding some code path
>> from suspend/hibernation is good, and that is one reason why I wanted
>> to fix this API ASAP. But as long as memory hotplug is the only direct user
>> of pm_mutex, is it justified to remove the CONFIG_HIBERNATE_CALLBACKS
>> restriction and make it generic? I don't know...
>>
>> Or, are you saying that we should use these APIs even in suspend/hibernate
>> call paths? That's not such a bad idea either...
> 
> Yeap, all.  It's just confusing to have two different types of access
> to a single lock and I don't believe CONFIG_HIBERNATE_CALLBACKS is a
> meaningful optimization in this case.
> 

Ok that sounds good, I'll send a separate patch for that.
Rafael, do you also agree that this would be better?

Thanks,
Srivatsa S. Bhat

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

[Srivatsa S. Bhat]

On 11/21/2011 11:22 PM, Tejun Heo wrote:
> Hello,
> 
> On Mon, Nov 21, 2011 at 10:34:40PM +0530, Srivatsa S. Bhat wrote:
>>>> I haven't tested this solution yet. Let me know if this solution looks
>>>> good and I'll send it out as a patch after testing and analyzing some
>>>> corner cases, if any.
>>
>> I tested this, and it works great! I'll send the patch in some time.
> 
> Awesome.
> 
>>> * I think it would be better to remove direct access to pm_mutex and
>>>   use [un]lock_system_sleep() universally.  I don't think hinging it
>>>   on CONFIG_HIBERNATE_CALLBACKS buys us anything.
>>>
>>
>> Which direct access to pm_mutex are you referring to?
>> Other than suspend/hibernation call paths, I think mem-hotplug is the only
>> subsystem trying to access pm_mutex. I haven't checked thoroughly though. 
>>
>> But yes, using lock_system_sleep() for mutually excluding some code path
>> from suspend/hibernation is good, and that is one reason why I wanted
>> to fix this API ASAP. But as long as memory hotplug is the only direct user
>> of pm_mutex, is it justified to remove the CONFIG_HIBERNATE_CALLBACKS
>> restriction and make it generic? I don't know...
>>
>> Or, are you saying that we should use these APIs even in suspend/hibernate
>> call paths? That's not such a bad idea either...
> 
> Yeap, all.  It's just confusing to have two different types of access
> to a single lock and I don't believe CONFIG_HIBERNATE_CALLBACKS is a
> meaningful optimization in this case.
> 

Ok that sounds good, I'll send a separate patch for that.
Rafael, do you also agree that this would be better?

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>

[PATCH v4] 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.

But it is to be noted that, since this task tries to acquire pm_mutex, if it
blocks due to this, we are *100% sure* that this task is not going to run
as long as hibernation sequence is in progress, since hibernation releases
'pm_mutex' only at the very end, when everything is done.
And this means, this task is going to be anyway blocked for much more longer
than what the freezer intends to achieve; which means, freezing and thawing
doesn't really make any difference to this task!

So, to fix freezing failures, we just ask the freezer to skip freezing this
task, since it is already "frozen enough".

But instead of calling freezer_do_not_count() and freezer_count() as it is,
we use only the relevant parts of those functions, because restrictions
such as 'the task should be a userspace one' etc., might not be relevant in
this scenario.

v4: Redesigned the whole fix, to ask the freezer to skip freezing the task
    which is blocked trying to acquire 'pm_mutex' lock.

v3: Tejun suggested avoiding busy-looping by adding an msleep() since
    it is not guaranteed that we will get frozen immediately.

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 |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 57a6924..1f7fff4 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -380,12 +380,16 @@ static inline void unlock_system_sleep(void) {}
 
 static inline void lock_system_sleep(void)
 {
+	/* simplified freezer_do_not_count() */
+	current->flags |= PF_FREEZER_SKIP;
 	mutex_lock(&pm_mutex);
 }
 
 static inline void unlock_system_sleep(void)
 {
 	mutex_unlock(&pm_mutex);
+	/* simplified freezer_count() */
+	current->flags &= ~PF_FREEZER_SKIP;
 }
 #endif
 

[PATCH v4] 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.

But it is to be noted that, since this task tries to acquire pm_mutex, if it
blocks due to this, we are *100% sure* that this task is not going to run
as long as hibernation sequence is in progress, since hibernation releases
'pm_mutex' only at the very end, when everything is done.
And this means, this task is going to be anyway blocked for much more longer
than what the freezer intends to achieve; which means, freezing and thawing
doesn't really make any difference to this task!

So, to fix freezing failures, we just ask the freezer to skip freezing this
task, since it is already "frozen enough".

But instead of calling freezer_do_not_count() and freezer_count() as it is,
we use only the relevant parts of those functions, because restrictions
such as 'the task should be a userspace one' etc., might not be relevant in
this scenario.

v4: Redesigned the whole fix, to ask the freezer to skip freezing the task
    which is blocked trying to acquire 'pm_mutex' lock.

v3: Tejun suggested avoiding busy-looping by adding an msleep() since
    it is not guaranteed that we will get frozen immediately.

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 |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 57a6924..1f7fff4 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -380,12 +380,16 @@ static inline void unlock_system_sleep(void) {}
 
 static inline void lock_system_sleep(void)
 {
+	/* simplified freezer_do_not_count() */
+	current->flags |= PF_FREEZER_SKIP;
 	mutex_lock(&pm_mutex);
 }
 
 static inline void unlock_system_sleep(void)
 {
 	mutex_unlock(&pm_mutex);
+	/* simplified freezer_count() */
+	current->flags &= ~PF_FREEZER_SKIP;
 }
 #endif
 



--
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 v4] PM / Memory-hotplug: Avoid task freezing failures

[Tejun Heo]

On Mon, Nov 21, 2011 at 11:42:54PM +0530, Srivatsa S. Bhat wrote:
> 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.
> 
> But it is to be noted that, since this task tries to acquire pm_mutex, if it
> blocks due to this, we are *100% sure* that this task is not going to run
> as long as hibernation sequence is in progress, since hibernation releases
> 'pm_mutex' only at the very end, when everything is done.
> And this means, this task is going to be anyway blocked for much more longer
> than what the freezer intends to achieve; which means, freezing and thawing
> doesn't really make any difference to this task!
> 
> So, to fix freezing failures, we just ask the freezer to skip freezing this
> task, since it is already "frozen enough".
> 
> But instead of calling freezer_do_not_count() and freezer_count() as it is,
> we use only the relevant parts of those functions, because restrictions
> such as 'the task should be a userspace one' etc., might not be relevant in
> this scenario.
> 
> v4: Redesigned the whole fix, to ask the freezer to skip freezing the task
>     which is blocked trying to acquire 'pm_mutex' lock.
> 
> v3: Tejun suggested avoiding busy-looping by adding an msleep() since
>     it is not guaranteed that we will get frozen immediately.
> 
> 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>

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

Thanks a lot. :)

-- 
tejun

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

[Tejun Heo]

On Mon, Nov 21, 2011 at 11:42:54PM +0530, Srivatsa S. Bhat wrote:
> 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.
> 
> But it is to be noted that, since this task tries to acquire pm_mutex, if it
> blocks due to this, we are *100% sure* that this task is not going to run
> as long as hibernation sequence is in progress, since hibernation releases
> 'pm_mutex' only at the very end, when everything is done.
> And this means, this task is going to be anyway blocked for much more longer
> than what the freezer intends to achieve; which means, freezing and thawing
> doesn't really make any difference to this task!
> 
> So, to fix freezing failures, we just ask the freezer to skip freezing this
> task, since it is already "frozen enough".
> 
> But instead of calling freezer_do_not_count() and freezer_count() as it is,
> we use only the relevant parts of those functions, because restrictions
> such as 'the task should be a userspace one' etc., might not be relevant in
> this scenario.
> 
> v4: Redesigned the whole fix, to ask the freezer to skip freezing the task
>     which is blocked trying to acquire 'pm_mutex' lock.
> 
> v3: Tejun suggested avoiding busy-looping by adding an msleep() since
>     it is not guaranteed that we will get frozen immediately.
> 
> 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>

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

Thanks a lot. :)

-- 
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 v4] PM / Memory-hotplug: Avoid task freezing failures

[Srivatsa S. Bhat]

On 11/21/2011 11:53 PM, Tejun Heo wrote:
> On Mon, Nov 21, 2011 at 11:42:54PM +0530, Srivatsa S. Bhat wrote:
>> 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.
>>
>> But it is to be noted that, since this task tries to acquire pm_mutex, if it
>> blocks due to this, we are *100% sure* that this task is not going to run
>> as long as hibernation sequence is in progress, since hibernation releases
>> 'pm_mutex' only at the very end, when everything is done.
>> And this means, this task is going to be anyway blocked for much more longer
>> than what the freezer intends to achieve; which means, freezing and thawing
>> doesn't really make any difference to this task!
>>
>> So, to fix freezing failures, we just ask the freezer to skip freezing this
>> task, since it is already "frozen enough".
>>
>> But instead of calling freezer_do_not_count() and freezer_count() as it is,
>> we use only the relevant parts of those functions, because restrictions
>> such as 'the task should be a userspace one' etc., might not be relevant in
>> this scenario.
>>
>> v4: Redesigned the whole fix, to ask the freezer to skip freezing the task
>>     which is blocked trying to acquire 'pm_mutex' lock.
>>
>> v3: Tejun suggested avoiding busy-looping by adding an msleep() since
>>     it is not guaranteed that we will get frozen immediately.
>>
>> 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>
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
> Thanks a lot. :)
> 

Thank you too :-)

Regards,
Srivatsa S. Bhat

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

[Srivatsa S. Bhat]

On 11/21/2011 11:53 PM, Tejun Heo wrote:
> On Mon, Nov 21, 2011 at 11:42:54PM +0530, Srivatsa S. Bhat wrote:
>> 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.
>>
>> But it is to be noted that, since this task tries to acquire pm_mutex, if it
>> blocks due to this, we are *100% sure* that this task is not going to run
>> as long as hibernation sequence is in progress, since hibernation releases
>> 'pm_mutex' only at the very end, when everything is done.
>> And this means, this task is going to be anyway blocked for much more longer
>> than what the freezer intends to achieve; which means, freezing and thawing
>> doesn't really make any difference to this task!
>>
>> So, to fix freezing failures, we just ask the freezer to skip freezing this
>> task, since it is already "frozen enough".
>>
>> But instead of calling freezer_do_not_count() and freezer_count() as it is,
>> we use only the relevant parts of those functions, because restrictions
>> such as 'the task should be a userspace one' etc., might not be relevant in
>> this scenario.
>>
>> v4: Redesigned the whole fix, to ask the freezer to skip freezing the task
>>     which is blocked trying to acquire 'pm_mutex' lock.
>>
>> v3: Tejun suggested avoiding busy-looping by adding an msleep() since
>>     it is not guaranteed that we will get frozen immediately.
>>
>> 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>
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
> Thanks a lot. :)
> 

Thank you too :-)

Regards,
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 v3] PM/Memory-hotplug: Avoid task freezing failures

[Rafael J. Wysocki]

On Monday, November 21, 2011, Srivatsa S. Bhat wrote:
> On 11/21/2011 11:22 PM, Tejun Heo wrote:
> > Hello,
> > 
> > On Mon, Nov 21, 2011 at 10:34:40PM +0530, Srivatsa S. Bhat wrote:
> >>>> I haven't tested this solution yet. Let me know if this solution looks
> >>>> good and I'll send it out as a patch after testing and analyzing some
> >>>> corner cases, if any.
> >>
> >> I tested this, and it works great! I'll send the patch in some time.
> > 
> > Awesome.
> > 
> >>> * I think it would be better to remove direct access to pm_mutex and
> >>>   use [un]lock_system_sleep() universally.  I don't think hinging it
> >>>   on CONFIG_HIBERNATE_CALLBACKS buys us anything.
> >>>
> >>
> >> Which direct access to pm_mutex are you referring to?
> >> Other than suspend/hibernation call paths, I think mem-hotplug is the only
> >> subsystem trying to access pm_mutex. I haven't checked thoroughly though. 
> >>
> >> But yes, using lock_system_sleep() for mutually excluding some code path
> >> from suspend/hibernation is good, and that is one reason why I wanted
> >> to fix this API ASAP. But as long as memory hotplug is the only direct user
> >> of pm_mutex, is it justified to remove the CONFIG_HIBERNATE_CALLBACKS
> >> restriction and make it generic? I don't know...
> >>
> >> Or, are you saying that we should use these APIs even in suspend/hibernate
> >> call paths? That's not such a bad idea either...
> > 
> > Yeap, all.  It's just confusing to have two different types of access
> > to a single lock and I don't believe CONFIG_HIBERNATE_CALLBACKS is a
> > meaningful optimization in this case.
> > 
> 
> Ok that sounds good, I'll send a separate patch for that.
> Rafael, do you also agree that this would be better?

Yes, it would.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Monday, November 21, 2011, Srivatsa S. Bhat wrote:
> On 11/21/2011 11:22 PM, Tejun Heo wrote:
> > Hello,
> > 
> > On Mon, Nov 21, 2011 at 10:34:40PM +0530, Srivatsa S. Bhat wrote:
> >>>> I haven't tested this solution yet. Let me know if this solution looks
> >>>> good and I'll send it out as a patch after testing and analyzing some
> >>>> corner cases, if any.
> >>
> >> I tested this, and it works great! I'll send the patch in some time.
> > 
> > Awesome.
> > 
> >>> * I think it would be better to remove direct access to pm_mutex and
> >>>   use [un]lock_system_sleep() universally.  I don't think hinging it
> >>>   on CONFIG_HIBERNATE_CALLBACKS buys us anything.
> >>>
> >>
> >> Which direct access to pm_mutex are you referring to?
> >> Other than suspend/hibernation call paths, I think mem-hotplug is the only
> >> subsystem trying to access pm_mutex. I haven't checked thoroughly though. 
> >>
> >> But yes, using lock_system_sleep() for mutually excluding some code path
> >> from suspend/hibernation is good, and that is one reason why I wanted
> >> to fix this API ASAP. But as long as memory hotplug is the only direct user
> >> of pm_mutex, is it justified to remove the CONFIG_HIBERNATE_CALLBACKS
> >> restriction and make it generic? I don't know...
> >>
> >> Or, are you saying that we should use these APIs even in suspend/hibernate
> >> call paths? That's not such a bad idea either...
> > 
> > Yeap, all.  It's just confusing to have two different types of access
> > to a single lock and I don't believe CONFIG_HIBERNATE_CALLBACKS is a
> > meaningful optimization in this case.
> > 
> 
> Ok that sounds good, I'll send a separate patch for that.
> Rafael, do you also agree that this would be better?

Yes, it would.

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>

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

[Rafael J. Wysocki]

On Monday, November 21, 2011, Tejun Heo wrote:
> On Mon, Nov 21, 2011 at 11:42:54PM +0530, Srivatsa S. Bhat wrote:
> > 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.
> > 
> > But it is to be noted that, since this task tries to acquire pm_mutex, if it
> > blocks due to this, we are *100% sure* that this task is not going to run
> > as long as hibernation sequence is in progress, since hibernation releases
> > 'pm_mutex' only at the very end, when everything is done.
> > And this means, this task is going to be anyway blocked for much more longer
> > than what the freezer intends to achieve; which means, freezing and thawing
> > doesn't really make any difference to this task!
> > 
> > So, to fix freezing failures, we just ask the freezer to skip freezing this
> > task, since it is already "frozen enough".
> > 
> > But instead of calling freezer_do_not_count() and freezer_count() as it is,
> > we use only the relevant parts of those functions, because restrictions
> > such as 'the task should be a userspace one' etc., might not be relevant in
> > this scenario.
> > 
> > v4: Redesigned the whole fix, to ask the freezer to skip freezing the task
> >     which is blocked trying to acquire 'pm_mutex' lock.
> > 
> > v3: Tejun suggested avoiding busy-looping by adding an msleep() since
> >     it is not guaranteed that we will get frozen immediately.
> > 
> > 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>
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
> Thanks a lot. :)

Applied to linux-pm/linux-next.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Monday, November 21, 2011, Tejun Heo wrote:
> On Mon, Nov 21, 2011 at 11:42:54PM +0530, Srivatsa S. Bhat wrote:
> > 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.
> > 
> > But it is to be noted that, since this task tries to acquire pm_mutex, if it
> > blocks due to this, we are *100% sure* that this task is not going to run
> > as long as hibernation sequence is in progress, since hibernation releases
> > 'pm_mutex' only at the very end, when everything is done.
> > And this means, this task is going to be anyway blocked for much more longer
> > than what the freezer intends to achieve; which means, freezing and thawing
> > doesn't really make any difference to this task!
> > 
> > So, to fix freezing failures, we just ask the freezer to skip freezing this
> > task, since it is already "frozen enough".
> > 
> > But instead of calling freezer_do_not_count() and freezer_count() as it is,
> > we use only the relevant parts of those functions, because restrictions
> > such as 'the task should be a userspace one' etc., might not be relevant in
> > this scenario.
> > 
> > v4: Redesigned the whole fix, to ask the freezer to skip freezing the task
> >     which is blocked trying to acquire 'pm_mutex' lock.
> > 
> > v3: Tejun suggested avoiding busy-looping by adding an msleep() since
> >     it is not guaranteed that we will get frozen immediately.
> > 
> > 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>
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
> Thanks a lot. :)

Applied to linux-pm/linux-next.

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>