* [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