Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751578AbdFIDZu (ORCPT ); Thu, 8 Jun 2017 23:25:50 -0400 Received: from sub5.mail.dreamhost.com ([208.113.200.129]:41245 "EHLO homiemail-a124.g.dreamhost.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751469AbdFIDZs (ORCPT ); Thu, 8 Jun 2017 23:25:48 -0400 Date: Thu, 8 Jun 2017 20:25:46 -0700 From: Krister Johansen To: Ingo Molnar , Peter Zijlstra , "Paul E. McKenney" Cc: linux-kernel@vger.kernel.org, Steven Rostedt , Paul Gortmaker , Thomas Gleixner Subject: [PATCH tip/sched/core] Add comments to aid in safer usage of swake_up. Message-ID: <20170609032546.GF2553@templeofstupid.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3713 Lines: 95 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 --- 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