Re: [PATCH -v4 5/8] locking/mutex: Add lock handoff to avoid starvation

[Waiman Long]

On 10/07/2016 10:52 AM, Peter Zijlstra wrote:
>   /*
>    * Actual trylock that will work on any unlocked state.
> + *
> + * When setting the owner field, we must preserve the low flag bits.
> + *
> + * Be careful with @handoff, only set that in a wait-loop (where you set
> + * HANDOFF) to avoid recursive lock attempts.
>    */
> -static inline bool __mutex_trylock(struct mutex *lock)
> +static inline bool __mutex_trylock(struct mutex *lock, const bool handoff)
>   {
>   	unsigned long owner, curr = (unsigned long)current;
>
>   	owner = atomic_long_read(&lock->owner);
>   	for (;;) { /* must loop, can race against a flag */
> -		unsigned long old;
> +		unsigned long old, flags = __owner_flags(owner);
> +
> +		if (__owner_task(owner)) {
> +			if (handoff&&  unlikely(__owner_task(owner) == current)) {
> +				/*
> +				 * Provide ACQUIRE semantics for the lock-handoff.
> +				 *
> +				 * We cannot easily use load-acquire here, since
> +				 * the actual load is a failed cmpxchg, which
> +				 * doesn't imply any barriers.
> +				 *
> +				 * Also, this is a fairly unlikely scenario, and
> +				 * this contains the cost.
> +				 */

I am not so sure about your comment here. I guess you are referring to 
the atomic_long_cmpxchg_acquire below for the failed cmpxchg. However, 
it is also possible that the path can be triggered on the first round 
without cmpxchg. Maybe we can do a load_acquire on the owner again to 
satisfy this requirement without a smp_mb().

> +				smp_mb(); /* ACQUIRE */
> +				return true;
> +			}
>
> -		if (__owner_task(owner))
>   			return false;
> +		}
>
> -		old = atomic_long_cmpxchg_acquire(&lock->owner, owner,
> -						  curr | __owner_flags(owner));
> +		/*
> +		 * We set the HANDOFF bit, we must make sure it doesn't live
> +		 * past the point where we acquire it. This would be possible
> +		 * if we (accidentally) set the bit on an unlocked mutex.
> +		 */
> +		if (handoff)
> +			flags&= ~MUTEX_FLAG_HANDOFF;
> +
> +		old = atomic_long_cmpxchg_acquire(&lock->owner, owner, curr | flags);
>   		if (old == owner)
>   			return true;

Other than that, the code is fine.

Reviewed-by: Waiman Long <Waiman.Long@xxxxxxx>