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

[Waiman Long]

On 12/26/2016 12:50 AM, Boqun Feng wrote:
> Hi Wainman,
>
> On Sun, Dec 25, 2016 at 03:26:01PM -0500, Waiman Long wrote:
>> A number of cmpxchg calls in qspinlock_paravirt.h were replaced by more
>> relaxed versions to improve performance on architectures that use LL/SC.
>>
>> All the locking related cmpxchg's are replaced with the _acquire
>> variants:
>>  - pv_queued_spin_steal_lock()
>>  - trylock_clear_pending()
>>
>> The cmpxchg's related to hashing are replaced by either by the _release
>> or the _relaxed variants. See the inline comment for details.
>>
>> Signed-off-by: Waiman Long <longman@xxxxxxxxxx>
>>
>>  v1->v2:
>>   - Add comments in changelog and code for the rationale of the change.
>>
>> ---
>>  kernel/locking/qspinlock_paravirt.h | 50 ++++++++++++++++++++++++-------------
>>  1 file changed, 33 insertions(+), 17 deletions(-)
>>
>> diff --git a/kernel/locking/qspinlock_paravirt.h b/kernel/locking/qspinlock_paravirt.h
>> index e3b5520..c31d1ab 100644
>> --- a/kernel/locking/qspinlock_paravirt.h
>> +++ b/kernel/locking/qspinlock_paravirt.h
>> @@ -72,7 +72,7 @@ static inline bool pv_queued_spin_steal_lock(struct qspinlock *lock)
>>  	struct __qspinlock *l = (void *)lock;
>>  
>>  	if (!(atomic_read(&lock->val) & _Q_LOCKED_PENDING_MASK) &&
>> -	    (cmpxchg(&l->locked, 0, _Q_LOCKED_VAL) == 0)) {
>> +	    (cmpxchg_acquire(&l->locked, 0, _Q_LOCKED_VAL) == 0)) {
>>  		qstat_inc(qstat_pv_lock_stealing, true);
>>  		return true;
>>  	}
>> @@ -101,16 +101,16 @@ static __always_inline void clear_pending(struct qspinlock *lock)
>>  
>>  /*
>>   * The pending bit check in pv_queued_spin_steal_lock() isn't a memory
>> - * barrier. Therefore, an atomic cmpxchg() is used to acquire the lock
>> - * just to be sure that it will get it.
>> + * barrier. Therefore, an atomic cmpxchg_acquire() is used to acquire the
>> + * lock to provide the proper memory barrier.
>>   */
>>  static __always_inline int trylock_clear_pending(struct qspinlock *lock)
>>  {
>>  	struct __qspinlock *l = (void *)lock;
>>  
>>  	return !READ_ONCE(l->locked) &&
>> -	       (cmpxchg(&l->locked_pending, _Q_PENDING_VAL, _Q_LOCKED_VAL)
>> -			== _Q_PENDING_VAL);
>> +	       (cmpxchg_acquire(&l->locked_pending, _Q_PENDING_VAL,
>> +				_Q_LOCKED_VAL) == _Q_PENDING_VAL);
>>  }
>>  #else /* _Q_PENDING_BITS == 8 */
>>  static __always_inline void set_pending(struct qspinlock *lock)
>> @@ -138,7 +138,7 @@ static __always_inline int trylock_clear_pending(struct qspinlock *lock)
>>  		 */
>>  		old = val;
>>  		new = (val & ~_Q_PENDING_MASK) | _Q_LOCKED_VAL;
>> -		val = atomic_cmpxchg(&lock->val, old, new);
>> +		val = atomic_cmpxchg_acquire(&lock->val, old, new);
>>  
>>  		if (val == old)
>>  			return 1;
>> @@ -209,9 +209,15 @@ static struct qspinlock **pv_hash(struct qspinlock *lock, struct pv_node *node)
>>  	struct pv_hash_entry *he;
>>  	int hopcnt = 0;
>>  
>> +	/*
>> +	 * Synchronizing with the node state variable will control who does
>> +	 * the hashing - the lock holder or lock waiter. The control
>> +	 * dependency will ensure that node value is written after the lock
>> +	 * value. So we don't need other ordering guarantee.
>> +	 */
> By this comment, you mean that
> 	
> 	cmpxchg_relaxed(&he->lock, NULL, lock);
> 	  r1 = ll he->lock;
> 	  <compare part>
> 	  sc he->lock, lock // successed
>
> 	if (r1)
> 		WRITE_ONCE(he->node, node);
>
>
> the sc and WRITE_ONCE() can not be reordered because of the control
> dependency? I dont think this is true. Yes the sc must execute before
> the WRITE_ONCE(), but the memory/cache effects may be reordered. IOW,
> the following may happen
>
>
> 	CPU 0			CPU 1
> 	===================	=======================
> 	{x = 0, y = 0}		if (!cmpxchg_relaxed(&y, 0, 1))
> 					WRITE_ONCE(x, 1);
> 	r1 = READ_ONCE(x);
>
> 	smp_rmb();
>
> 	r2 = READ_ONCE(y);
>
> The following result is possible:
>
> 	y = 1 && r1 = 1 && r2 = 0
>
> Or I'm missing your point here? ;-) 
>
> Regards,
> Boqun
>
You are probably right. I know the code is somewhat risky. That is why I
am waiting for expert like you to see if this is really the case. Now it
seems that it may not be the case. I will revise the patch to take that out.

Cheers,
Longman