locking/rwsem: Add __always_inline annotation to __down_write_common() and inlined callers

RFC PATCH Superseded John Stultz 2 participants 2 messages Apr 5, 2024 – Apr 6, 2024 Locking primitives

Share & tools

John Stultz

author

2024-04-05 20:05 UTC #0 lore raw

RFC PATCH 1/1 locking/rwsem: Add __always_inline annotation to __down_write_common() and inlined callers

Apparently despite it being marked inline, the compiler may not inline __down_write_common() which makes it difficult to identify the cause of lock contention, as the blocked function in traceevents will always be listed as __down_write_common().

So add __always_inline annotation to the common function (as well as the inlined helper callers) to force it to be inlined so the blocking function will be listed (via Wchan) in traceevents.

This mirrors commit 92cc5d00a431 ("locking/rwsem: Add __always_inline annotation to __down_read_common() and inlined callers") which did the same for __down_read_common.

I sort of worry that I'm playing wack-a-mole here, and talking with compiler people, they tell me inline means nothing, which makes me want to cry a little. So I'm wondering if we need to replace all the inlines with __always_inline, or remove them because either we mean something by it, or not.

Cc Tim Murray <[email protected]>

Cc Nick Desaulniers <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Will Deacon <[email protected]>

Cc Waiman Long <[email protected]>

Cc Boqun Feng <[email protected]>

Cc [email protected]

Fixes c995e638ccbb ("locking/rwsem: Fold __down_{read,write}*()")

Reported-by Tim Murray <[email protected]>

Signed-off-by John Stultz <[email protected]>

---
 kernel/locking/rwsem.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

kernel/locking/rwsem.c +3 -3

index c6d17aee4209..33cac79e3994 100644
@@ -1297,7 +1297,7 @@ static inline int __down_read_trylock(struct rw_semaphore *sem)
 /*
  * lock for writing
  */
-static inline int __down_write_common(struct rw_semaphore *sem, int state)
+static __always_inline int __down_write_common(struct rw_semaphore *sem, int state)
 {
 	int ret = 0;

@@ -1310,12 +1310,12 @@ static inline int __down_write_common(struct rw_semaphore *sem, int state)
 	return ret;
 }
 
-static inline void __down_write(struct rw_semaphore *sem)
+static __always_inline void __down_write(struct rw_semaphore *sem)
 {
 	__down_write_common(sem, TASK_UNINTERRUPTIBLE);
 }
 
-static inline int __down_write_killable(struct rw_semaphore *sem)
+static __always_inline int __down_write_killable(struct rw_semaphore *sem)
 {
 	return __down_write_common(sem, TASK_KILLABLE);
 }

Signature · 1 line

2.44.0.478.gd926399ef9-goog

Waiman Long

reviewer Acked-by on v1 1/1

2024-04-06 02:40 UTC #1 lore raw

Quoted 54 lines · John Stultz

Apparently despite it being marked inline, the compiler may not inline __down_write_common() which makes it difficult to identify the cause of lock contention, as the blocked function in traceevents will always be listed as __down_write_common().
So add __always_inline annotation to the common function (as well as the inlined helper callers) to force it to be inlined so the blocking function will be listed (via Wchan) in traceevents.
This mirrors commit 92cc5d00a431 ("locking/rwsem: Add __always_inline annotation to __down_read_common() and inlined callers") which did the same for __down_read_common.
I sort of worry that I'm playing wack-a-mole here, and talking with compiler people, they tell me inline means nothing, which makes me want to cry a little. So I'm wondering if we need to replace all the inlines with __always_inline, or remove them because either we mean something by it, or not.
Cc Tim Murray <[email protected]>
Cc Nick Desaulniers <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Will Deacon <[email protected]>
Cc Waiman Long <[email protected]>
Cc Boqun Feng <[email protected]>
Cc [email protected]
Fixes c995e638ccbb ("locking/rwsem: Fold __down_{read,write}*()")
Reported-by Tim Murray <[email protected]>
Signed-off-by John Stultz <[email protected]>
---
  kernel/locking/rwsem.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
kernel/locking/rwsem.c +1 -2
index c6d17aee4209..33cac79e3994 100644
@@ -1297,7 +1297,7 @@ static inline int __down_read_trylock(struct rw_semaphore *sem)
  /*
   * lock for writing
   */
-static inline int __down_write_common(struct rw_semaphore *sem, int state)
+static __always_inline int __down_write_common(struct rw_semaphore *sem, int state)
  {
  	int ret = 0;
 
@@ -1310,12 +1310,12 @@ static inline int __down_write_common(struct rw_semaphore *sem, int state)
  	return ret;
  }
 
-static inline void __down_write(struct rw_semaphore *sem)

kernel/locking/rwsem.c static inline int __down_write_common(struct rw_semaphore *sem, int state) · line 1321

Quoted 10 lines

kernel/locking/rwsem.c +2 -1

+static __always_inline void __down_write(struct rw_semaphore *sem)
  {
  	__down_write_common(sem, TASK_UNINTERRUPTIBLE);
  }
 
-static inline int __down_write_killable(struct rw_semaphore *sem)
+static __always_inline int __down_write_killable(struct rw_semaphore *sem)
  {
  	return __down_write_common(sem, TASK_KILLABLE);
  }

Whether inlining happens or not really depends on the compiler used. Anyway,

Acked-by Waiman Long <[email protected]>

Thanks,
Longman