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


:-)

Regards,

Srivatsa S. Bhat