Re: [PATCH] PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path

From: Rafael J. Wysocki
Date: Thu Feb 02 2012 - 15:14:34 EST


On Thursday, February 02, 2012, Srivatsa S. Bhat wrote:
> On 02/03/2012 12:41 AM, Rafael J. Wysocki wrote:
>
> > On Thursday, February 02, 2012, Srivatsa S. Bhat wrote:
> >> In the hibernation call path, the kernel threads are frozen inside
> >> hibernation_snapshot(). If we happen to encounter an error further down
> >> the road or if we are exiting early due to a successful freezer test,
> >> then thaw kernel threads before returning to the caller.
> >>
> >> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>
> >> ---
> >>
> >> kernel/power/hibernate.c | 6 ++++--
> >> kernel/power/user.c | 8 ++------
> >> 2 files changed, 6 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> >> index a5d4cf0..c6dee73 100644
> >> --- a/kernel/power/hibernate.c
> >> +++ b/kernel/power/hibernate.c
> >> @@ -343,13 +343,13 @@ int hibernation_snapshot(int platform_mode)
> >> * successful freezer test.
> >> */
> >> freezer_test_done = true;
> >> - goto Cleanup;
> >> + goto Thaw;
> >> }
> >>
> >> error = dpm_prepare(PMSG_FREEZE);
> >> if (error) {
> >> dpm_complete(PMSG_RECOVER);
> >> - goto Cleanup;
> >> + goto Thaw;
> >> }
> >>
> >> suspend_console();
> >> @@ -385,6 +385,8 @@ int hibernation_snapshot(int platform_mode)
> >> platform_end(platform_mode);
> >> return error;
> >>
> >> + Thaw:
> >> + thaw_kernel_threads();
> >
> > Actaully, no. You have to do swsusp_free() before thawing, otherwise
> > some allocations made by the just thawed kernel threads may fail.
> >
>
>
> But then what about the case (in the existing code) when
> freeze_kernel_threads() fails? It would first thaw kernel threads (in
> fact it used to thaw even userspace tasks earlier!) before calling
> swsusp_free(). So, the existing code itself seems to be brittle, considering
> the point you raised. Right?

Well, that's why freeze_kernel_threads() originally hadn't tried to thaw anything
and started to do that after one of the Tejun's commits (and I forgot about
this particular issue back then).

> >> Cleanup:
> >> swsusp_free();
> >> goto Close;
> >> diff --git a/kernel/power/user.c b/kernel/power/user.c
> >> index 3e10007..7bee91f 100644
> >> --- a/kernel/power/user.c
> >> +++ b/kernel/power/user.c
> >> @@ -249,16 +249,12 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
> >> }
> >> pm_restore_gfp_mask();
> >> error = hibernation_snapshot(data->platform_support);
> >> - if (error) {
> >> - thaw_kernel_threads();
> >> - } else {
> >> + if (!error) {
> >> error = put_user(in_suspend, (int __user *)arg);
> >> if (!error && !freezer_test_done)
> >> data->ready = 1;
> >> - if (freezer_test_done) {
> >> + if (freezer_test_done)
> >> freezer_test_done = false;
> >> - thaw_kernel_threads();
> >> - }
> >> }
> >> break;
> >
> > Overall, this seems to be a cleanup, or is it a bug fix?
> >
>
>
> This was intended as a cleanup only, not a bug fix. But now, (see my point
> above), I am beginning to feel that the existing code itself is not robust
> enough...

Well, let's pretend that we think it is safe to thaw kernel threads before
freeing memory (actually, they are frozen after the preallocation, so it
really should be OK).

So I think I'll apply your patch after all.

Thanks,
Rafael
--
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/