| Message ID | 20220301010412.431299-4-namhyung@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | locking: Add new lock contention tracepoints (v2) | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, Feb 28, 2022 at 05:04:11PM -0800, Namhyung Kim wrote: > The __mutex_lock_slowpath() and friends are declared as noinline and > _RET_IP_ returns its caller as mutex_lock which is not meaningful. > Pass the ip from mutex_lock() to have actual caller info in the trace. Blergh, can't you do a very limited unwind when you do the tracing instead? 3 or 4 levels should be plenty fast and sufficient.
On Tue, 1 Mar 2022 10:05:12 +0100 Peter Zijlstra <peterz@infradead.org> wrote: > On Mon, Feb 28, 2022 at 05:04:11PM -0800, Namhyung Kim wrote: > > The __mutex_lock_slowpath() and friends are declared as noinline and > > _RET_IP_ returns its caller as mutex_lock which is not meaningful. > > Pass the ip from mutex_lock() to have actual caller info in the trace. > > Blergh, can't you do a very limited unwind when you do the tracing > instead? 3 or 4 levels should be plenty fast and sufficient. Is there a fast and sufficient way that works across architectures? Could objtool help here? -- Steve
On Tue, Mar 1, 2022 at 1:05 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Mon, Feb 28, 2022 at 05:04:11PM -0800, Namhyung Kim wrote: > > The __mutex_lock_slowpath() and friends are declared as noinline and > > _RET_IP_ returns its caller as mutex_lock which is not meaningful. > > Pass the ip from mutex_lock() to have actual caller info in the trace. > > Blergh, can't you do a very limited unwind when you do the tracing > instead? 3 or 4 levels should be plenty fast and sufficient. Are you talking about getting rid of the ip from the tracepoints? Having stacktraces together is good, but it'd be nice if it provided a way to identify the lock without them too. Thanks, Namhyung
On Tue, Mar 01, 2022 at 09:53:54AM -0500, Steven Rostedt wrote: > On Tue, 1 Mar 2022 10:05:12 +0100 > Peter Zijlstra <peterz@infradead.org> wrote: > > > On Mon, Feb 28, 2022 at 05:04:11PM -0800, Namhyung Kim wrote: > > > The __mutex_lock_slowpath() and friends are declared as noinline and > > > _RET_IP_ returns its caller as mutex_lock which is not meaningful. > > > Pass the ip from mutex_lock() to have actual caller info in the trace. > > > > Blergh, can't you do a very limited unwind when you do the tracing > > instead? 3 or 4 levels should be plenty fast and sufficient. > > Is there a fast and sufficient way that works across architectures? The normal stacktrace API? Or the fancy new arch_stack_walk() which is already available on most architectures you actually care about and risc-v :-) Remember, this is the contention path, we're going to stall anyway, doing a few levels of unwind shouldn't really hurt at that point. Anyway; when I wrote that this morning, I was thinking: unsigned long ips[4]; stack_trace_save(ips, 4, 0); > Could objtool help here? There's a contradition there... objtool is still x86_64 only :-/ IIRC there's been work on s390, arm64 and power objtool, but so far none of them actually made it in.
On Tue, Mar 1, 2022 at 11:47 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Tue, Mar 01, 2022 at 09:53:54AM -0500, Steven Rostedt wrote: > > On Tue, 1 Mar 2022 10:05:12 +0100 > > Peter Zijlstra <peterz@infradead.org> wrote: > > > > > On Mon, Feb 28, 2022 at 05:04:11PM -0800, Namhyung Kim wrote: > > > > The __mutex_lock_slowpath() and friends are declared as noinline and > > > > _RET_IP_ returns its caller as mutex_lock which is not meaningful. > > > > Pass the ip from mutex_lock() to have actual caller info in the trace. > > > > > > Blergh, can't you do a very limited unwind when you do the tracing > > > instead? 3 or 4 levels should be plenty fast and sufficient. > > > > Is there a fast and sufficient way that works across architectures? > > The normal stacktrace API? Or the fancy new arch_stack_walk() which is > already available on most architectures you actually care about and > risc-v :-) > > Remember, this is the contention path, we're going to stall anyway, > doing a few levels of unwind shouldn't really hurt at that point. > > Anyway; when I wrote that this morning, I was thinking: > > unsigned long ips[4]; > stack_trace_save(ips, 4, 0); When I collected stack traces in a BPF, it already consumed 3 or 4 entries in the BPF so I had to increase the size to 8 and skip 4. But it didn't add noticeable overheads in my test. Thanks, Namhyung
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c index 756624c14dfd..126b014098f3 100644 --- a/kernel/locking/mutex.c +++ b/kernel/locking/mutex.c @@ -254,7 +254,7 @@ static void __mutex_handoff(struct mutex *lock, struct task_struct *task) * We also put the fastpath first in the kernel image, to make sure the * branch is predicted by the CPU as default-untaken. */ -static void __sched __mutex_lock_slowpath(struct mutex *lock); +static void __sched __mutex_lock_slowpath(struct mutex *lock, unsigned long ip); /** * mutex_lock - acquire the mutex @@ -282,7 +282,7 @@ void __sched mutex_lock(struct mutex *lock) might_sleep(); if (!__mutex_trylock_fast(lock)) - __mutex_lock_slowpath(lock); + __mutex_lock_slowpath(lock, _RET_IP_); } EXPORT_SYMBOL(mutex_lock); #endif @@ -947,10 +947,10 @@ static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigne * mutex_lock_interruptible() and mutex_trylock(). */ static noinline int __sched -__mutex_lock_killable_slowpath(struct mutex *lock); +__mutex_lock_killable_slowpath(struct mutex *lock, unsigned long ip); static noinline int __sched -__mutex_lock_interruptible_slowpath(struct mutex *lock); +__mutex_lock_interruptible_slowpath(struct mutex *lock, unsigned long ip); /** * mutex_lock_interruptible() - Acquire the mutex, interruptible by signals. @@ -971,7 +971,7 @@ int __sched mutex_lock_interruptible(struct mutex *lock) if (__mutex_trylock_fast(lock)) return 0; - return __mutex_lock_interruptible_slowpath(lock); + return __mutex_lock_interruptible_slowpath(lock, _RET_IP_); } EXPORT_SYMBOL(mutex_lock_interruptible); @@ -995,7 +995,7 @@ int __sched mutex_lock_killable(struct mutex *lock) if (__mutex_trylock_fast(lock)) return 0; - return __mutex_lock_killable_slowpath(lock); + return __mutex_lock_killable_slowpath(lock, _RET_IP_); } EXPORT_SYMBOL(mutex_lock_killable); @@ -1020,36 +1020,36 @@ void __sched mutex_lock_io(struct mutex *lock) EXPORT_SYMBOL_GPL(mutex_lock_io); static noinline void __sched -__mutex_lock_slowpath(struct mutex *lock) +__mutex_lock_slowpath(struct mutex *lock, unsigned long ip) { - __mutex_lock(lock, TASK_UNINTERRUPTIBLE, 0, NULL, _RET_IP_); + __mutex_lock(lock, TASK_UNINTERRUPTIBLE, 0, NULL, ip); } static noinline int __sched -__mutex_lock_killable_slowpath(struct mutex *lock) +__mutex_lock_killable_slowpath(struct mutex *lock, unsigned long ip) { - return __mutex_lock(lock, TASK_KILLABLE, 0, NULL, _RET_IP_); + return __mutex_lock(lock, TASK_KILLABLE, 0, NULL, ip); } static noinline int __sched -__mutex_lock_interruptible_slowpath(struct mutex *lock) +__mutex_lock_interruptible_slowpath(struct mutex *lock, unsigned long ip) { - return __mutex_lock(lock, TASK_INTERRUPTIBLE, 0, NULL, _RET_IP_); + return __mutex_lock(lock, TASK_INTERRUPTIBLE, 0, NULL, ip); } static noinline int __sched -__ww_mutex_lock_slowpath(struct ww_mutex *lock, struct ww_acquire_ctx *ctx) +__ww_mutex_lock_slowpath(struct ww_mutex *lock, struct ww_acquire_ctx *ctx, + unsigned long ip) { - return __ww_mutex_lock(&lock->base, TASK_UNINTERRUPTIBLE, 0, - _RET_IP_, ctx); + return __ww_mutex_lock(&lock->base, TASK_UNINTERRUPTIBLE, 0, ip, ctx); } static noinline int __sched __ww_mutex_lock_interruptible_slowpath(struct ww_mutex *lock, - struct ww_acquire_ctx *ctx) + struct ww_acquire_ctx *ctx, + unsigned long ip) { - return __ww_mutex_lock(&lock->base, TASK_INTERRUPTIBLE, 0, - _RET_IP_, ctx); + return __ww_mutex_lock(&lock->base, TASK_INTERRUPTIBLE, 0, ip, ctx); } #endif @@ -1094,7 +1094,7 @@ ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ctx) return 0; } - return __ww_mutex_lock_slowpath(lock, ctx); + return __ww_mutex_lock_slowpath(lock, ctx, _RET_IP_); } EXPORT_SYMBOL(ww_mutex_lock); @@ -1109,7 +1109,7 @@ ww_mutex_lock_interruptible(struct ww_mutex *lock, struct ww_acquire_ctx *ctx) return 0; } - return __ww_mutex_lock_interruptible_slowpath(lock, ctx); + return __ww_mutex_lock_interruptible_slowpath(lock, ctx, _RET_IP_); } EXPORT_SYMBOL(ww_mutex_lock_interruptible);
The __mutex_lock_slowpath() and friends are declared as noinline and _RET_IP_ returns its caller as mutex_lock which is not meaningful. Pass the ip from mutex_lock() to have actual caller info in the trace. Signed-off-by: Namhyung Kim <namhyung@kernel.org> --- kernel/locking/mutex.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-)