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

[Waiman Long]

On 10/17/2016 02:45 PM, Waiman Long wrote:
> 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>

One more thing, I think it may be worthwhile to add another comment 
about what happens when the HANDOFF bit was set while we take the error 
path (goto err). As the actual handoff is serialized by the wait_lock, 
the code will still do the right thing. Either the next one in the queue 
will be handed off or it will be unlocked if the queue is empty.

Cheers,
Longman