From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753836AbbA3HOu (ORCPT ); Fri, 30 Jan 2015 02:14:50 -0500 Received: from smtp2.provo.novell.com ([137.65.250.81]:40268 "EHLO smtp2.provo.novell.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751684AbbA3HOt (ORCPT ); Fri, 30 Jan 2015 02:14:49 -0500 Message-ID: <1422602080.2005.9.camel@stgolabs.net> Subject: Re: Refactoring mutex spin on owner code From: Davidlohr Bueso To: Jason Low Cc: Peter Zijlstra , Ingo Molnar , "Paul E. McKenney" , Michel Lespinasse , Tim Chen , linux-kernel@vger.kernel.org Date: Thu, 29 Jan 2015 23:14:40 -0800 In-Reply-To: <1422582768.2418.31.camel@j-VirtualBox> References: <1422257769-14083-1-git-send-email-dave@stgolabs.net> <1422257769-14083-5-git-send-email-dave@stgolabs.net> <1422379430.6710.6.camel@j-VirtualBox> <1422417294.4604.15.camel@stgolabs.net> <1422479028.4111.34.camel@j-VirtualBox> <1422493812.4604.29.camel@stgolabs.net> <1422562401.2418.13.camel@j-VirtualBox> <1422562731.2418.16.camel@j-VirtualBox> <1422573326.2005.7.camel@stgolabs.net> <1422582768.2418.31.camel@j-VirtualBox> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.7 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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?