linux-kernel - Re: [RFC][PATCH] locking/rwsem: Add __always_inline annotation to __down_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <221c28a1-cb61-4a8f-96b5-d9407e53d759@redhat.com>
Date: Fri, 5 Apr 2024 22:39:55 -0400
From: Waiman Long <longman@...hat.com>
To: John Stultz <jstultz@...gle.com>, LKML <linux-kernel@...r.kernel.org>
Cc: Tim Murray <timmurray@...gle.com>,
 Nick Desaulniers <ndesaulniers@...gle.com>,
 Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Will Deacon <will@...nel.org>, Boqun Feng <boqun.feng@...il.com>,
 kernel-team@...roid.com
Subject: Re: [RFC][PATCH] locking/rwsem: Add __always_inline annotation to
 __down_write_common() and inlined callers

On 4/5/24 16:05, John Stultz wrote:
> 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 <timmurray@...gle.com>
> Cc: Nick Desaulniers <ndesaulniers@...gle.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Will Deacon <will@...nel.org>
> Cc: Waiman Long <longman@...hat.com>
> Cc: Boqun Feng <boqun.feng@...il.com>
> Cc: kernel-team@...roid.com
> Fixes: c995e638ccbb ("locking/rwsem: Fold __down_{read,write}*()")
> Reported-by: Tim Murray <timmurray@...gle.com>
> Signed-off-by: John Stultz <jstultz@...gle.com>
> ---
>   kernel/locking/rwsem.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
> index c6d17aee4209..33cac79e3994 100644
> --- a/kernel/locking/rwsem.c
> +++ b/kernel/locking/rwsem.c
> @@ -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);
>   }

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

Acked-by: Waiman Long <longman@...hat.com>


Thanks,
Longman