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

From: Rafael J. Wysocki
Date: Wed Jan 18 2012 - 17:19:31 EST


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

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

Also, have you actually tested this?

Rafael


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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/