linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [tip:sched/core] sched/wait: Reimplement wait_event_freezable()
@ 2014-11-04 16:08 tip-bot for Peter Zijlstra
  2014-11-04 20:12 ` Pavel Machek
  0 siblings, 1 reply; 4+ messages in thread
From: tip-bot for Peter Zijlstra @ 2014-11-04 16:08 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: mingo, torvalds, peterz, hpa, rjw, linux-kernel, tglx, pavel, len.brown

Commit-ID:  36df04bc5273a046f53b5e359febc1225f85aa7b
Gitweb:     http://git.kernel.org/tip/36df04bc5273a046f53b5e359febc1225f85aa7b
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 29 Oct 2014 12:21:57 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 4 Nov 2014 07:17:45 +0100

sched/wait: Reimplement wait_event_freezable()

Provide better implementations of wait_event_freezable() APIs.

The problem is with freezer_do_not_count(), it hides the thread from
the freezer, even though this thread might not actually freeze/sleep
at all.

Cc: oleg@redhat.com
Cc: Rafael Wysocki <rjw@rjwysocki.net>

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Len Brown <len.brown@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: linux-pm@vger.kernel.org
Link: http://lkml.kernel.org/n/tip-d86fz1jmso9wjxa8jfpinp8o@git.kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/freezer.h | 38 ---------------------------------
 include/linux/wait.h    | 57 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 38 deletions(-)

diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index 7fd81b8..e203665 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -265,35 +265,6 @@ static inline int freezable_schedule_hrtimeout_range(ktime_t *expires,
 	__retval;							\
 })
 
-#define wait_event_freezable(wq, condition)				\
-({									\
-	int __retval;							\
-	freezer_do_not_count();						\
-	__retval = wait_event_interruptible(wq, (condition));		\
-	freezer_count();						\
-	__retval;							\
-})
-
-#define wait_event_freezable_timeout(wq, condition, timeout)		\
-({									\
-	long __retval = timeout;					\
-	freezer_do_not_count();						\
-	__retval = wait_event_interruptible_timeout(wq,	(condition),	\
-				__retval);				\
-	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; }
@@ -331,15 +302,6 @@ static inline void set_freezable(void) {}
 #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)
 
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 0421775..2232ed1 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -267,6 +267,31 @@ do {									\
 	__wait_event(wq, condition);					\
 } while (0)
 
+#define __wait_event_freezable(wq, condition)				\
+	___wait_event(wq, condition, TASK_INTERRUPTIBLE, 0, 0,		\
+			    schedule(); try_to_freeze())
+
+/**
+ * wait_event - sleep (or freeze) until a condition gets true
+ * @wq: the waitqueue to wait on
+ * @condition: a C expression for the event to wait for
+ *
+ * The process is put to sleep (TASK_INTERRUPTIBLE -- so as not to contribute
+ * to system load) until the @condition evaluates to true. The
+ * @condition is checked each time the waitqueue @wq is woken up.
+ *
+ * wake_up() has to be called after changing any variable that could
+ * change the result of the wait condition.
+ */
+#define wait_event_freezable(wq, condition)				\
+({									\
+	int __ret = 0;							\
+	might_sleep();							\
+	if (!(condition))						\
+		__ret = __wait_event_freezable(wq, condition);		\
+	__ret;								\
+})
+
 #define __wait_event_timeout(wq, condition, timeout)			\
 	___wait_event(wq, ___wait_cond_timeout(condition),		\
 		      TASK_UNINTERRUPTIBLE, 0, timeout,			\
@@ -300,6 +325,24 @@ do {									\
 	__ret;								\
 })
 
+#define __wait_event_freezable_timeout(wq, condition, timeout)		\
+	___wait_event(wq, ___wait_cond_timeout(condition),		\
+		      TASK_INTERRUPTIBLE, 0, timeout,			\
+		      __ret = schedule_timeout(__ret); try_to_freeze())
+
+/*
+ * like wait_event_timeout() -- except it uses TASK_INTERRUPTIBLE to avoid
+ * increasing load and is freezable.
+ */
+#define wait_event_freezable_timeout(wq, condition, timeout)		\
+({									\
+	long __ret = timeout;						\
+	might_sleep();							\
+	if (!___wait_cond_timeout(condition))				\
+		__ret = __wait_event_freezable_timeout(wq, condition, timeout);	\
+	__ret;								\
+})
+
 #define __wait_event_cmd(wq, condition, cmd1, cmd2)			\
 	(void)___wait_event(wq, condition, TASK_UNINTERRUPTIBLE, 0, 0,	\
 			    cmd1; schedule(); cmd2)
@@ -480,6 +523,20 @@ do {									\
 })
 
 
+#define __wait_event_freezable_exclusive(wq, condition)			\
+	___wait_event(wq, condition, TASK_INTERRUPTIBLE, 1, 0,		\
+			schedule(); try_to_freeze())
+
+#define wait_event_freezable_exclusive(wq, condition)			\
+({									\
+	int __ret = 0;							\
+	might_sleep();							\
+	if (!(condition))						\
+		__ret = __wait_event_freezable_exclusive(wq, condition);\
+	__ret;								\
+})
+
+
 #define __wait_event_interruptible_locked(wq, condition, exclusive, irq) \
 ({									\
 	int __ret = 0;							\

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [tip:sched/core] sched/wait: Reimplement wait_event_freezable()
  2014-11-04 16:08 [tip:sched/core] sched/wait: Reimplement wait_event_freezable() tip-bot for Peter Zijlstra
@ 2014-11-04 20:12 ` Pavel Machek
  2014-11-05  8:18   ` Peter Zijlstra
  0 siblings, 1 reply; 4+ messages in thread
From: Pavel Machek @ 2014-11-04 20:12 UTC (permalink / raw)
  To: len.brown, linux-kernel, tglx, mingo, peterz, torvalds, rjw, hpa
  Cc: linux-tip-commits

Hi!

> Commit-ID:  36df04bc5273a046f53b5e359febc1225f85aa7b
> Gitweb:     http://git.kernel.org/tip/36df04bc5273a046f53b5e359febc1225f85aa7b
> Author:     Peter Zijlstra <peterz@infradead.org>
> AuthorDate: Wed, 29 Oct 2014 12:21:57 +0100
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Tue, 4 Nov 2014 07:17:45 +0100
> 
> sched/wait: Reimplement wait_event_freezable()
> 
> Provide better implementations of wait_event_freezable() APIs.
> 
> The problem is with freezer_do_not_count(), it hides the thread from
> the freezer, even though this thread might not actually freeze/sleep
> at all.

Can you elaborate?

The thread will be in freezer_do_not_count() area, but it is just
waiting for event there, it should not do much damage.

If this is bugfix, should it be cc-ed to stable?

Did you test it with suspend/hibernation? Because I can't really see
how it works.

> +#define __wait_event_freezable(wq, condition)				\
> +	___wait_event(wq, condition, TASK_INTERRUPTIBLE, 0, 0,		\
> +			    schedule(); try_to_freeze())
> +

_Three_ underscores. And two underscore version exists, too,
fortunately it at least has different number of arguments.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [tip:sched/core] sched/wait: Reimplement wait_event_freezable()
  2014-11-04 20:12 ` Pavel Machek
