[PATCH v2 0/4] mutex: Modifications to mutex

[PATCH v2 0/4] mutex: Modifications to mutex

[Jason Low]

v1->v2:
- Remove the MUTEX_SHOW_NO_WAITER() macro.
- Use !mutex_is_locked() instead of introducing a new MUTEX_IS_UNLOCKED() macro.
- Add more information to the changelog of the "Optimize mutex trylock slowpath"
  patch

This patchset contains documentation fixes and optimizations for mutex.

Jason Low (4):
  mutex: Correct documentation on mutex optimistic spinning
  mutex: Delete the MUTEX_SHOW_NO_WAITER macro
  mutex: Try to acquire mutex only if it is unlocked
  mutex: Optimize mutex trylock slowpath

 kernel/locking/mutex.c |   35 ++++++++++++++++++-----------------
 1 files changed, 18 insertions(+), 17 deletions(-)

[PATCH v2 1/4] mutex: Correct documentation on mutex optimistic spinning

[Jason Low]

The mutex optimistic spinning documentation states that we spin for
acquisition when we find that there are no pending waiters. However,
in actuality, whether or not there are waiters for the mutex doesn't
determine if we will spin for it.
  
This patch removes that statement and also adds a comment which
mentions that we spin for the mutex while we don't need to reschedule.

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

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index bc73d33..dd26bf6 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -388,12 +388,10 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 	/*
 	 * Optimistic spinning.
 	 *
-	 * We try to spin for acquisition when we find that there are no
-	 * pending waiters and the lock owner is currently running on a
-	 * (different) CPU.
-	 *
-	 * The rationale is that if the lock owner is running, it is likely to
-	 * release the lock soon.
+	 * We try to spin for acquisition when we find that the lock owner
+	 * is currently running on a (different) CPU and while we don't
+	 * need to reschedule. The rationale is that if the lock owner is
+	 * running, it is likely to release the lock soon.
 	 *
 	 * Since this needs the lock owner, and this mutex implementation
 	 * doesn't track the owner atomically in the lock field, we need to
-- 
1.7.1

[tip:locking/core] locking/mutexes: Correct documentation on mutex optimistic spinning

[tip-bot for Jason Low]

Commit-ID:  0c3c0f0d6e56422cef60a33726d062e9923005c3
Gitweb:     http://git.kernel.org/tip/0c3c0f0d6e56422cef60a33726d062e9923005c3
Author:     Jason Low <jason.low2@hp.com>
AuthorDate: Wed, 11 Jun 2014 11:37:20 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 5 Jul 2014 11:25:41 +0200

locking/mutexes: Correct documentation on mutex optimistic spinning

The mutex optimistic spinning documentation states that we spin for
acquisition when we find that there are no pending waiters. However,
in actuality, whether or not there are waiters for the mutex doesn't
determine if we will spin for it.

This patch removes that statement and also adds a comment which
mentions that we spin for the mutex while we don't need to reschedule.

Signed-off-by: Jason Low <jason.low2@hp.com>
Acked-by: Davidlohr Bueso <davidlohr@hp.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Cc: akpm@linux-foundation.org
Cc: tim.c.chen@linux.intel.com
Cc: paulmck@linux.vnet.ibm.com
Cc: rostedt@goodmis.org
Cc: Waiman.Long@hp.com
Cc: scott.norton@hp.com
Cc: aswin@hp.com
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/1402511843-4721-2-git-send-email-jason.low2@hp.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/mutex.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index bc73d33..dd26bf6de 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -388,12 +388,10 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 	/*
 	 * Optimistic spinning.
 	 *
-	 * We try to spin for acquisition when we find that there are no
-	 * pending waiters and the lock owner is currently running on a
-	 * (different) CPU.
-	 *
-	 * The rationale is that if the lock owner is running, it is likely to
-	 * release the lock soon.
+	 * We try to spin for acquisition when we find that the lock owner
+	 * is currently running on a (different) CPU and while we don't
+	 * need to reschedule. The rationale is that if the lock owner is
+	 * running, it is likely to release the lock soon.
 	 *
 	 * Since this needs the lock owner, and this mutex implementation
 	 * doesn't track the owner atomically in the lock field, we need to

[PATCH v2 2/4] mutex: Delete the MUTEX_SHOW_NO_WAITER macro

[Jason Low]

v1->v2:
- There were discussions in v1 about a possible mutex_has_waiters()
  function. This patch didn't use that function because the places which
  used MUTEX_SHOW_NO_WAITER requires checking for lock->count while an
  actual mutex_has_waiters() should check for !list_empty(wait_list).
  We'll just delete the macro and directly use atomic_read() + comments.

MUTEX_SHOW_NO_WAITER() is a macro which checks for if there are
"no waiters" on a mutex by checking if the lock count is non-negative.
Based on feedback from the discussion in the earlier version of this
patchset, the macro is not very readable.
  
Furthermore, checking lock->count isn't always the correct way to
determine if there are "no waiters" on a mutex. For example, a negative
count on a mutex really only means that there "potentially" are
waiters. Likewise, there can be waiters on the mutex even if the count is
non-negative. Thus, "MUTEX_SHOW_NO_WAITER" doesn't always do what the name
of the macro suggests.
 
So this patch deletes the MUTEX_SHOW_NO_WAITERS() macro, directly
use atomic_read() instead of the macro, and adds comments which
elaborate on how the extra atomic_read() checks can help reduce
unnecessary xchg() operations.

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

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index dd26bf6..4bd9546 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -46,12 +46,6 @@
 # include <asm/mutex.h>
 #endif
 
-/*
- * A negative mutex count indicates that waiters are sleeping waiting for the
- * mutex.
- */
-#define	MUTEX_SHOW_NO_WAITER(mutex)	(atomic_read(&(mutex)->count) >= 0)
-
 void
 __mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key)
 {
@@ -483,8 +477,11 @@ slowpath:
 #endif
 	spin_lock_mutex(&lock->wait_lock, flags);
 
-	/* once more, can we acquire the lock? */
-	if (MUTEX_SHOW_NO_WAITER(lock) && (atomic_xchg(&lock->count, 0) == 1))
+	/*
+	 * Once more, try to acquire the lock. Only try-lock the mutex if
+	 * lock->count >= 0 to reduce unnecessary xchg operations.
+	 */
+	if (atomic_read(&lock->count) >= 0 && (atomic_xchg(&lock->count, 0) == 1))
 		goto skip_wait;
 
 	debug_mutex_lock_common(lock, &waiter);
@@ -504,9 +501,10 @@ slowpath:
 		 * it's unlocked. Later on, if we sleep, this is the
 		 * operation that gives us the lock. We xchg it to -1, so
 		 * that when we release the lock, we properly wake up the
-		 * other waiters:
+		 * other waiters. We only attempt the xchg if the count is
+		 * non-negative in order to avoid unnecessary xchg operations:
 		 */
-		if (MUTEX_SHOW_NO_WAITER(lock) &&
+		if (atomic_read(&lock->count) >= 0 &&
 		    (atomic_xchg(&lock->count, -1) == 1))
 			break;
 
-- 
1.7.1

Re: [PATCH v2 2/4] mutex: Delete the MUTEX_SHOW_NO_WAITER macro

[Long, Wai Man]

On 6/11/2014 2:37 PM, Jason Low wrote:
> v1->v2:
> - There were discussions in v1 about a possible mutex_has_waiters()
>    function. This patch didn't use that function because the places which
>    used MUTEX_SHOW_NO_WAITER requires checking for lock->count while an
>    actual mutex_has_waiters() should check for !list_empty(wait_list).
>    We'll just delete the macro and directly use atomic_read() + comments.
>
> MUTEX_SHOW_NO_WAITER() is a macro which checks for if there are
> "no waiters" on a mutex by checking if the lock count is non-negative.
> Based on feedback from the discussion in the earlier version of this
> patchset, the macro is not very readable.
>    
> Furthermore, checking lock->count isn't always the correct way to
> determine if there are "no waiters" on a mutex. For example, a negative
> count on a mutex really only means that there "potentially" are
> waiters. Likewise, there can be waiters on the mutex even if the count is
> non-negative. Thus, "MUTEX_SHOW_NO_WAITER" doesn't always do what the name
> of the macro suggests.
>   
> So this patch deletes the MUTEX_SHOW_NO_WAITERS() macro, directly
> use atomic_read() instead of the macro, and adds comments which
> elaborate on how the extra atomic_read() checks can help reduce
> unnecessary xchg() operations.
>
> Signed-off-by: Jason Low <jason.low2@hp.com>
> ---
>   kernel/locking/mutex.c |   18 ++++++++----------
>   1 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> index dd26bf6..4bd9546 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -46,12 +46,6 @@
>   # include <asm/mutex.h>
>   #endif
>   
> -/*
> - * A negative mutex count indicates that waiters are sleeping waiting for the
> - * mutex.
> - */
> -#define	MUTEX_SHOW_NO_WAITER(mutex)	(atomic_read(&(mutex)->count) >= 0)
> -
>   void
>   __mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key)
>   {
> @@ -483,8 +477,11 @@ slowpath:
>   #endif
>   	spin_lock_mutex(&lock->wait_lock, flags);
>   
> -	/* once more, can we acquire the lock? */
> -	if (MUTEX_SHOW_NO_WAITER(lock) && (atomic_xchg(&lock->count, 0) == 1))
> +	/*
> +	 * Once more, try to acquire the lock. Only try-lock the mutex if
> +	 * lock->count >= 0 to reduce unnecessary xchg operations.
> +	 */
> +	if (atomic_read(&lock->count) >= 0 && (atomic_xchg(&lock->count, 0) == 1))
>   		goto skip_wait;
>   
>   	debug_mutex_lock_common(lock, &waiter);
> @@ -504,9 +501,10 @@ slowpath:
>   		 * it's unlocked. Later on, if we sleep, this is the
>   		 * operation that gives us the lock. We xchg it to -1, so
>   		 * that when we release the lock, we properly wake up the
> -		 * other waiters:
> +		 * other waiters. We only attempt the xchg if the count is
> +		 * non-negative in order to avoid unnecessary xchg operations:
>   		 */
> -		if (MUTEX_SHOW_NO_WAITER(lock) &&
> +		if (atomic_read(&lock->count) >= 0 &&
>   		    (atomic_xchg(&lock->count, -1) == 1))
>   			break;
>   

Acked-by: Waiman Long <Waiman.Long@hp.com>

[tip:locking/core] locking/mutexes: Delete the MUTEX_SHOW_NO_WAITER macro

[tip-bot for Jason Low]

Commit-ID:  1e820c9608eace237e2c519d8fd9074aec479d81
Gitweb:     http://git.kernel.org/tip/1e820c9608eace237e2c519d8fd9074aec479d81
Author:     Jason Low <jason.low2@hp.com>
AuthorDate: Wed, 11 Jun 2014 11:37:21 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 5 Jul 2014 11:25:41 +0200

locking/mutexes: Delete the MUTEX_SHOW_NO_WAITER macro

MUTEX_SHOW_NO_WAITER() is a macro which checks for if there are
"no waiters" on a mutex by checking if the lock count is non-negative.
Based on feedback from the discussion in the earlier version of this
patchset, the macro is not very readable.

Furthermore, checking lock->count isn't always the correct way to
determine if there are "no waiters" on a mutex. For example, a negative
count on a mutex really only means that there "potentially" are
waiters. Likewise, there can be waiters on the mutex even if the count is
non-negative. Thus, "MUTEX_SHOW_NO_WAITER" doesn't always do what the name
of the macro suggests.

So this patch deletes the MUTEX_SHOW_NO_WAITERS() macro, directly
use atomic_read() instead of the macro, and adds comments which
elaborate on how the extra atomic_read() checks can help reduce
unnecessary xchg() operations.

Signed-off-by: Jason Low <jason.low2@hp.com>
Acked-by: Waiman Long <Waiman.Long@hp.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Cc: akpm@linux-foundation.org
Cc: tim.c.chen@linux.intel.com
Cc: paulmck@linux.vnet.ibm.com
Cc: rostedt@goodmis.org
Cc: davidlohr@hp.com
Cc: scott.norton@hp.com
Cc: aswin@hp.com
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/1402511843-4721-3-git-send-email-jason.low2@hp.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/mutex.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index dd26bf6de..4bd9546 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -46,12 +46,6 @@
 # include <asm/mutex.h>
 #endif
 
-/*
- * A negative mutex count indicates that waiters are sleeping waiting for the
- * mutex.
- */
-#define	MUTEX_SHOW_NO_WAITER(mutex)	(atomic_read(&(mutex)->count) >= 0)
-
 void
 __mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key)
 {
@@ -483,8 +477,11 @@ slowpath:
 #endif
 	spin_lock_mutex(&lock->wait_lock, flags);
 
-	/* once more, can we acquire the lock? */
-	if (MUTEX_SHOW_NO_WAITER(lock) && (atomic_xchg(&lock->count, 0) == 1))
+	/*
+	 * Once more, try to acquire the lock. Only try-lock the mutex if
+	 * lock->count >= 0 to reduce unnecessary xchg operations.
+	 */
+	if (atomic_read(&lock->count) >= 0 && (atomic_xchg(&lock->count, 0) == 1))
 		goto skip_wait;
 
 	debug_mutex_lock_common(lock, &waiter);
@@ -504,9 +501,10 @@ slowpath:
 		 * it's unlocked. Later on, if we sleep, this is the
 		 * operation that gives us the lock. We xchg it to -1, so
 		 * that when we release the lock, we properly wake up the
-		 * other waiters:
+		 * other waiters. We only attempt the xchg if the count is
+		 * non-negative in order to avoid unnecessary xchg operations:
 		 */
-		if (MUTEX_SHOW_NO_WAITER(lock) &&
+		if (atomic_read(&lock->count) >= 0 &&
 		    (atomic_xchg(&lock->count, -1) == 1))
 			break;
 

[PATCH v2 3/4] mutex: Try to acquire mutex only if it is unlocked

[Jason Low]

Upon entering the slowpath in __mutex_lock_common(), we try once more to
acquire the mutex. We only try to acquire if (lock->count >= 0). However,
what we actually want here is to try to acquire if the mutex is unlocked
(lock->count == 1). 
  
This patch changes it so that we only try-acquire the mutex upon entering
the slowpath if it is unlocked, rather than if the lock count is non-negative.
This helps further reduce unnecessary atomic xchg() operations.

Furthermore, this patch uses !mutex_is_locked(lock) to do the initial
checks for if the lock is free rather than directly calling atomic_read()
on the lock->count, in order to improve readability.

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

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 4bd9546..e4d997b 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -432,7 +432,8 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 		if (owner && !mutex_spin_on_owner(lock, owner))
 			break;
 
-		if ((atomic_read(&lock->count) == 1) &&
+		/* Try to acquire the mutex if it is unlocked. */
+		if (!mutex_is_locked(lock) &&
 		    (atomic_cmpxchg(&lock->count, 1, 0) == 1)) {
 			lock_acquired(&lock->dep_map, ip);
 			if (use_ww_ctx) {
@@ -479,9 +480,9 @@ slowpath:
 
 	/*
 	 * Once more, try to acquire the lock. Only try-lock the mutex if
-	 * lock->count >= 0 to reduce unnecessary xchg operations.
+	 * it is unlocked to reduce unnecessary xchg() operations.
 	 */
-	if (atomic_read(&lock->count) >= 0 && (atomic_xchg(&lock->count, 0) == 1))
+	if (!mutex_is_locked(lock) && (atomic_xchg(&lock->count, 0) == 1))
 		goto skip_wait;
 
 	debug_mutex_lock_common(lock, &waiter);
-- 
1.7.1

Re: [PATCH v2 3/4] mutex: Try to acquire mutex only if it is unlocked

[Long, Wai Man]

On 6/11/2014 2:37 PM, Jason Low wrote:
> Upon entering the slowpath in __mutex_lock_common(), we try once more to
> acquire the mutex. We only try to acquire if (lock->count >= 0). However,
> what we actually want here is to try to acquire if the mutex is unlocked
> (lock->count == 1).
>    
> This patch changes it so that we only try-acquire the mutex upon entering
> the slowpath if it is unlocked, rather than if the lock count is non-negative.
> This helps further reduce unnecessary atomic xchg() operations.
>
> Furthermore, this patch uses !mutex_is_locked(lock) to do the initial
> checks for if the lock is free rather than directly calling atomic_read()
> on the lock->count, in order to improve readability.
>
> Signed-off-by: Jason Low <jason.low2@hp.com>
> ---
>   kernel/locking/mutex.c |    7 ++++---
>   1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> index 4bd9546..e4d997b 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -432,7 +432,8 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>   		if (owner && !mutex_spin_on_owner(lock, owner))
>   			break;
>   
> -		if ((atomic_read(&lock->count) == 1) &&
> +		/* Try to acquire the mutex if it is unlocked. */
> +		if (!mutex_is_locked(lock) &&
>   		    (atomic_cmpxchg(&lock->count, 1, 0) == 1)) {
>   			lock_acquired(&lock->dep_map, ip);
>   			if (use_ww_ctx) {
> @@ -479,9 +480,9 @@ slowpath:
>   
>   	/*
>   	 * Once more, try to acquire the lock. Only try-lock the mutex if
> -	 * lock->count >= 0 to reduce unnecessary xchg operations.
> +	 * it is unlocked to reduce unnecessary xchg() operations.
>   	 */
> -	if (atomic_read(&lock->count) >= 0 && (atomic_xchg(&lock->count, 0) == 1))
> +	if (!mutex_is_locked(lock) && (atomic_xchg(&lock->count, 0) == 1))
>   		goto skip_wait;
>   
>   	debug_mutex_lock_common(lock, &waiter);

Acked-by: Waiman Long <Waiman.Long@hp.com>

Re: [PATCH v2 3/4] mutex: Try to acquire mutex only if it is unlocked

[Davidlohr Bueso]

On Wed, 2014-06-11 at 11:37 -0700, Jason Low wrote:
> Upon entering the slowpath in __mutex_lock_common(), we try once more to
> acquire the mutex. We only try to acquire if (lock->count >= 0). However,
> what we actually want here is to try to acquire if the mutex is unlocked
> (lock->count == 1). 
>   
> This patch changes it so that we only try-acquire the mutex upon entering
> the slowpath if it is unlocked, rather than if the lock count is non-negative.
> This helps further reduce unnecessary atomic xchg() operations.
> 
> Furthermore, this patch uses !mutex_is_locked(lock) to do the initial
> checks for if the lock is free rather than directly calling atomic_read()
> on the lock->count, in order to improve readability.

I think this patch can be merged in 2/4, like you had in v1. Otherwise
looks good.

Re: [PATCH v2 3/4] mutex: Try to acquire mutex only if it is unlocked

[Jason Low]

On Thu, 2014-06-12 at 12:37 -0700, Davidlohr Bueso wrote:
> On Wed, 2014-06-11 at 11:37 -0700, Jason Low wrote:
> > Upon entering the slowpath in __mutex_lock_common(), we try once more to
> > acquire the mutex. We only try to acquire if (lock->count >= 0). However,
> > what we actually want here is to try to acquire if the mutex is unlocked
> > (lock->count == 1). 
> >   
> > This patch changes it so that we only try-acquire the mutex upon entering
> > the slowpath if it is unlocked, rather than if the lock count is non-negative.
> > This helps further reduce unnecessary atomic xchg() operations.
> > 
> > Furthermore, this patch uses !mutex_is_locked(lock) to do the initial
> > checks for if the lock is free rather than directly calling atomic_read()
> > on the lock->count, in order to improve readability.
> 
> I think this patch can be merged in 2/4, like you had in v1. Otherwise
> looks good.

Ah, I was thinking that removing the macro would be considered a
separate change whereas this 3/4 patch was more of an "optimization".
But yes, those 2 patches could also have been kept as 1 patch as well.

Thanks for the reviews David and Waiman.

[tip:locking/core] locking/mutexes: Try to acquire mutex only if it is unlocked

[tip-bot for Jason Low]

Commit-ID:  0d968dd8c6aced585b86fa7ba8ce4573bf19e848
Gitweb:     http://git.kernel.org/tip/0d968dd8c6aced585b86fa7ba8ce4573bf19e848
Author:     Jason Low <jason.low2@hp.com>
AuthorDate: Wed, 11 Jun 2014 11:37:22 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 5 Jul 2014 11:25:42 +0200

locking/mutexes: Try to acquire mutex only if it is unlocked

Upon entering the slowpath in __mutex_lock_common(), we try once more to
acquire the mutex. We only try to acquire if (lock->count >= 0). However,
what we actually want here is to try to acquire if the mutex is unlocked
(lock->count == 1).

This patch changes it so that we only try-acquire the mutex upon entering
the slowpath if it is unlocked, rather than if the lock count is non-negative.
This helps further reduce unnecessary atomic xchg() operations.

Furthermore, this patch uses !mutex_is_locked(lock) to do the initial
checks for if the lock is free rather than directly calling atomic_read()
on the lock->count, in order to improve readability.

Signed-off-by: Jason Low <jason.low2@hp.com>
Acked-by: Waiman Long <Waiman.Long@hp.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Cc: akpm@linux-foundation.org
Cc: tim.c.chen@linux.intel.com
Cc: paulmck@linux.vnet.ibm.com
Cc: rostedt@goodmis.org
Cc: davidlohr@hp.com
Cc: scott.norton@hp.com
Cc: aswin@hp.com
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/1402511843-4721-4-git-send-email-jason.low2@hp.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/mutex.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 4bd9546..e4d997b 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -432,7 +432,8 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 		if (owner && !mutex_spin_on_owner(lock, owner))
 			break;
 
-		if ((atomic_read(&lock->count) == 1) &&
+		/* Try to acquire the mutex if it is unlocked. */
+		if (!mutex_is_locked(lock) &&
 		    (atomic_cmpxchg(&lock->count, 1, 0) == 1)) {
 			lock_acquired(&lock->dep_map, ip);
 			if (use_ww_ctx) {
@@ -479,9 +480,9 @@ slowpath:
 
 	/*
 	 * Once more, try to acquire the lock. Only try-lock the mutex if
-	 * lock->count >= 0 to reduce unnecessary xchg operations.
+	 * it is unlocked to reduce unnecessary xchg() operations.
 	 */
-	if (atomic_read(&lock->count) >= 0 && (atomic_xchg(&lock->count, 0) == 1))
+	if (!mutex_is_locked(lock) && (atomic_xchg(&lock->count, 0) == 1))
 		goto skip_wait;
 
 	debug_mutex_lock_common(lock, &waiter);

[PATCH v2 4/4] mutex: Optimize mutex trylock slowpath

[Jason Low]

The mutex_trylock() function calls into __mutex_trylock_fastpath() when
trying to obtain the mutex. On 32 bit x86, in the !__HAVE_ARCH_CMPXCHG
case, __mutex_trylock_fastpath() calls directly into __mutex_trylock_slowpath()
regardless of whether or not the mutex is locked.

In __mutex_trylock_slowpath(), we then acquire the wait_lock spinlock, xchg()
lock->count with -1, then set lock->count back to 0 if there are no waiters,
and return true if the prev lock count was 1.

However, if the mutex is already locked, then there isn't much point
in attempting all of the above expensive operations. In this patch, we only
attempt the above trylock operations if the mutex is unlocked.

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

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index e4d997b..11b103d 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -820,6 +820,10 @@ static inline int __mutex_trylock_slowpath(atomic_t *lock_count)
 	unsigned long flags;
 	int prev;
 
+	/* No need to trylock if the mutex is locked. */
+	if (mutex_is_locked(lock))
+		return 0;
+
 	spin_lock_mutex(&lock->wait_lock, flags);
 
 	prev = atomic_xchg(&lock->count, -1);
-- 
1.7.1

Re: [PATCH v2 4/4] mutex: Optimize mutex trylock slowpath

[Davidlohr Bueso]

On Wed, 2014-06-11 at 11:37 -0700, Jason Low wrote:
> The mutex_trylock() function calls into __mutex_trylock_fastpath() when
> trying to obtain the mutex. On 32 bit x86, in the !__HAVE_ARCH_CMPXCHG
> case, __mutex_trylock_fastpath() calls directly into __mutex_trylock_slowpath()
> regardless of whether or not the mutex is locked.
> 
> In __mutex_trylock_slowpath(), we then acquire the wait_lock spinlock, xchg()
> lock->count with -1, then set lock->count back to 0 if there are no waiters,
> and return true if the prev lock count was 1.
> 
> However, if the mutex is already locked, then there isn't much point
> in attempting all of the above expensive operations. In this patch, we only
> attempt the above trylock operations if the mutex is unlocked.
> 
> Signed-off-by: Jason Low <jason.low2@hp.com>

This is significantly cleaner than the v1 patch.

Reviewed-by: Davidlohr Bueso <davidlohr@hp.com>

[tip:locking/core] locking/mutexes: Optimize mutex trylock slowpath

[tip-bot for Jason Low]

Commit-ID:  72d5305dcb3637913c2c37e847a4de9028e49244
Gitweb:     http://git.kernel.org/tip/72d5305dcb3637913c2c37e847a4de9028e49244
Author:     Jason Low <jason.low2@hp.com>
AuthorDate: Wed, 11 Jun 2014 11:37:23 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 5 Jul 2014 11:25:42 +0200

locking/mutexes: Optimize mutex trylock slowpath

The mutex_trylock() function calls into __mutex_trylock_fastpath() when
trying to obtain the mutex. On 32 bit x86, in the !__HAVE_ARCH_CMPXCHG
case, __mutex_trylock_fastpath() calls directly into __mutex_trylock_slowpath()
regardless of whether or not the mutex is locked.

In __mutex_trylock_slowpath(), we then acquire the wait_lock spinlock, xchg()
lock->count with -1, then set lock->count back to 0 if there are no waiters,
and return true if the prev lock count was 1.

However, if the mutex is already locked, then there isn't much point
in attempting all of the above expensive operations. In this patch, we only
attempt the above trylock operations if the mutex is unlocked.

Signed-off-by: Jason Low <jason.low2@hp.com>
Reviewed-by: Davidlohr Bueso <davidlohr@hp.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Cc: akpm@linux-foundation.org
Cc: tim.c.chen@linux.intel.com
Cc: paulmck@linux.vnet.ibm.com
Cc: rostedt@goodmis.org
Cc: Waiman.Long@hp.com
Cc: scott.norton@hp.com
Cc: aswin@hp.com
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/1402511843-4721-5-git-send-email-jason.low2@hp.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/mutex.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index e4d997b..11b103d 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -820,6 +820,10 @@ static inline int __mutex_trylock_slowpath(atomic_t *lock_count)
 	unsigned long flags;
 	int prev;
 
+	/* No need to trylock if the mutex is locked. */
+	if (mutex_is_locked(lock))
+		return 0;
+
 	spin_lock_mutex(&lock->wait_lock, flags);
 
 	prev = atomic_xchg(&lock->count, -1);