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

Groups > linux.kernel > #1707402 > unrolled thread

Re: [RESEND PATCH v5] locking/pvqspinlock: Relax cmpxchg's to improve performance on some archs

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

  Re: [RESEND PATCH v5] locking/pvqspinlock: Relax cmpxchg's to  improve performance on some archs Peter Zijlstra <peterz@infradead.org> - 2017-08-09 17:10 +0200
    Re: [RESEND PATCH v5] locking/pvqspinlock: Relax cmpxchg's to  improve performance on some archs Peter Zijlstra <peterz@infradead.org> - 2017-08-09 17:20 +0200
      Re: [RESEND PATCH v5] locking/pvqspinlock: Relax cmpxchg's to  improve performance on some archs Boqun Feng <boqun.feng@gmail.com> - 2017-08-10 10:20 +0200
        Re: [RESEND PATCH v5] locking/pvqspinlock: Relax cmpxchg's to  improve performance on some archs Peter Zijlstra <peterz@infradead.org> - 2017-08-10 11:20 +0200
          Re: [RESEND PATCH v5] locking/pvqspinlock: Relax cmpxchg's to  improve performance on some archs "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-08-10 22:50 +0200

#1707402 — Re: [RESEND PATCH v5] locking/pvqspinlock: Relax cmpxchg's to improve performance on some archs

On Wed, May 24, 2017 at 09:38:28AM -0400, Waiman Long wrote:

> @@ -361,6 +361,13 @@ static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
>  	 * observe its next->locked value and advance itself.
>  	 *
>  	 * Matches with smp_store_mb() and cmpxchg() in pv_wait_node()
> +	 *
> +	 * The write to next->locked in arch_mcs_spin_unlock_contended()
> +	 * must be ordered before the read of pn->state in the cmpxchg()
> +	 * below for the code to work correctly. However, this is not
> +	 * guaranteed on all architectures when the cmpxchg() call fails.
> +	 * Both x86 and PPC can provide that guarantee, but other
> +	 * architectures not necessarily.
>  	 */
>  	if (cmpxchg(&pn->state, vcpu_halted, vcpu_hashed) != vcpu_halted)
>  		return;

Instead of documenting this, should we not fix it properly?

So what we want is to order:

	smp_store_release(&x, 1);
	cmpxchg(&y, 0, 1);

Such that the store to x is before the load of y. Now, we document
cmpxchg() to have smp_mb() before and smp_mb() after (if success). So
per that definition, there would appear no way the load of y can be
reordered before the store to x.

Now, ARM64 for instance plays funny games, it does something along the
lines of:

cmpxchg(ptr, old, new)
{
	do {
		r = LL(ptr);
		if (r != old)
			return r; /* no barriers */
		r = new
	} while (SC_release(ptr, r));
	smp_mb();
	return r;
}

Thereby ordering things relative to the store on ptr, but the load can
very much escape. The thinking is that if success, we must observe the
latest value of ptr, but even in that case the load is not ordered and
could happen before.

However, since we're guaranteed to observe the latest value of ptr (on
success) it doesn't matter if we reordered the load, there is no newer
value possible.

So heaps of tricky, but correct afaict. Will?


Of course, since we need that load to be ordered even in case of a
failed cmpxchg() we _should_ add an unconditional smp_mb() here. Which I
understand you not wanting to do. Even smp_mb__before_atomic() is no
help, because that's smp_mb() for both PPC and ARM64.

[toc] | [next] | [standalone]

#1707410

On Wed, Aug 09, 2017 at 05:06:03PM +0200, Peter Zijlstra wrote:
> Now, ARM64 for instance plays funny games, it does something along the
> lines of:
> 
> cmpxchg(ptr, old, new)
> {
> 	do {
> 		r = LL(ptr);
> 		if (r != old)
> 			return r; /* no barriers */
> 		r = new
> 	} while (SC_release(ptr, r));
> 	smp_mb();
> 	return r;
> }
> 
> Thereby ordering things relative to the store on ptr, but the load can
> very much escape. The thinking is that if success, we must observe the
> latest value of ptr, but even in that case the load is not ordered and
> could happen before.
> 
> However, since we're guaranteed to observe the latest value of ptr (on
> success) it doesn't matter if we reordered the load, there is no newer
> value possible.
> 
> So heaps of tricky, but correct afaict. Will?

And could not PPC do something similar:

cmpxchg(ptr, old, new)
{
	lwsync();
	dp {
		r = LL(ptr);
		if (r != old)
			return;
		r = new;
	} while (SC(ptr, r));
	sync();
	return r;
}

?

the lwsync would make it store-release on SC with similar reasoning as
above.

And lwsync allows 'stores reordered after loads', which allows the prior
smp_store_release() to leak past.

Or is the reason this doesn't work on PPC that its RCpc?

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

