[PATCH 00/10] optimize freezing tasks by reducing task wakeups

[PATCH 00/10] optimize freezing tasks by reducing task wakeups

[Colin Cross]

On slow cpus the large number of task wakeups and context switches
triggered by freezing and thawing tasks can take a significant amount
of cpu time.  This patch series reduces the amount of work done during
freezing tasks by avoiding waking up tasks that are already in a freezable
state.

The first patch reduces the wasted time in try_to_freeze_tasks() by
starting with a 1 ms sleep during the first loop and backing off
up to an 8 ms sleep if all tasks are not frozen.

The second patch modifies the try_to_freeze_tasks() loop to skip tasks
that have set the PF_FREEZER_SKIP flag by calling freezer_do_not_count().
These tasks will not enter the refrigerator during the suspend/resume
cycle unless they woken up by something else, in which case they will
enter the refrigerator in freezer_count() before they access any
resources that would not be available in suspend or deadlock with
another freezing/frozen task.

The rest of the series adds a few more freezable helpers and converts the
top call sites that userspace tasks are usually blocked at to freezable
helpers.  The list of call sites was collected on a Nexus 10 (ARM Exynos
5250 SoC), but all the top call sites other than binder show up at the
top of the list on Ubuntu x86-64 as well.

This series cuts the time for freezing tasks from 50 ms to 5 ms when
the cpu speed is locked at its lowest setting (200MHz), and reduces
the number of context switches and restarted syscalls from 1000 to
25.

[PATCH 01/10] freezer: shorten freezer sleep time using exponential backoff

[Colin Cross]

All tasks can easily be frozen in under 10 ms, switch to using
an initial 1 ms sleep followed by exponential backoff until
8 ms.  Also convert the printed time to ms instead of centiseconds.

Signed-off-by: Colin Cross <ccross@android.com>
---
 kernel/power/process.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/kernel/power/process.c b/kernel/power/process.c
index 98088e0..eb7e6c1 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -30,9 +30,10 @@ static int try_to_freeze_tasks(bool user_only)
 	unsigned int todo;
 	bool wq_busy = false;
 	struct timeval start, end;
-	u64 elapsed_csecs64;
-	unsigned int elapsed_csecs;
+	u64 elapsed_msecs64;
+	unsigned int elapsed_msecs;
 	bool wakeup = false;
+	int sleep_usecs = USEC_PER_MSEC;
 
 	do_gettimeofday(&start);
 
@@ -70,20 +71,22 @@ static int try_to_freeze_tasks(bool user_only)
 		 * We need to retry, but first give the freezing tasks some
 		 * time to enter the refrigerator.
 		 */
-		msleep(10);
+		usleep_range(sleep_usecs / 2, sleep_usecs);
+		if (sleep_usecs < 8 * USEC_PER_MSEC)
+			sleep_usecs *= 2;
 	}
 
 	do_gettimeofday(&end);
-	elapsed_csecs64 = timeval_to_ns(&end) - timeval_to_ns(&start);
-	do_div(elapsed_csecs64, NSEC_PER_SEC / 100);
-	elapsed_csecs = elapsed_csecs64;
+	elapsed_msecs64 = timeval_to_ns(&end) - timeval_to_ns(&start);
+	do_div(elapsed_msecs64, NSEC_PER_MSEC);
+	elapsed_msecs = elapsed_msecs64;
 
 	if (todo) {
 		printk("\n");
-		printk(KERN_ERR "Freezing of tasks %s after %d.%02d seconds "
+		printk(KERN_ERR "Freezing of tasks %s after %d.%03d seconds "
 		       "(%d tasks refusing to freeze, wq_busy=%d):\n",
 		       wakeup ? "aborted" : "failed",
-		       elapsed_csecs / 100, elapsed_csecs % 100,
+		       elapsed_msecs / 1000, elapsed_msecs % 1000,
 		       todo - wq_busy, wq_busy);
 
 		if (!wakeup) {
@@ -96,8 +99,8 @@ static int try_to_freeze_tasks(bool user_only)
 			read_unlock(&tasklist_lock);
 		}
 	} else {
-		printk("(elapsed %d.%02d seconds) ", elapsed_csecs / 100,
-			elapsed_csecs % 100);
+		printk("(elapsed %d.%03d seconds) ", elapsed_msecs / 1000,
+			elapsed_msecs % 1000);
 	}
 
 	return todo ? -EBUSY : 0;
-- 
1.8.2.1

Re: [PATCH 01/10] freezer: shorten freezer sleep time using exponential backoff

[Pavel Machek]

On Mon 2013-04-29 14:45:37, Colin Cross wrote:
> All tasks can easily be frozen in under 10 ms, switch to using
> an initial 1 ms sleep followed by exponential backoff until
> 8 ms.  Also convert the printed time to ms instead of centiseconds.
> 
> Signed-off-by: Colin Cross <ccross@android.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[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@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);
 }
 
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@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.

