From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759343AbbA3Bwx (ORCPT ); Thu, 29 Jan 2015 20:52:53 -0500 Received: from g4t3425.houston.hp.com ([15.201.208.53]:16784 "EHLO g4t3425.houston.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752241AbbA3Bww (ORCPT ); Thu, 29 Jan 2015 20:52:52 -0500 Message-ID: <1422582768.2418.31.camel@j-VirtualBox> Subject: Re: Refactoring mutex spin on owner code From: Jason Low To: Davidlohr Bueso Cc: Peter Zijlstra , Ingo Molnar , "Paul E. McKenney" , Michel Lespinasse , Tim Chen , linux-kernel@vger.kernel.org, jason.low2@hp.com Date: Thu, 29 Jan 2015 17:52:48 -0800 In-Reply-To: <1422573326.2005.7.camel@stgolabs.net> 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> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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) { + ret = true; break; + } + + /* + * 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(); + + /* + * Stop spinning if we need to reschedule or if owner is + * not running. + */ + if (!owner->on_cpu || need_resched()) { + ret = false; + break; + } cpu_relax_lowlatency(); } rcu_read_unlock(); - /* - * 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 != owner; + return ret; } /*