No one use this macro after it was introduced. Better to remove it?
Signed-off-by: Alex Shi <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: [email protected]
---
kernel/locking/rtmutex.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 851bbb10819d..a849964a348d 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -141,7 +141,6 @@ static void fixup_rt_mutex_waiters(struct rt_mutex *lock)
* set up.
*/
#ifndef CONFIG_DEBUG_RT_MUTEXES
-# define rt_mutex_cmpxchg_relaxed(l,c,n) (cmpxchg_relaxed(&l->owner, c, n) == c)
# define rt_mutex_cmpxchg_acquire(l,c,n) (cmpxchg_acquire(&l->owner, c, n) == c)
# define rt_mutex_cmpxchg_release(l,c,n) (cmpxchg_release(&l->owner, c, n) == c)
--
1.8.3.1
Cc'ing tglx.
On Tue, 21 Jan 2020, Alex Shi wrote:
>No one use this macro after it was introduced. Better to remove it?
You also need to remove it for the CONFIG_DEBUG_RT_MUTEXES=y case.
>
>Signed-off-by: Alex Shi <[email protected]>
>Cc: Peter Zijlstra <[email protected]>
>Cc: Davidlohr Bueso <[email protected]>
>Cc: Ingo Molnar <[email protected]>
>Cc: Will Deacon <[email protected]>
>Cc: [email protected]
>---
> kernel/locking/rtmutex.c | 1 -
> 1 file changed, 1 deletion(-)
>
>diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
>index 851bbb10819d..a849964a348d 100644
>--- a/kernel/locking/rtmutex.c
>+++ b/kernel/locking/rtmutex.c
>@@ -141,7 +141,6 @@ static void fixup_rt_mutex_waiters(struct rt_mutex *lock)
> * set up.
> */
> #ifndef CONFIG_DEBUG_RT_MUTEXES
>-# define rt_mutex_cmpxchg_relaxed(l,c,n) (cmpxchg_relaxed(&l->owner, c, n) == c)
> # define rt_mutex_cmpxchg_acquire(l,c,n) (cmpxchg_acquire(&l->owner, c, n) == c)
> # define rt_mutex_cmpxchg_release(l,c,n) (cmpxchg_release(&l->owner, c, n) == c)
Hmm unrelated, but do we want CCAS for rtmutex fastpath? Ie:
(l->owner == c && cmpxchg_acquire(&l->owner, c, n) == c)
That would optimize for the contended case and avoid the cmpxchg - it would
also help if we ever do the top-waiter spin thing.
Thanks,
Davidlohr
Davidlohr Bueso <[email protected]> writes:
> On Tue, 21 Jan 2020, Alex Shi wrote:
Subject: locking/rtmutex: remove unused cmpxchg_relaxed
should be
Subject: locking/rtmutex: Remove unused rt_mutex_cmpxchg_relaxed()
You're not removing cmpxchg_relaxed, right?
>> No one use this macro after it was introduced. Better to remove it?
Please make that factual.
The macro was never used at all. Remove it.
> You also need to remove it for the CONFIG_DEBUG_RT_MUTEXES=y case.
Yes.
> Hmm unrelated, but do we want CCAS for rtmutex fastpath? Ie:
>
> (l->owner == c && cmpxchg_acquire(&l->owner, c, n) == c)
>
> That would optimize for the contended case and avoid the cmpxchg - it would
> also help if we ever do the top-waiter spin thing.
Not sure if it buys much, but it kinda makes sense.
Thanks,
tglx
?? 2020/2/1 ????4:23, Thomas Gleixner ะด??:
> Davidlohr Bueso <[email protected]> writes:
>> On Tue, 21 Jan 2020, Alex Shi wrote:
>
> Subject: locking/rtmutex: remove unused cmpxchg_relaxed
>
> should be
>
> Subject: locking/rtmutex: Remove unused rt_mutex_cmpxchg_relaxed()
>
> You're not removing cmpxchg_relaxed, right?
>
>>> No one use this macro after it was introduced. Better to remove it?
>
> Please make that factual.
>
> The macro was never used at all. Remove it.
>
>> You also need to remove it for the CONFIG_DEBUG_RT_MUTEXES=y case.
>
> Yes.
>
>> Hmm unrelated, but do we want CCAS for rtmutex fastpath? Ie:
>>
>> (l->owner == c && cmpxchg_acquire(&l->owner, c, n) == c)
>>
>> That would optimize for the contended case and avoid the cmpxchg - it would
>> also help if we ever do the top-waiter spin thing.
>
> Not sure if it buys much, but it kinda makes sense.
>
> Thanks,
>
> tglx
>
Thanks Thomas and David!
Is this following patch ok?
Thanks
Alex
---
From 4cf9e38a73c67c6894f3addb2ddca26bb51b1a28 Mon Sep 17 00:00:00 2001
From: Alex Shi <[email protected]>
Date: Tue, 21 Jan 2020 15:03:33 +0800
Subject: [PATCH v2] locking/rtmutex: optimize rt_mutex_cmpxchg_xxx series func
rt_mutex_cmpxchg_relexed isn't interested by anyone, so remove it.
And Davidlohr Bueso suggests check l->owner before cmpxchg to reduce
lock contention.
Signed-off-by: Alex Shi <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: [email protected]
---
kernel/locking/rtmutex.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 851bbb10819d..eb26f4e57ce4 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -141,9 +141,10 @@ static void fixup_rt_mutex_waiters(struct rt_mutex *lock)
* set up.
*/
#ifndef CONFIG_DEBUG_RT_MUTEXES
-# define rt_mutex_cmpxchg_relaxed(l,c,n) (cmpxchg_relaxed(&l->owner, c, n) == c)
-# define rt_mutex_cmpxchg_acquire(l,c,n) (cmpxchg_acquire(&l->owner, c, n) == c)
-# define rt_mutex_cmpxchg_release(l,c,n) (cmpxchg_release(&l->owner, c, n) == c)
+# define rt_mutex_cmpxchg_acquire(l,c,n) \
+ (l->owner == c && cmpxchg_acquire(&l->owner, c, n) == c)
+# define rt_mutex_cmpxchg_release(l,c,n) \
+ (l->owner == c && cmpxchg_release(&l->owner, c, n) == c)
/*
* Callers must hold the ->wait_lock -- which is the whole purpose as we force
@@ -202,7 +203,6 @@ static inline bool unlock_rt_mutex_safe(struct rt_mutex *lock,
}
#else
-# define rt_mutex_cmpxchg_relaxed(l,c,n) (0)
# define rt_mutex_cmpxchg_acquire(l,c,n) (0)
# define rt_mutex_cmpxchg_release(l,c,n) (0)
--
1.8.3.1
On Tue, 04 Feb 2020, Alex Shi wrote:
>Thanks Thomas and David!
>Is this following patch ok?
So if anything, this really wants to be two patches.
>---
>From 4cf9e38a73c67c6894f3addb2ddca26bb51b1a28 Mon Sep 17 00:00:00 2001
>From: Alex Shi <[email protected]>
>Date: Tue, 21 Jan 2020 15:03:33 +0800
>Subject: [PATCH v2] locking/rtmutex: optimize rt_mutex_cmpxchg_xxx series func
>
>rt_mutex_cmpxchg_relexed isn't interested by anyone, so remove it.
>And Davidlohr Bueso suggests check l->owner before cmpxchg to reduce
>lock contention.
>
>Signed-off-by: Alex Shi <[email protected]>
>Cc: Thomas Gleixner <[email protected]>
>Cc: Davidlohr Bueso <[email protected]>
>Cc: Ingo Molnar <[email protected]>
>Cc: Will Deacon <[email protected]>
>Cc: [email protected]
>---
> kernel/locking/rtmutex.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
>index 851bbb10819d..eb26f4e57ce4 100644
>--- a/kernel/locking/rtmutex.c
>+++ b/kernel/locking/rtmutex.c
>@@ -141,9 +141,10 @@ static void fixup_rt_mutex_waiters(struct rt_mutex *lock)
> * set up.
> */
> #ifndef CONFIG_DEBUG_RT_MUTEXES
>-# define rt_mutex_cmpxchg_relaxed(l,c,n) (cmpxchg_relaxed(&l->owner, c, n) == c)
>-# define rt_mutex_cmpxchg_acquire(l,c,n) (cmpxchg_acquire(&l->owner, c, n) == c)
>-# define rt_mutex_cmpxchg_release(l,c,n) (cmpxchg_release(&l->owner, c, n) == c)
>+# define rt_mutex_cmpxchg_acquire(l,c,n) \
>+ (l->owner == c && cmpxchg_acquire(&l->owner, c, n) == c)
>+# define rt_mutex_cmpxchg_release(l,c,n) \
>+ (l->owner == c && cmpxchg_release(&l->owner, c, n) == c)
Thomas, should I resend the top-waiter spin series? Otherwise yeah,
I see little point to the CCAS fastpath thing.
Thanks,
Davidlohr
This macro isn't interested by anyone, so remove it.
Signed-off-by: Alex Shi <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: [email protected]
---
kernel/locking/rtmutex.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 851bbb10819d..7ad22eade1cc 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -141,7 +141,6 @@ static void fixup_rt_mutex_waiters(struct rt_mutex *lock)
* set up.
*/
#ifndef CONFIG_DEBUG_RT_MUTEXES
-# define rt_mutex_cmpxchg_relaxed(l,c,n) (cmpxchg_relaxed(&l->owner, c, n) == c)
# define rt_mutex_cmpxchg_acquire(l,c,n) (cmpxchg_acquire(&l->owner, c, n) == c)
# define rt_mutex_cmpxchg_release(l,c,n) (cmpxchg_release(&l->owner, c, n) == c)
@@ -202,7 +201,6 @@ static inline bool unlock_rt_mutex_safe(struct rt_mutex *lock,
}
#else
-# define rt_mutex_cmpxchg_relaxed(l,c,n) (0)
# define rt_mutex_cmpxchg_acquire(l,c,n) (0)
# define rt_mutex_cmpxchg_release(l,c,n) (0)
--
1.8.3.1
Checking l->owner first to skip time cost cmpxchgs.
Suggested-by: Davidlohr Bueso <[email protected]>
Signed-off-by: Alex Shi <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Davidlohr Bueso <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: [email protected]
---
kernel/locking/rtmutex.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 7ad22eade1cc..e8df466af8ab 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -141,8 +141,10 @@ static void fixup_rt_mutex_waiters(struct rt_mutex *lock)
* set up.
*/
#ifndef CONFIG_DEBUG_RT_MUTEXES
-# define rt_mutex_cmpxchg_acquire(l,c,n) (cmpxchg_acquire(&l->owner, c, n) == c)
-# define rt_mutex_cmpxchg_release(l,c,n) (cmpxchg_release(&l->owner, c, n) == c)
+# define rt_mutex_cmpxchg_acquire(l, c, n) \
+ (l->owner == c && cmpxchg_acquire(&l->owner, c, n) == c)
+# define rt_mutex_cmpxchg_release(l, c, n) \
+ (l->owner == c && cmpxchg_release(&l->owner, c, n) == c)
/*
* Callers must hold the ->wait_lock -- which is the whole purpose as we force
--
1.8.3.1