-- 
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@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.

-- 
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.

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

[Oleg Nesterov]

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.

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

[Oleg Nesterov]

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.

[PATCH 03/10] freezer: add new freezable helpers using freezer_do_not_count()

[Colin Cross]

Freezing tasks will wake up almost every userspace task from
where it is blocking and force it to run until it hits a
call to try_to_sleep(), generally on the exit path from the syscall
it is blocking in.  On resume each task will run again, usually
restarting the syscall and running until it hits the same
blocking call as it was originally blocked in.

To allow tasks to avoid running on every suspend/resume cycle,
this patch adds additional freezable wrappers around blocking calls
that call freezer_do_not_count().  Combined with the previous patch,
these tasks will not run during suspend or resume unless they wake
up for another reason, in which case they will run until they hit
the try_to_freeze() in freezer_count(), and then continue processing
the wakeup after tasks are thawed.  This patch also converts the
existing wait_event_freezable* wrappers to use freezer_do_not_count().

Additional patches will convert the most common locations that
userspace blocks in to use freezable helpers.

Signed-off-by: Colin Cross <ccross@android.com>
---
 include/linux/freezer.h | 72 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 14 deletions(-)

diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index e70df40..b239f45 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -152,6 +152,26 @@ static inline bool freezer_should_skip(struct task_struct *p)
 	freezer_count();						\
 })
 
+/* Like schedule_timeout(), but should not block the freezer. */
+#define freezable_schedule_timeout(timeout)				\
+({									\
+	long __retval;							\
+	freezer_do_not_count();						\
+	__retval = schedule_timeout(timeout);				\
+	freezer_count();						\
+	__retval;							\
+})
+
+/* Like schedule_timeout_interruptible(), but should not block the freezer. */
+#define freezable_schedule_timeout_interruptible(timeout)		\
+({									\
+	long __retval;							\
+	freezer_do_not_count();						\
+	__retval = schedule_timeout_interruptible(timeout);		\
+	freezer_count();						\
+	__retval;							\
+})
+
 /* Like schedule_timeout_killable(), but should not block the freezer. */
 #define freezable_schedule_timeout_killable(timeout)			\
 ({									\
@@ -162,6 +182,17 @@ static inline bool freezer_should_skip(struct task_struct *p)
 	__retval;							\
 })
 
+/* Like schedule_hrtimeout_range(), but should not block the freezer. */
+#define freezable_schedule_hrtimeout_range(expires, delta, mode)	\
+({									\
+	int __retval;							\
+	freezer_do_not_count();						\
+	__retval = schedule_hrtimeout_range(expires, delta, mode);	\
+	freezer_count();						\
+	__retval;							\
+})
+
+
 /*
  * Freezer-friendly wrappers around wait_event_interruptible(),
  * wait_event_killable() and wait_event_interruptible_timeout(), originally
@@ -180,30 +211,32 @@ static inline bool freezer_should_skip(struct task_struct *p)
 #define wait_event_freezable(wq, condition)				\
 ({									\
 	int __retval;							\
-	for (;;) {							\
-		__retval = wait_event_interruptible(wq, 		\
-				(condition) || freezing(current));	\
-		if (__retval || (condition))				\
-			break;						\
-		try_to_freeze();					\
-	}								\
+	freezer_do_not_count();						\
+	__retval = wait_event_interruptible(wq, (condition));		\
+	freezer_count();						\
 	__retval;							\
 })
 
 #define wait_event_freezable_timeout(wq, condition, timeout)		\
 ({									\
 	long __retval = timeout;					\
-	for (;;) {							\
-		__retval = wait_event_interruptible_timeout(wq,		\
-				(condition) || freezing(current),	\
+	freezer_do_not_count();						\
+	__retval = wait_event_interruptible_timeout(wq,	(condition),	\
 				__retval); 				\
-		if (__retval <= 0 || (condition))			\
-			break;						\
-		try_to_freeze();					\
-	}								\
+	freezer_count();						\
 	__retval;							\
 })
 
+#define wait_event_freezable_exclusive(wq, condition)		\
+({									\
+	int __retval;							\
+	freezer_do_not_count();						\
+	__retval = wait_event_interruptible_exclusive(wq, condition);	\
+	freezer_count();						\
+	__retval;							\
+})
+
+
 #else /* !CONFIG_FREEZER */
 static inline bool frozen(struct task_struct *p) { return false; }
 static inline bool freezing(struct task_struct *p) { return false; }
