lkml.org 
[lkml]   [2014]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -tip] locking/mutexes: Avoid bogus wakeups after lock stealing
On 08/14/2014 01:57 AM, Davidlohr Bueso wrote:
> Mutexes lock-stealing functionality allows another task to
> skip its turn in the wait-queue and atomically acquire the lock.
> This is fine and a nice optimization, however, when releasing
> the mutex, we always wakeup the next task in FIFO order. When
> the lock has been stolen, this leads to wasting waking up a
> task just to immediately realize it cannot acquire the lock
> and just go back to sleep. While in practice this window is
> quite small, it is not about performance or avoid taking the
> wait_lock, but because avoiding bogus wakeups is the right thing
> to do.
>
> In order to deal with the race when potentially missing the
> unlock slowpath (details in the comments), we pessimistically set
> the lock to have waiters. The downside of this is that the task
> that now stole the lock would always have to acquire the mutex in
> its slowpath (as mutex_try_to_acquire() would never succeed.
> However, since this path is rarely called, the cost is really
> never noticed.
>
> Signed-off-by: Davidlohr Bueso<davidlohr@hp.com>
> ---
> Original thread: https://lkml.org/lkml/2014/8/8/37
>
> kernel/locking/mutex.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 43 insertions(+)
>
> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> index dadbf88..4570611 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -383,12 +383,26 @@ done:
>
> return false;
> }
> +
> +static inline bool mutex_has_owner(struct mutex *lock)
> +{
> + struct task_struct *owner = ACCESS_ONCE(lock->owner);
> +
> + return owner != NULL;
> +}
> +
> #else
> +
> static bool mutex_optimistic_spin(struct mutex *lock,
> struct ww_acquire_ctx *ww_ctx, const bool use_ww_ctx)
> {
> return false;
> }
> +
> +static inline bool mutex_has_owner(struct mutex *lock)
> +{
> + return false;
> +}
> #endif
>
> __visible __used noinline
> @@ -715,6 +729,35 @@ __mutex_unlock_common_slowpath(struct mutex *lock, int nested)
> {
> unsigned long flags;
>
> +/*
> + * Skipping the mutex_has_owner() check when DEBUG, allows us to
> + * avoid taking the wait_lock in order to do not call mutex_release()
> + * and debug_mutex_unlock() when !DEBUG. This can otherwise result in
> + * hung it a hung task when another one enters the lock's slowpath in
> + * mutex_lock().
> + */
> +#ifndef CONFIG_DEBUG_MUTEXES
> + /*
> + * Abort the wakeup operation if there is an another mutex owner, as the
> + * lock was stolen. mutex_unlock() should have cleared the owner field
> + * before calling this function. If that field is now set, another task
> + * must have acquired the mutex. Note that this is a very tiny window.
> + */
> + if (unlikely(mutex_has_owner(lock))) {
> + /*
> + * Unconditionally set the lock to have waiters. Otherwise
> + * we can race with another task that grabbed the mutex via
> + * optimistic spinning and sets the lock to 0. When done,
> + * the unlock logic never reaches the slowpath, thus never
> + * waking the next task in the queue.
> + * Furthermore, this is safe as we've already acknowledged
> + * the fact that the lock was stolen and now a new owner
> + * exists.
> + */
> + atomic_set(&lock->count, -1);
> + return;
> + }
> +#endif
> /*
> * As a performance measurement, release the lock before doing other
> * wakeup related duties to follow. This allows other tasks to acquire

I still think it is better to do that after spin_lock_mutex(). In
addition, the atomic_set() is racy. It is better to something like

if (atomic_cmpxchg(&lock->count, 0, -1) <= 0)
return;

BTW, the current patch has no performance impact on x86 because
__mutex_slowpath_needs_to_unlock() is true.

-Longman


\
 
 \ /
  Last update: 2014-08-14 19:41    [W:2.089 / U:0.672 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site