LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [RESEND PATCH] documentation: memory-barriers: fix smp_mb__before_spinlock() semantics
@ 2015-03-31  8:39 Will Deacon
  2015-03-31 17:50 ` Oleg Nesterov
  2015-04-01 15:26 ` Paul E. McKenney
  0 siblings, 2 replies; 7+ messages in thread
From: Will Deacon @ 2015-03-31  8:39 UTC (permalink / raw)
  To: linux-kernel; +Cc: Will Deacon, Oleg Nesterov, Paul E. McKenney, Peter Zijlstra

Our current documentation claims that, when followed by an ACQUIRE,
smp_mb__before_spinlock() orders prior loads against subsequent loads
and stores, which isn't actually true.

Fix the documentation to state that this sequence orders only prior
stores against subsequent loads and stores.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---

Could somebody pick this up please? I guess I could route it via the arm64
tree with an Ack, but I'd rather it went through Paul or -tip.

 Documentation/memory-barriers.txt | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index ca2387ef27ab..fa28a0c1e2b1 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1768,10 +1768,9 @@ for each construct.  These operations all imply certain barriers:
 
      Memory operations issued before the ACQUIRE may be completed after
      the ACQUIRE operation has completed.  An smp_mb__before_spinlock(),
-     combined with a following ACQUIRE, orders prior loads against
-     subsequent loads and stores and also orders prior stores against
-     subsequent stores.  Note that this is weaker than smp_mb()!  The
-     smp_mb__before_spinlock() primitive is free on many architectures.
+     combined with a following ACQUIRE, orders prior stores against
+     subsequent loads and stores. Note that this is weaker than smp_mb()!
+     The smp_mb__before_spinlock() primitive is free on many architectures.
 
  (2) RELEASE operation implication:
 
-- 
2.1.4


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH] documentation: memory-barriers: fix smp_mb__before_spinlock() semantics
  2015-03-31  8:39 [RESEND PATCH] documentation: memory-barriers: fix smp_mb__before_spinlock() semantics Will Deacon
@ 2015-03-31 17:50 ` Oleg Nesterov
  2015-04-01 15:31   ` Paul E. McKenney
  2015-04-01 15:26 ` Paul E. McKenney
  1 sibling, 1 reply; 7+ messages in thread
From: Oleg Nesterov @ 2015-03-31 17:50 UTC (permalink / raw)
  To: Will Deacon; +Cc: linux-kernel, Paul E. McKenney, Peter Zijlstra

On 03/31, Will Deacon wrote:
>
> Could somebody pick this up please? I guess I could route it via the arm64
> tree with an Ack, but I'd rather it went through Paul or -tip.

I think this would be the best route ;)

> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -1768,10 +1768,9 @@ for each construct.  These operations all imply certain barriers:
>  
>       Memory operations issued before the ACQUIRE may be completed after
>       the ACQUIRE operation has completed.  An smp_mb__before_spinlock(),
> -     combined with a following ACQUIRE, orders prior loads against
> -     subsequent loads and stores and also orders prior stores against
> -     subsequent stores.  Note that this is weaker than smp_mb()!  The
> -     smp_mb__before_spinlock() primitive is free on many architectures.
> +     combined with a following ACQUIRE, orders prior stores against
> +     subsequent loads and stores. Note that this is weaker than smp_mb()!
> +     The smp_mb__before_spinlock() primitive is free on many architectures.

I agree, this description was always wrong.

But perhaps you can also update the comment above smp_mb__before_spinlock?
It only documents the STORE - LOAD serialization, and this was on purpose.

But people started to use this helper assuming that it can also serialize
the STOREs. Perhaps the changelog could also mention this fact, this is why
we need to update this comment and fix memory-barriers.txt.

Oleg.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH] documentation: memory-barriers: fix smp_mb__before_spinlock() semantics
  2015-03-31  8:39 [RESEND PATCH] documentation: memory-barriers: fix smp_mb__before_spinlock() semantics Will Deacon
  2015-03-31 17:50 ` Oleg Nesterov