@@ -225,15 +258,26 @@ static inline void set_freezable(void) {}
 
 #define freezable_schedule()  schedule()
 
+#define freezable_schedule_timeout(timeout)  schedule_timeout(timeout)
+
+#define freezable_schedule_timeout_interruptible(timeout)		\
+	schedule_timeout_interruptible(timeout)
+
 #define freezable_schedule_timeout_killable(timeout)			\
 	schedule_timeout_killable(timeout)
 
+#define freezable_schedule_hrtimeout_range(expires, delta, mode)	\
+	schedule_hrtimeout_range(expires, delta, mode)
+
 #define wait_event_freezable(wq, condition)				\
 		wait_event_interruptible(wq, condition)
 
 #define wait_event_freezable_timeout(wq, condition, timeout)		\
 		wait_event_interruptible_timeout(wq, condition, timeout)
 
+#define wait_event_freezable_exclusive(wq, condition)			\
+		wait_event_interruptible_exclusive(wq, condition)
+
 #define wait_event_freezekillable(wq, condition)		\
 		wait_event_killable(wq, condition)
 
-- 
1.8.2.1

Re: [PATCH 03/10] freezer: add new freezable helpers using freezer_do_not_count()

[Pavel Machek]

On Mon 2013-04-29 14:45:39, Colin Cross wrote:
> Freezing tasks will wake up almost every userspace task from
> where it is blocking and force it to run until it hits a
> call to try_to_sleep(), generally on the exit path from the syscall
> it is blocking in.  On resume each task will run again, usually
> restarting the syscall and running until it hits the same
> blocking call as it was originally blocked in.

Ok, so you are optimizing suspend at the cost of runtime operations,
right?

Would it make sense to do suspends entirely without freezer in your
configurations? With the right drivers, it should work ok.

Actually, would it make sense to simply enter deep idle and do runtime
powersaving in the drivers... n900 does that rather successfully and
it is certainly cleaner design.

> +/* Like schedule_timeout(), but should not block the freezer. */
> +#define freezable_schedule_timeout(timeout)				\
> +({									\
> +	long __retval;							\
> +	freezer_do_not_count();						\
> +	__retval = schedule_timeout(timeout);				\
> +	freezer_count();						\
> +	__retval;							\
> +})

You plan to use this a lot during normal operation, right? Is it going
to have some measureable impact?

Also, why not static inline?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 03/10] freezer: add new freezable helpers using freezer_do_not_count()

[Oliver Neukum]

On Thursday 02 May 2013 14:48:26 Pavel Machek wrote:
> On Mon 2013-04-29 14:45:39, Colin Cross wrote:
> > Freezing tasks will wake up almost every userspace task from
> > where it is blocking and force it to run until it hits a
> > call to try_to_sleep(), generally on the exit path from the syscall
> > it is blocking in.  On resume each task will run again, usually
> > restarting the syscall and running until it hits the same
> > blocking call as it was originally blocked in.
> 
> Ok, so you are optimizing suspend at the cost of runtime operations,
> right?
> 
> Would it make sense to do suspends entirely without freezer in your
> configurations? With the right drivers, it should work ok.

Right now drivers now that they will not be busy when runtime
suspend happens. The freezer has the same effect for system PM.
If you remove that certainty it becomes impossible for simple drivers
to declare their devices busy upon open and do no synchronization
between IO and PM.

	Regards
		Oliver

