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

From: Srivatsa S. Bhat
Date: Thu Feb 02 2012 - 15:38:31 EST


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

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