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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