Re: [PATCH 03/10] freezer: add new freezable helpers using freezer_do_not_count()

[Pavel Machek]

On Thu 2013-05-02 15:05:13, Oliver Neukum wrote:
> On Thursday 02 May 2013 14:48:26 Pavel Machek wrote:
> > On Mon 2013-04-29 14:45:39, Colin Cross wrote:
> > > Freezing tasks will wake up almost every userspace task from
> > > where it is blocking and force it to run until it hits a
> > > call to try_to_sleep(), generally on the exit path from the syscall
> > > it is blocking in.  On resume each task will run again, usually
> > > restarting the syscall and running until it hits the same
> > > blocking call as it was originally blocked in.
> > 
> > Ok, so you are optimizing suspend at the cost of runtime operations,
> > right?
> > 
> > Would it make sense to do suspends entirely without freezer in your
> > configurations? With the right drivers, it should work ok.
> 
> Right now drivers now that they will not be busy when runtime
> suspend happens. The freezer has the same effect for system PM.
> If you remove that certainty it becomes impossible for simple drivers
> to declare their devices busy upon open and do no synchronization
> between IO and PM.

I know. Drivers can mostly ignore suspend.

But android people do suspend multiple times a second, and are
optimizing freezer. I'm suggesting they should take a look at their
drivers, and perhaps they can optimize freezer out totally.

(Or perhaps switch to n900-like solution, and avoid suspend
entirely. They keep machines functioning in suspend mode. That aint no
suspend.)
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 03/10] freezer: add new freezable helpers using freezer_do_not_count()

[Colin Cross]

On Thu, May 2, 2013 at 5:48 AM, Pavel Machek <pavel@ucw.cz> wrote:
> On Mon 2013-04-29 14:45:39, Colin Cross wrote:
>> Freezing tasks will wake up almost every userspace task from
>> where it is blocking and force it to run until it hits a
>> call to try_to_sleep(), generally on the exit path from the syscall
>> it is blocking in.  On resume each task will run again, usually
>> restarting the syscall and running until it hits the same
>> blocking call as it was originally blocked in.
>
> Ok, so you are optimizing suspend at the cost of runtime operations,
> right?
The runtime operations are negligible, it is setting a bit before
blocking, and then clearing the bit and checking a single global
counter as the fast-path after blocking.  Both will be lost in the
noise of the context switch that is happening.

> Would it make sense to do suspends entirely without freezer in your
> configurations? With the right drivers, it should work ok.

No, we need userspace tasks to freeze.  That is one of the main
reasons we go to suspend, to prevent recurring timers from firing.

> Actually, would it make sense to simply enter deep idle and do runtime
> powersaving in the drivers... n900 does that rather successfully and
> it is certainly cleaner design.

It depends on the SoC if that is even possible, and if we did do it we
would still use cgroup freezers to prevent apps from running and
eating the battery so this patch would still be applicable.

>> +/* Like schedule_timeout(), but should not block the freezer. */
>> +#define freezable_schedule_timeout(timeout)                          \
>> +({                                                                   \
>> +     long __retval;                                                  \
>> +     freezer_do_not_count();                                         \
>> +     __retval = schedule_timeout(timeout);                           \
>> +     freezer_count();                                                \
>> +     __retval;                                                       \
>> +})
>
> You plan to use this a lot during normal operation, right? Is it going
> to have some measureable impact?

It has a very measurable impact on suspend/resume operations (reducing
freeze time by a factor of 10, reducing context switches from 1000 to
25).  I haven't measured the impact on total energy used for a
suspend/resume cycle, but based on previous measurements I expect it
to be beneficial.

> Also, why not static inline?

I copied the style of the existing helpers.  I can change them all to
static inlines if you prefer.

Re: [PATCH 03/10] freezer: add new freezable helpers using freezer_do_not_count()

[Pavel Machek]

Hi!

> > Also, why not static inline?
> 
> I copied the style of the existing helpers.  I can change them all to
> static inlines if you prefer.

That would be better, yes.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 04/10] binder: use freezable blocking calls

[Colin Cross]

Avoid waking up every thread sleeping in a binder call during
suspend and resume by calling a freezable blocking call.