@ 2015-04-01 15:26 ` Paul E. McKenney
  1 sibling, 0 replies; 7+ messages in thread
From: Paul E. McKenney @ 2015-04-01 15:26 UTC (permalink / raw)
  To: Will Deacon; +Cc: linux-kernel, Oleg Nesterov, Peter Zijlstra, linuxppc-dev

On Tue, Mar 31, 2015 at 09:39:41AM +0100, Will Deacon wrote:
> Our current documentation claims that, when followed by an ACQUIRE,
> smp_mb__before_spinlock() orders prior loads against subsequent loads
> and stores, which isn't actually true.
> 
> Fix the documentation to state that this sequence orders only prior
> stores against subsequent loads and stores.
> 
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> 
> Could somebody pick this up please? I guess I could route it via the arm64
> tree with an Ack, but I'd rather it went through Paul or -tip.

Queued for 4.2, along with a separate patch for PowerPC that make it so
that PowerPC actually behaves as described below.  ;-)

							Thanx, Paul

>  Documentation/memory-barriers.txt | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index ca2387ef27ab..fa28a0c1e2b1 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -1768,10 +1768,9 @@ for each construct.  These operations all imply certain barriers:
> 
>       Memory operations issued before the ACQUIRE may be completed after
>       the ACQUIRE operation has completed.  An smp_mb__before_spinlock(),
> -     combined with a following ACQUIRE, orders prior loads against
> -     subsequent loads and stores and also orders prior stores against
> -     subsequent stores.  Note that this is weaker than smp_mb()!  The
> -     smp_mb__before_spinlock() primitive is free on many architectures.
> +     combined with a following ACQUIRE, orders prior stores against
> +     subsequent loads and stores. Note that this is weaker than smp_mb()!
> +     The smp_mb__before_spinlock() primitive is free on many architectures.
> 
>   (2) RELEASE operation implication:

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

    powerpc: Fix smp_mb__before_spinlock()
    
    Currently, smp_mb__before_spinlock() is defined to be smp_wmb()
    in core code, but this is not sufficient on PowerPC.  This patch
    therefore supplies an override for the generic definition to
    strengthen smp_mb__before_spinlock() to smp_mb(), as is needed
    on PowerPC.
    
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    Cc: <linuxppc-dev@lists.ozlabs.org>

diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
index a3bf5be111ff..1124f59b8df4 100644
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h
@@ -89,5 +89,6 @@ do {									\
 
 #define smp_mb__before_atomic()     smp_mb()
 #define smp_mb__after_atomic()      smp_mb()
+#define smp_mb__before_spinlock()   smp_mb()
 
 #endif /* _ASM_POWERPC_BARRIER_H */


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH] documentation: memory-barriers: fix smp_mb__before_spinlock() semantics
  2015-03-31 17:50 ` Oleg Nesterov
@ 2015-04-01 15:31   ` Paul E. McKenney
  2015-04-01 15:50     ` Oleg Nesterov
  0 siblings, 1 reply; 7+ messages in thread
From: Paul E. McKenney @ 2015-04-01 15:31 UTC (permalink / raw)
  To: Oleg Nesterov; +Cc: Will Deacon, linux-kernel, Peter Zijlstra

On Tue, Mar 31, 2015 at 07:50:50PM +0200, Oleg Nesterov wrote:
> On 03/31, Will Deacon wrote:
> >
> > Could somebody pick this up please? I guess I could route it via the arm64
> > tree with an Ack, but I'd rather it went through Paul or -tip.
> 
> I think this would be the best route ;)
> 
> > --- a/Documentation/memory-barriers.txt
> > +++ b/Documentation/memory-barriers.txt
> > @@ -1768,10 +1768,9 @@ for each construct.  These operations all imply certain barriers:
> >  
> >       Memory operations issued before the ACQUIRE may be completed after
> >       the ACQUIRE operation has completed.  An smp_mb__before_spinlock(),
> > -     combined with a following ACQUIRE, orders prior loads against
> > -     subsequent loads and stores and also orders prior stores against
> > -     subsequent stores.  Note that this is weaker than smp_mb()!  The
> > -     smp_mb__before_spinlock() primitive is free on many architectures.
> > +     combined with a following ACQUIRE, orders prior stores against
> > +     subsequent loads and stores. Note that this is weaker than smp_mb()!
> > +     The smp_mb__before_spinlock() primitive is free on many architectures.
> 
> I agree, this description was always wrong.
> 
> But perhaps you can also update the comment above smp_mb__before_spinlock?
> It only documents the STORE - LOAD serialization, and this was on purpose.
> 
> But people started to use this helper assuming that it can also serialize
> the STOREs. Perhaps the changelog could also mention this fact, this is why
> we need to update this comment and fix memory-barriers.txt.

