Re: [PATCH] locking/mutex: Reduce chance of setting HANDOFF bit on unlocked mutex

["Xu, Yanfei" <yanfei.xu@windriver.com>]

On 6/30/21 6:21 PM, Peter Zijlstra wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
> 
> On Tue, Jun 29, 2021 at 04:11:38PM -0400, Waiman Long wrote:
> 
>> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
>> index d2df5e68b503..472ab21b5b8e 100644
>> --- a/kernel/locking/mutex.c
>> +++ b/kernel/locking/mutex.c
>> @@ -118,9 +118,9 @@ static inline struct task_struct *__mutex_trylock_or_owner(struct mutex *lock)
>>                }
>>
>>                /*
>> -              * 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.
>> +              * Always clear the HANDOFF bit before acquiring the lock.
>> +              * Note that if the bit is accidentally set on an unlocked
>> +              * mutex, anyone can acquire it.
>>                 */
>>                flags &= ~MUTEX_FLAG_HANDOFF;
>>
>> @@ -180,6 +180,11 @@ static inline void __mutex_set_flag(struct mutex *lock, unsigned long flag)
>>        atomic_long_or(flag, &lock->owner);
>>   }
>>
>> +static inline long __mutex_fetch_set_flag(struct mutex *lock, unsigned long flag)
>> +{
>> +     return atomic_long_fetch_or_relaxed(flag, &lock->owner);
>> +}
>> +
>>   static inline void __mutex_clear_flag(struct mutex *lock, unsigned long flag)
>>   {
> 
> Hurmph, so we already have a cmpxchg loop in trylock, might as well make
> that do exactly what we want without holes on.
> 
> How's something like the below? Boot tested, but please verify.
> 
> ---
>   kernel/locking/mutex.c | 89 ++++++++++++++++++++++++++------------------------
>   1 file changed, 46 insertions(+), 43 deletions(-)
> 
> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> index d2df5e68b503..53f7fcadce85 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -91,44 +91,54 @@ static inline unsigned long __owner_flags(unsigned long owner)
>          return owner & MUTEX_FLAGS;
>   }
> 
> +#ifdef CONFIG_DEBUG_MUTEXES
> +#define MUTEX_WARN_ON(cond) DEBUG_LOCKS_WARN_ON(cond)
> +#else
> +#define MUTEX_WARN_ON(cond)
> +#endif
> +
>   /*
>    * Trylock variant that returns the owning task on failure.
>    */
> -static inline struct task_struct *__mutex_trylock_or_owner(struct mutex *lock)
> +static inline struct task_struct *__mutex_trylock_or_owner(struct mutex *lock, 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, flags = __owner_flags(owner);
> +               unsigned long flags = __owner_flags(owner);
>                  unsigned long task = owner & ~MUTEX_FLAGS;
> 
>                  if (task) {
> -                       if (likely(task != curr))
> -                               break;
> +                       if (flags & MUTEX_FLAG_PICKUP) {
> 
> -                       if (likely(!(flags & MUTEX_FLAG_PICKUP)))
> -                               break;
> +                               if (task != curr)
> +                                       break;
> +
> +                               flags &= ~MUTEX_FLAG_PICKUP;
> +

Hmm.. Should we also clear HANDOFF bit here? I don't find where it is 
cleared.

Regards,
Yanfei

> +                       } else if (handoff) {
> 
> -                       flags &= ~MUTEX_FLAG_PICKUP;
> +                               if (flags & MUTEX_FLAG_HANDOFF)
> +                                       break;
> +
> +                               flags |= MUTEX_FLAG_HANDOFF;
> +
> +                       } else {
> +
> +                               break;
> +                       }
>                  } else {
> -#ifdef CONFIG_DEBUG_MUTEXES
> -                       DEBUG_LOCKS_WARN_ON(flags & MUTEX_FLAG_PICKUP);
> -#endif
> +                       MUTEX_WARN_ON(flags & (MUTEX_FLAG_HANDOFF | MUTEX_FLAG_PICKUP));
> +                       task = curr;
>                  }
> 
> -               /*
> -                * 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.
> -                */
> -               flags &= ~MUTEX_FLAG_HANDOFF;
> +               if (atomic_long_try_cmpxchg_acquire(&lock->owner, &owner, task | flags)) {
> +                       if (task == curr)
> +                               return NULL;
> 
> -               old = atomic_long_cmpxchg_acquire(&lock->owner, owner, curr | flags);
> -               if (old == owner)
> -                       return NULL;
> -
> -               owner = old;
> +                       break;
> +               }
>          }
> 
>          return __owner_task(owner);
> @@ -139,7 +149,7 @@ static inline struct task_struct *__mutex_trylock_or_owner(struct mutex *lock)
>    */
>   static inline bool __mutex_trylock(struct mutex *lock)
>   {
> -       return !__mutex_trylock_or_owner(lock);
> +       return !__mutex_trylock_or_owner(lock, false);
>   }
> 
>   #ifndef CONFIG_DEBUG_LOCK_ALLOC
> @@ -226,7 +236,7 @@ static void __mutex_handoff(struct mutex *lock, struct task_struct *task)
>          unsigned long owner = atomic_long_read(&lock->owner);
> 
>          for (;;) {
> -               unsigned long old, new;
> +               unsigned long new;
> 
>   #ifdef CONFIG_DEBUG_MUTEXES
>                  DEBUG_LOCKS_WARN_ON(__owner_task(owner) != current);
> @@ -238,11 +248,8 @@ static void __mutex_handoff(struct mutex *lock, struct task_struct *task)
>                  if (task)
>                          new |= MUTEX_FLAG_PICKUP;
> 
> -               old = atomic_long_cmpxchg_release(&lock->owner, owner, new);
> -               if (old == owner)
> +               if (atomic_long_try_cmpxchg_release(&lock->owner, &owner, new))
>                          break;
> -
> -               owner = old;
>          }
>   }
> 
> @@ -662,7 +669,7 @@ mutex_optimistic_spin(struct mutex *lock, struct ww_acquire_ctx *ww_ctx,
>                  struct task_struct *owner;
> 
>                  /* Try to acquire the mutex... */
> -               owner = __mutex_trylock_or_owner(lock);
> +               owner = __mutex_trylock_or_owner(lock, false);
>                  if (!owner)
>                          break;
> 
> @@ -928,7 +935,6 @@ __mutex_lock_common(struct mutex *lock, unsigned int state, unsigned int subclas
>                      struct ww_acquire_ctx *ww_ctx, const bool use_ww_ctx)
>   {
>          struct mutex_waiter waiter;
> -       bool first = false;
>          struct ww_mutex *ww;
>          int ret;
> 
> @@ -1007,6 +1013,8 @@ __mutex_lock_common(struct mutex *lock, unsigned int state, unsigned int subclas
> 
>          set_current_state(state);
>          for (;;) {
> +               bool first;
> +
>                  /*
>                   * Once we hold wait_lock, we're serialized against
>                   * mutex_unlock() handing the lock off to us, do a trylock
> @@ -1035,23 +1043,18 @@ __mutex_lock_common(struct mutex *lock, unsigned int state, unsigned int subclas
>                  spin_unlock(&lock->wait_lock);
>                  schedule_preempt_disabled();
> 
> -               /*
> -                * ww_mutex needs to always recheck its position since its waiter
> -                * list is not FIFO ordered.
> -                */
> -               if (ww_ctx || !first) {
> -                       first = __mutex_waiter_is_first(lock, &waiter);
> -                       if (first)
> -                               __mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
> -               }
> -
>                  set_current_state(state);
> +
> +               first = __mutex_waiter_is_first(lock, &waiter);
> +
>                  /*
> -                * Here we order against unlock; we must either see it change
> -                * state back to RUNNING and fall through the next schedule(),
> -                * or we must see its unlock and acquire.
> +                * We got woken up, see if we can acquire the mutex now. If
> +                * not, and we're the first waiter, make sure to mark the mutex
> +                * for HANDOFF to avoid starvation.
> +                *
> +                * XXX spin-wait vs sigpending
>                   */
> -               if (__mutex_trylock(lock) ||
> +               if (!__mutex_trylock_or_owner(lock, first) ||
>                      (first && mutex_optimistic_spin(lock, ww_ctx, &waiter)))
>                          break;
>