Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1422419 > unrolled thread

[RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier Waiman Long <Waiman.Long@hpe.com> - 2016-06-15 00:50 +0200
    Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Boqun Feng <boqun.feng@gmail.com> - 2016-06-15 10:10 +0200
      Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Peter Zijlstra <peterz@infradead.org> - 2016-06-15 19:20 +0200
      Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Boqun Feng <boqun.feng@gmail.com> - 2016-06-16 04:20 +0200
        Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Will Deacon <will.deacon@arm.com> - 2016-06-16 12:20 +0200
        Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Boqun Feng <boqun.feng@gmail.com> - 2016-06-17 02:50 +0200
          Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Will Deacon <will.deacon@arm.com> - 2016-06-17 17:50 +0200
            Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Boqun Feng <boqun.feng@gmail.com> - 2016-06-18 10:50 +0200
              Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Will Deacon <will.deacon@arm.com> - 2016-06-20 10:10 +0200
    Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Davidlohr Bueso <dave@stgolabs.net> - 2016-06-15 19:00 +0200
      Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Peter Zijlstra <peterz@infradead.org> - 2016-06-15 19:20 +0200
        Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Davidlohr Bueso <dave@stgolabs.net> - 2016-06-15 20:30 +0200
          Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Peter Zijlstra <peterz@infradead.org> - 2016-06-15 20:50 +0200
            Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Davidlohr Bueso <dave@stgolabs.net> - 2016-06-15 21:00 +0200
            Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Davidlohr Bueso <dave@stgolabs.net> - 2016-06-17 03:20 +0200
              Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Davidlohr Bueso <dave@stgolabs.net> - 2016-06-17 18:30 +0200
                Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Davidlohr Bueso <dave@stgolabs.net> - 2016-06-17 18:50 +0200
        Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper  acquire/release barrier Peter Zijlstra <peterz@infradead.org> - 2016-06-16 00:00 +0200

#1422419 — [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

The osq_lock() and osq_unlock() function may not provide the necessary
acquire and release barrier in some cases. This patch makes sure
that the proper barriers are provided when osq_lock() is successful
or when osq_unlock() is called.

Signed-off-by: Waiman Long <Waiman.Long@hpe.com>
---
 kernel/locking/osq_lock.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
index 05a3785..7dd4ee5 100644
--- a/kernel/locking/osq_lock.c
+++ b/kernel/locking/osq_lock.c
@@ -115,7 +115,7 @@ bool osq_lock(struct optimistic_spin_queue *lock)
 	 * cmpxchg in an attempt to undo our queueing.
 	 */
 
-	while (!READ_ONCE(node->locked)) {
+	while (!smp_load_acquire(&node->locked)) {
 		/*
 		 * If we need to reschedule bail... so we can block.
 		 */
@@ -198,7 +198,7 @@ void osq_unlock(struct optimistic_spin_queue *lock)
 	 * Second most likely case.
 	 */
 	node = this_cpu_ptr(&osq_node);
-	next = xchg(&node->next, NULL);
+	next = xchg_release(&node->next, NULL);
 	if (next) {
 		WRITE_ONCE(next->locked, 1);
 		return;
-- 
1.7.1

[toc] | [next] | [standalone]

#1422766 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

[Multipart message — attachments visible in raw view] — view raw

Hi Waiman,

On Tue, Jun 14, 2016 at 06:48:04PM -0400, Waiman Long wrote:
> The osq_lock() and osq_unlock() function may not provide the necessary
> acquire and release barrier in some cases. This patch makes sure
> that the proper barriers are provided when osq_lock() is successful
> or when osq_unlock() is called.
> 
> Signed-off-by: Waiman Long <Waiman.Long@hpe.com>
> ---
>  kernel/locking/osq_lock.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
> index 05a3785..7dd4ee5 100644
> --- a/kernel/locking/osq_lock.c
> +++ b/kernel/locking/osq_lock.c
> @@ -115,7 +115,7 @@ bool osq_lock(struct optimistic_spin_queue *lock)
>  	 * cmpxchg in an attempt to undo our queueing.
>  	 */
>  
> -	while (!READ_ONCE(node->locked)) {
> +	while (!smp_load_acquire(&node->locked)) {
>  		/*
>  		 * If we need to reschedule bail... so we can block.
>  		 */
> @@ -198,7 +198,7 @@ void osq_unlock(struct optimistic_spin_queue *lock)
>  	 * Second most likely case.
>  	 */
>  	node = this_cpu_ptr(&osq_node);
> -	next = xchg(&node->next, NULL);
> +	next = xchg_release(&node->next, NULL);
>  	if (next) {
>  		WRITE_ONCE(next->locked, 1);

So we still use WRITE_ONCE() rather than smp_store_release() here?

Though, IIUC, This is fine for all the archs but ARM64, because there
will always be a xchg_release()/xchg() before the WRITE_ONCE(), which
carries a necessary barrier to upgrade WRITE_ONCE() to a RELEASE.

Not sure whether it's a problem on ARM64, but I think we certainly need
to add some comments here, if we count on this trick.

Am I missing something or misunderstanding you here?

Regards,
Boqun

>  		return;
> -- 
> 1.7.1
> 

[toc] | [prev] | [next] | [standalone]

#1423249 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

On Wed, Jun 15, 2016 at 04:04:46PM +0800, Boqun Feng wrote:
> On Tue, Jun 14, 2016 at 06:48:04PM -0400, Waiman Long wrote:

> > @@ -198,7 +198,7 @@ void osq_unlock(struct optimistic_spin_queue *lock)
> >  	 * Second most likely case.
> >  	 */
> >  	node = this_cpu_ptr(&osq_node);
> > -	next = xchg(&node->next, NULL);
> > +	next = xchg_release(&node->next, NULL);
> >  	if (next) {
> >  		WRITE_ONCE(next->locked, 1);
> 
> So we still use WRITE_ONCE() rather than smp_store_release() here?
> 
> Though, IIUC, This is fine for all the archs but ARM64, because there
> will always be a xchg_release()/xchg() before the WRITE_ONCE(), which
> carries a necessary barrier to upgrade WRITE_ONCE() to a RELEASE.

Not sure. On PPC for example, you'll use lwsync() but will that not
attach to the store to &node->next instead?

Still leaving that store and the WRITE_ONCE() unordered.

Also I don't see the control dependency between xchg-load and WRITE_ONCE
helping anything to order the two stores.


So yeah, subtle if not broken, definitely needs more explanation.

[toc] | [prev] | [next] | [standalone]

#1423643 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

[Multipart message — attachments visible in raw view] — view raw

On Wed, Jun 15, 2016 at 03:01:19PM -0400, Waiman Long wrote:
> On 06/15/2016 04:04 AM, Boqun Feng wrote:
> > Hi Waiman,
> > 
> > On Tue, Jun 14, 2016 at 06:48:04PM -0400, Waiman Long wrote:
> > > The osq_lock() and osq_unlock() function may not provide the necessary
> > > acquire and release barrier in some cases. This patch makes sure
> > > that the proper barriers are provided when osq_lock() is successful
> > > or when osq_unlock() is called.
> > > 
> > > Signed-off-by: Waiman Long<Waiman.Long@hpe.com>
> > > ---
> > >   kernel/locking/osq_lock.c |    4 ++--
> > >   1 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
> > > index 05a3785..7dd4ee5 100644
> > > --- a/kernel/locking/osq_lock.c
> > > +++ b/kernel/locking/osq_lock.c
> > > @@ -115,7 +115,7 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> > >   	 * cmpxchg in an attempt to undo our queueing.
> > >   	 */
> > > 
> > > -	while (!READ_ONCE(node->locked)) {
> > > +	while (!smp_load_acquire(&node->locked)) {
> > >   		/*
> > >   		 * If we need to reschedule bail... so we can block.
> > >   		 */
> > > @@ -198,7 +198,7 @@ void osq_unlock(struct optimistic_spin_queue *lock)
> > >   	 * Second most likely case.
> > >   	 */
> > >   	node = this_cpu_ptr(&osq_node);
> > > -	next = xchg(&node->next, NULL);
> > > +	next = xchg_release(&node->next, NULL);
> > >   	if (next) {
> > >   		WRITE_ONCE(next->locked, 1);
> > So we still use WRITE_ONCE() rather than smp_store_release() here?
> > 
> > Though, IIUC, This is fine for all the archs but ARM64, because there
> > will always be a xchg_release()/xchg() before the WRITE_ONCE(), which
> > carries a necessary barrier to upgrade WRITE_ONCE() to a RELEASE.
> > 
> > Not sure whether it's a problem on ARM64, but I think we certainly need
> > to add some comments here, if we count on this trick.
> > 
> > Am I missing something or misunderstanding you here?
> > 
> > Regards,
> > Boqun
> 
> The change on the unlock side is more for documentation purpose than is
> actually needed. As you had said, the xchg() call has provided the necessary
> memory barrier. Using the _release variant, however, may have some

But I'm afraid the barrier doesn't remain if we replace xchg() with
xchg_release() on ARM64v8, IIUC, xchg_release() is just a ldxr+stlxr
loop with no barrier on ARM64v8. This means the following code:

	CPU 0					CPU 1 (next)
	========================		==================
	WRITE_ONCE(x, 1);			r1 = smp_load_acquire(next->locked, 1);
	xchg_release(&node->next, NULL);	r2 = READ_ONCE(x);
	WRITE_ONCE(next->locked, 1);

could result in (r1 == 1 && r2 == 0) on ARM64v8, IIUC.

I translated it into a litmus test:

	AArch64 stlxr+str
	""
	{
	0:X4=x; 0:X5=node; node=next;
	1:X4=x; 1:X5=next;
	}
	 P0              | P1          ;
	 MOV W0,#1       | LDAR W1,[X5];
	 STR W0,[X4]     | LDR W2,[X4] ;
	 MOV X0,#0       |             ;
	 LDXR X2,[X5]    |             ;
	 STLXR W1,X0,[X5]|             ;
	 CBNZ W1, fail   |             ;
	 MOV W0, #1      |             ;
	 STR W0,[X2]     |             ;
	 fail:           |             ;

	exists
	(0:X0 = 1 /\ 1:X1 = 1 /\ 1:X2 = 0)

and herd said "Sometimes".

But I may miss something here or make a mistake in the translation. So
add Will in Cc list ;-)

> performance benefit in some architectures.
> 
> BTW, osq_lock/osq_unlock aren't general purpose locking primitives. So there
> is some leeways on how fancy we want on the lock and unlock sides.
> 

Understood, I think it's fine if we rely on something subtle here, but
I just want to make we won't be bitten by some corner cases.

Regards,
Boqun

> Cheers,
> Longman

[toc] | [prev] | [next] | [standalone]

#1423907 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

Hi guys,

On Thu, Jun 16, 2016 at 10:19:51AM +0800, Boqun Feng wrote:
> On Wed, Jun 15, 2016 at 03:01:19PM -0400, Waiman Long wrote:
> > On 06/15/2016 04:04 AM, Boqun Feng wrote:
> > > On Tue, Jun 14, 2016 at 06:48:04PM -0400, Waiman Long wrote:
> > > > @@ -198,7 +198,7 @@ void osq_unlock(struct optimistic_spin_queue *lock)
> > > >   	 * Second most likely case.
> > > >   	 */
> > > >   	node = this_cpu_ptr(&osq_node);
> > > > -	next = xchg(&node->next, NULL);
> > > > +	next = xchg_release(&node->next, NULL);
> > > >   	if (next) {
> > > >   		WRITE_ONCE(next->locked, 1);
> > > So we still use WRITE_ONCE() rather than smp_store_release() here?
> > > 
> > > Though, IIUC, This is fine for all the archs but ARM64, because there
> > > will always be a xchg_release()/xchg() before the WRITE_ONCE(), which
> > > carries a necessary barrier to upgrade WRITE_ONCE() to a RELEASE.
> > > 
> > > Not sure whether it's a problem on ARM64, but I think we certainly need
> > > to add some comments here, if we count on this trick.
> > > 
> > > Am I missing something or misunderstanding you here?
> > > 
> > The change on the unlock side is more for documentation purpose than is
> > actually needed. As you had said, the xchg() call has provided the necessary
> > memory barrier. Using the _release variant, however, may have some
> 
> But I'm afraid the barrier doesn't remain if we replace xchg() with
> xchg_release() on ARM64v8, IIUC, xchg_release() is just a ldxr+stlxr
> loop with no barrier on ARM64v8. This means the following code:
> 
> 	CPU 0					CPU 1 (next)
> 	========================		==================
> 	WRITE_ONCE(x, 1);			r1 = smp_load_acquire(next->locked, 1);
> 	xchg_release(&node->next, NULL);	r2 = READ_ONCE(x);
> 	WRITE_ONCE(next->locked, 1);
> 
> could result in (r1 == 1 && r2 == 0) on ARM64v8, IIUC.

Yes, of course. Why is that unexpected? You could just as easily make
the xchg_release an smp_store_release and this would still be permitted,
that's the whole point of acquire/release -- they're semi-permeable
barriers that allow accesses outside of the critical section to leak in,
but not the other way around.

It's worth noting that you've omitted the control dependency from
xchg_release to the subsequent write in your litmus tests, but I don't
think that actually changes anything here.

Will

[toc] | [prev] | [next] | [standalone]

#1424568 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jun 16, 2016 at 05:35:54PM -0400, Waiman Long wrote:
> On 06/15/2016 10:19 PM, Boqun Feng wrote:
> > On Wed, Jun 15, 2016 at 03:01:19PM -0400, Waiman Long wrote:
> > > On 06/15/2016 04:04 AM, Boqun Feng wrote:
> > > > Hi Waiman,
> > > > 
> > > > On Tue, Jun 14, 2016 at 06:48:04PM -0400, Waiman Long wrote:
> > > > > The osq_lock() and osq_unlock() function may not provide the necessary
> > > > > acquire and release barrier in some cases. This patch makes sure
> > > > > that the proper barriers are provided when osq_lock() is successful
> > > > > or when osq_unlock() is called.
> > > > > 
> > > > > Signed-off-by: Waiman Long<Waiman.Long@hpe.com>
> > > > > ---
> > > > >    kernel/locking/osq_lock.c |    4 ++--
> > > > >    1 files changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
> > > > > index 05a3785..7dd4ee5 100644
> > > > > --- a/kernel/locking/osq_lock.c
> > > > > +++ b/kernel/locking/osq_lock.c
> > > > > @@ -115,7 +115,7 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> > > > >    	 * cmpxchg in an attempt to undo our queueing.
> > > > >    	 */
> > > > > 
> > > > > -	while (!READ_ONCE(node->locked)) {
> > > > > +	while (!smp_load_acquire(&node->locked)) {
> > > > >    		/*
> > > > >    		 * If we need to reschedule bail... so we can block.
> > > > >    		 */
> > > > > @@ -198,7 +198,7 @@ void osq_unlock(struct optimistic_spin_queue *lock)
> > > > >    	 * Second most likely case.
> > > > >    	 */
> > > > >    	node = this_cpu_ptr(&osq_node);
> > > > > -	next = xchg(&node->next, NULL);
> > > > > +	next = xchg_release(&node->next, NULL);
> > > > >    	if (next) {
> > > > >    		WRITE_ONCE(next->locked, 1);
> > > > So we still use WRITE_ONCE() rather than smp_store_release() here?
> > > > 
> > > > Though, IIUC, This is fine for all the archs but ARM64, because there
> > > > will always be a xchg_release()/xchg() before the WRITE_ONCE(), which
> > > > carries a necessary barrier to upgrade WRITE_ONCE() to a RELEASE.
> > > > 
> > > > Not sure whether it's a problem on ARM64, but I think we certainly need
> > > > to add some comments here, if we count on this trick.
> > > > 
> > > > Am I missing something or misunderstanding you here?
> > > > 
> > > > Regards,
> > > > Boqun
> > > The change on the unlock side is more for documentation purpose than is
> > > actually needed. As you had said, the xchg() call has provided the necessary
> > > memory barrier. Using the _release variant, however, may have some
> > But I'm afraid the barrier doesn't remain if we replace xchg() with
> > xchg_release() on ARM64v8, IIUC, xchg_release() is just a ldxr+stlxr
> > loop with no barrier on ARM64v8. This means the following code:
> > 
> > 	CPU 0					CPU 1 (next)
> > 	========================		==================
> > 	WRITE_ONCE(x, 1);			r1 = smp_load_acquire(next->locked, 1);
> > 	xchg_release(&node->next, NULL);	r2 = READ_ONCE(x);
> > 	WRITE_ONCE(next->locked, 1);
> > 
> > could result in (r1 == 1&&  r2 == 0) on ARM64v8, IIUC.
> 
> If you look into the actual code:
> 
>         next = xchg_release(&node->next, NULL);
>         if (next) {
>                 WRITE_ONCE(next->locked, 1);
>                 return;
>         }
> 
> There is a control dependency that WRITE_ONCE() won't happen until

But a control dependency only orders LOAD->STORE pairs, right? And here
the control dependency orders the LOAD part of xchg_release() and the
WRITE_ONCE().

Along with the fact that RELEASE only orders the STORE part of xchg with
the memory operations preceding the STORE part, so for the following
code:

	WRTIE_ONCE(x,1);
	next = xchg_release(&node->next, NULL);
	if (next)
		WRITE_ONCE(next->locked, 1);

such a reordering is allowed to happen on ARM64v8

	next = ldxr [&node->next] // LOAD part of xchg_release()

	if (next)
		WRITE_ONCE(next->locked, 1);

	WRITE_ONCE(x,1);
	stlxr NULL [&node->next]  // STORE part of xchg_releae()

Am I missing your point here?

Regards,
Boqun

> xchg_release() returns. For your particular example, I will change it to
> 
>     CPU 0
>     ===================
>     WRITE_ONCE(x, 1);
>     xchg_relaxed(&node->next, NULL);
>     smp_store_release(next->locked, 1);
> 
> I don't change WRITE_ONCE to a smp_store_release() because it may not always
> execute.
> 
> Cheers,
> Longman
> 

[toc] | [prev] | [next] | [standalone]

#1425258 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

On Fri, Jun 17, 2016 at 11:26:41AM -0400, Waiman Long wrote:
> On 06/16/2016 08:48 PM, Boqun Feng wrote:
> >On Thu, Jun 16, 2016 at 05:35:54PM -0400, Waiman Long wrote:
> >>If you look into the actual code:
> >>
> >>         next = xchg_release(&node->next, NULL);
> >>         if (next) {
> >>                 WRITE_ONCE(next->locked, 1);
> >>                 return;
> >>         }
> >>
> >>There is a control dependency that WRITE_ONCE() won't happen until
> >But a control dependency only orders LOAD->STORE pairs, right? And here
> >the control dependency orders the LOAD part of xchg_release() and the
> >WRITE_ONCE().
> >
> >Along with the fact that RELEASE only orders the STORE part of xchg with
> >the memory operations preceding the STORE part, so for the following
> >code:
> >
> >	WRTIE_ONCE(x,1);
> >	next = xchg_release(&node->next, NULL);
> >	if (next)
> >		WRITE_ONCE(next->locked, 1);
> >
> >such a reordering is allowed to happen on ARM64v8
> >
> >	next = ldxr [&node->next] // LOAD part of xchg_release()
> >
> >	if (next)
> >		WRITE_ONCE(next->locked, 1);
> >
> >	WRITE_ONCE(x,1);
> >	stlxr NULL [&node->next]  // STORE part of xchg_releae()
> >
> >Am I missing your point here?
> 
> My understanding of the release barrier is that both prior LOADs and STOREs
> can't move after the barrier. If WRITE_ONCE(x, 1) can move to below as shown
> above, it is not a real release barrier and we may need to change the
> barrier code.

You seem to be missing the point.

{READ,WRITE}_ONCE accesses appearing in program order after a release
are not externally ordered with respect to the release unless they
access the same location.

This is illustrated by Boqun's example, which shows two WRITE_ONCE
accesses being reordered before a store-release forming the write
component of an xchg_release. In both cases, WRITE_ONCE(x, 1) remains
ordered before the store-release.

Will

[toc] | [prev] | [next] | [standalone]

#1425674 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jun 17, 2016 at 02:17:27PM -0400, Waiman Long wrote:
> On 06/17/2016 11:45 AM, Will Deacon wrote:
> > On Fri, Jun 17, 2016 at 11:26:41AM -0400, Waiman Long wrote:
> > > On 06/16/2016 08:48 PM, Boqun Feng wrote:
> > > > On Thu, Jun 16, 2016 at 05:35:54PM -0400, Waiman Long wrote:
> > > > > If you look into the actual code:
> > > > > 
> > > > >          next = xchg_release(&node->next, NULL);
> > > > >          if (next) {
> > > > >                  WRITE_ONCE(next->locked, 1);
> > > > >                  return;
> > > > >          }
> > > > > 
> > > > > There is a control dependency that WRITE_ONCE() won't happen until
> > > > But a control dependency only orders LOAD->STORE pairs, right? And here
> > > > the control dependency orders the LOAD part of xchg_release() and the
> > > > WRITE_ONCE().
> > > > 
> > > > Along with the fact that RELEASE only orders the STORE part of xchg with
> > > > the memory operations preceding the STORE part, so for the following
> > > > code:
> > > > 
> > > > 	WRTIE_ONCE(x,1);
> > > > 	next = xchg_release(&node->next, NULL);
> > > > 	if (next)
> > > > 		WRITE_ONCE(next->locked, 1);
> > > > 
> > > > such a reordering is allowed to happen on ARM64v8
> > > > 
> > > > 	next = ldxr [&node->next] // LOAD part of xchg_release()
> > > > 
> > > > 	if (next)
> > > > 		WRITE_ONCE(next->locked, 1);
> > > > 
> > > > 	WRITE_ONCE(x,1);
> > > > 	stlxr NULL [&node->next]  // STORE part of xchg_releae()
> > > > 
> > > > Am I missing your point here?
> > > My understanding of the release barrier is that both prior LOADs and STOREs
> > > can't move after the barrier. If WRITE_ONCE(x, 1) can move to below as shown
> > > above, it is not a real release barrier and we may need to change the
> > > barrier code.
> > You seem to be missing the point.
> > 
> > {READ,WRITE}_ONCE accesses appearing in program order after a release
> > are not externally ordered with respect to the release unless they
> > access the same location.
> > 
> > This is illustrated by Boqun's example, which shows two WRITE_ONCE
> > accesses being reordered before a store-release forming the write
> > component of an xchg_release. In both cases, WRITE_ONCE(x, 1) remains
> > ordered before the store-release.
> > 
> > Will
> 
> I am sorry that I misread the mail. I am not used to treating xchg as two
> separate instructions. Yes, it is a problem. In that case, we have to either

And sorry for the Red Pill ;-)

> keep the xchg() function as it is or use smp_store_release(&next->locked,
> 1). So which one is a better alternative for ARM or PPC?
> 

For PPC, I think xchg_release() + smp_store_release() is better than the 
current code, because the former has two lwsync while the latter has two
sync, and sync is quite expensive than lwsync on PPC.

I need to leave the ARM part to Will ;-)

Regards,
Boqun

> Cheers,
> Longman

[toc] | [prev] | [next] | [standalone]

#1426309 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

On Sat, Jun 18, 2016 at 04:46:20PM +0800, Boqun Feng wrote:
> On Fri, Jun 17, 2016 at 02:17:27PM -0400, Waiman Long wrote:
> > keep the xchg() function as it is or use smp_store_release(&next->locked,
> > 1). So which one is a better alternative for ARM or PPC?
> > 
> 
> For PPC, I think xchg_release() + smp_store_release() is better than the 
> current code, because the former has two lwsync while the latter has two
> sync, and sync is quite expensive than lwsync on PPC.
> 
> I need to leave the ARM part to Will ;-)

I doubt there's much in it, but xchg() has DMB + release, so xchg_release +
smp_store_release is probably slightly better for us too.

Will

[toc] | [prev] | [next] | [standalone]

#1423234 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

On Tue, 14 Jun 2016, Waiman Long wrote:

>The osq_lock() and osq_unlock() function may not provide the necessary
>acquire and release barrier in some cases. This patch makes sure
>that the proper barriers are provided when osq_lock() is successful
>or when osq_unlock() is called.
>
>Signed-off-by: Waiman Long <Waiman.Long@hpe.com>
>---
> kernel/locking/osq_lock.c |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
>index 05a3785..7dd4ee5 100644
>--- a/kernel/locking/osq_lock.c
>+++ b/kernel/locking/osq_lock.c
>@@ -115,7 +115,7 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> 	 * cmpxchg in an attempt to undo our queueing.
> 	 */
>
>-	while (!READ_ONCE(node->locked)) {
>+	while (!smp_load_acquire(&node->locked)) {

Hmm this being a polling path, that barrier can get pretty expensive and
last I checked it was unnecessary:

036cc30c6b6 (locking/osq: No need for load/acquire when acquire-polling)

Thanks,
Davidlohr

[toc] | [prev] | [next] | [standalone]

#1423250 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

On Wed, Jun 15, 2016 at 09:56:59AM -0700, Davidlohr Bueso wrote:
> On Tue, 14 Jun 2016, Waiman Long wrote:

> >+++ b/kernel/locking/osq_lock.c
> >@@ -115,7 +115,7 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> >	 * cmpxchg in an attempt to undo our queueing.
> >	 */
> >
> >-	while (!READ_ONCE(node->locked)) {
> >+	while (!smp_load_acquire(&node->locked)) {
> 
> Hmm this being a polling path, that barrier can get pretty expensive and
> last I checked it was unnecessary:

I think he'll go rely on it later on.

In any case, its fairly simple to cure, just add
smp_acquire__after_ctrl_dep() at the end. If we bail because
need_resched() we don't need the acquire I think.

[toc] | [prev] | [next] | [standalone]

#1423306 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

On Wed, 15 Jun 2016, Peter Zijlstra wrote:

>In any case, its fairly simple to cure, just add
>smp_acquire__after_ctrl_dep() at the end. If we bail because
>need_resched() we don't need the acquire I think.

I was just considering this for your smp_cond_acquire/smp_cond_load_acquire
rework, so yeah I guess an smp_acquire__after_ctrl_dep would be a nice
compromise. 

However, I was always under the impression that races with node->locked were
rather harmless (as indicated in the mentioned commit) -- which is why ->locked
are simple load/stores, with the exception of the unqueueing -- but yeah, that's
not even paired.

Thanks,
Davidlohr

[toc] | [prev] | [next] | [standalone]

#1423324 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

On Wed, Jun 15, 2016 at 11:27:24AM -0700, Davidlohr Bueso wrote:
> On Wed, 15 Jun 2016, Peter Zijlstra wrote:
> 
> >In any case, its fairly simple to cure, just add
> >smp_acquire__after_ctrl_dep() at the end. If we bail because
> >need_resched() we don't need the acquire I think.
> 
> I was just considering this for your smp_cond_acquire/smp_cond_load_acquire

Right, so that need_resched break makes that a bit awkward. Not to
mention the cpu_relaxed() vs cpu_relaxed_lowlatency() difference.

> rework, so yeah I guess an smp_acquire__after_ctrl_dep would be a nice
> compromise.
> 
> However, I was always under the impression that races with node->locked were
> rather harmless (as indicated in the mentioned commit) -- which is why ->locked
> are simple load/stores, with the exception of the unqueueing -- but yeah, that's
> not even paired.

Yeah, see a few patches further in this series, where he guards a
variables with the osq_lock.

[toc] | [prev] | [next] | [standalone]

#1423327 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

On Wed, 15 Jun 2016, Peter Zijlstra wrote:

>On Wed, Jun 15, 2016 at 11:27:24AM -0700, Davidlohr Bueso wrote:
>> On Wed, 15 Jun 2016, Peter Zijlstra wrote:
>>
>> >In any case, its fairly simple to cure, just add
>> >smp_acquire__after_ctrl_dep() at the end. If we bail because
>> >need_resched() we don't need the acquire I think.
>>
>> I was just considering this for your smp_cond_acquire/smp_cond_load_acquire
>
>Right, so that need_resched break makes that a bit awkward. Not to
>mention the cpu_relaxed() vs cpu_relaxed_lowlatency() difference.

Oh sure, I was merely refering to the ordering semantics, not the calls
themselves -- although at some point, as archs begin to port locking/core
optimizations, we _will_ need the variants for dealing with '_lowlatency'.

>
>> rework, so yeah I guess an smp_acquire__after_ctrl_dep would be a nice
>> compromise.
>>
>> However, I was always under the impression that races with node->locked were
>> rather harmless (as indicated in the mentioned commit) -- which is why ->locked
>> are simple load/stores, with the exception of the unqueueing -- but yeah, that's
>> not even paired.
>
>Yeah, see a few patches further in this series, where he guards a
>variables with the osq_lock.

*sigh*

[toc] | [prev] | [next] | [standalone]

#1424584 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

On Wed, 15 Jun 2016, Peter Zijlstra wrote:

>Yeah, see a few patches further in this series, where he guards a
>variables with the osq_lock.

So one problem I have with all this is that if we are hardening osq_lock/unlock()
because of some future use that is specific to rwsems, then we will immediately
be hurting mutexes for no good reason.

Thanks,
Davidlohr

[toc] | [prev] | [next] | [standalone]

#1425307 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

On Fri, 17 Jun 2016, Waiman Long wrote:

>On 06/16/2016 09:11 PM, Davidlohr Bueso wrote:
>>On Wed, 15 Jun 2016, Peter Zijlstra wrote:
>>
>>>Yeah, see a few patches further in this series, where he guards a
>>>variables with the osq_lock.
>>
>>So one problem I have with all this is that if we are hardening 
>>osq_lock/unlock()
>>because of some future use that is specific to rwsems, then we will 
>>immediately
>>be hurting mutexes for no good reason.
>>
>
>I am going to change it to use smp_acquire__after_ctrl_dep() as 
>suggested by PeterZ. Is that a good enough compromise? I have also 
>changed the xchg in the unlock side to xchg_release which could help 
>performance in some archs. The thing is when developers see the name 
>osq_lock/osq_unlock, they will naturally assume the proper barrriers 
>are provided which is not currently the case.

Oh, from your discussions with Boqun, I was under the impression that ->locked
was now going to be properly ordered in all cases now, which is why
I worry about mutexes.

>Anyway, the change won't affect x86, it is probably ARM or PPC that 
>may have an impact.

Yes, that xchg() won't affect x86, but adding an smp_store_release(node->locked, 1)
or such will obviously.

Thanks,
Davidlohr

[toc] | [prev] | [next] | [standalone]

#1425319 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

On Fri, 17 Jun 2016, Davidlohr Bueso wrote:

>On Fri, 17 Jun 2016, Waiman Long wrote:
>
>>On 06/16/2016 09:11 PM, Davidlohr Bueso wrote:
>>>On Wed, 15 Jun 2016, Peter Zijlstra wrote:
>>>
>>>>Yeah, see a few patches further in this series, where he guards a
>>>>variables with the osq_lock.
>>>
>>>So one problem I have with all this is that if we are hardening 
>>>osq_lock/unlock()
>>>because of some future use that is specific to rwsems, then we 
>>>will immediately
>>>be hurting mutexes for no good reason.
>>>
>>
>>I am going to change it to use smp_acquire__after_ctrl_dep() as 
>>suggested by PeterZ. Is that a good enough compromise? I have also 
>>changed the xchg in the unlock side to xchg_release which could help 
>>performance in some archs. The thing is when developers see the name 
>>osq_lock/osq_unlock, they will naturally assume the proper barrriers 
>>are provided which is not currently the case.
>
>Oh, from your discussions with Boqun, I was under the impression that ->locked
>was now going to be properly ordered in all cases now, which is why
>I worry about mutexes.
>
>>Anyway, the change won't affect x86, it is probably ARM or PPC that 
>>may have an impact.
>
>Yes, that xchg() won't affect x86, but adding an smp_store_release(node->locked, 1)
>or such will obviously.

nm this last part, you're right, x86 smp_store_release is a nop.

[toc] | [prev] | [next] | [standalone]

#1423503 — Re: [RFC PATCH-tip v2 1/6] locking/osq: Make lock/unlock proper acquire/release barrier

On Wed, Jun 15, 2016 at 04:04:07PM -0400, Waiman Long wrote:
> 
> BTW, when will the smp_acquire__after_ctrl_dep() patch goes into the tip
> tree? My patch will have a dependency on that when I make the change.

Should already be in as part of that spin_unlock_wait() fixup.

/me checks..

tip/locking/core contains:

33ac279677dc ("locking/barriers: Introduce smp_acquire__after_ctrl_dep()")

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web