[PATCH 02/10] freezer: skip waking up tasks with PF_FREEZER_SKIP set

[PATCH 02/10] freezer: skip waking up tasks with PF_FREEZER_SKIP set

[Colin Cross]

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-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
---
 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);
 }
 
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);
-- 
1.8.2.1

Re: [PATCH 02/10] freezer: skip waking up tasks with PF_FREEZER_SKIP set

[Tejun Heo]

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-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> ---
>  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.

-- 
tejun

Re: [PATCH 02/10] freezer: skip waking up tasks with PF_FREEZER_SKIP set

[Tejun Heo]

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.

-- 
tejun

Re: [PATCH 02/10] freezer: skip waking up tasks with PF_FREEZER_SKIP set

[Colin Cross]

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.

Re: [PATCH 02/10] freezer: skip waking up tasks with PF_FREEZER_SKIP set

[Tejun Heo]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> (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.

-- 
tejun

Re: [PATCH 02/10] freezer: skip waking up tasks with PF_FREEZER_SKIP set

[Tejun Heo]

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.

-- 
tejun

Re: [PATCH 02/10] freezer: skip waking up tasks with PF_FREEZER_SKIP set

[Rafael J. Wysocki]

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


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.