* [PATCH v2 1/4] mutex: Correct documentation on mutex optimistic spinning
2014-06-11 18:37 [PATCH v2 0/4] mutex: Modifications to mutex Jason Low
@ 2014-06-11 18:37 ` Jason Low
2014-07-05 10:47 ` [tip:locking/core] locking/mutexes: " tip-bot for Jason Low
2014-06-11 18:37 ` [PATCH v2 2/4] mutex: Delete the MUTEX_SHOW_NO_WAITER macro Jason Low
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Jason Low @ 2014-06-11 18:37 UTC (permalink / raw)
To: mingo, peterz, tglx, akpm
Cc: linux-kernel, tim.c.chen, paulmck, rostedt, davidlohr,
Waiman.Long, scott.norton, aswin, jason.low2
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [tip:locking/core] locking/mutexes: Correct documentation on mutex optimistic spinning
2014-06-11 18:37 ` [PATCH v2 1/4] mutex: Correct documentation on mutex optimistic spinning Jason Low
@ 2014-07-05 10:47 ` tip-bot for Jason Low
0 siblings, 0 replies; 14+ messages in thread
From: tip-bot for Jason Low @ 2014-07-05 10:47 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, torvalds, peterz, jason.low2, tglx, davidlohr
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/4] mutex: Delete the MUTEX_SHOW_NO_WAITER macro
2014-06-11 18:37 [PATCH v2 0/4] mutex: Modifications to mutex Jason Low
2014-06-11 18:37 ` [PATCH v2 1/4] mutex: Correct documentation on mutex optimistic spinning Jason Low
@ 2014-06-11 18:37 ` Jason Low
2014-06-12 1:27 ` Long, Wai Man
2014-07-05 10:47 ` [tip:locking/core] locking/mutexes: " tip-bot for Jason Low
2014-06-11 18:37 ` [PATCH v2 3/4] mutex: Try to acquire mutex only if it is unlocked Jason Low
2014-06-11 18:37 ` [PATCH v2 4/4] mutex: Optimize mutex trylock slowpath Jason Low
3 siblings, 2 replies; 14+ messages in thread
From: Jason Low @ 2014-06-11 18:37 UTC (permalink / raw)
To: mingo, peterz, tglx, akpm
Cc: linux-kernel, tim.c.chen, paulmck, rostedt, davidlohr,
Waiman.Long, scott.norton, aswin, jason.low2
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/4] mutex: Delete the MUTEX_SHOW_NO_WAITER macro
2014-06-11 18:37 ` [PATCH v2 2/4] mutex: Delete the MUTEX_SHOW_NO_WAITER macro Jason Low
@ 2014-06-12 1:27 ` Long, Wai Man
2014-07-05 10:47 ` [tip:locking/core] locking/mutexes: " tip-bot for Jason Low
1 sibling, 0 replies; 14+ messages in thread
From: Long, Wai Man @ 2014-06-12 1:27 UTC (permalink / raw)
To: Jason Low, mingo, peterz, tglx, akpm
Cc: linux-kernel, tim.c.chen, paulmck, rostedt, davidlohr,
scott.norton, aswin
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tip:locking/core] locking/mutexes: Delete the MUTEX_SHOW_NO_WAITER macro
2014-06-11 18:37 ` [PATCH v2 2/4] mutex: Delete the MUTEX_SHOW_NO_WAITER macro Jason Low
2014-06-12 1:27 ` Long, Wai Man
@ 2014-07-05 10:47 ` tip-bot for Jason Low
1 sibling, 0 replies; 14+ messages in thread
From: tip-bot for Jason Low @ 2014-07-05 10:47 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, torvalds, peterz, jason.low2,
Waiman.Long, tglx
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;
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 3/4] mutex: Try to acquire mutex only if it is unlocked
2014-06-11 18:37 [PATCH v2 0/4] mutex: Modifications to mutex Jason Low
2014-06-11 18:37 ` [PATCH v2 1/4] mutex: Correct documentation on mutex optimistic spinning Jason Low
2014-06-11 18:37 ` [PATCH v2 2/4] mutex: Delete the MUTEX_SHOW_NO_WAITER macro Jason Low
@ 2014-06-11 18:37 ` Jason Low
2014-06-12 1:28 ` Long, Wai Man
` (2 more replies)
2014-06-11 18:37 ` [PATCH v2 4/4] mutex: Optimize mutex trylock slowpath Jason Low
3 siblings, 3 replies; 14+ messages in thread
From: Jason Low @ 2014-06-11 18:37 UTC (permalink / raw)
To: mingo, peterz, tglx, akpm
Cc: linux-kernel, tim.c.chen, paulmck, rostedt, davidlohr,
Waiman.Long, scott.norton, aswin, jason.low2
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/4] mutex: Try to acquire mutex only if it is unlocked
2014-06-11 18:37 ` [PATCH v2 3/4] mutex: Try to acquire mutex only if it is unlocked Jason Low
@ 2014-06-12 1:28 ` Long, Wai Man
2014-06-12 19:37 ` Davidlohr Bueso
2014-07-05 10:47 ` [tip:locking/core] locking/mutexes: " tip-bot for Jason Low
2 siblings, 0 replies; 14+ messages in thread
From: Long, Wai Man @ 2014-06-12 1:28 UTC (permalink / raw)
To: Jason Low, mingo, peterz, tglx, akpm
Cc: linux-kernel, tim.c.chen, paulmck, rostedt, davidlohr,
scott.norton, aswin
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/4] mutex: Try to acquire mutex only if it is unlocked
2014-06-11 18:37 ` [PATCH v2 3/4] mutex: Try to acquire mutex only if it is unlocked Jason Low
2014-06-12 1:28 ` Long, Wai Man
@ 2014-06-12 19:37 ` Davidlohr Bueso
2014-06-12 19:54 ` Jason Low
2014-07-05 10:47 ` [tip:locking/core] locking/mutexes: " tip-bot for Jason Low
2 siblings, 1 reply; 14+ messages in thread
From: Davidlohr Bueso @ 2014-06-12 19:37 UTC (permalink / raw)
To: Jason Low
Cc: mingo, peterz, tglx, akpm, linux-kernel, tim.c.chen, paulmck,
rostedt, Waiman.Long, scott.norton, aswin
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/4] mutex: Try to acquire mutex only if it is unlocked
2014-06-12 19:37 ` Davidlohr Bueso
@ 2014-06-12 19:54 ` Jason Low
0 siblings, 0 replies; 14+ messages in thread
From: Jason Low @ 2014-06-12 19:54 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: mingo, peterz, tglx, akpm, linux-kernel, tim.c.chen, paulmck,
rostedt, Waiman.Long, scott.norton, aswin
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tip:locking/core] locking/mutexes: Try to acquire mutex only if it is unlocked
2014-06-11 18:37 ` [PATCH v2 3/4] mutex: Try to acquire mutex only if it is unlocked Jason Low
2014-06-12 1:28 ` Long, Wai Man
2014-06-12 19:37 ` Davidlohr Bueso
@ 2014-07-05 10:47 ` tip-bot for Jason Low
2 siblings, 0 replies; 14+ messages in thread
From: tip-bot for Jason Low @ 2014-07-05 10:47 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, torvalds, peterz, jason.low2,
Waiman.Long, tglx
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);
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 4/4] mutex: Optimize mutex trylock slowpath
2014-06-11 18:37 [PATCH v2 0/4] mutex: Modifications to mutex Jason Low
` (2 preceding siblings ...)
2014-06-11 18:37 ` [PATCH v2 3/4] mutex: Try to acquire mutex only if it is unlocked Jason Low
@ 2014-06-11 18:37 ` Jason Low
2014-06-12 18:25 ` Davidlohr Bueso
2014-07-05 10:48 ` [tip:locking/core] locking/mutexes: " tip-bot for Jason Low
3 siblings, 2 replies; 14+ messages in thread
From: Jason Low @ 2014-06-11 18:37 UTC (permalink / raw)
To: mingo, peterz, tglx, akpm
Cc: linux-kernel, tim.c.chen, paulmck, rostedt, davidlohr,
Waiman.Long, scott.norton, aswin, jason.low2
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/4] mutex: Optimize mutex trylock slowpath
2014-06-11 18:37 ` [PATCH v2 4/4] mutex: Optimize mutex trylock slowpath Jason Low
@ 2014-06-12 18:25 ` Davidlohr Bueso
2014-07-05 10:48 ` [tip:locking/core] locking/mutexes: " tip-bot for Jason Low
1 sibling, 0 replies; 14+ messages in thread
From: Davidlohr Bueso @ 2014-06-12 18:25 UTC (permalink / raw)
To: Jason Low
Cc: mingo, peterz, tglx, akpm, linux-kernel, tim.c.chen, paulmck,
rostedt, Waiman.Long, scott.norton, aswin
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [tip:locking/core] locking/mutexes: Optimize mutex trylock slowpath
2014-06-11 18:37 ` [PATCH v2 4/4] mutex: Optimize mutex trylock slowpath Jason Low
2014-06-12 18:25 ` Davidlohr Bueso
@ 2014-07-05 10:48 ` tip-bot for Jason Low
1 sibling, 0 replies; 14+ messages in thread
From: tip-bot for Jason Low @ 2014-07-05 10:48 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, torvalds, peterz, jason.low2, tglx, davidlohr
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);
^ permalink raw reply related [flat|nested] 14+ messages in thread