@ 2014-11-05  8:18   ` Peter Zijlstra
  2014-11-13 14:03     ` Pavel Machek
  0 siblings, 1 reply; 4+ messages in thread
From: Peter Zijlstra @ 2014-11-05  8:18 UTC (permalink / raw)
  To: Pavel Machek
  Cc: len.brown, linux-kernel, tglx, mingo, torvalds, rjw, hpa,
	linux-tip-commits

On Tue, Nov 04, 2014 at 09:12:03PM +0100, Pavel Machek wrote:
> Hi!
> 
> > Commit-ID:  36df04bc5273a046f53b5e359febc1225f85aa7b
> > Gitweb:     http://git.kernel.org/tip/36df04bc5273a046f53b5e359febc1225f85aa7b
> > Author:     Peter Zijlstra <peterz@infradead.org>
> > AuthorDate: Wed, 29 Oct 2014 12:21:57 +0100
> > Committer:  Ingo Molnar <mingo@kernel.org>
> > CommitDate: Tue, 4 Nov 2014 07:17:45 +0100
> > 
> > sched/wait: Reimplement wait_event_freezable()
> > 
> > Provide better implementations of wait_event_freezable() APIs.
> > 
> > The problem is with freezer_do_not_count(), it hides the thread from
> > the freezer, even though this thread might not actually freeze/sleep
> > at all.
> 
> Can you elaborate?
> 
> The thread will be in freezer_do_not_count() area, but it is just
> waiting for event there, it should not do much damage.

