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

[PATCH v2 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 freeze_task() function 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.

v2 moves the skip check to freeze_task(), and expands the commit
messages.

[PATCH v2 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 v2 01/10] freezer: shorten freezer sleep time using exponential backoff

[Tejun Heo]

Hello,

On Wed, May 01, 2013 at 06:34:59PM -0700, 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.

Can you please put the above as comment in the code?  Other than that,

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

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

[Colin Cross]

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

Combined with patches to convert freezable helpers to use
freezer_do_not_count() and convert common sites where idle userspace
tasks are blocked to use the freezable helpers, this reduces the
time and energy required to suspend and resume.

Signed-off-by: Colin Cross <ccross@android.com>
---
v2:  move check to freeze_task()

 kernel/freezer.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index c38893b..8b2afc1 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -110,6 +110,18 @@ bool freeze_task(struct task_struct *p)
 {
 	unsigned long flags;
 
+	/*
+	 * This check can race with freezer_do_not_count, but worst case that
+	 * will result in an extra wakeup being sent to the task.  It does not
+	 * race with freezer_count(), the barriers in freezer_count() and
+	 * freezer_should_skip() ensure that either freezer_count() sees
+	 * freezing == true in try_to_freeze() and freezes, or
+	 * freezer_should_skip() sees !PF_FREEZE_SKIP and freezes the task
+	 * normally.
+	 */
+	if (freezer_should_skip(p))
+		return false;
+
 	spin_lock_irqsave(&freezer_lock, flags);
 	if (!freezing(p) || frozen(p)) {
 		spin_unlock_irqrestore(&freezer_lock, flags);
-- 
1.8.2.1

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

[Tejun Heo]

On Wed, May 01, 2013 at 06:35:00PM -0700, Colin Cross wrote:
> 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.  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.
> 
> Combined with patches to convert freezable helpers to use
> freezer_do_not_count() and convert common sites where idle userspace
> tasks are blocked to use the freezable helpers, this reduces the
> time and energy required to suspend and resume.
> 
> Signed-off-by: Colin Cross <ccross@android.com>
> ---
> v2:  move check to freeze_task()
> 
>  kernel/freezer.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index c38893b..8b2afc1 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -110,6 +110,18 @@ bool freeze_task(struct task_struct *p)
>  {
>  	unsigned long flags;
>  
> +	/*
> +	 * This check can race with freezer_do_not_count, but worst case that
> +	 * will result in an extra wakeup being sent to the task.  It does not
> +	 * race with freezer_count(), the barriers in freezer_count() and
> +	 * freezer_should_skip() ensure that either freezer_count() sees
> +	 * freezing == true in try_to_freeze() and freezes, or
> +	 * freezer_should_skip() sees !PF_FREEZE_SKIP and freezes the task
> +	 * normally.
> +	 */
> +	if (freezer_should_skip(p))
> +		return false;

Maybe a line or two explaining that this matters for power saving?
Other than that,

Acked-by: Tejun Heo <tj@kernel.org>

Oleg, this looks correct to me.  Can you please ack too?

Thanks.

-- 
tejun

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

[Pavel Machek]

On Wed 2013-05-01 18:35:00, Colin Cross wrote:
> 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.  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.
> 
> Combined with patches to convert freezable helpers to use
> freezer_do_not_count() and convert common sites where idle userspace
> tasks are blocked to use the freezable helpers, this reduces the
> time and energy required to suspend and resume.
> 
> 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 v2 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 v2 03/10] freezer: add new freezable helpers using freezer_do_not_count()

[Tejun Heo]

Hello,

On Wed, May 01, 2013 at 06:35:01PM -0700, Colin Cross wrote:
> 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().

I'd much prefer the latter part being a separate patch because the
change isn't really trivial and it isn't exactly equivalent - before
we prioritized meeting the condition over freezing, now it's the other
way around.  It's fine but does deserve description in the log so that
if something gets tracked down to the commit, it's easy to tell how
the behavior flipped and why.

> +/* 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_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;							\
> +})

(cc'ing Jeff Layton)

So, one worry that I have about this is that freezer essentially
behaves like a huge lock that everyone grabs and while scattering
try_to_freeze() calls around might seem innocuous, it effectively
adding a dependency to that single giant lock to that place, so
whenever you're adding try_to_freeze() call while holding any kind of
locks, it substiantially increases the chance of possible deadlock
scenarios.  NFS recently got bitten by it and now is trying to get rid
of them as it's fundamentally broken.

So, the freezable interface can't be something that people can use
casually.  It is something which should be carefully and strategically
deployed where we *know* that lock dependency risks don't exist or at
least are acceptable.  I'm a bit weary that this patch is expanding
the interface a lot that they now look like the equivalents of normal
schedule calls.  Not exactly sure what to do here but can we please at
least have RED BOLD BLINKING comments which scream to people not to
use these unless they know what they're doing?

Thanks.

-- 
tejun

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

[Tejun Heo]

On Thu, May 02, 2013 at 04:55:05PM -0700, Tejun Heo wrote:
> So, the freezable interface can't be something that people can use
> casually.  It is something which should be carefully and strategically
> deployed where we *know* that lock dependency risks don't exist or at
> least are acceptable.  I'm a bit weary that this patch is expanding
> the interface a lot that they now look like the equivalents of normal
> schedule calls.  Not exactly sure what to do here but can we please at
> least have RED BOLD BLINKING comments which scream to people not to
> use these unless they know what they're doing?

Maybe we should trigger WARN_ON_ONCE() if lockdep_depth() > 0 by
default and have ugly variants which can be used if the caller is sure
that it's okay possibly with list of locks which are held?

-- 
tejun

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

[Colin Cross]

This sounds the same as what ended up getting reverted in
https://lkml.org/lkml/2013/3/4/221
I can add the WARN_ON_ONCE to all my new calls, and leave them out of
existing calls, but that seems a little odd, and will be redundant if
the lockdep call in try_to_freeze goes back in in 3.11.  Do you still
want it in the new apis?

On Thu, May 2, 2013 at 5:03 PM, Tejun Heo <tj@kernel.org> wrote:
> On Thu, May 02, 2013 at 04:55:05PM -0700, Tejun Heo wrote:
>> So, the freezable interface can't be something that people can use
>> casually.  It is something which should be carefully and strategically
>> deployed where we *know* that lock dependency risks don't exist or at
>> least are acceptable.  I'm a bit weary that this patch is expanding
>> the interface a lot that they now look like the equivalents of normal
>> schedule calls.  Not exactly sure what to do here but can we please at
>> least have RED BOLD BLINKING comments which scream to people not to
>> use these unless they know what they're doing?
>
> Maybe we should trigger WARN_ON_ONCE() if lockdep_depth() > 0 by
> default and have ugly variants which can be used if the caller is sure
> that it's okay possibly with list of locks which are held?
>
> --
> tejun

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

[Colin Cross]

On Thu, May 2, 2013 at 7:16 PM, Colin Cross <ccross@android.com> wrote:
> This sounds the same as what ended up getting reverted in
> https://lkml.org/lkml/2013/3/4/221
> I can add the WARN_ON_ONCE to all my new calls, and leave them out of
> existing calls, but that seems a little odd, and will be redundant if
> the lockdep call in try_to_freeze goes back in in 3.11.  Do you still
> want it in the new apis?
>
> On Thu, May 2, 2013 at 5:03 PM, Tejun Heo <tj@kernel.org> wrote:
>> On Thu, May 02, 2013 at 04:55:05PM -0700, Tejun Heo wrote:
>>> So, the freezable interface can't be something that people can use
>>> casually.  It is something which should be carefully and strategically
>>> deployed where we *know* that lock dependency risks don't exist or at
>>> least are acceptable.  I'm a bit weary that this patch is expanding
>>> the interface a lot that they now look like the equivalents of normal
>>> schedule calls.  Not exactly sure what to do here but can we please at
>>> least have RED BOLD BLINKING comments which scream to people not to
>>> use these unless they know what they're doing?
>>
>> Maybe we should trigger WARN_ON_ONCE() if lockdep_depth() > 0 by
>> default and have ugly variants which can be used if the caller is sure
>> that it's okay possibly with list of locks which are held?
>>
>> --
>> tejun

(sorry for the top post)

I could also put the lockdep check that was reveted back into
try_to_freeze(), and add a freezable_schedule_unsafe() that skips it
for use in the known-unsafe users in nfs, with a big comment not to
add new users of it.

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

[Tejun Heo]

Hello,

On Thu, May 02, 2013 at 07:41:39PM -0700, Colin Cross wrote:
> On Thu, May 2, 2013 at 7:16 PM, Colin Cross <ccross@android.com> wrote:
> > This sounds the same as what ended up getting reverted in
> > https://lkml.org/lkml/2013/3/4/221
> > I can add the WARN_ON_ONCE to all my new calls, and leave them out of
> > existing calls, but that seems a little odd, and will be redundant if
> > the lockdep call in try_to_freeze goes back in in 3.11.  Do you still
> > want it in the new apis?
...
> I could also put the lockdep check that was reveted back into
> try_to_freeze(), and add a freezable_schedule_unsafe() that skips it
> for use in the known-unsafe users in nfs, with a big comment not to
> add new users of it.

Oh yeah, that sounds like a good idea, and as for the non-ugly
variants, at least in the mid/long term, I think it'd be best to add
the lockdep annotation to try_to_freeze() with
__try_to_freeze_unsafe_youre_gonna_burn_in_hell_if_you_use_this() for
the existing users which should be gradually converted, but if that's
too burdensome, adding warnings to the wrappers should do for now, I
guess.

And I *hope* the lockdep annotation is stricter than what was added
before.  I think it better be "no lock ever should be held at this
point" rather than "consider this a big lock".  I'm not sure how much
consensus we have on this but I'd like to, in the longer term, merge
freezer into job control stop so that frozen tasks don't appear as
being in uninterruptible sleep.  Currently, the frozen tasks are
essentially in UNINTERRUPTIBLE sleep which is okay for suspend but for
cgroup freezer, it sucks.  You end up with tasks which you can't even
kill.

Combined with the locking problems, I was planning to update the
freezer such that the frozen state is implemented as a form of jobctl
stop, so that things like ptrace / kill -9 could work on them and we
also have the clear definition of the frozen state rather than the
current "it may get stuck somewhere in the kernel".

But that conflicts with what you're doing here which seems pretty
useful, so, to satisfy both goals, when somebody needs to put a
pseudo-frozen task into the actual frozen jobctl stop, those spots
which are currently using try_to_stop() would have to return an error,
most likely -EINTR with TIF_SIGPENDING set, and the control should
return towards userland so that signal handling path can be invoked.
ie. It should be possible to steer the tasks which are considered
frozen but not in the frozen jobctl stop into the jobctl stop without
any side effect.  To do that, those spots basically have to be pretty
close to the userland boundary where it can easily leave the kernel
with -EINTR and AFAICS all the spots that you converted are like that
(which I think is natural).  While not holding any locks doesn't
guarantee that, I think there'd be a fairly high correlation at least
and it'd be able to drive people towards finding out what's going on.

So, that's my agenda.  Not sure how many actually agree with it.
Rafael, Oleg, Jeff?

Thanks.

-- 
tejun

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

[Tejun Heo]

On Thu, May 02, 2013 at 09:09:34PM -0700, Tejun Heo wrote:
> But that conflicts with what you're doing here which seems pretty
> useful, so, to satisfy both goals, when somebody needs to put a
> pseudo-frozen task into the actual frozen jobctl stop, those spots
> which are currently using try_to_stop() would have to return an error,
> most likely -EINTR with TIF_SIGPENDING set, and the control should

Let's make that -ERESTART*.

-- 
tejun

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

[Colin Cross]

On Thu, May 2, 2013 at 9:09 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Thu, May 02, 2013 at 07:41:39PM -0700, Colin Cross wrote:
>> On Thu, May 2, 2013 at 7:16 PM, Colin Cross <ccross@android.com> wrote:
>> > This sounds the same as what ended up getting reverted in
>> > https://lkml.org/lkml/2013/3/4/221
>> > I can add the WARN_ON_ONCE to all my new calls, and leave them out of
>> > existing calls, but that seems a little odd, and will be redundant if
>> > the lockdep call in try_to_freeze goes back in in 3.11.  Do you still
>> > want it in the new apis?
> ...
>> I could also put the lockdep check that was reveted back into
>> try_to_freeze(), and add a freezable_schedule_unsafe() that skips it
>> for use in the known-unsafe users in nfs, with a big comment not to
>> add new users of it.
>
> Oh yeah, that sounds like a good idea, and as for the non-ugly
> variants, at least in the mid/long term, I think it'd be best to add
> the lockdep annotation to try_to_freeze() with
> __try_to_freeze_unsafe_youre_gonna_burn_in_hell_if_you_use_this() for
> the existing users which should be gradually converted, but if that's
> too burdensome, adding warnings to the wrappers should do for now, I
> guess.
>
> And I *hope* the lockdep annotation is stricter than what was added
> before.  I think it better be "no lock ever should be held at this
> point" rather than "consider this a big lock".

The previous patch (6aa9707099c4b25700940eb3d016f16c4434360d in Linus'
tree) already required that no locks be held, it wasn't using a lock
annotation.

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

[Tejun Heo]

On Thu, May 02, 2013 at 09:17:21PM -0700, Colin Cross wrote:
> > And I *hope* the lockdep annotation is stricter than what was added
> > before.  I think it better be "no lock ever should be held at this
> > point" rather than "consider this a big lock".
> 
> The previous patch (6aa9707099c4b25700940eb3d016f16c4434360d in Linus'
> tree) already required that no locks be held, it wasn't using a lock
> annotation.

Ooh, cool.  Thanks for the pointer.  Forget about my rambling and
let's just add an unsafe version of try_to_freeze() and be done with
it.  :)

-- 
tejun

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

[Alan Stern]

On Thu, 2 May 2013, Tejun Heo wrote:

> Combined with the locking problems, I was planning to update the
> freezer such that the frozen state is implemented as a form of jobctl
> stop, so that things like ptrace / kill -9 could work on them and we
> also have the clear definition of the frozen state rather than the
> current "it may get stuck somewhere in the kernel".
> 
> But that conflicts with what you're doing here which seems pretty
> useful, so, to satisfy both goals, when somebody needs to put a
> pseudo-frozen task into the actual frozen jobctl stop, those spots
> which are currently using try_to_stop() would have to return an error,
> most likely -EINTR with TIF_SIGPENDING set, and the control should
> return towards userland so that signal handling path can be invoked.
> ie. It should be possible to steer the tasks which are considered
> frozen but not in the frozen jobctl stop into the jobctl stop without
> any side effect.  To do that, those spots basically have to be pretty
> close to the userland boundary where it can easily leave the kernel
> with -EINTR and AFAICS all the spots that you converted are like that
> (which I think is natural).  While not holding any locks doesn't
> guarantee that, I think there'd be a fairly high correlation at least
> and it'd be able to drive people towards finding out what's going on.

Don't forget about freezable kernel threads.  They never cross the 
kernel/user boundary.

Alan Stern

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

[Tejun Heo]

On Fri, May 03, 2013 at 10:18:17AM -0400, Alan Stern wrote:
> Don't forget about freezable kernel threads.  They never cross the 
> kernel/user boundary.

Doesn't really matter for this purpose.  They first of all can't be
moved into !root cgroups and thus can't be cgroup-frozen and even if
they could they can't be killed or ptraced, so they just don't play
the same game.

Thanks.

-- 
tejun

[PATCH v2 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.  Previous
patches modified the freezer to avoid sending wakeups to threads
that are blocked in freezable blocking calls.

This call was selected to 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, and is a common site where idle userspace tasks are
blocked.

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 v2 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.  Previous
patches modified the freezer to avoid sending wakeups to threads
that are blocked in freezable blocking calls.

This call was selected to 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, and is a common site where idle userspace tasks are
blocked.

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 v2 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.  Previous
patches modified the freezer to avoid sending wakeups to threads
that are blocked in freezable blocking calls.

This call was selected to 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, and is a common site where idle userspace tasks are
blocked.

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 v2 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.  Previous
patches modified the freezer to avoid sending wakeups to threads
that are blocked in freezable blocking calls.

This call was selected to 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, and is a common site where idle userspace tasks are
blocked.

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 v2 07/10] futex: use freezable blocking call

[Thomas Gleixner]

On Wed, 1 May 2013, Colin Cross wrote:

> Avoid waking up every thread sleeping in a futex_wait call during
> suspend and resume by calling a freezable blocking call.  Previous
> patches modified the freezer to avoid sending wakeups to threads
> that are blocked in freezable blocking calls.
> 
> This call was selected to 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, and is a common site where idle userspace tasks are
> blocked.
> 
> Signed-off-by: Colin Cross <ccross@android.com>

Acked-by: Thomas Gleixner <tglx@linutronix.de>

> ---
>  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 v2 07/10] futex: use freezable blocking call

[Darren Hart]

On 05/02/2013 12:45 PM, Thomas Gleixner wrote:
> On Wed, 1 May 2013, Colin Cross wrote:
> 
>> Avoid waking up every thread sleeping in a futex_wait call during
>> suspend and resume by calling a freezable blocking call.  Previous
>> patches modified the freezer to avoid sending wakeups to threads
>> that are blocked in freezable blocking calls.
>>
>> This call was selected to 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, and is a common site where idle userspace tasks are
>> blocked.
>>
>> Signed-off-by: Colin Cross <ccross@android.com>
> 
> Acked-by: Thomas Gleixner <tglx@linutronix.de>

Beat me to it :-) Also agreed.

Acked-by: Darren Hart <dvhart@linux.intel.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
>>
>>

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

[tip:core/locking] futex: Use freezable blocking call

[tip-bot for Colin Cross]

Commit-ID:  88c8004fd3a5fdd2378069de86b90b21110d33a4
Gitweb:     http://git.kernel.org/tip/88c8004fd3a5fdd2378069de86b90b21110d33a4
Author:     Colin Cross <ccross@android.com>
AuthorDate: Wed, 1 May 2013 18:35:05 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 25 Jun 2013 23:11:19 +0200

futex: Use freezable blocking call

Avoid waking up every thread sleeping in a futex_wait call during
suspend and resume by calling a freezable blocking call.  Previous
patches modified the freezer to avoid sending wakeups to threads
that are blocked in freezable blocking calls.

This call was selected to 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, and is a common site where idle userspace tasks are
blocked.

Signed-off-by: Colin Cross <ccross@android.com>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: arve@android.com
Cc: Tejun Heo <tj@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Darren Hart <dvhart@linux.intel.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Link: http://lkml.kernel.org/r/1367458508-9133-8-git-send-email-ccross@android.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/futex.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 49dacfb..c3a1a55 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -62,6 +62,7 @@
 #include <linux/ptrace.h>
 #include <linux/sched/rt.h>
 #include <linux/hugetlb.h>
+#include <linux/freezer.h>
 
 #include <asm/futex.h>
 
@@ -1808,7 +1809,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);
 }

[PATCH v2 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.  Previous
patches modified the freezer to avoid sending wakeups to threads
that are blocked in freezable blocking calls.

This call was selected to 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, and is a common site where idle userspace tasks are
blocked.

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

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

[Thomas Gleixner]

On Wed, 1 May 2013, Colin Cross wrote:

> Avoid waking up every thread sleeping in a nanosleep call during
> suspend and resume by calling a freezable blocking call.  Previous
> patches modified the freezer to avoid sending wakeups to threads
> that are blocked in freezable blocking calls.
> 
> This call was selected to 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, and is a common site where idle userspace tasks are
> blocked.
> 
> Signed-off-by: Colin Cross <ccross@android.com>

Acked-by: Thomas Gleixner <tglx@linutronix.de>

> ---
>  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 v2 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.  Previous
patches modified the freezer to avoid sending wakeups to threads
that are blocked in freezable blocking calls.

This call was selected to 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, and is a common site where idle userspace tasks are
blocked.

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

[PATCH v2 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.  Previous patches modified the freezer to avoid sending
wakeups to threads that are blocked in freezable blocking calls.

This call was selected to 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, and is a common site where idle userspace tasks are
blocked.

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

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

[Tejun Heo]

On Wed, May 01, 2013 at 06:35:08PM -0700, Colin Cross wrote:
> Avoid waking up every thread sleeping in read call on an AF_UNIX
> socket during suspend and resume by calling a freezable blocking
> call.  Previous patches modified the freezer to avoid sending
> wakeups to threads that are blocked in freezable blocking calls.
> 
> This call was selected to 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, and is a common site where idle userspace tasks are
> blocked.

Heh, so you are aware of the deadlock possibilities.  Good selection
of spots.  For all the conversion patches.

 Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

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

[Rafael J. Wysocki]

On Thursday, May 02, 2013 05:08:12 PM Tejun Heo wrote:
> On Wed, May 01, 2013 at 06:35:08PM -0700, Colin Cross wrote:
> > Avoid waking up every thread sleeping in read call on an AF_UNIX
> > socket during suspend and resume by calling a freezable blocking
> > call.  Previous patches modified the freezer to avoid sending
> > wakeups to threads that are blocked in freezable blocking calls.
> > 
> > This call was selected to 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, and is a common site where idle userspace tasks are
> > blocked.
> 
> Heh, so you are aware of the deadlock possibilities.  Good selection
> of spots.  For all the conversion patches.
> 
>  Acked-by: Tejun Heo <tj@kernel.org>

I wonder if that includes [3/10] (just to get the record straight)?

Rafael


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

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

[Tejun Heo]

Hello, Rafael.

On Sat, May 4, 2013 at 12:19 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> Heh, so you are aware of the deadlock possibilities.  Good selection
>> of spots.  For all the conversion patches.
>>
>>  Acked-by: Tejun Heo <tj@kernel.org>
>
> I wonder if that includes [3/10] (just to get the record straight)?

I think we want the lockdep annotations before these go in. I'll
review Colin's new series later today.

Thanks!

--
tejun

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

[Colin Cross]

On Sat, May 4, 2013 at 1:39 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Rafael.
>
> On Sat, May 4, 2013 at 12:19 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>> Heh, so you are aware of the deadlock possibilities.  Good selection
>>> of spots.  For all the conversion patches.
>>>
>>>  Acked-by: Tejun Heo <tj@kernel.org>
>>
>> I wonder if that includes [3/10] (just to get the record straight)?
>
> I think we want the lockdep annotations before these go in. I'll
> review Colin's new series later today.

Just to make sure the merge order is clear, the two patches I posted
yesterday to reintroduce the lockdep warning in try_to_freeze()
(https://lkml.org/lkml/2013/5/3/488) need to be merged first, then I
will repost this series on top of that one.

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

[Rafael J. Wysocki]

On Saturday, May 04, 2013 03:23:30 PM Colin Cross wrote:
> On Sat, May 4, 2013 at 1:39 PM, Tejun Heo <tj@kernel.org> wrote:
> > Hello, Rafael.
> >
> > On Sat, May 4, 2013 at 12:19 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >>> Heh, so you are aware of the deadlock possibilities.  Good selection
> >>> of spots.  For all the conversion patches.
> >>>
> >>>  Acked-by: Tejun Heo <tj@kernel.org>
> >>
> >> I wonder if that includes [3/10] (just to get the record straight)?
> >
> > I think we want the lockdep annotations before these go in. I'll
> > review Colin's new series later today.
> 
> Just to make sure the merge order is clear, the two patches I posted
> yesterday to reintroduce the lockdep warning in try_to_freeze()
> (https://lkml.org/lkml/2013/5/3/488) need to be merged first, then I
> will repost this series on top of that one.

OK, thanks for the clarification.

Rafael


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

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

[Rafael J. Wysocki]

On Wednesday, May 01, 2013 06:34:58 PM Colin Cross wrote:
> 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 freeze_task() function 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.
> 
> v2 moves the skip check to freeze_task(), and expands the commit
> messages.

The entire series makes sense to me, so are there any objections?

Tejun, Oleg??

Rafael


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

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

[Tejun Heo]

Hey, Rafael.

On Thu, May 02, 2013 at 11:06:35PM +0200, Rafael J. Wysocki wrote:
> > v2 moves the skip check to freeze_task(), and expands the commit
> > messages.
> 
> The entire series makes sense to me, so are there any objections?
> 
> Tejun, Oleg??

Generally looks good to me.  I'm a bit uncomfortable with the
generic-looking expansion of the freezable API as it can't be
something which can be used in any generic manner.  It'd be great if
we just add only the used ones (do we already?) and add some sort of
lockdep annotation so that people don't use them while holding any
locks.

Thanks.

-- 
tejun

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

[Rafael J. Wysocki]

On Thursday, May 02, 2013 05:10:26 PM Tejun Heo wrote:
> Hey, Rafael.
> 
> On Thu, May 02, 2013 at 11:06:35PM +0200, Rafael J. Wysocki wrote:
> > > v2 moves the skip check to freeze_task(), and expands the commit
> > > messages.
> > 
> > The entire series makes sense to me, so are there any objections?
> > 
> > Tejun, Oleg??
> 
> Generally looks good to me.  I'm a bit uncomfortable with the
> generic-looking expansion of the freezable API as it can't be
> something which can be used in any generic manner.  It'd be great if
> we just add only the used ones (do we already?)

I'm not sure about that, unfortunately.

> and add some sort of lockdep annotation so that people don't use them while
> holding any locks.

I guess we can add that anyway?

Rafael


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