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

Krister Johansen
2017-06-09T03:25:50Z

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