On Thu, 2014-08-14 at 13:17 -0400, Waiman Long wrote: > 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<davidl...@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().
As mentioned, this causes all sorts of hung tasks when the another task enters the slowpath when locking. There's a big fat comment above. > In > addition, the atomic_set() is racy. It is better to something like Why is it racy? Atomically setting the lock to -1 given that the lock was stolen should be safe. The alternative we discussed with Jason was to set the counter to -1 in the spinning path. But given that we need to serialize the counter check with the list_empty() check that would require the wait_lock. This is very messy and unnecessarily complicates things. > if (atomic_cmpxchg(&lock->count, 0, -1) <= 0) > return; Not really because some archs leave the lock at 1 after the unlock fastpath. Thanks, Davidlohr -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
