Re: [PATCH v7 1/5] locking/qspinlock: relaxes cmpxchg & xchg ops in native code

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Oct 13, 2015 at 04:38:19PM -0400, Waiman Long wrote:
> On 10/13/2015 02:02 PM, Peter Zijlstra wrote:
> >On Tue, Sep 22, 2015 at 04:50:40PM -0400, Waiman Long wrote:
> >>This patch replaces the cmpxchg() and xchg() calls in the native
> >>qspinlock code with more relaxed versions of those calls to enable
> >>other architectures to adopt queued spinlocks with less performance
> >>overhead.
> >>@@ -62,7 +63,7 @@ static __always_inline int queued_spin_is_contended(struct qspinlock *lock)
> >>  static __always_inline int queued_spin_trylock(struct qspinlock *lock)
> >>  {
> >>  	if (!atomic_read(&lock->val)&&
> >>-	   (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) == 0))
> >>+	   (atomic_cmpxchg_acquire(&lock->val, 0, _Q_LOCKED_VAL) == 0))
> >>  		return 1;
> >>  	return 0;
> >>  }
> >>@@ -77,7 +78,7 @@ static __always_inline void queued_spin_lock(struct qspinlock *lock)
> >>  {
> >>  	u32 val;
> >>
> >>-	val = atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL);
> >>+	val = atomic_cmpxchg_acquire(&lock->val, 0, _Q_LOCKED_VAL);
> >>  	if (likely(val == 0))
> >>  		return;
> >>  	queued_spin_lock_slowpath(lock, val);
> >>@@ -319,7 +329,7 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
> >>  		if (val == new)
> >>  			new |= _Q_PENDING_VAL;
> >>
> >>-		old = atomic_cmpxchg(&lock->val, val, new);
> >>+		old = atomic_cmpxchg_acquire(&lock->val, val, new);
> >>  		if (old == val)
> >>  			break;
> >>
> >So given recent discussion, all this _release/_acquire stuff is starting
> >to worry me.
> >
> >So we've not declared if they should be RCsc or RCpc, and given this
> >patch (and the previous ones) these lock primitives turn into RCpc if
> >the atomic primitives are RCpc.
> >
> >So far only the proposed PPC implementation is RCpc -- and their current
> >spinlock implementation is also RCpc, but that is a point of discussion.
> >
> >Just saying..
> 
> Davidlohr's patches to make similar changes in other locking code will also
> have this issue. 

Yes, I only fully appreciated the RCpc pain last week :/

> Anyway, the goal of this patch is to make the generic
> qspinlock code less costly when ported to other architectures. 

As long as we stay away from PPC this will be fine ;-) Luckily they are
unlikely to start using it since their LPAR hypervisor thingy isn't
really co-operative.

> This change
> will have no effect on the x86 architecture which is the only one using

I know ARM and ARGH64 will want to start using this, luckily both are
RCsc so no worries there.