Signed-off-by: Colin Cross <ccross@android.com>
---
 drivers/staging/android/binder.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 24456a0..af8fba4 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -20,6 +20,7 @@
 #include <asm/cacheflush.h>
 #include <linux/fdtable.h>
 #include <linux/file.h>
+#include <linux/freezer.h>
 #include <linux/fs.h>
 #include <linux/list.h>
 #include <linux/miscdevice.h>
@@ -2140,13 +2141,13 @@ retry:
 			if (!binder_has_proc_work(proc, thread))
 				ret = -EAGAIN;
 		} else
-			ret = wait_event_interruptible_exclusive(proc->wait, binder_has_proc_work(proc, thread));
+			ret = wait_event_freezable_exclusive(proc->wait, binder_has_proc_work(proc, thread));
 	} else {
 		if (non_block) {
 			if (!binder_has_thread_work(thread))
 				ret = -EAGAIN;
 		} else
-			ret = wait_event_interruptible(thread->wait, binder_has_thread_work(thread));
+			ret = wait_event_freezable(thread->wait, binder_has_thread_work(thread));
 	}
 
 	binder_lock(__func__);
-- 
1.8.2.1

[PATCH 05/10] epoll: use freezable blocking call

[Colin Cross]

Avoid waking up every thread sleeping in an epoll_wait call during
suspend and resume by calling a freezable blocking call.

Signed-off-by: Colin Cross <ccross@android.com>
---
 fs/eventpoll.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 9fec183..65245e7 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -34,6 +34,7 @@
 #include <linux/mutex.h>
 #include <linux/anon_inodes.h>
 #include <linux/device.h>
+#include <linux/freezer.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
 #include <asm/mman.h>
@@ -1543,7 +1544,8 @@ fetch_events:
 			}
 
 			spin_unlock_irqrestore(&ep->lock, flags);
-			if (!schedule_hrtimeout_range(to, slack, HRTIMER_MODE_ABS))
+			if (!freezable_schedule_hrtimeout_range(to, slack,
+								HRTIMER_MODE_ABS))
 				timed_out = 1;
 
 			spin_lock_irqsave(&ep->lock, flags);
-- 
1.8.2.1

[PATCH 06/10] select: use freezable blocking call

[Colin Cross]

Avoid waking up every thread sleeping in a select call during
suspend and resume by calling a freezable blocking call.

Signed-off-by: Colin Cross <ccross@android.com>
---
 fs/select.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/select.c b/fs/select.c
index 8c1c96c..6b14dc7 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -27,6 +27,7 @@
 #include <linux/rcupdate.h>
 #include <linux/hrtimer.h>
 #include <linux/sched/rt.h>
+#include <linux/freezer.h>
 
 #include <asm/uaccess.h>
 
@@ -236,7 +237,8 @@ int poll_schedule_timeout(struct poll_wqueues *pwq, int state,
 
 	set_current_state(state);
 	if (!pwq->triggered)
-		rc = schedule_hrtimeout_range(expires, slack, HRTIMER_MODE_ABS);
+		rc = freezable_schedule_hrtimeout_range(expires, slack,
+							HRTIMER_MODE_ABS);
 	__set_current_state(TASK_RUNNING);
 
 	/*
-- 
1.8.2.1

[PATCH 07/10] futex: use freezable blocking call

[Colin Cross]

Avoid waking up every thread sleeping in a futex_wait call during
suspend and resume by calling a freezable blocking call.

Signed-off-by: Colin Cross <ccross@android.com>
---
 kernel/futex.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index b26dcfc..d710fae 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -61,6 +61,7 @@
 #include <linux/nsproxy.h>
 #include <linux/ptrace.h>
 #include <linux/sched/rt.h>
+#include <linux/freezer.h>
 
 #include <asm/futex.h>
 
@@ -1807,7 +1808,7 @@ static void futex_wait_queue_me(struct futex_hash_bucket *hb, struct futex_q *q,
 		 * is no timeout, or if it has yet to expire.
 		 */
 		if (!timeout || timeout->task)
-			schedule();
+			freezable_schedule();
 	}
 	__set_current_state(TASK_RUNNING);
 }
-- 
1.8.2.1

Re: [PATCH 07/10] futex: use freezable blocking call

[Darren Hart]

Colin,

