Apparently despite it being marked inline, the compiler may not inline __down_read_common() which makes it difficult to identify the cause of lock contention, as the blocked function will always be listed as __down_read_common().
So this patch adds __sched annotation to the function so the calling function will instead be listed.
Cc: Minchan Kim minchan@kernel.org Cc: Tim Murray timmurray@google.com Cc: Peter Zijlstra peterz@infradead.org Cc: Ingo Molnar mingo@redhat.com Cc: Will Deacon will@kernel.org Cc: Waiman Long longman@redhat.com Cc: Boqun Feng boqun.feng@gmail.com Cc: kernel-team@android.com Cc: stable@vger.kernel.org Fixes: c995e638ccbb ("locking/rwsem: Fold __down_{read,write}*()") Reported-by: Tim Murray timmurray@google.com Signed-off-by: John Stultz jstultz@google.com --- kernel/locking/rwsem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c index acb5a50309a1..cde2f22e65a8 100644 --- a/kernel/locking/rwsem.c +++ b/kernel/locking/rwsem.c @@ -1240,7 +1240,7 @@ static struct rw_semaphore *rwsem_downgrade_wake(struct rw_semaphore *sem) /* * lock for reading */ -static inline int __down_read_common(struct rw_semaphore *sem, int state) +static inline __sched int __down_read_common(struct rw_semaphore *sem, int state) { int ret = 0; long count;
On 4/11/23 22:38, John Stultz wrote:
Apparently despite it being marked inline, the compiler may not inline __down_read_common() which makes it difficult to identify the cause of lock contention, as the blocked function will always be listed as __down_read_common().
So this patch adds __sched annotation to the function so the calling function will instead be listed.
Cc: Minchan Kim minchan@kernel.org Cc: Tim Murray timmurray@google.com Cc: Peter Zijlstra peterz@infradead.org Cc: Ingo Molnar mingo@redhat.com Cc: Will Deacon will@kernel.org Cc: Waiman Long longman@redhat.com Cc: Boqun Feng boqun.feng@gmail.com Cc: kernel-team@android.com Cc: stable@vger.kernel.org Fixes: c995e638ccbb ("locking/rwsem: Fold __down_{read,write}*()") Reported-by: Tim Murray timmurray@google.com Signed-off-by: John Stultz jstultz@google.com
kernel/locking/rwsem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c index acb5a50309a1..cde2f22e65a8 100644 --- a/kernel/locking/rwsem.c +++ b/kernel/locking/rwsem.c @@ -1240,7 +1240,7 @@ static struct rw_semaphore *rwsem_downgrade_wake(struct rw_semaphore *sem) /*
- lock for reading
*/ -static inline int __down_read_common(struct rw_semaphore *sem, int state) +static inline __sched int __down_read_common(struct rw_semaphore *sem, int state) { int ret = 0; long count;
Change inline to __always_inline instead of adding __sched. __down_read_common() is not supposed to be a standalone function.
Cheers, Longman
On Tue, Apr 11, 2023 at 7:58 PM Waiman Long longman@redhat.com wrote:
On 4/11/23 22:38, John Stultz wrote:
Apparently despite it being marked inline, the compiler may not inline __down_read_common() which makes it difficult to identify the cause of lock contention, as the blocked function will always be listed as __down_read_common().
So this patch adds __sched annotation to the function so the calling function will instead be listed.
...
-static inline int __down_read_common(struct rw_semaphore *sem, int state) +static inline __sched int __down_read_common(struct rw_semaphore *sem, int state) { int ret = 0; long count;
Change inline to __always_inline instead of adding __sched. __down_read_common() is not supposed to be a standalone function.
Sounds good. I'll give that a go and will re-submit! Thanks for the review! -john
Apparently despite it being marked inline, the compiler may not inline __down_read_common() which makes it difficult to identify the cause of lock contention, as the blocked function will always be listed as __down_read_common().
So this patch adds __always_inline annotation to the function to force it to be inlines so the calling function will be listed.
Cc: Minchan Kim minchan@kernel.org Cc: Tim Murray timmurray@google.com Cc: Peter Zijlstra peterz@infradead.org Cc: Ingo Molnar mingo@redhat.com Cc: Will Deacon will@kernel.org Cc: Waiman Long longman@redhat.com Cc: Boqun Feng boqun.feng@gmail.com Cc: kernel-team@android.com Cc: stable@vger.kernel.org Fixes: c995e638ccbb ("locking/rwsem: Fold __down_{read,write}*()") Reported-by: Tim Murray timmurray@google.com Signed-off-by: John Stultz jstultz@google.com --- v2: Reworked to use __always_inline instead of __sched as suggested by Waiman Long --- kernel/locking/rwsem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c index acb5a50309a1..e99eef8ea552 100644 --- a/kernel/locking/rwsem.c +++ b/kernel/locking/rwsem.c @@ -1240,7 +1240,7 @@ static struct rw_semaphore *rwsem_downgrade_wake(struct rw_semaphore *sem) /* * lock for reading */ -static inline int __down_read_common(struct rw_semaphore *sem, int state) +static __always_inline int __down_read_common(struct rw_semaphore *sem, int state) { int ret = 0; long count;
On 4/11/23 23:59, John Stultz wrote:
Apparently despite it being marked inline, the compiler may not inline __down_read_common() which makes it difficult to identify the cause of lock contention, as the blocked function will always be listed as __down_read_common().
So this patch adds __always_inline annotation to the function to force it to be inlines so the calling function will be listed.
Cc: Minchan Kim minchan@kernel.org Cc: Tim Murray timmurray@google.com Cc: Peter Zijlstra peterz@infradead.org Cc: Ingo Molnar mingo@redhat.com Cc: Will Deacon will@kernel.org Cc: Waiman Long longman@redhat.com Cc: Boqun Feng boqun.feng@gmail.com Cc: kernel-team@android.com Cc: stable@vger.kernel.org Fixes: c995e638ccbb ("locking/rwsem: Fold __down_{read,write}*()") Reported-by: Tim Murray timmurray@google.com Signed-off-by: John Stultz jstultz@google.com
v2: Reworked to use __always_inline instead of __sched as suggested by Waiman Long
kernel/locking/rwsem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c index acb5a50309a1..e99eef8ea552 100644 --- a/kernel/locking/rwsem.c +++ b/kernel/locking/rwsem.c @@ -1240,7 +1240,7 @@ static struct rw_semaphore *rwsem_downgrade_wake(struct rw_semaphore *sem) /*
- lock for reading
*/ -static inline int __down_read_common(struct rw_semaphore *sem, int state) +static __always_inline int __down_read_common(struct rw_semaphore *sem, int state) { int ret = 0; long count;
Reviewed-by: Waiman Long longman@redhat.com
On Wed, Apr 12, 2023 at 03:59:05AM +0000, John Stultz wrote:
Apparently despite it being marked inline, the compiler may not inline __down_read_common() which makes it difficult to identify the cause of lock contention, as the blocked function will always be listed as __down_read_common().
So this patch adds __always_inline annotation to the function to force it to be inlines so the calling function will be listed.
I'm a wee bit confused; what are you looking at? Wchan? What is stopping the compiler from now handing you __down_read{,_interruptible,_killable}() instead? Is that fine?
On 4/17/23 07:19, Peter Zijlstra wrote:
On Wed, Apr 12, 2023 at 03:59:05AM +0000, John Stultz wrote:
Apparently despite it being marked inline, the compiler may not inline __down_read_common() which makes it difficult to identify the cause of lock contention, as the blocked function will always be listed as __down_read_common().
So this patch adds __always_inline annotation to the function to force it to be inlines so the calling function will be listed.
I'm a wee bit confused; what are you looking at? Wchan? What is stopping the compiler from now handing you __down_read{,_interruptible,_killable}() instead? Is that fine?
My theory is that the compiler may refuse to inline __down_read_common() because it is called 3 times in order to reduce overall code size. The other __down_read*() functions you listed are only called once.
My 2 cents.
Cheers, Longman
On Mon, Apr 17, 2023 at 1:19 PM Peter Zijlstra peterz@infradead.org wrote:
On Wed, Apr 12, 2023 at 03:59:05AM +0000, John Stultz wrote:
Apparently despite it being marked inline, the compiler may not inline __down_read_common() which makes it difficult to identify the cause of lock contention, as the blocked function will always be listed as __down_read_common().
So this patch adds __always_inline annotation to the function to force it to be inlines so the calling function will be listed.
I'm a wee bit confused; what are you looking at? Wchan?
Apologies! Yes, traceevent data via wchan, sorry I didn't make that clear.
What is stopping the compiler from now handing you __down_read{,_interruptible,_killable}() instead? Is that fine?
No, we want to make the blocked calling function, rather than the locking functions, visible in the tracepoints captured. That said, the other __down_read* functions seem to be properly inlined in practice (Waiman's theory as to why sounds convincing to me).
If you'd like I can add those as well to be always_inline, as well so it's more consistent?
thanks -john
On Mon, Apr 17, 2023 at 06:22:14PM +0200, John Stultz wrote:
On Mon, Apr 17, 2023 at 1:19 PM Peter Zijlstra peterz@infradead.org wrote:
On Wed, Apr 12, 2023 at 03:59:05AM +0000, John Stultz wrote:
Apparently despite it being marked inline, the compiler may not inline __down_read_common() which makes it difficult to identify the cause of lock contention, as the blocked function will always be listed as __down_read_common().
So this patch adds __always_inline annotation to the function to force it to be inlines so the calling function will be listed.
I'm a wee bit confused; what are you looking at? Wchan?
Apologies! Yes, traceevent data via wchan, sorry I didn't make that clear.
No worries; good addition to the v3 Changelog ;-)
What is stopping the compiler from now handing you __down_read{,_interruptible,_killable}() instead? Is that fine?
No, we want to make the blocked calling function, rather than the locking functions, visible in the tracepoints captured. That said, the other __down_read* functions seem to be properly inlined in practice (Waiman's theory as to why sounds convincing to me).
Right, but we should not rely on the compiler heuristics for correctness :-)
If you'd like I can add those as well to be always_inline, as well so it's more consistent?
Yes please. I'm not sure I care much about the whole 'inline __sched' vs '__always_inline' thing, but I do feel it should all be consistently applied.
On Tue, Apr 18, 2023 at 12:30 PM Peter Zijlstra peterz@infradead.org wrote:
On Mon, Apr 17, 2023 at 06:22:14PM +0200, John Stultz wrote:
On Mon, Apr 17, 2023 at 1:19 PM Peter Zijlstra peterz@infradead.org wrote:
On Wed, Apr 12, 2023 at 03:59:05AM +0000, John Stultz wrote:
Apparently despite it being marked inline, the compiler may not inline __down_read_common() which makes it difficult to identify the cause of lock contention, as the blocked function will always be listed as __down_read_common().
So this patch adds __always_inline annotation to the function to force it to be inlines so the calling function will be listed.
I'm a wee bit confused; what are you looking at? Wchan?
Apologies! Yes, traceevent data via wchan, sorry I didn't make that clear.
No worries; good addition to the v3 Changelog ;-)
What is stopping the compiler from now handing you __down_read{,_interruptible,_killable}() instead? Is that fine?
No, we want to make the blocked calling function, rather than the locking functions, visible in the tracepoints captured. That said, the other __down_read* functions seem to be properly inlined in practice (Waiman's theory as to why sounds convincing to me).
Right, but we should not rely on the compiler heuristics for correctness :-)
If you'd like I can add those as well to be always_inline, as well so it's more consistent?
Yes please. I'm not sure I care much about the whole 'inline __sched' vs '__always_inline' thing, but I do feel it should all be consistently applied.
Sounds good. I'll respin with this.
Thanks so much for the review! -john
linux-stable-mirror@lists.linaro.org
-
John Stultz -
Peter Zijlstra -
Waiman Long