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 02:03 AM, Rafael J. Wysocki wrote:
> 
> > 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.
> > 
> 
> 
> Right, I agree. So we need to get this into stable as well then..

No, we don't, this is a 3.3 merge window fallout.

> Do you want me to re-spin the patch with the commit message explicitly stating
> that this is a regression fix and so on or is the current patch good enough?

Yes, it would be nice to rework the changelog to reflect the real importance
of the patch.

Thanks,
Rafael