There are wait_event()s for which the cond expands to quite a lot of
code. This code can still be running while the freezer reports success.
This can happen because we're hidden by the do_not_count logic.

Also, I initially overlooked that freezer_count() did a try_to_freeze().

> If this is bugfix, should it be cc-ed to stable?

Its not a bugfix per se, the issue above is rare and extremely hard to
hit, esp since the freezer tries multiple times before giving up. But
its a theoretical possibility.

> Did you test it with suspend/hibernation? Because I can't really see
> how it works.

It calls try_to_freeze() after each schedule(). But no, I've not
actually tried.

If we're stuck in the schedule, waiting for the event, freeze_task()
will wake us up and then we'll find freeing() true and call into
__refrigerator().

If we're not stuck in schedule() we're running and the freezer
can see us running and will wait for us to hit another freeze point.

> > +#define __wait_event_freezable(wq, condition)				\
> > +	___wait_event(wq, condition, TASK_INTERRUPTIBLE, 0, 0,		\
> > +			    schedule(); try_to_freeze())
> > +

> _Three_ underscores. And two underscore version exists, too,
> fortunately it at least has different number of arguments.

Hehe, yeah, there was a discount on underscores that day. The double
underscore variants can be used in code along with the no underscores
variants. The tripple one is implementation goo.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [tip:sched/core] sched/wait: Reimplement wait_event_freezable()
  2014-11-05  8:18   ` Peter Zijlstra
@ 2014-11-13 14:03     ` Pavel Machek
  0 siblings, 0 replies; 4+ messages in thread
From: Pavel Machek @ 2014-11-13 14:03 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: len.brown, linux-kernel, tglx, mingo, torvalds, rjw, hpa,
	linux-tip-commits

Hi!

> > Did you test it with suspend/hibernation? Because I can't really see
> > how it works.
> 
> It calls try_to_freeze() after each schedule(). But no, I've not
> actually tried.
> 
> If we're stuck in the schedule, waiting for the event, freeze_task()
> will wake us up and then we'll find freeing() true and call into
> __refrigerator().
> 
> If we're not stuck in schedule() we're running and the freezer
> can see us running and will wait for us to hit another freeze point.

Ok, let me try it here.

> > > +#define __wait_event_freezable(wq, condition)				\
> > > +	___wait_event(wq, condition, TASK_INTERRUPTIBLE, 0, 0,		\
> > > +			    schedule(); try_to_freeze())
> > > +
> 
> > _Three_ underscores. And two underscore version exists, too,
> > fortunately it at least has different number of arguments.
> 
> Hehe, yeah, there was a discount on underscores that day. The double
> underscore variants can be used in code along with the no underscores
> variants. The tripple one is implementation goo.

Lets see, add 5 underscore variant, and we have contestant for
obfuscated C code contest...

Anyway, it seems it works for me (tested suspend, not hibernation).

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

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2014-11-13 14:03 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-04 16:08 [tip:sched/core] sched/wait: Reimplement wait_event_freezable() tip-bot for Peter Zijlstra
2014-11-04 20:12 ` Pavel Machek
2014-11-05  8:18   ` Peter Zijlstra
2014-11-13 14:03     ` Pavel Machek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).