power: set PF_SUSPEND_TASK flag on tasks that call freeze_processes
Subsystems (4)
PATCH 1/1 power: set PF_SUSPEND_TASK flag on tasks that call freeze_processes
Calling freeze_processes sets a global flag that will cause any process that calls try_to_freeze to enter the refrigerator. It skips sending a signal to the current task, but if the current task ever hits try_to_freeze all threads will be frozen and the system will deadlock.
Set a new flag, PF_SUSPEND_TASK, on the task that calls freeze_processes. The flag notifies the freezer that the thread is involved in suspend and should not be frozen. Also add a WARN_ON in thaw_processes if the caller does not have the PF_SUSPEND_TASK flag set to catch if a different task calls thaw_processes than the one that called freeze_processes, leaving a task with PF_SUSPEND_TASK permanently set on it.
Threads that spawn off a task with PF_SUSPEND_TASK set (which swsusp does) will also have PF_SUSPEND_TASK set, preventing them from freezing while they are helping with suspend, but they need to be dead by the time suspend is triggered, otherwise they may run when userspace is expected to be frozen. Add a WARN_ON in thaw_processes if more than one thread has the PF_SUSPEND_TASK flag set.
---
Resending not as an attachment for review.
If the extra process flag is considered too precious for this (there are only 2 left after this patch) I could get the same functionality by having freeze_processes() reject calls from a PF_KTHREAD|PF_NOFREEZE thread, and use PF_KTHREAD to determine if PF_NOFREEZE should be cleared in thaw_processes().
include/linux/sched.h | 1 + kernel/freezer.c | 2 +- kernel/power/process.c | 11 +++++++++++ 3 files changed, 13 insertions(+), 1 deletion(-)
include/linux/sched.h +1 -0
@@ -1628,6 +1628,7 @@ extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t *ut,#define PF_MEMPOLICY 0x10000000/* Non-default NUMA mempolicy */ #define PF_MUTEX_TESTER 0x20000000/* Thread belongs to the rt mutex tester */ #define PF_FREEZER_SKIP 0x40000000/* Freezer should not count it as freezable */ +#define PF_SUSPEND_TASK 0x80000000/* this thread called freeze_processes and should not be frozen */ /* * Only the _current_ task can read/write to tsk->flags, but other
kernel/freezer.c +1 -1
@@ -33,7 +33,7 @@ static DEFINE_SPINLOCK(freezer_lock);*/ bool freezing_slow_path (struct task_struct *p) { -if (p->flags &PF_NOFREEZE ) +if (p->flags & (PF_NOFREEZE |PF_SUSPEND_TASK ))return false;if (pm_nosig_freezing ||cgroup_freezing (p))
kernel/power/process.c +11 -0
@@ -109,6 +109,8 @@ static int try_to_freeze_tasks(bool user_only)/** * freeze_processes - Signal user space processes to enter the refrigerator. +* The current thread will not be frozen. The same process that calls +* freeze_processes must later call thaw_processes. * * On success, returns 0. On failure, -errno and system is fully thawed. */
@@ -120,6 +122,9 @@ int freeze_processes(void)if (error)return error; +/* Make sure this task doesn't get frozen */ + current->flags |=PF_SUSPEND_TASK ; +if (!pm_freezing)atomic_inc (&system_freezing_cnt);
@@ -168,6 +173,7 @@ int freeze_kernel_threads(void)void thaw_processes (void ) {struct task_struct *g, *p; +struct task_struct *curr = current;if (pm_freezing)atomic_dec (&system_freezing_cnt);
@@ -182,10 +188,15 @@ void thaw_processes(void)read_lock (&tasklist_lock); do_each_thread(g, p) { +/* No other threads should have PF_SUSPEND_TASK set */ +WARN_ON ((p != curr) && (p->flags &PF_SUSPEND_TASK ));__thaw_task (p); }while_each_thread (g, p);read_unlock (&tasklist_lock); +WARN_ON (!(curr->flags &PF_SUSPEND_TASK )); + curr->flags &= ~PF_SUSPEND_TASK ; +usermodehelper_enable ();schedule ();
Signature · 1 line
1.8.3
Quoted 22 lines · Colin Cross
Calling
freeze_processessets a global flag that will cause any process that callstry_to_freezeto enter the refrigerator. It skips sending a signal to the current task, but if the current task ever hitstry_to_freezeall threads will be frozen and the system will deadlock.Set a new flag,
PF_SUSPEND_TASK, on the task that callsfreeze_processes. The flag notifies the freezer that the thread is involved in suspend and should not be frozen. Also add aWARN_ONinthaw_processesif the caller does not have thePF_SUSPEND_TASKflag set to catch if a different task callsthaw_processesthan the one that calledfreeze_processes, leaving a task withPF_SUSPEND_TASKpermanently set on it.Threads that spawn off a task with
PF_SUSPEND_TASKset (which swsusp does) will also havePF_SUSPEND_TASKset, preventing them from freezing while they are helping with suspend, but they need to be dead by the time suspend is triggered, otherwise they may run when userspace is expected to be frozen. Add aWARN_ONinthaw_processesif more than one thread has thePF_SUSPEND_TASKflag set.
Quoted 6 lines
Reported-by Michael Leun <[email protected]>Tested-by Michael Leun <[email protected]>Signed-off-by Colin Cross <[email protected]>---
Resending not as an attachment for review.
I like this, but I wonder what other people think.
Quoted 5 lines
If the extra process flag is considered too precious for this (there are only 2 left after this patch) I could get the same functionality by having
freeze_processes()reject calls from a PF_KTHREAD|PF_NOFREEZE thread, and usePF_KTHREADto determine ifPF_NOFREEZEshould be cleared inthaw_processes().
Can we spend an extra process flag on that?
Rafael
Quoted 78 lines
include/linux/sched.h | 1 + kernel/freezer.c | 2 +- kernel/power/process.c | 11 +++++++++++ 3 files changed, 13 insertions(+), 1 deletion(-)include/linux/sched.h +1
-0@@ -1628,6 +1628,7 @@ extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t *ut,#define PF_MEMPOLICY 0x10000000/* Non-default NUMA mempolicy */ #define PF_MUTEX_TESTER 0x20000000/* Thread belongs to the rt mutex tester */ #define PF_FREEZER_SKIP 0x40000000/* Freezer should not count it as freezable */ +#define PF_SUSPEND_TASK 0x80000000/* this thread called freeze_processes and should not be frozen */ /* * Only the _current_ task can read/write to tsk->flags, but other kernel/freezer.c +1
-1@@ -33,7 +33,7 @@ static DEFINE_SPINLOCK(freezer_lock);*/ bool freezing_slow_path (struct task_struct *p) { -if (p->flags &PF_NOFREEZE ) +if (p->flags & (PF_NOFREEZE |PF_SUSPEND_TASK ))return false;if (pm_nosig_freezing ||cgroup_freezing (p))kernel/power/process.c +11
-0@@ -109,6 +109,8 @@ static int try_to_freeze_tasks(bool user_only)/** * freeze_processes - Signal user space processes to enter the refrigerator. +* The current thread will not be frozen. The same process that calls +* freeze_processes must later call thaw_processes. * * On success, returns 0. On failure, -errno and system is fully thawed. */ @@ -120,6 +122,9 @@ int freeze_processes(void)if (error)return error; +/* Make sure this task doesn't get frozen */ + current->flags |=PF_SUSPEND_TASK ; +if (!pm_freezing)atomic_inc (&system_freezing_cnt);@@ -168,6 +173,7 @@ int freeze_kernel_threads(void)void thaw_processes (void ) {struct task_struct *g, *p; +struct task_struct *curr = current;if (pm_freezing)atomic_dec (&system_freezing_cnt);@@ -182,10 +188,15 @@ void thaw_processes(void)read_lock (&tasklist_lock); do_each_thread(g, p) { +/* No other threads should have PF_SUSPEND_TASK set */ +WARN_ON ((p != curr) && (p->flags &PF_SUSPEND_TASK ));__thaw_task (p); }while_each_thread (g, p);read_unlock (&tasklist_lock); +WARN_ON (!(curr->flags &PF_SUSPEND_TASK )); + curr->flags &= ~PF_SUSPEND_TASK ; +usermodehelper_enable ();schedule ();
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Quoted 2 lines · Rafael J. Wysocki
Can we spend an extra process flag on that?
I suspect we can. But if somebody hollers later, we'll need to do some compaction..
Linus
Quoted 6 lines · Linus Torvalds
Quoted 2 lines · Rafael J. Wysocki
Can we spend an extra process flag on that?
I suspect we can. But if somebody hollers later, we'll need to do some compaction..
OK, if we need that flag for something more important, it shouldn't be too difficult to recycle.
And I'm not seeing any complaints, so I'm going to fast-track this, as it fixes a recent regression.
Rafael