All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>,
	"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>
Subject: Re: [RFC PATCH v2] memory-barriers: remove smp_mb__after_unlock_lock()
Date: Mon, 13 Jul 2015 16:04:06 -0700	[thread overview]
Message-ID: <20150713230405.GB3717@linux.vnet.ibm.com> (raw)
In-Reply-To: <20150713222346.GE19282@twins.programming.kicks-ass.net>

On Tue, Jul 14, 2015 at 12:23:46AM +0200, Peter Zijlstra wrote:
> On Mon, Jul 13, 2015 at 01:20:32PM -0700, Paul E. McKenney wrote:
> > On Mon, Jul 13, 2015 at 06:50:29PM +0100, Will Deacon wrote:
> 
> > > So if I'm following along, smp_mb__after_unlock_lock *does* provide
> > > transitivity when used with UNLOCK + LOCK, which is stronger than your
> > > example here.
> > 
> > Yes, that is indeed the intent.
> 
> Maybe good to state this explicitly somewhere.

Fair enough!  Please see patch below.

> > > I don't think we want to make the same guarantee for general RELEASE +
> > > ACQUIRE, because we'd end up forcing most architectures to implement the
> > > expensive macro for a case that currently has no users.
> > 
> > Agreed, smp_mb__after_unlock_lock() makes a limited guarantee.
> 
> I'm still not seeing how the archs that implement load_acquire and
> store_release with smp_mb() are a problem.

I don't know that I ever claimed such architectures to be a problem.
Color me confused.

> If we look at the inside of the critical section again -- similar
> argument as before:
> 
> 	*A = a
> 	smp_mb()
> 	store M
> 	load N
> 	smp_mb()
> 	*B = b
> 
> A and B are fully ordered, and in this case even transitivity is
> provided.
> 
> I'm stating that the order of M and N don't matter, only the
> load/stores that are inside the acquire/release are constrained.

No argument here.

> IOW, I think smp_mb__after_unlock_lock() already works as advertised
> with all our acquire/release thingies -- as is stated by the
> documentation.
> 
> That said, I'm not aware of anybody but RCU actually using this, so its
> not used in that capacity.

OK, I might actually understand what you are getting at.  And, yes, if
someone actually comes up with a need to combine smp_mb__after_unlock_lock()
with something other than locking, we should worry about it at that point.
And probably rename smp_mb__after_unlock_lock() at that point, as well.
Until then, why lock ourselves into semantics that no one needs, and
that it is quite possible that no one will ever need?

> > > In which case, it boils down to the question of how expensive it would
> > > be to implement an SC UNLOCK operation on PowerPC and whether that justifies
> > > the existence of a complicated barrier macro that isn't used outside of
> > > RCU.
> > 
> > Given that it is either smp_mb() or nothing, I am not seeing the
> > "complicated" part...
> 
> The 'complicated' part is that we need think about it; that is we need
> to realized and remember that UNLOCK+LOCK is a load-store barrier but
> fails to provide transitivity.

...  unless you are holding the lock.  So in the common case, you do
get transitivity.

							Thanx, Paul

------------------------------------------------------------------------

commit bae5cf1e2973bb1e8f852abb54f8b1948ffd82e4
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Mon Jul 13 15:55:52 2015 -0700

    doc: Call out smp_mb__after_unlock_lock() transitivity
    
    Although "full barrier" should be interpreted as providing transitivity,
    it is worth eliminating any possible confusion.  This commit therefore
    adds "(including transitivity)" to eliminate any possible confusion.
    
    Reported-by: Peter Zijlstra <peterz@infradead.org>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 470c07c868e4..318523872db5 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1858,11 +1858,12 @@ Similarly, the reverse case of a RELEASE followed by an ACQUIRE does not
 imply a full memory barrier.  If it is necessary for a RELEASE-ACQUIRE
 pair to produce a full barrier, the ACQUIRE can be followed by an
 smp_mb__after_unlock_lock() invocation.  This will produce a full barrier
