From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757440AbbJVM4V (ORCPT ); Thu, 22 Oct 2015 08:56:21 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:60910 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757279AbbJVM4T (ORCPT ); Thu, 22 Oct 2015 08:56:19 -0400 Date: Thu, 22 Oct 2015 14:56:15 +0200 From: Peter Zijlstra To: Kosuke Tatsukawa Cc: Ingo Molnar , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v2] wait: add comment before waitqueue_active noting memory barrier is required Message-ID: <20151022125615.GL3604@twins.programming.kicks-ass.net> References: <17EC94B0A072C34B8DCF0D30AD16044A0287A450@BPXM09GP.gisp.nec.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <17EC94B0A072C34B8DCF0D30AD16044A0287A450@BPXM09GP.gisp.nec.co.jp> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Oct 22, 2015 at 08:01:37AM +0000, Kosuke Tatsukawa wrote: Its somewhat unfortunate you chose the whole wait_woken() thing, its 'rare'. > Second, on the waiting thread side, the CPU can reorder the load of > CONDITION to occur during add_wait_queue active, before the entry is > added to the wait queue. > wake_up thread waiting thread > (reordered) > ------------------------------------------------------------------------ > spin_lock_irqsave(...) > if (CONDITION) > CONDITION = 1; > if (waitqueue_active(wq)) wake_up(); > __add_wait_queue(...) > spin_unlock_irqrestore(...) > wait_woken(&wait, ...); > ------------------------------------------------------------------------ This isn't actually a problem IIRC, because wait_woken() will test WQ_FLAG_WOKEN and not actually sleep. > However, if that is too expensive, the reordering could be prevented by > adding memory barriers in the following places. > wake_up thread waiting thread > ------------------------------------------------------------------------ > CONDITION = 1; add_wait_queue(wq, &wait); > smp_mb(); smp_mb(); > if (waitqueue_active(wq)) for (;;) { > wake_up(wq); if (CONDITION) > break; > wait_woken(&wait, ...); > } So for wait_woken, WQ_FLAG_WOKEN should 'fix' that, and for pretty much anything else you must have a set_current_state() before testing CONDITION and you're good (as you state elsewhere). > +++ b/include/linux/wait.h > @@ -102,6 +102,19 @@ init_waitqueue_func_entry(wait_queue_t *q, wait_queue_func_t func) > q->func = func; > } > > +/* > + * Note: When adding waitqueue_active before calling wake_up for > + * optimization, some sort of memory barrier is required on SMP so > + * that the waiting thread does not miss the wake up. > + * > + * A memory barrier is required before waitqueue_active to prevent > + * waitqueue_active from being reordered by the CPU before any writes > + * done prior to it. > + * > + * The waiting side also needs a memory barrier which pairs with the > + * wake_up side. If prepare_to_wait() or wait_event*() is used, they > + * contain the memory barrier in set_current_state(). > + */ > static inline int waitqueue_active(wait_queue_head_t *q) > { > return !list_empty(&q->task_list); How about something like: /** * waitqueue_active -- locklessly test for waiters on the queue * @q: the waitqueue 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 wait_queue_head_t::lock or when used for * wakeups with an extra smp_mb() like: * * CPU0 - waker CPU1 - waiter * * for (;;) { * @cond = true; prepare_to_wait(&wq, &wait, state); * smp_mb(); /* smp_mb() from set_current_state() */ * if (waitqueue_active(wq)) if (@cond) * wake_up(wq); break; * schedule(); * } * * Because without the explicit smp_mb() its possible for the * waitqueue_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. */ Does that work for you?