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

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Thursday, February 02, 2012, Srivatsa S. Bhat wrote:
> On 02/03/2012 01:48 AM, Rafael J. Wysocki wrote:
> 
> > 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@linux.vnet.ibm.com>
> >>>> ---
> >>>>
> >>>>  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).
> >
> 
> 
> Yeah, even I had the same reasoning in mind when writing this patch.
> 
>  
> > So I think I'll apply your patch after all.
> > 
> 
> 
> :-)

However, this one should probably be regarded as a regression fix:

http://marc.info/?l=linux-kernel&m=132813572708843&w=4

because thawing all processes before calling swsusp_free() is definitely not
a good idea.

Thanks,
Rafael