Messages in this thread | | | Subject | Re: Refactoring mutex spin on owner code | From | Davidlohr Bueso <> | Date | Thu, 29 Jan 2015 23:14:40 -0800 |
| |
On Thu, 2015-01-29 at 17:52 -0800, Jason Low wrote: > On Thu, 2015-01-29 at 15:15 -0800, Davidlohr Bueso wrote: > > On Thu, 2015-01-29 at 12:18 -0800, Jason Low wrote: > > > /* > > > - * We break out the loop above on need_resched() and when the > > > - * owner changed, which is a sign for heavy contention. Return > > > - * success only when lock->owner is NULL. > > > + * We break out the loop above on either need_resched(), when > > > + * the owner is not running, or when the lock owner changed. > > > + * Return success only when the lock owner changed. > > > */ > > > - return lock->owner == NULL; > > > + return lock->owner != owner; > > > } > > > > Ideally we would refactor all this, along with getting rid of > > owner_running() at some point. It no longer makes sense to split up > > mutex_spin_on_owner() and we're doing duplicate owner checks. It would > > also be simpler than having to guess why we broke out of the loop, for > > example. > > Sure, that makes sense. What do you think of this additional change for > refactoring the mutex version? > > --- > diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c > index 8711505..b6a8633 100644 > --- a/kernel/locking/mutex.c > +++ b/kernel/locking/mutex.c > @@ -204,44 +204,45 @@ ww_mutex_set_context_fastpath(struct ww_mutex *lock, > * Mutex spinning code migrated from kernel/sched/core.c > */ > > -static inline bool owner_running(struct mutex *lock, struct task_struct *owner) > -{ > - if (lock->owner != owner) > - return false; > - > - /* > - * Ensure we emit the owner->on_cpu, dereference _after_ checking > - * lock->owner still matches owner, if that fails, owner might > - * point to free()d memory, if it still matches, the rcu_read_lock() > - * ensures the memory stays valid. > - */ > - barrier(); > - > - return owner->on_cpu; > -} > - > /* > * Look out! "owner" is an entirely speculative pointer > * access and not reliable. > */ > static noinline > -int mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner) > +bool mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner) > { > + bool ret; > + > rcu_read_lock(); > - while (owner_running(lock, owner)) { > - if (need_resched()) > + while (true) { > + /* Return success when the lock owner changed */ > + if (lock->owner != owner) {
Shouldn't this be a READ_ONCE(lock->owner)? We're in a loop and need to avoid gcc giving us stale data if the owner is updated after a few iterations, no?
| |