With the introduction of thaw_kernel_threads(), we can now finally fix the semantics of thawing tasks, in such a way as to be just the opposite of how tasks are frozen. This opens up some opportunities for code cleanup as well. This patchset implements both of these improvements. This patchset applies on top of the patch posted at https://lkml.org/lkml/2012/1/30/439 -- Srivatsa S. Bhat (4): PM/Freezer: Make thaw_processes() thaw only userspace tasks PM/Freezer: Use thaw_processes() and thaw_kernel_threads() correctly PM/Hibernate: Thaw kernel threads in hibernation_snapshot() in error/test path PM/Hibernate: Refactor and simplify freezer_test_done kernel/power/hibernate.c | 22 +++++++++++++++------- kernel/power/power.h | 15 +++++++++++++-- kernel/power/process.c | 13 ++++++------- kernel/power/user.c | 20 ++++++++++++-------- 4 files changed, 46 insertions(+), 24 deletions(-) Regards, Srivatsa S. Bhat IBM Linux Technology Center
Currently the situation is: freeze_processes() - freezes only userspace tasks freeze_kernel_threads() - freezes only kernel threads thaw_kernel_threads() - thaws only kernel threads thaw_processes() - thaws *everything* (both userspace tasks and kernel threads) The point that thaw_processes() thaws everything is rather unintuitive and can lead to bugs. So, modify thaw_processes() so that it thaws only userspace processes. This way we can also have more control over what exactly gets thawed in different situations. Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> --- kernel/power/process.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-) diff --git a/kernel/power/process.c b/kernel/power/process.c index eeca003..def6b1b 100644 --- a/kernel/power/process.c +++ b/kernel/power/process.c @@ -170,17 +170,15 @@ void thaw_processes(void) if (pm_freezing) atomic_dec(&system_freezing_cnt); pm_freezing = false; - pm_nosig_freezing = false; oom_killer_enable(); printk("Restarting tasks ... "); - thaw_workqueues(); - read_lock(&tasklist_lock); do_each_thread(g, p) { - __thaw_task(p); + if (!(p->flags & (PF_KTHREAD | PF_WQ_WORKER))) + __thaw_task(p); } while_each_thread(g, p); read_unlock(&tasklist_lock);
Now thaw_processes() thaws only userspace tasks (instead of thawing everything, unlike what it used to do before). So fix the uses of thaw_processes() by using thaw_kernel_threads() where appropriate. Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> --- kernel/power/hibernate.c | 5 +++++ kernel/power/power.h | 15 +++++++++++++-- kernel/power/process.c | 7 ++++--- kernel/power/user.c | 8 ++++++++ 4 files changed, 30 insertions(+), 5 deletions(-) diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c index a5d4cf0..1670387 100644 --- a/kernel/power/hibernate.c +++ b/kernel/power/hibernate.c @@ -656,6 +656,11 @@ int hibernate(void) } Thaw: + /* + * Kernel threads had been frozen inside hibernation_snapshot(). + * So thaw them here, on the success path. + */ + thaw_kernel_threads(); thaw_processes(); Finish: free_basic_memory_bitmaps(); diff --git a/kernel/power/power.h b/kernel/power/power.h index 0c4defe..e37a1e0 100644 --- a/kernel/power/power.h +++ b/kernel/power/power.h @@ -231,12 +231,23 @@ extern int pm_test_level; #ifdef CONFIG_SUSPEND_FREEZER static inline int suspend_freeze_processes(void) { - int error = freeze_processes(); - return error ? : freeze_kernel_threads(); + int error; + + error = freeze_processes(); + if (error) + goto Finish; + + error = freeze_kernel_threads(); + if (error) + thaw_processes(); + +Finish: + return error; } static inline void suspend_thaw_processes(void) { + thaw_kernel_threads(); thaw_processes(); } #else diff --git a/kernel/power/process.c b/kernel/power/process.c index def6b1b..8ae6c4b 100644 --- a/kernel/power/process.c +++ b/kernel/power/process.c @@ -116,7 +116,8 @@ static int try_to_freeze_tasks(bool user_only) /** * freeze_processes - Signal user space processes to enter the refrigerator. * - * On success, returns 0. On failure, -errno and system is fully thawed. + * On success, returns 0. On failure, -errno and all user space processes + * are thawed. */ int freeze_processes(void) { @@ -143,7 +144,7 @@ int freeze_processes(void) /** * freeze_kernel_threads - Make freezable kernel threads go to the refrigerator. * - * On success, returns 0. On failure, -errno and system is fully thawed. + * On success, returns 0. On failure, -errno and all kernel threads are thawed. */ int freeze_kernel_threads(void) { @@ -159,7 +160,7 @@ int freeze_kernel_threads(void) BUG_ON(in_atomic()); if (error) - thaw_processes(); + thaw_kernel_threads(); return error; } diff --git a/kernel/power/user.c b/kernel/power/user.c index 3e10007..eaf5c97 100644 --- a/kernel/power/user.c +++ b/kernel/power/user.c @@ -119,6 +119,7 @@ static int snapshot_release(struct inode *inode, struct file *filp) free_all_swap_pages(data->swap); if (data->frozen) { pm_restore_gfp_mask(); + thaw_kernel_threads(); thaw_processes(); } pm_notifier_call_chain(data->mode == O_RDONLY ? @@ -237,6 +238,13 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd, if (!data->frozen || data->ready) break; pm_restore_gfp_mask(); + + /* + * Be paranoid and thaw kernel threads as well, in order to + * make sure that the system gets thawed fully, irrespective + * of which (error) path we took to come here. + */ + thaw_kernel_threads(); thaw_processes(); usermodehelper_enable(); data->frozen = 0;
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 | 9 ++++++--- kernel/power/user.c | 8 ++------ 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c index 1670387..0ce1220 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; @@ -655,12 +657,13 @@ int hibernate(void) pr_debug("PM: Image restored successfully.\n"); } - Thaw: /* * Kernel threads had been frozen inside hibernation_snapshot(). * So thaw them here, on the success path. */ thaw_kernel_threads(); + + Thaw: thaw_processes(); Finish: free_basic_memory_bitmaps(); diff --git a/kernel/power/user.c b/kernel/power/user.c index eaf5c97..62dbe93 100644 --- a/kernel/power/user.c +++ b/kernel/power/user.c @@ -257,16 +257,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;
The code related to 'freezer_test_done' is needlessly convoluted. Refactor the code and simplify the implementation. Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> --- kernel/power/hibernate.c | 10 +++++----- kernel/power/user.c | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c index 0ce1220..30b315a 100644 --- a/kernel/power/hibernate.c +++ b/kernel/power/hibernate.c @@ -629,12 +629,8 @@ int hibernate(void) goto Finish; error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM); - if (error) - goto Thaw; - if (freezer_test_done) { - freezer_test_done = false; + if (error || freezer_test_done) goto Thaw; - } if (in_suspend) { unsigned int flags = 0; @@ -665,6 +661,10 @@ int hibernate(void) Thaw: thaw_processes(); + + /* Don't bother checking whether freezer_test_done is true */ + freezer_test_done = false; + Finish: free_basic_memory_bitmaps(); usermodehelper_enable(); diff --git a/kernel/power/user.c b/kernel/power/user.c index 62dbe93..056088e 100644 --- a/kernel/power/user.c +++ b/kernel/power/user.c @@ -259,10 +259,10 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd, error = hibernation_snapshot(data->platform_support); if (!error) { error = put_user(in_suspend, (int __user *)arg); - if (!error && !freezer_test_done) - data->ready = 1; if (freezer_test_done) freezer_test_done = false; + else if (!error) + data->ready = 1; } break;
On Tue, Jan 31, 2012 at 03:34:57AM +0530, Srivatsa S. Bhat wrote:
> Currently the situation is:
>
> freeze_processes() - freezes only userspace tasks
> freeze_kernel_threads() - freezes only kernel threads
> thaw_kernel_threads() - thaws only kernel threads
> thaw_processes() - thaws *everything* (both userspace tasks and kernel threads)
>
> The point that thaw_processes() thaws everything is rather unintuitive
> and can lead to bugs. So, modify thaw_processes() so that it thaws only
> userspace processes. This way we can also have more control over what
> exactly gets thawed in different situations.
>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Maybe I'm misreading it but doesn't this introduce window where kernel
tasks aren't thawed between this patch and the following ones? It
looks like this one should come later.
Thanks.
--
tejun
On 01/31/2012 03:39 AM, Tejun Heo wrote:
> On Tue, Jan 31, 2012 at 03:34:57AM +0530, Srivatsa S. Bhat wrote:
>> Currently the situation is:
>>
>> freeze_processes() - freezes only userspace tasks
>> freeze_kernel_threads() - freezes only kernel threads
>> thaw_kernel_threads() - thaws only kernel threads
>> thaw_processes() - thaws *everything* (both userspace tasks and kernel threads)
>>
>> The point that thaw_processes() thaws everything is rather unintuitive
>> and can lead to bugs. So, modify thaw_processes() so that it thaws only
>> userspace processes. This way we can also have more control over what
>> exactly gets thawed in different situations.
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>
> Maybe I'm misreading it but doesn't this introduce window where kernel
> tasks aren't thawed between this patch and the following ones? It
> looks like this one should come later.
>
Yes, I was aware that it introduces such a window. But I ignored it in the
interest of making the patch series sensible (as in, for example, patch
2/4 wouldn't make much sense without patch 1/4).
Maybe I will interchange patch 1 and patch 2 and just reword the patch
descriptions suitably so that they still make sense..
Thanks a lot!
Regards,
Srivatsa S. Bhat
On Tue, Jan 31, 2012 at 03:53:05AM +0530, Srivatsa S. Bhat wrote:
> Yes, I was aware that it introduces such a window. But I ignored it in the
> interest of making the patch series sensible (as in, for example, patch
> 2/4 wouldn't make much sense without patch 1/4).
> Maybe I will interchange patch 1 and patch 2 and just reword the patch
> descriptions suitably so that they still make sense..
Yeah, I think saying sth like "Make extra calls to thaw_kthread in
preparation for blah blah.." should do.
Thanks.
--
tejun