[PATCH tip/sched/core] Add comments to aid in safer usage of swake_up.

* [PATCH tip/sched/core] Add comments to aid in safer usage of swake_up.
@ 2017-06-09  3:25 Krister Johansen
  2017-06-09  7:19 ` Peter Zijlstra
  0 siblings, 1 reply; 16+ messages in thread
From: Krister Johansen @ 2017-06-09  3:25 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Paul E. McKenney
  Cc: linux-kernel, Steven Rostedt, Paul Gortmaker, Thomas Gleixner

The behavior of swake_up() differs from that of wake_up(), and from the
swake_up() that came from RT linux. A memory barrier, or some other
synchronization, is needed prior to a swake_up so that the waiter sees
the condition set by the waker, and so that the waker does not see an
empty wait list.

Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
---
 include/linux/swait.h | 27 +++++++++++++++++++++++++++
 kernel/sched/swait.c  | 18 +++++++++++++++++-
 2 files changed, 44 insertions(+), 1 deletion(-)

 This came out of a discussion that Paul McKenney and I had about
 whether other callers of swake_up() knew that they needed to issue
 some kind of barrier prior to invoking swake_up.  In the case of
 wake_up(), the caller will always wake any waiters.  With the simple
 queues, a swait_active occurs locklessly prior to the wakeup so
 additional synchronization may be needed on behalf of the caller.

diff --git a/include/linux/swait.h b/include/linux/swait.h
index c1f9c62..fede974 100644
--- a/include/linux/swait.h
+++ b/include/linux/swait.h
@@ -79,6 +79,33 @@ extern void __init_swait_queue_head(struct swait_queue_head *q, const char *name
 	DECLARE_SWAIT_QUEUE_HEAD(name)
 #endif
 
+/**
+ * swait_active -- locklessly test for waiters on the queue
+ * @q: the swait_queue to test for waiters
+ *
+ * returns true if the wait list is not empty
+ *
+ * NOTE: this function is lockless and requires care, incorrect usage _will_
+ * lead to sporadic and non-obvious failure.
+ *
+ * Use either while holding swait_queue_head_t::lock or when used for wakeups
+ * with an extra smp_mb() like:
+ *
+ *      CPU0 - waker                    CPU1 - waiter
+ *
+ *      @cond = true;
+ *      smp_mb();
+ *      if (swait_active(wq))           swait_event(wq, cond);
+ *        swake_up(wq);
+ *
+ *
+ * Because without the explicit smp_mb() it's possible for the
+ * swait_active() load to get hoisted over the @cond store such that we'll
+ * observe an empty wait list while the waiter might not observe @cond.
+ *
+ * Also note that this 'optimization' trades a spin_lock() for an smp_mb(),
+ * which (when the lock is uncontended) are of roughly equal cost.
+ */
 static inline int swait_active(struct swait_queue_head *q)
 {
 	return !list_empty(&q->task_list);
diff --git a/kernel/sched/swait.c b/kernel/sched/swait.c
index 3d5610d..6e949a8 100644
--- a/kernel/sched/swait.c
+++ b/kernel/sched/swait.c
@@ -29,6 +29,16 @@ void swake_up_locked(struct swait_queue_head *q)
 }
 EXPORT_SYMBOL(swake_up_locked);
 
+/**
+ * swake_up - wake up a waiter on a simple waitqueue
+ * @q: the simple wait queue head
+ *
+ * In order for this to function properly, since it uses swait_active()
+ * internally, some kind of memory barrier must occur prior to calling this.
+ * Typically, this will be smp_mb__after_atomic(), but if the value is
+ * manipulated non-atomically, one may need to use a less regular barrier, such
+ * as smp_mb().  spin_unlock() does not guarantee a memory barrier.
+ */
 void swake_up(struct swait_queue_head *q)
 {
 	unsigned long flags;
@@ -45,7 +55,13 @@ EXPORT_SYMBOL(swake_up);
 /*
  * Does not allow usage from IRQ disabled, since we must be able to
  * release IRQs to guarantee bounded hold time.
- */
+ *
+ * In order for this to function properly, since it uses swait_active()
+ * internally, some kind of memory barrier must occur prior to calling this.
+ * Typically, this will be smp_mb__after_atomic(), but if the value is
+ * manipulated non-atomically, one may need to use a less regular barrier, such
+ * as smp_mb().  spin_unlock() does not guarantee a memory barrier.
+ */
 void swake_up_all(struct swait_queue_head *q)
 {
 	struct swait_queue *curr;
-- 
2.7.4

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