From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
To: Tejun Heo <tj@kernel.org>
Cc: rjw@sisk.pl, pavel@ucw.cz, lenb@kernel.org, ak@linux.intel.com,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
linux-mm@kvack.org
Subject: Re: [PATCH] PM/Memory-hotplug: Avoid task freezing failures
Date: Tue, 15 Nov 2011 12:38:54 +0530 [thread overview]
Message-ID: <4EC21006.7030101@linux.vnet.ibm.com> (raw)
In-Reply-To: <20111114200506.GE30922@google.com>
On 11/15/2011 01:35 AM, Tejun Heo wrote:
> Hello,
>
> On Thu, Nov 10, 2011 at 10:12:43PM +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.
>>
>> This patch fixes this issue by modifying lock_system_sleep() to use
>> mutex_lock_interruptible() instead of mutex_lock(), so that it blocks in the
>> TASK_INTERRUPTIBLE state. This would allow the freezer to freeze the blocked
>> task. Also, since the freezer could use signals to freeze tasks, it is quite
>> likely that mutex_lock_interruptible() returns -EINTR (and fails to acquire
>> the lock). Hence we keep retrying in a loop until we acquire the lock. Also,
>> we call try_to_freeze() within the loop, so that we don't cause freezing
>> failures due to busy looping.
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ...
>> static inline void lock_system_sleep(void)
>> {
>> - mutex_lock(&pm_mutex);
>> + /*
>> + * We should not use mutex_lock() here because, in case we fail to
>> + * acquire the lock, it would put us to sleep in TASK_UNINTERRUPTIBLE
>> + * state, which would lead to task freezing failures. As a
>> + * consequence, hibernation would fail (even though it had acquired
>> + * the 'pm_mutex' lock).
>> + *
>> + * Note that mutex_lock_interruptible() returns -EINTR if we happen
>> + * to get a signal when we are waiting to acquire the lock (and this
>> + * is very likely here because the freezer could use signals to freeze
>> + * tasks). Hence we have to keep retrying until we get the lock. But
>> + * we have to use try_to_freeze() in the loop, so that we don't cause
>> + * freezing failures due to busy looping.
>> + */
>> + while (mutex_lock_interruptible(&pm_mutex))
>> + try_to_freeze();
>
> Hmmm... is this a problem that we need to worry about? If not, I'm
> not sure this is a good idea. What if the task calling
> lock_system_sleep() is a userland one and has actual outstanding
> signal? It would busy spin until it acquire pm_mutex. Maybe that's
> okay too given how pm_mutex is used but it's still nasty. If this
> isn't a real problem, maybe leave this alone for now?
>
Hi Tejun,
Thank you very much for taking a look. I haven't encountered this problem,
but while going through the code, I felt this would be problematic for
hibernation.
The other reason I was looking into this was, to make use of this API in
another scenario (specifically, the x86 microcode update module
initialization, where I couldn't think of any other solution to block
suspend) by making this API (lock_system_sleep) a bit more generic so that
it works for suspend also. So I thought of fixing it up before doing that.
Considering the point you have raised, how about something like this?
while (!mutex_trylock(&pm_mutex))
try_to_freeze();
This should address your concern right?
At first sight, I felt this looked quite hacky.. but after trying to
understand the meaning it conveys, it struck me that this version of the
code seems to itself convey what exactly we are trying to achieve/avoid
here :-)
By the way, to tell the truth, I don't really like this way of blocking
suspend/hibernation by grabbing the 'pm_mutex' lock directly.
Probably the design that Rafael and Alan Stern are working out would be a
cleaner solution in the long run, for subsystems to block system sleep.
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>
prev parent reply other threads:[~2011-11-15 7:52 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-11-10 16:42 [PATCH] PM/Memory-hotplug: Avoid task freezing failures Srivatsa S. Bhat
2011-11-14 20:05 ` Tejun Heo
2011-11-15 7:08 ` Srivatsa S. Bhat [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4EC21006.7030101@linux.vnet.ibm.com \
--to=srivatsa.bhat@linux.vnet.ibm.com \
--cc=ak@linux.intel.com \
--cc=lenb@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-pm@vger.kernel.org \
--cc=pavel@ucw.cz \
--cc=rjw@sisk.pl \
--cc=tj@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).