-if either (a) the RELEASE and the ACQUIRE are executed by the same
-CPU or task, or (b) the RELEASE and ACQUIRE act on the same variable.
-The smp_mb__after_unlock_lock() primitive is free on many architectures.
-Without smp_mb__after_unlock_lock(), the CPU's execution of the critical
-sections corresponding to the RELEASE and the ACQUIRE can cross, so that:
+(including transitivity) if either (a) the RELEASE and the ACQUIRE are
+executed by the same CPU or task, or (b) the RELEASE and ACQUIRE act on
+the same variable.  The smp_mb__after_unlock_lock() primitive is free
+on many architectures.  Without smp_mb__after_unlock_lock(), the CPU's
+execution of the critical sections corresponding to the RELEASE and the
+ACQUIRE can cross, so that:
 
 	*A = a;
 	RELEASE M


  reply	other threads:[~2015-07-13 23:04 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-13 12:15 [RFC PATCH v2] memory-barriers: remove smp_mb__after_unlock_lock() Will Deacon
2015-07-13 13:09 ` Peter Hurley
2015-07-13 14:24   ` Will Deacon
2015-07-13 15:56     ` Peter Zijlstra
2015-07-13 13:11 ` Peter Zijlstra
2015-07-13 14:09   ` Will Deacon
2015-07-13 14:21     ` Will Deacon
2015-07-13 15:54       ` Peter Zijlstra
2015-07-13 17:50         ` Will Deacon
2015-07-13 20:20           ` Paul E. McKenney
2015-07-13 22:23             ` Peter Zijlstra
2015-07-13 23:04               ` Paul E. McKenney [this message]
2015-07-14 10:04                 ` Will Deacon
2015-07-14 12:45                   ` Paul E. McKenney
2015-07-14 12:51                     ` Will Deacon
2015-07-14 14:00                       ` Paul E. McKenney
2015-07-14 14:12                         ` Will Deacon
2015-07-14 19:31                           ` Paul E. McKenney
2015-07-15  1:38                             ` Paul E. McKenney
2015-07-15 10:51                               ` Will Deacon
2015-07-15 13:12                                 ` Paul E. McKenney
2015-07-24 11:31                                   ` Will Deacon
2015-07-24 15:30                                     ` Paul E. McKenney
2015-08-12 13:44                                       ` Will Deacon
2015-08-12 15:43                                         ` Paul E. McKenney
2015-08-12 17:59                                           ` Paul E. McKenney
2015-08-13 10:49                                             ` Will Deacon
2015-08-13 13:10                                               ` Paul E. McKenney
2015-08-17  4:06                                           ` Michael Ellerman
2015-08-17  6:15                                             ` Paul E. McKenney
2015-08-17  8:57                                               ` Will Deacon
2015-08-18  1:50                                                 ` Michael Ellerman
2015-08-18  8:37                                                   ` Will Deacon
2015-08-20  9:45                                                     ` Michael Ellerman
2015-08-20 15:56                                                       ` Will Deacon
2015-08-26  0:27                                                         ` Paul E. McKenney
2015-08-26  4:06                                                           ` Michael Ellerman
2015-07-13 18:23         ` Paul E. McKenney
2015-07-13 19:41           ` Peter Hurley
2015-07-13 20:16             ` Paul E. McKenney
2015-07-13 22:15               ` Peter Zijlstra
2015-07-13 22:43                 ` Benjamin Herrenschmidt
2015-07-14  8:34                   ` Peter Zijlstra
2015-07-13 22:53                 ` Paul E. McKenney
2015-07-13 22:37         ` Benjamin Herrenschmidt
2015-07-13 22:31 ` Benjamin Herrenschmidt
2015-07-14 10:16   ` Will Deacon
2015-07-15  3:06   ` Michael Ellerman
2015-07-15 10:44     ` Will Deacon
2015-07-16  2:00       ` Michael Ellerman
2015-07-16  5:03         ` Benjamin Herrenschmidt
2015-07-16  5:14           ` Benjamin Herrenschmidt
2015-07-16 15:11             ` Paul E. McKenney
2015-07-16 22:54               ` Benjamin Herrenschmidt
2015-07-17  9:32                 ` Will Deacon
2015-07-17 10:15                   ` Peter Zijlstra
2015-07-17 12:40                     ` Paul E. McKenney
2015-07-17 22:14                   ` Benjamin Herrenschmidt
2015-07-20 13:39                     ` Will Deacon
2015-07-20 13:48                       ` Paul E. McKenney
2015-07-20 13:56                         ` Will Deacon
2015-07-20 21:18                       ` Benjamin Herrenschmidt
2015-07-22 16:49                         ` Will Deacon
2015-07-22 16:49                           ` Will Deacon
2015-07-22 16:49                           ` Will Deacon
2015-09-01  2:57             ` Paul Mackerras
2015-07-15 14:18     ` Paul E. McKenney
2015-07-16  1:34       ` Michael Ellerman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150713230405.GB3717@linux.vnet.ibm.com \
    --to=paulmck@linux.vnet.ibm.com \
    --cc=benh@kernel.crashing.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=will.deacon@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.