From: Will Deacon <will.deacon@arm.com>
To: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
"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: Wed, 15 Jul 2015 11:51:35 +0100 [thread overview]
Message-ID: <20150715105135.GE1005@arm.com> (raw)
In-Reply-To: <20150715013820.GA21971@linux.vnet.ibm.com>
Hi Paul,
On Wed, Jul 15, 2015 at 02:38:20AM +0100, Paul E. McKenney wrote:
> On Tue, Jul 14, 2015 at 12:31:44PM -0700, Paul E. McKenney wrote:
> > On Tue, Jul 14, 2015 at 03:12:16PM +0100, Will Deacon wrote:
> > > On Tue, Jul 14, 2015 at 03:00:14PM +0100, Paul E. McKenney wrote:
> > > > On Tue, Jul 14, 2015 at 01:51:46PM +0100, Will Deacon wrote:
> > > > > On Tue, Jul 14, 2015 at 01:45:40PM +0100, Paul E. McKenney wrote:
> > > > > > On Tue, Jul 14, 2015 at 11:04:29AM +0100, Will Deacon wrote:
> > > > > > > Given that RCU is currently the only user of this barrier, how would you
> > > > > > > feel about making the barrier local to RCU and not part of the general
> > > > > > > memory-barrier API?
> > > > > >
> > > > > > In theory, no objection. Your thought is to leave the definitions where
> > > > > > they are, mark them as being used only by RCU, and removing mention from
> > > > > > memory-barriers.txt? Or did you have something else in mind?
> > > > >
> > > > > Actually, I was thinking of defining them in an RCU header file with an
> > > > > #ifdef CONFIG_POWERPC for the smb_mb() version. Then you could have a big
> > > > > comment describing the semantics, or put that in an RCU Documentation file
> > > > > instead of memory-barriers.txt.
> > > > >
> > > > > That *should* then mean we notice anybody else trying to use the barrier,
> > > > > because they'd need to send patches to either add something equivalent
> > > > > or move the definition out again.
> > > >
> > > > My concern with this approach is that someone putting together a new
> > > > architecture might miss this. That said, this approach certainly would
> > > > work for the current architectures.
> > >
> > > I don't think they're any more likely to miss it than with the current
> > > situation where the generic code defines the macro as a NOP unless you
> > > explicitly override it.
> >
> > Fair enough...
>
> Like this?
Precisely! Thanks for cooking the patch -- this lays all my worries to
rest, so:
Acked-by: Will Deacon <will.deacon@arm.com>
We should continue the discussion with Ben and Michael about whether or
not the PowerPC locking code can be strengthened, though (making this
barrier a NOP on all currently supported archs).
Will
> commit 695c05d4b9666c50b40a1c022678b5f6e2e3e771
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date: Tue Jul 14 18:35:23 2015 -0700
>
> rcu,locking: Privatize smp_mb__after_unlock_lock()
>
> RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
> likely the only thing that ever will use it, so this commit makes this
> macro private to RCU.
>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: "linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>
>
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index 318523872db5..eafa6a53f72c 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -1854,16 +1854,10 @@ RELEASE are to the same lock variable, but only from the perspective of
> another CPU not holding that lock. In short, a ACQUIRE followed by an
> RELEASE may -not- be assumed to be a full memory barrier.
>
> -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
> -(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:
> +Similarly, the reverse case of a RELEASE followed by an ACQUIRE does
> +not imply a full memory barrier. Therefore, the CPU's execution of the
> +critical sections corresponding to the RELEASE and the ACQUIRE can cross,
> +so that:
>
> *A = a;
> RELEASE M
> @@ -1901,29 +1895,6 @@ the RELEASE would simply complete, thereby avoiding the deadlock.
> a sleep-unlock race, but the locking primitive needs to resolve
> such races properly in any case.
>
> -With smp_mb__after_unlock_lock(), the two critical sections cannot overlap.
> -For example, with the following code, the store to *A will always be
> -seen by other CPUs before the store to *B:
> -
> - *A = a;
> - RELEASE M
> - ACQUIRE N
> - smp_mb__after_unlock_lock();
> - *B = b;
> -
> -The operations will always occur in one of the following orders:
> -
> - STORE *A, RELEASE, ACQUIRE, smp_mb__after_unlock_lock(), STORE *B
> - STORE *A, ACQUIRE, RELEASE, smp_mb__after_unlock_lock(), STORE *B
> - ACQUIRE, STORE *A, RELEASE, smp_mb__after_unlock_lock(), STORE *B
> -
> -If the RELEASE and ACQUIRE were instead both operating on the same lock
> -variable, only the first of these alternatives can occur. In addition,
> -the more strongly ordered systems may rule out some of the above orders.
> -But in any case, as noted earlier, the smp_mb__after_unlock_lock()
> -ensures that the store to *A will always be seen as happening before
> -the store to *B.
> -
> Locks and semaphores may not provide any guarantee of ordering on UP compiled
> systems, and so cannot be counted on in such a situation to actually achieve
> anything at all - especially with respect to I/O accesses - unless combined
> @@ -2154,40 +2125,6 @@ But it won't see any of:
> *E, *F or *G following RELEASE Q
>
>
> -However, if the following occurs:
> -
> - CPU 1 CPU 2
> - =============================== ===============================
> - WRITE_ONCE(*A, a);
> - ACQUIRE M [1]
> - WRITE_ONCE(*B, b);
> - WRITE_ONCE(*C, c);
> - RELEASE M [1]
> - WRITE_ONCE(*D, d); WRITE_ONCE(*E, e);
> - ACQUIRE M [2]
> - smp_mb__after_unlock_lock();
> - WRITE_ONCE(*F, f);
> - WRITE_ONCE(*G, g);
> - RELEASE M [2]
> - WRITE_ONCE(*H, h);
> -
> -CPU 3 might see:
> -
> - *E, ACQUIRE M [1], *C, *B, *A, RELEASE M [1],
> - ACQUIRE M [2], *H, *F, *G, RELEASE M [2], *D
> -
> -But assuming CPU 1 gets the lock first, CPU 3 won't see any of:
> -
> - *B, *C, *D, *F, *G or *H preceding ACQUIRE M [1]
> - *A, *B or *C following RELEASE M [1]
> - *F, *G or *H preceding ACQUIRE M [2]
> - *A, *B, *C, *E, *F or *G following RELEASE M [2]
> -
> -Note that the smp_mb__after_unlock_lock() is critically important
> -here: Without it CPU 3 might see some of the above orderings.
> -Without smp_mb__after_unlock_lock(), the accesses are not guaranteed
> -to be seen in order unless CPU 3 holds lock M.
> -
>
> ACQUIRES VS I/O ACCESSES
> ------------------------
> diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
> index 4dbe072eecbe..523673d7583c 100644
> --- a/arch/powerpc/include/asm/spinlock.h
> +++ b/arch/powerpc/include/asm/spinlock.h
> @@ -28,8 +28,6 @@
> #include <asm/synch.h>
> #include <asm/ppc-opcode.h>
>
> -#define smp_mb__after_unlock_lock() smp_mb() /* Full ordering for lock. */
> -
> #ifdef CONFIG_PPC64
> /* use 0x800000yy when locked, where yy == CPU number */
> #ifdef __BIG_ENDIAN__
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> index 80d974df0ea0..a9fea7395ba2 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -645,3 +645,15 @@ static inline void rcu_nocb_q_lengths(struct rcu_data *rdp, long *ql, long *qll)
> #endif /* #else #ifdef CONFIG_RCU_NOCB_CPU */
> }
> #endif /* #ifdef CONFIG_RCU_TRACE */
> +
> +/*
> + * Place this after a lock-acquisition primitive to guarantee that
> + * an UNLOCK+LOCK pair act as a full barrier. This guarantee applies
> + * if the UNLOCK and LOCK are executed by the same CPU or if the
> + * UNLOCK and LOCK operate on the same lock variable.
> + */
> +#ifdef CONFIG_PPC
> +#define smp_mb__after_unlock_lock() smp_mb() /* Full ordering for lock. */
> +#else /* #ifdef CONFIG_PPC */
> +#define smp_mb__after_unlock_lock() do { } while (0)
> +#endif /* #else #ifdef CONFIG_PPC */
>
next prev parent reply other threads:[~2015-07-15 10:51 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
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 [this message]
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=20150715105135.GE1005@arm.com \
--to=will.deacon@arm.com \
--cc=benh@kernel.crashing.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=paulmck@linux.vnet.ibm.com \
--cc=peterz@infradead.org \
/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.