I don't know anything about when or when not to use freezable*, and I
suspect that may be true for others as well. A more complete
description of why it's acceptable here in the commit log might help
expedite acceptance.


Matt,

I have a vague memory of discussing something similar to this with you.
Do you see any potential problems here?

--
Darren

On 04/29/2013 02:45 PM, Colin Cross wrote:
> Avoid waking up every thread sleeping in a futex_wait call during
> suspend and resume by calling a freezable blocking call.
> 
> Signed-off-by: Colin Cross <ccross@android.com>
> ---
>  kernel/futex.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index b26dcfc..d710fae 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -61,6 +61,7 @@
>  #include <linux/nsproxy.h>
>  #include <linux/ptrace.h>
>  #include <linux/sched/rt.h>
> +#include <linux/freezer.h>
>  
>  #include <asm/futex.h>
>  
> @@ -1807,7 +1808,7 @@ static void futex_wait_queue_me(struct futex_hash_bucket *hb, struct futex_q *q,
>  		 * is no timeout, or if it has yet to expire.
>  		 */
>  		if (!timeout || timeout->task)
> -			schedule();
> +			freezable_schedule();
>  	}
>  	__set_current_state(TASK_RUNNING);
>  }
> 

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel

Re: [PATCH 07/10] futex: use freezable blocking call

[Colin Cross]

On Mon, Apr 29, 2013 at 3:52 PM, Darren Hart <dvhart@linux.intel.com> wrote:
> Colin,
>
> I don't know anything about when or when not to use freezable*, and I
> suspect that may be true for others as well. A more complete
> description of why it's acceptable here in the commit log might help
> expedite acceptance.
>

Sure, how about:
This call can be converted to a freezable call because it doesn't hold
any locks or release any resources when interrupted that might be
needed by another freezing task or a kernel driver during suspend.

Re: [PATCH 07/10] futex: use freezable blocking call

[Matt Helsley]

On Mon, Apr 29, 2013 at 03:52:27PM -0700, Darren Hart wrote:
> Colin,
> 
> I don't know anything about when or when not to use freezable*, and I
> suspect that may be true for others as well. A more complete
> description of why it's acceptable here in the commit log might help
> expedite acceptance.
> 
> 
> Matt,
> 
> I have a vague memory of discussing something similar to this with you.
> Do you see any potential problems here?

Re: vague memories: We discussed futexes in the context of the old
checkpoint/restart patch series (<= 2.6.32).

This change looks correct to me.

> --
> Darren
> 
> On 04/29/2013 02:45 PM, Colin Cross wrote:
> > Avoid waking up every thread sleeping in a futex_wait call during
> > suspend and resume by calling a freezable blocking call.

