Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754494AbbBQSCF (ORCPT ); Tue, 17 Feb 2015 13:02:05 -0500 Received: from e34.co.us.ibm.com ([32.97.110.152]:45143 "EHLO e34.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751791AbbBQSCC (ORCPT ); Tue, 17 Feb 2015 13:02:02 -0500 Date: Tue, 17 Feb 2015 10:01:54 -0800 From: "Paul E. McKenney" To: Peter Zijlstra Cc: Kirill Tkhai , linux-kernel@vger.kernel.org, Ingo Molnar , Josh Poimboeuf , oleg@redhat.com Subject: Re: [PATCH 2/2] [PATCH] sched: Add smp_rmb() in task rq locking cycles Message-ID: <20150217180154.GA28082@linux.vnet.ibm.com> Reply-To: paulmck@linux.vnet.ibm.com References: <20150217104516.12144.85911.stgit@tkhai> <1424170021.5749.22.camel@tkhai> <20150217121258.GM5029@twins.programming.kicks-ass.net> <20150217130523.GV24151@twins.programming.kicks-ass.net> <20150217160532.GW4166@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150217160532.GW4166@linux.vnet.ibm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15021718-0017-0000-0000-000008CD5D34 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4760 Lines: 131 On Tue, Feb 17, 2015 at 08:05:32AM -0800, Paul E. McKenney wrote: > On Tue, Feb 17, 2015 at 02:05:23PM +0100, Peter Zijlstra wrote: > > On Tue, Feb 17, 2015 at 01:12:58PM +0100, Peter Zijlstra wrote: > > > --- a/kernel/sched/core.c > > > +++ b/kernel/sched/core.c > > > @@ -341,6 +341,22 @@ static struct rq *task_rq_lock(struct ta > > > raw_spin_lock_irqsave(&p->pi_lock, *flags); > > > rq = task_rq(p); > > > raw_spin_lock(&rq->lock); > > > + /* > > > + * move_queued_task() task_rq_lock() > > > + * > > > + * ACQUIRE (rq->lock) > > > + * [S] ->on_rq = MIGRATING [L] rq = task_rq() > > > + * WMB (__set_task_cpu()) ACQUIRE (rq->lock); > > > + * [S] ->cpu = new_cpu [L] task_rq() > > > + * [L] ->on_rq > > > + * RELEASE (rq->lock) > > > + * > > > + * If we observe the old cpu in task_rq_lock, the acquire of > > > + * the old rq->lock will fully serialize against the stores. > > > + * > > > + * If we observe the new cpu in task_rq_lock, the acquire will > > > + * pair with the WMB to ensure we must then also see migrating. > > > + */ > > > if (likely(rq == task_rq(p) && !task_on_rq_migrating(p))) > > > return rq; > > > raw_spin_unlock(&rq->lock); > > > > Hey Paul, remember this: https://lkml.org/lkml/2014/7/16/310 > > I do now. ;-) > > > I just used a creative one :-) > > The scenario above? > > > BTW, should we attempt to include that table in memory-barriers.txt like > > Mathieu said? As a cheat sheet with references to longer explanations > > for the 'interesting' ones? > > > > FWIW, we should probably update that table to include control > > dependencies too; we didn't (formally) have those back then I think. > > > > The blob under SMP BARRIER PAIRING does not mention pairing with control > > dependencies; and I'm rather sure I've done so. And here is a patch for the control-dependency pairing. Thoughts? Thanx, Paul ------------------------------------------------------------------------ documentation: Clarify control-dependency pairing This commit explicitly states that control dependencies pair normally with other barriers, and gives an example of such pairing. Reported-by: Peter Zijlstra Signed-off-by: Paul E. McKenney diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt index ca2387ef27ab..b09880086d96 100644 --- a/Documentation/memory-barriers.txt +++ b/Documentation/memory-barriers.txt @@ -592,9 +592,9 @@ See also the subsection on "Cache Coherency" for a more thorough example. CONTROL DEPENDENCIES -------------------- -A control dependency requires a full read memory barrier, not simply a data -dependency barrier to make it work correctly. Consider the following bit of -code: +A load-load control dependency requires a full read memory barrier, not +simply a data dependency barrier to make it work correctly. Consider the +following bit of code: q = ACCESS_ONCE(a); if (q) { @@ -615,14 +615,15 @@ case what's actually required is: } However, stores are not speculated. This means that ordering -is- provided -in the following example: +for load-store control dependencies, as in the following example: q = ACCESS_ONCE(a); if (q) { ACCESS_ONCE(b) = p; } -Please note that ACCESS_ONCE() is not optional! Without the +Control dependencies pair normally with other types of barriers. +That said, please note that ACCESS_ONCE() is not optional! Without the ACCESS_ONCE(), might combine the load from 'a' with other loads from 'a', and the store to 'b' with other stores to 'b', with possible highly counterintuitive effects on ordering. @@ -813,6 +814,8 @@ In summary: barrier() can help to preserve your control dependency. Please see the Compiler Barrier section for more information. + (*) Control dependencies pair normally with other types of barriers. + (*) Control dependencies do -not- provide transitivity. If you need transitivity, use smp_mb(). @@ -850,6 +853,19 @@ Or: y = *x; +Or even: + + CPU 1 CPU 2 + =============== =============================== + r1 = ACCESS_ONCE(y); + + ACCESS_ONCE(y) = 1; if (r2 = ACCESS_ONCE(x)) { + + ACCESS_ONCE(y) = 1; + } + + assert(r1 == 0 || r2 == 0); + Basically, the read barrier always has to be there, even though it can be of the "weaker" type. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/