Messages in this thread | | | Date | Fri, 08 Aug 2014 14:30:02 -0400 | From | Waiman Long <> | Subject | Re: [PATCH v2 1/7] locking/rwsem: check for active writer/spinner before wakeup |
| |
On 08/08/2014 01:39 AM, Davidlohr Bueso wrote: > On Thu, 2014-08-07 at 17:45 -0700, Davidlohr Bueso wrote: >> On Thu, 2014-08-07 at 18:26 -0400, Waiman Long wrote: >>> On a highly contended rwsem, spinlock contention due to the slow >>> rwsem_wake() call can be a significant portion of the total CPU cycles >>> used. With writer lock stealing and writer optimistic spinning, there >>> is also a pretty good chance that the lock may have been stolen >>> before the waker wakes up the waiters. The woken tasks, if any, >>> will have to go back to sleep again. >> Good catch! And this applies to mutexes as well. How about something >> like this: >> >> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c >> index dadbf88..e037588 100644 >> --- a/kernel/locking/mutex.c >> +++ b/kernel/locking/mutex.c >> @@ -707,6 +707,20 @@ EXPORT_SYMBOL_GPL(__ww_mutex_lock_interruptible); >> >> #endif >> >> +#if defined(CONFIG_DEBUG_MUTEXES) || defined(CONFIG_MUTEX_SPIN_ON_OWNER) > If DEBUG, we don't clear the owner when unlocking. This can just be > > +#ifdef CONFIG_MUTEX_SPIN_ON_OWNER > >> +static inline bool mutex_has_owner(struct mutex *lock) >> +{ >> + struct task_struct *owner = ACCESS_ONCE(lock->owner); >> + >> + return owner != NULL; >> +} >> +#else >> +static inline bool mutex_has_owner(struct mutex *lock) >> +{ >> + return false; >> +} >> +#endif >> + >> /* >> * Release the lock, slowpath: >> */ >> @@ -734,6 +748,15 @@ __mutex_unlock_common_slowpath(struct mutex *lock, int nested) >> mutex_release(&lock->dep_map, nested, _RET_IP_); >> debug_mutex_unlock(lock); >> >> + /* >> + * Abort the wakeup operation if there is an active writer as the >> + * lock was stolen. mutex_unlock() should have cleared the owner field >> + * before calling this function. If that field is now set, there must >> + * be an active writer present. >> + */ >> + if (mutex_has_owner(lock)) >> + goto done; > Err so we actually deadlock here because we do the check with the > lock->wait_lock held and at the same time another task comes into the > slowpath of a mutex_lock() call which also tries to take the wait_lock. > Ending up with hung tasks. Here's a more tested patch against > peterz-queue, survives aim7 and kernel builds on a 80core box. Thanks.
I couldn't figure out why there will be hang tasks. The logic looks OK to me.
> > 8<--------------------------------------------------------------- > From: Davidlohr Bueso<davidlohr@hp.com> > Subject: [PATCH] locking/mutex: Do not falsely wake-up tasks > > 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. This is specially true on highly > contended mutexes that stress the wait_lock. > > Signed-off-by: Davidlohr Bueso<davidlohr@hp.com> > --- > kernel/locking/mutex.c | 32 +++++++++++++++++++++++++++++++- > 1 file changed, 31 insertions(+), 1 deletion(-) > > diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c > index dadbf88..52e1136 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 > @@ -730,6 +744,23 @@ __mutex_unlock_common_slowpath(struct mutex *lock, int nested) > if (__mutex_slowpath_needs_to_unlock()) > atomic_set(&lock->count, 1); > > +/* > + * 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 > + * deadlocks when another task 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. > + */ > + if (mutex_has_owner(lock)) > + return; > +#endif > + > spin_lock_mutex(&lock->wait_lock, flags); > mutex_release(&lock->dep_map, nested, _RET_IP_); > debug_mutex_unlock(lock); > @@ -744,7 +775,6 @@ __mutex_unlock_common_slowpath(struct mutex *lock, int nested) > > wake_up_process(waiter->task); > } > - > spin_unlock_mutex(&lock->wait_lock, flags); > } >
I have 2 issues about this. First of all, the timing windows between atomic_set() and mutex_has_owner() check is really small, I doubt it will be that effective. Secondly, I think you may need to call mutex_release() and debug_mutex_unlock() to make the debugging code work, but they seems to be called only under the wait_lock. So I think there is more work that need to be done before this patch is ready.
-Longman
| |