#1708306

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

On Wed, Aug 09, 2017 at 05:15:33PM +0200, Peter Zijlstra wrote:
> On Wed, Aug 09, 2017 at 05:06:03PM +0200, Peter Zijlstra wrote:
> > Now, ARM64 for instance plays funny games, it does something along the
> > lines of:
> > 
> > cmpxchg(ptr, old, new)
> > {
> > 	do {
> > 		r = LL(ptr);
> > 		if (r != old)
> > 			return r; /* no barriers */
> > 		r = new
> > 	} while (SC_release(ptr, r));
> > 	smp_mb();
> > 	return r;
> > }
> > 
> > Thereby ordering things relative to the store on ptr, but the load can
> > very much escape. The thinking is that if success, we must observe the
> > latest value of ptr, but even in that case the load is not ordered and
> > could happen before.
> > 
> > However, since we're guaranteed to observe the latest value of ptr (on
> > success) it doesn't matter if we reordered the load, there is no newer
> > value possible.
> > 
> > So heaps of tricky, but correct afaict. Will?
> 
> And could not PPC do something similar:
> 
> cmpxchg(ptr, old, new)
> {
> 	lwsync();
> 	dp {
> 		r = LL(ptr);
> 		if (r != old)
> 			return;
> 		r = new;
> 	} while (SC(ptr, r));
> 	sync();
> 	return r;
> }
> 
> ?
> 
> the lwsync would make it store-release on SC with similar reasoning as
> above.
> 
> And lwsync allows 'stores reordered after loads', which allows the prior
> smp_store_release() to leak past.
> 
> Or is the reason this doesn't work on PPC that its RCpc?

Here is an example why PPC needs a sync() before the cmpxchg():

	https://marc.info/?l=linux-kernel&m=144485396224519&w=2

and Paul Mckenney's detailed explanation about why this could happen:

	https://marc.info/?l=linux-kernel&m=144485909826241&w=2

(Somehow, I feel like he was answering to a similar question question as
you ask here ;-))

And I think aarch64 doesn't have a problem here because it is "(other)
multi-copy atomic". Will?

Regards,
Boqun

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

#1708357

On Thu, Aug 10, 2017 at 04:12:13PM +0800, Boqun Feng wrote:

> > Or is the reason this doesn't work on PPC that its RCpc?

So that :-)

> Here is an example why PPC needs a sync() before the cmpxchg():
> 
> 	https://marc.info/?l=linux-kernel&m=144485396224519&w=2
> 
> and Paul Mckenney's detailed explanation about why this could happen:
> 
> 	https://marc.info/?l=linux-kernel&m=144485909826241&w=2
> 
> (Somehow, I feel like he was answering to a similar question question as
> you ask here ;-))

Yes, and I had vague memories of having gone over this before, but
couldn't quickly find things. Thanks!

> And I think aarch64 doesn't have a problem here because it is "(other)
> multi-copy atomic". Will?

Right, its the RCpc vs RCsc thing. The ARM64 release is as you say
multi-copy atomic, whereas the PPC lwsync is not.


This still leaves us with the situation that we need an smp_mb() between
smp_store_release() and a possibly failing cmpxchg() if we want to
guarantee the cmpxchg()'s load comes after the store-release.

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

#1709022

On Thu, Aug 10, 2017 at 11:13:17AM +0200, Peter Zijlstra wrote:
> On Thu, Aug 10, 2017 at 04:12:13PM +0800, Boqun Feng wrote:
> 
> > > Or is the reason this doesn't work on PPC that its RCpc?
> 
> So that :-)
> 
> > Here is an example why PPC needs a sync() before the cmpxchg():
> > 
> > 	https://marc.info/?l=linux-kernel&m=144485396224519&w=2
> > 
> > and Paul Mckenney's detailed explanation about why this could happen:
> > 
> > 	https://marc.info/?l=linux-kernel&m=144485909826241&w=2
> > 
> > (Somehow, I feel like he was answering to a similar question question as
> > you ask here ;-))
> 
> Yes, and I had vague memories of having gone over this before, but
> couldn't quickly find things. Thanks!
> 
> > And I think aarch64 doesn't have a problem here because it is "(other)
> > multi-copy atomic". Will?
> 
> Right, its the RCpc vs RCsc thing. The ARM64 release is as you say
> multi-copy atomic, whereas the PPC lwsync is not.
> 
> This still leaves us with the situation that we need an smp_mb() between
> smp_store_release() and a possibly failing cmpxchg() if we want to
> guarantee the cmpxchg()'s load comes after the store-release.

For whatever it is worth, this is why C11 allows specifying one
memory-order strength for the success case and another for the failure
case.  But it is not immediately clear that we need another level
of combinatorial API explosion...

							Thanx, Paul

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web