LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jason Low <jason.low2@hp.com>
To: Davidlohr Bueso <dave@stgolabs.net>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@kernel.org>,
	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
	Michel Lespinasse <walken@google.com>,
	Tim Chen <tim.c.chen@linux.intel.com>,
	linux-kernel@vger.kernel.org, jason.low2@hp.com
Subject: Re: [PATCH 4/6] locking/rwsem: Avoid deceiving lock spinners
Date: Thu, 29 Jan 2015 12:18:51 -0800	[thread overview]
Message-ID: <1422562731.2418.16.camel@j-VirtualBox> (raw)
In-Reply-To: <1422562401.2418.13.camel@j-VirtualBox>

On Thu, 2015-01-29 at 12:13 -0800, Jason Low wrote:
> On Wed, 2015-01-28 at 17:10 -0800, Davidlohr Bueso wrote:
> 
> > 	if (READ_ONCE(sem->owner))
> > 		return true; /* new owner, continue spinning */
> 
> In terms of the sem->owner check, I agree. This also reminds me of a
> similar patch I was going to send out, but for mutex. The idea is that
> before we added all the MCS logic, we wanted to return false if owner
> isn't NULL to prevent too many threads simultaneously spinning on the
> owner at the same time.
> 
> With the new MCS queuing, we don't have to worry about that issue, so it
> makes sense to continue spinning as long as owner is running and the
> spinner doesn't need to reschedule.

By the way, here was the patch that I was referring to:

-------------------------------------------------------

In the mutex_spin_on_owner(), we return true only if lock->owner == NULL.
This was beneficial in situations where there were multiple threads
simultaneously spinning for the mutex. If another thread got the lock
while other spinner(s) were also doing mutex_spin_on_owner(), then the
other spinners would stop spinning. This workaround helped reduce the
chance that many spinners were simultaneously spinning for the mutex
which can help improve performance in highly contended cases.

However, recent changes were made to the optimistic spinning code such
that instead of having all spinners simultaneously spin for the mutex,
we queue the spinners with an MCS lock such that only one thread spins
for the mutex at a time.

Now, we don't have to worry about multiple threads spinning on owner
at the same time, and if lock->owner is not NULL at this point, it likely
means another thread happen to obtain the lock in the fastpath.

This patch changes this so that mutex_spin_on_owner() return true when
the lock owner changes, which means a thread will only stop spinning
if it either needs to reschedule or if the lock owner is not running.

We saw up to a 5% performance improvement in the fserver workload with
this patch.

Signed-off-by: Jason Low <jason.low2@hp.com>
---
 kernel/locking/mutex.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 8d1e2c1..e94e6b8 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -163,11 +163,11 @@ int mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner)
 	rcu_read_unlock();
 
 	/*
-	 * 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;
 }
 
 /*
-- 
1.7.1




  reply	other threads:[~2015-01-29 20:18 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-26  7:36 [PATCH -tip 0/6] rwsem: Fine tuning Davidlohr Bueso
2015-01-26  7:36 ` [PATCH 1/6] locking/rwsem: Use task->state helpers Davidlohr Bueso
2015-02-04 14:38   ` [tip:locking/core] " tip-bot for Davidlohr Bueso
2015-01-26  7:36 ` [PATCH 2/6] locking/rwsem: Document barrier need when waking tasks Davidlohr Bueso
2015-01-27 17:07   ` Peter Zijlstra
2015-01-27 20:30     ` Davidlohr Bueso
2015-01-26  7:36 ` [PATCH 3/6] locking/rwsem: Set lock ownership ASAP Davidlohr Bueso
2015-01-27 17:10   ` Peter Zijlstra
2015-01-27 19:18     ` Davidlohr Bueso
2015-01-26  7:36 ` [PATCH 4/6] locking/rwsem: Avoid deceiving lock spinners Davidlohr Bueso
2015-01-27 17:23   ` Jason Low
2015-01-28  3:54     ` Davidlohr Bueso
2015-01-28 17:01       ` Tim Chen
2015-01-28 21:03       ` Jason Low
2015-01-29  1:10         ` Davidlohr Bueso
2015-01-29 20:13           ` Jason Low
2015-01-29 20:18             ` Jason Low [this message]
2015-01-29 23:15               ` Davidlohr Bueso
2015-01-30  1:52                 ` Refactoring mutex spin on owner code Jason Low
2015-01-30  7:14                   ` Davidlohr Bueso
2015-01-30  7:51                     ` Peter Zijlstra
2015-01-26  7:36 ` [PATCH 5/6] locking/rwsem: Optimize slowpath/sleeping Davidlohr Bueso
2015-01-27 17:34   ` Peter Zijlstra
2015-01-27 21:57     ` Davidlohr Bueso
2015-01-26  7:36 ` [PATCH 6/6] locking/rwsem: Check for active lock before bailing on spinning Davidlohr Bueso
2015-01-27 18:11   ` Jason Low

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1422562731.2418.16.camel@j-VirtualBox \
    --to=jason.low2@hp.com \
    --cc=dave@stgolabs.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=peterz@infradead.org \
    --cc=tim.c.chen@linux.intel.com \
    --cc=walken@google.com \
    --subject='Re: [PATCH 4/6] locking/rwsem: Avoid deceiving lock spinners' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).