If Will agrees, like the following?

							Thanx, Paul

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

    documentation: memory-barriers: Fix smp_mb__before_spinlock() semantics
    
    Our current documentation claims that, when followed by an ACQUIRE,
    smp_mb__before_spinlock() orders prior loads against subsequent loads
    and stores, which isn't the intent.  This commit therefore fixes the
    documentation to state that this sequence orders only prior stores
    against subsequent loads and stores.
    
    In addition, the original intent of smp_mb__before_spinlock() was to only
    order prior loads against subsequent stores, however, people have started
    using it as if it ordered prior loads against subsequent loads and stores.
    This commit therefore also updates smp_mb__before_spinlock()'s header
    comment to reflect this new reality.
    
    Cc: Oleg Nesterov <oleg@redhat.com>
    Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Signed-off-by: Will Deacon <will.deacon@arm.com>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 6974f1c2b4e1..52c320e3f107 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1784,10 +1784,9 @@ for each construct.  These operations all imply certain barriers:
 
      Memory operations issued before the ACQUIRE may be completed after
      the ACQUIRE operation has completed.  An smp_mb__before_spinlock(),
-     combined with a following ACQUIRE, orders prior loads against
-     subsequent loads and stores and also orders prior stores against
-     subsequent stores.  Note that this is weaker than smp_mb()!  The
-     smp_mb__before_spinlock() primitive is free on many architectures.
+     combined with a following ACQUIRE, orders prior stores against
+     subsequent loads and stores. Note that this is weaker than smp_mb()!
+     The smp_mb__before_spinlock() primitive is free on many architectures.
 
  (2) RELEASE operation implication:
 
diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 3e18379dfa6f..0063b24b4f36 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -120,7 +120,7 @@ do {								\
 /*
  * Despite its name it doesn't necessarily has to be a full barrier.
  * It should only guarantee that a STORE before the critical section
- * can not be reordered with a LOAD inside this section.
+ * can not be reordered with LOADs and STOREs inside this section.
  * spin_lock() is the one-way barrier, this LOAD can not escape out
  * of the region. So the default implementation simply ensures that
  * a STORE can not move into the critical section, smp_wmb() should


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH] documentation: memory-barriers: fix smp_mb__before_spinlock() semantics
  2015-04-01 15:31   ` Paul E. McKenney
@ 2015-04-01 15:50     ` Oleg Nesterov
  2015-04-01 16:13       ` Will Deacon
  0 siblings, 1 reply; 7+ messages in thread
From: Oleg Nesterov @ 2015-04-01 15:50 UTC (permalink / raw)
  To: Paul E. McKenney; +Cc: Will Deacon, linux-kernel, Peter Zijlstra

On 04/01, Paul E. McKenney wrote:
>
> If Will agrees, like the following?

Looks good to me, thanks ;)

>     documentation: memory-barriers: Fix smp_mb__before_spinlock() semantics
>
>     Our current documentation claims that, when followed by an ACQUIRE,
>     smp_mb__before_spinlock() orders prior loads against subsequent loads
>     and stores, which isn't the intent.  This commit therefore fixes the
>     documentation to state that this sequence orders only prior stores
>     against subsequent loads and stores.
>
>     In addition, the original intent of smp_mb__before_spinlock() was to only
>     order prior loads against subsequent stores, however, people have started
>     using it as if it ordered prior loads against subsequent loads and stores.
>     This commit therefore also updates smp_mb__before_spinlock()'s header
>     comment to reflect this new reality.
>
>     Cc: Oleg Nesterov <oleg@redhat.com>
>     Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
>     Cc: Peter Zijlstra <peterz@infradead.org>
>     Signed-off-by: Will Deacon <will.deacon@arm.com>
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index 6974f1c2b4e1..52c320e3f107 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -1784,10 +1784,9 @@ for each construct.  These operations all imply certain barriers:
>
>       Memory operations issued before the ACQUIRE may be completed after
>       the ACQUIRE operation has completed.  An smp_mb__before_spinlock(),
> -     combined with a following ACQUIRE, orders prior loads against
> -     subsequent loads and stores and also orders prior stores against
> -     subsequent stores.  Note that this is weaker than smp_mb()!  The
> -     smp_mb__before_spinlock() primitive is free on many architectures.
> +     combined with a following ACQUIRE, orders prior stores against
> +     subsequent loads and stores. Note that this is weaker than smp_mb()!
> +     The smp_mb__before_spinlock() primitive is free on many architectures.
>
>   (2) RELEASE operation implication:
>
> diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
> index 3e18379dfa6f..0063b24b4f36 100644
> --- a/include/linux/spinlock.h
> +++ b/include/linux/spinlock.h
> @@ -120,7 +120,7 @@ do {								\
>  /*
>   * Despite its name it doesn't necessarily has to be a full barrier.
>   * It should only guarantee that a STORE before the critical section
> - * can not be reordered with a LOAD inside this section.
> + * can not be reordered with LOADs and STOREs inside this section.
>   * spin_lock() is the one-way barrier, this LOAD can not escape out
>   * of the region. So the default implementation simply ensures that
>   * a STORE can not move into the critical section, smp_wmb() should
>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH] documentation: memory-barriers: fix smp_mb__before_spinlock() semantics
  2015-04-01 15:50     ` Oleg Nesterov
@ 2015-04-01 16:13       ` Will Deacon
  2015-04-01 16:27         ` Paul E. McKenney
  0 siblings, 1 reply; 7+ messages in thread
From: Will Deacon @ 2015-04-01 16:13 UTC (permalink / raw)
  To: Oleg Nesterov; +Cc: Paul E. McKenney, linux-kernel, Peter Zijlstra

On Wed, Apr 01, 2015 at 04:50:55PM +0100, Oleg Nesterov wrote:
> On 04/01, Paul E. McKenney wrote:
> >
> > If Will agrees, like the following?
> 
> Looks good to me, thanks ;)

Same here, thanks for sorting that out Paul.

Will

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND PATCH] documentation: memory-barriers: fix smp_mb__before_spinlock() semantics
  2015-04-01 16:13       ` Will Deacon
@ 2015-04-01 16:27         ` Paul E. McKenney
  0 siblings, 0 replies; 7+ messages in thread
From: Paul E. McKenney @ 2015-04-01 16:27 UTC (permalink / raw)
  To: Will Deacon; +Cc: Oleg Nesterov, linux-kernel, Peter Zijlstra

On Wed, Apr 01, 2015 at 05:13:42PM +0100, Will Deacon wrote:
> On Wed, Apr 01, 2015 at 04:50:55PM +0100, Oleg Nesterov wrote:
> > On 04/01, Paul E. McKenney wrote:
> > >
> > > If Will agrees, like the following?
> > 
> > Looks good to me, thanks ;)
> 
> Same here, thanks for sorting that out Paul.

Very good, thank you both!

							Thanx, Paul


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2015-04-01 16:27 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-31  8:39 [RESEND PATCH] documentation: memory-barriers: fix smp_mb__before_spinlock() semantics Will Deacon
2015-03-31 17:50 ` Oleg Nesterov
2015-04-01 15:31   ` Paul E. McKenney
2015-04-01 15:50     ` Oleg Nesterov
2015-04-01 16:13       ` Will Deacon
2015-04-01 16:27         ` Paul E. McKenney
2015-04-01 15:26 ` Paul E. McKenney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).