(in addition to suspend/resume: freeze/thaw via the cgroup freezer.
I'm going to call it freeze/thaw since that should cover both cases..)

Here's my shot at explaining what I *think* the commit is supposed fix:

I imagine that before this patch on a highly-contended futex there
could be a thundering herd during freeze/thaw -- the wakeups are
*likely* to be painful because lots of tasks could be woken from the
schedule() call by the freezer only to find that the futex state hasn't
changed.

With this change the freezer won't wake these tasks up because the
FREEZER_SKIP flag is set while in the schedule() call and thus the
thundering herd won't be triggered by the freezer.

Cheers,
	-Matt Helsley

[PATCH 08/10] nanosleep: use freezable blocking call

[Colin Cross]

Avoid waking up every thread sleeping in a nanosleep call during
suspend and resume by calling a freezable blocking call.

Signed-off-by: Colin Cross <ccross@android.com>
---
 kernel/hrtimer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 14be27f..e036276 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -47,6 +47,7 @@
 #include <linux/sched/sysctl.h>
 #include <linux/sched/rt.h>
 #include <linux/timer.h>
+#include <linux/freezer.h>
 
 #include <asm/uaccess.h>
 
@@ -1525,7 +1526,7 @@ static int __sched do_nanosleep(struct hrtimer_sleeper *t, enum hrtimer_mode mod
 			t->task = NULL;
 
 		if (likely(t->task))
-			schedule();
+			freezable_schedule();
 
 		hrtimer_cancel(&t->timer);
 		mode = HRTIMER_MODE_ABS;
-- 
1.8.2.1

[PATCH 09/10] sigtimedwait: use freezable blocking call

[Colin Cross]

Avoid waking up every thread sleeping in a sigtimedwait call during
suspend and resume by calling a freezable blocking call.

Signed-off-by: Colin Cross <ccross@android.com>
---
 kernel/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 598dc06..10a70a0 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2845,7 +2845,7 @@ int do_sigtimedwait(const sigset_t *which, siginfo_t *info,
 		recalc_sigpending();
 		spin_unlock_irq(&tsk->sighand->siglock);
 
-		timeout = schedule_timeout_interruptible(timeout);
+		timeout = freezable_schedule_timeout_interruptible(timeout);
 
 		spin_lock_irq(&tsk->sighand->siglock);
 		__set_task_blocked(tsk, &tsk->real_blocked);
-- 
1.8.2.1

Re: [PATCH 09/10] sigtimedwait: use freezable blocking call

[Oleg Nesterov]

On 04/29, Colin Cross wrote:
>
> Avoid waking up every thread sleeping in a sigtimedwait call during
> suspend and resume by calling a freezable blocking call.

This doesn't explain why do want this change...

OK, probably to avoid -EAGAIN from sigtimedwait() if the freezer wakes
up the caller.

> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2845,7 +2845,7 @@ int do_sigtimedwait(const sigset_t *which, siginfo_t *info,
>  		recalc_sigpending();
>  		spin_unlock_irq(&tsk->sighand->siglock);
>
> -		timeout = schedule_timeout_interruptible(timeout);
> +		timeout = freezable_schedule_timeout_interruptible(timeout);

And I guess freezable_schedule_timeout_interruptible() is added by
http://marc.info/?l=linux-kernel&m=136727195719575 ...

	+#define freezable_schedule_timeout_interruptible(timeout)		\
	+({									\
	+	long __retval;							\
	+	freezer_do_not_count();						\
	+	__retval = schedule_timeout_interruptible(timeout);		\
	+	freezer_count();						\
	+	__retval;							\
	+})

How this can help?

The task will be interrupted anyway and the syscall will return
-EAGAIN, this only changes the time when try_to_freeze() is called.

For what? The task will call do_signal/try_to_freeze really "soon".

Confused...

Oleg.

Re: [PATCH 09/10] sigtimedwait: use freezable blocking call

[Oleg Nesterov]

On 04/30, Oleg Nesterov wrote:
>
> On 04/29, Colin Cross wrote:
> >
> > Avoid waking up every thread sleeping in a sigtimedwait call during
> > suspend and resume by calling a freezable blocking call.
>
> This doesn't explain why do want this change...
>
> OK, probably to avoid -EAGAIN from sigtimedwait() if the freezer wakes
> up the caller.
>
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -2845,7 +2845,7 @@ int do_sigtimedwait(const sigset_t *which, siginfo_t *info,
> >  		recalc_sigpending();
> >  		spin_unlock_irq(&tsk->sighand->siglock);
> >
> > -		timeout = schedule_timeout_interruptible(timeout);
> > +		timeout = freezable_schedule_timeout_interruptible(timeout);
>
> And I guess freezable_schedule_timeout_interruptible() is added by
> http://marc.info/?l=linux-kernel&m=136727195719575 ...
>
> 	+#define freezable_schedule_timeout_interruptible(timeout)		\
> 	+({									\
> 	+	long __retval;							\
> 	+	freezer_do_not_count();						\
> 	+	__retval = schedule_timeout_interruptible(timeout);		\
> 	+	freezer_count();						\
> 	+	__retval;							\
> 	+})
>
> How this can help?
>
> The task will be interrupted anyway and the syscall will return
> -EAGAIN, this only changes the time when try_to_freeze() is called.

OK, I wasn't cced, I have found another patch
http://marc.info/?l=linux-kernel&m=136727200519602 which should make
a difference, it won't be woken if PF_FREEZER_SKIP was already set.

This is racy, but it seems that "avoid -EAGAIN" was not your goal...

> For what? The task will call do_signal/try_to_freeze really "soon".

It seems that you want to speed up the freezing.

Oleg.

Re: [PATCH 09/10] sigtimedwait: use freezable blocking call

[Colin Cross]

On Tue, Apr 30, 2013 at 9:38 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 04/29, Colin Cross wrote:
>>
>> Avoid waking up every thread sleeping in a sigtimedwait call during
>> suspend and resume by calling a freezable blocking call.
>
> This doesn't explain why do want this change...
>
> OK, probably to avoid -EAGAIN from sigtimedwait() if the freezer wakes
> up the caller.

See http://marc.info/?l=linux-pm&m=136727197819593&w=2 for the full
justification.  I will include a fuller description of the reason for
this patch in the next version.

>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -2845,7 +2845,7 @@ int do_sigtimedwait(const sigset_t *which, siginfo_t *info,
>>               recalc_sigpending();
>>               spin_unlock_irq(&tsk->sighand->siglock);
>>
>> -             timeout = schedule_timeout_interruptible(timeout);
>> +             timeout = freezable_schedule_timeout_interruptible(timeout);
>
> And I guess freezable_schedule_timeout_interruptible() is added by
> http://marc.info/?l=linux-kernel&m=136727195719575 ...
>
>         +#define freezable_schedule_timeout_interruptible(timeout)              \
>         +({                                                                     \
>         +       long __retval;                                                  \
>         +       freezer_do_not_count();                                         \
>         +       __retval = schedule_timeout_interruptible(timeout);             \
>         +       freezer_count();                                                \
>         +       __retval;                                                       \
>         +})
>
> How this can help?
>
> The task will be interrupted anyway and the syscall will return
> -EAGAIN, this only changes the time when try_to_freeze() is called.
>
> For what? The task will call do_signal/try_to_freeze really "soon".

See http://marc.info/?l=linux-pm&m=136727204919622&w=2, which removes
the wakeup sent to skipped tasks, so schedule_timeout_interruptible()
will only return if the timeout finishes or another task (not the
freezer) calls wake_up on the task.  If that happens during freezing
or while frozen freezer_count() will freeze.  If another task does not
wake up this task while frozen, this task will not need to run at all
during suspend or resume, saving cpu cycles, context switches, and
power.

> Confused...
>
> Oleg.
>

Re: [PATCH 09/10] sigtimedwait: use freezable blocking call

[Oleg Nesterov]

On 04/30, Colin Cross wrote:
> On Tue, Apr 30, 2013 at 9:38 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 04/29, Colin Cross wrote:
> >>
> >> Avoid waking up every thread sleeping in a sigtimedwait call during
> >> suspend and resume by calling a freezable blocking call.
> >
> > This doesn't explain why do want this change...
> >
> > OK, probably to avoid -EAGAIN from sigtimedwait() if the freezer wakes
> > up the caller.
>
> See http://marc.info/?l=linux-pm&m=136727197819593&w=2 for the full
> justification.  I will include a fuller description of the reason for
> this patch in the next version.

Yes, thanks, I already realized what are you trying to do.

> > The task will be interrupted anyway and the syscall will return
> > -EAGAIN, this only changes the time when try_to_freeze() is called.
> >
> > For what? The task will call do_signal/try_to_freeze really "soon".
>
> See http://marc.info/?l=linux-pm&m=136727204919622&w=2, which removes
> the wakeup sent to skipped tasks, so schedule_timeout_interruptible()
> will only return if the timeout finishes or another task

Or if freeze_task() was already called.

but I guess you do not care.

Oleg.

[PATCH 10/10] af_unix: use freezable blocking calls in read

[Colin Cross]

Avoid waking up every thread sleeping in read call on an AF_UNIX
socket during suspend and resume by calling a freezable blocking
call.

Signed-off-by: Colin Cross <ccross@android.com>
---
 net/unix/af_unix.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 2db702d..2bcac57 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -114,6 +114,7 @@
 #include <linux/mount.h>
 #include <net/checksum.h>
 #include <linux/security.h>
+#include <linux/freezer.h>
 
 struct hlist_head unix_socket_table[2 * UNIX_HASH_SIZE];
 EXPORT_SYMBOL_GPL(unix_socket_table);
@@ -1880,7 +1881,7 @@ static long unix_stream_data_wait(struct sock *sk, long timeo)
 
 		set_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
 		unix_state_unlock(sk);
-		timeo = schedule_timeout(timeo);
+		timeo = freezable_schedule_timeout(timeo);
 		unix_state_lock(sk);
 		clear_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
 	}
-- 
1.8.2.1