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

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

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..
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?

Regards,
Srivatsa S. Bhat