* [PATCH] PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path
@ 2012-02-02 0:34 Srivatsa S. Bhat
2012-02-02 0:35 ` Tejun Heo
2012-02-02 19:11 ` Rafael J. Wysocki
0 siblings, 2 replies; 9+ messages in thread
From: Srivatsa S. Bhat @ 2012-02-02 0:34 UTC (permalink / raw)
To: rjw; +Cc: pavel, len.brown, tj, linux-pm, linux-kernel, srivatsa.bhat
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();
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;
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path
2012-02-02 0:34 [PATCH] PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path Srivatsa S. Bhat
@ 2012-02-02 0:35 ` Tejun Heo
2012-02-02 19:11 ` Rafael J. Wysocki
1 sibling, 0 replies; 9+ messages in thread
From: Tejun Heo @ 2012-02-02 0:35 UTC (permalink / raw)
To: Srivatsa S. Bhat; +Cc: rjw, pavel, len.brown, linux-pm, linux-kernel
On Wed, Feb 1, 2012 at 4:34 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> 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>
Acked-by: Tejun Heo <tj@kernel.org>
Thanks. :)
--
tejun
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path
2012-02-02 0:34 [PATCH] PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path Srivatsa S. Bhat
2012-02-02 0:35 ` Tejun Heo
@ 2012-02-02 19:11 ` Rafael J. Wysocki
2012-02-02 20:01 ` Srivatsa S. Bhat
1 sibling, 1 reply; 9+ messages in thread
From: Rafael J. Wysocki @ 2012-02-02 19:11 UTC (permalink / raw)
To: Srivatsa S. Bhat; +Cc: pavel, len.brown, tj, linux-pm, linux-kernel
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.
> 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?
Thanks,
Rafael
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path
2012-02-02 19:11 ` Rafael J. Wysocki
@ 2012-02-02 20:01 ` Srivatsa S. Bhat
2012-02-02 20:18 ` Rafael J. Wysocki
0 siblings, 1 reply; 9+ messages in thread
From: Srivatsa S. Bhat @ 2012-02-02 20:01 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: pavel, len.brown, tj, linux-pm, linux-kernel
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?
>> 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...
Regards,
Srivatsa S. Bhat
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path
2012-02-02 20:01 ` Srivatsa S. Bhat
@ 2012-02-02 20:18 ` Rafael J. Wysocki
2012-02-02 20:19 ` Srivatsa S. Bhat
0 siblings, 1 reply; 9+ messages in thread
From: Rafael J. Wysocki @ 2012-02-02 20:18 UTC (permalink / raw)
To: Srivatsa S. Bhat; +Cc: pavel, len.brown, tj, linux-pm, linux-kernel
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).
So I think I'll apply your patch after all.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path
2012-02-02 20:18 ` Rafael J. Wysocki
@ 2012-02-02 20:19 ` Srivatsa S. Bhat
2012-02-02 20:33 ` Rafael J. Wysocki
0 siblings, 1 reply; 9+ messages in thread
From: Srivatsa S. Bhat @ 2012-02-02 20:19 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: pavel, len.brown, tj, linux-pm, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path
2012-02-02 20:19 ` Srivatsa S. Bhat
@ 2012-02-02 20:33 ` Rafael J. Wysocki
2012-02-02 20:37 ` Srivatsa S. Bhat
0 siblings, 1 reply; 9+ messages in thread
From: Rafael J. Wysocki @ 2012-02-02 20:33 UTC (permalink / raw)
To: Srivatsa S. Bhat; +Cc: pavel, len.brown, tj, linux-pm, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path
2012-02-02 20:33 ` Rafael J. Wysocki
@ 2012-02-02 20:37 ` Srivatsa S. Bhat
2012-02-02 20:50 ` Rafael J. Wysocki
0 siblings, 1 reply; 9+ messages in thread
From: Srivatsa S. Bhat @ 2012-02-02 20:37 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: pavel, len.brown, tj, linux-pm, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path
2012-02-02 20:37 ` Srivatsa S. Bhat
@ 2012-02-02 20:50 ` Rafael J. Wysocki
0 siblings, 0 replies; 9+ messages in thread
From: Rafael J. Wysocki @ 2012-02-02 20:50 UTC (permalink / raw)
To: Srivatsa S. Bhat; +Cc: pavel, len.brown, tj, linux-pm, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-02-02 20:46 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-02 0:34 [PATCH] PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path Srivatsa S. Bhat
2012-02-02 0:35 ` Tejun Heo
2012-02-02 19:11 ` Rafael J. Wysocki
2012-02-02 20:01 ` Srivatsa S. Bhat
2012-02-02 20:18 ` Rafael J. Wysocki
2012-02-02 20:19 ` Srivatsa S. Bhat
2012-02-02 20:33 ` Rafael J. Wysocki
2012-02-02 20:37 ` Srivatsa S. Bhat
2012-02-02 20:50 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).