| Message ID | 1367271946-7239-3-git-send-email-ccross@android.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
Hello, On Mon, Apr 29, 2013 at 02:45:38PM -0700, Colin Cross wrote: > If a task has called freezer_do_not_count(), don't bother waking it > up. If it happens to wake up later it will call freezer_count() and > immediately enter the refrigerator. > > Signed-off-by: Colin Cross <ccross@android.com> > --- > kernel/cgroup_freezer.c | 5 ++++- > kernel/power/process.c | 4 ++-- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c > index 75dda1e..406dd71 100644 > --- a/kernel/cgroup_freezer.c > +++ b/kernel/cgroup_freezer.c > @@ -331,8 +331,11 @@ static void freeze_cgroup(struct freezer *freezer) > struct task_struct *task; > > cgroup_iter_start(cgroup, &it); > - while ((task = cgroup_iter_next(cgroup, &it))) > + while ((task = cgroup_iter_next(cgroup, &it))) { > + if (freezer_should_skip(task)) > + continue; > freeze_task(task); > + } > cgroup_iter_end(cgroup, &it); I feel a bit weary of changes which try to optimize state checks for freezer because the synchronization rules are kinda fragile and things may not work reliably depending on who's testing the flag, and it has been subtly broken in various ways in the past (maybe even now). Can you please explain the benefits of this patch (in terms of actual overhead because not many use freezer_do_not_count()) and why this is correct? Thanks.
On Mon, Apr 29, 2013 at 02:51:57PM -0700, Tejun Heo wrote: > I feel a bit weary of changes which try to optimize state checks for > freezer because the synchronization rules are kinda fragile and things > may not work reliably depending on who's testing the flag, and it has > been subtly broken in various ways in the past (maybe even now). Can And BTW, this is why the function is only used when checking whether a task is frozen rather than to decide to issue freeze_task() on it, and it seems your change is correct now that we don't have per-task FREEZE flag but I can't say I like the change. I'd really like to keep things as dumb as possible for freezer. Thanks.
On Mon, Apr 29, 2013 at 2:57 PM, Tejun Heo <tj@kernel.org> wrote: > On Mon, Apr 29, 2013 at 02:51:57PM -0700, Tejun Heo wrote: >> I feel a bit weary of changes which try to optimize state checks for >> freezer because the synchronization rules are kinda fragile and things >> may not work reliably depending on who's testing the flag, and it has >> been subtly broken in various ways in the past (maybe even now). Can > > And BTW, this is why the function is only used when checking whether a > task is frozen rather than to decide to issue freeze_task() on it, and > it seems your change is correct now that we don't have per-task FREEZE > flag but I can't say I like the change. I'd really like to keep > things as dumb as possible for freezer. See the first patch in the series (which isn't available in the archive yet, so I can't link to it). The short version is that Android goes through suspend/resume very often (every few seconds when on a busy wifi network with the screen off), and a significant portion of the energy used to go in and out of suspend is spent in the freezer. This patch series takes the most common userspace sleep points and converts them to PF_FREEZER_SKIP, which reduces the number of context switches for every suspend or resume event on a freshly-booted Android device from 1000 to 25, and reduces the time spent freezing by a factor of 5. It will have a similar effect on a non-Android system, although those generally don't care about suspend/resume optimization. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey, On Mon, Apr 29, 2013 at 03:02:19PM -0700, Colin Cross wrote: > See the first patch in the series (which isn't available in the > archive yet, so I can't link to it). The short version is that It didn't arrive in my lkml folder either. Maybe vger is taking some time distributing emails. > Android goes through suspend/resume very often (every few seconds when > on a busy wifi network with the screen off), and a significant portion > of the energy used to go in and out of suspend is spent in the > freezer. This patch series takes the most common userspace sleep > points and converts them to PF_FREEZER_SKIP, which reduces the number > of context switches for every suspend or resume event on a > freshly-booted Android device from 1000 to 25, and reduces the time Ah, okay, so you're spreading PF_FREEZER_SKIP. When you post patches which touch the freezer can you please cc me and Oleg Nesterov <oleg@redhat.com> (I'll ping him this time)? Freezer has been very subtly broken in various ways and many kthread users are still broken, so let's tread carefully. > spent freezing by a factor of 5. It will have a similar effect on a > non-Android system, although those generally don't care about > suspend/resume optimization. Yeah, if it's something which makes actual difference rather than "this seems to be a good idea" thing, sure, let's find a way. Thanks.
On Mon, Apr 29, 2013 at 03:08:31PM -0700, Tejun Heo wrote: > > spent freezing by a factor of 5. It will have a similar effect on a > > non-Android system, although those generally don't care about > > suspend/resume optimization. > > Yeah, if it's something which makes actual difference rather than > "this seems to be a good idea" thing, sure, let's find a way. And a probably better approach would be rolling should_skip test into freeze_task() with a comment explaining the interlocking rather than adding should_skip test in its callers. Thanks.
On Monday, April 29, 2013 03:16:24 PM Tejun Heo wrote: > On Mon, Apr 29, 2013 at 03:08:31PM -0700, Tejun Heo wrote: > > > spent freezing by a factor of 5. It will have a similar effect on a > > > non-Android system, although those generally don't care about > > > suspend/resume optimization. > > > > Yeah, if it's something which makes actual difference rather than > > "this seems to be a good idea" thing, sure, let's find a way. > > And a probably better approach would be rolling should_skip test into > freeze_task() with a comment explaining the interlocking rather than > adding should_skip test in its callers. Agreed. Thanks, Rafael
On 04/29, Colin Cross wrote: > > @@ -46,10 +46,10 @@ static int try_to_freeze_tasks(bool user_only) > todo = 0; > read_lock(&tasklist_lock); > do_each_thread(g, p) { > - if (p == current || !freeze_task(p)) > + if (p == current || freezer_should_skip(p)) > continue; > > - if (!freezer_should_skip(p)) > + if (freeze_task(p)) > todo++; What if we race with freezer_count() ? try_to_freeze_tasks() can wrongly succeed leaving the running task unfrozen, no? Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/30, Oleg Nesterov wrote: > > On 04/29, Colin Cross wrote: > > > > @@ -46,10 +46,10 @@ static int try_to_freeze_tasks(bool user_only) > > todo = 0; > > read_lock(&tasklist_lock); > > do_each_thread(g, p) { > > - if (p == current || !freeze_task(p)) > > + if (p == current || freezer_should_skip(p)) > > continue; > > > > - if (!freezer_should_skip(p)) > > + if (freeze_task(p)) > > todo++; > > What if we race with freezer_count() ? > > try_to_freeze_tasks() can wrongly succeed leaving the running task > unfrozen, no? Ah, no, sorry for noise. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c index 75dda1e..406dd71 100644 --- a/kernel/cgroup_freezer.c +++ b/kernel/cgroup_freezer.c @@ -331,8 +331,11 @@ static void freeze_cgroup(struct freezer *freezer) struct task_struct *task; cgroup_iter_start(cgroup, &it); - while ((task = cgroup_iter_next(cgroup, &it))) + while ((task = cgroup_iter_next(cgroup, &it))) { + if (freezer_should_skip(task)) + continue; freeze_task(task); + } cgroup_iter_end(cgroup, &it); } diff --git a/kernel/power/process.c b/kernel/power/process.c index eb7e6c1..0680be2 100644 --- a/kernel/power/process.c +++ b/kernel/power/process.c @@ -46,10 +46,10 @@ static int try_to_freeze_tasks(bool user_only) todo = 0; read_lock(&tasklist_lock); do_each_thread(g, p) { - if (p == current || !freeze_task(p)) + if (p == current || freezer_should_skip(p)) continue; - if (!freezer_should_skip(p)) + if (freeze_task(p)) todo++; } while_each_thread(g, p); read_unlock(&tasklist_lock);
If a task has called freezer_do_not_count(), don't bother waking it up. If it happens to wake up later it will call freezer_count() and immediately enter the refrigerator. Signed-off-by: Colin Cross <ccross@android.com> --- kernel/cgroup_freezer.c | 5 ++++- kernel/power/process.c | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-)