| 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
| ||
|
Message-ID: <Z4Xgen-HsfcvUuGN@google.com>
Date: Mon, 13 Jan 2025 19:56:42 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: Chun-Tse Shao <ctshao@...gle.com>
Cc: linux-kernel@...r.kernel.org, peterz@...radead.org, mingo@...hat.com,
acme@...nel.org, mark.rutland@....com,
alexander.shishkin@...ux.intel.com, jolsa@...nel.org,
irogers@...gle.com, adrian.hunter@...el.com,
kan.liang@...ux.intel.com, linux-perf-users@...r.kernel.org,
bpf@...r.kernel.org
Subject: Re: [PATCH v2 4/4] perf lock: Report owner stack in usermode
On Sun, Jan 12, 2025 at 09:20:17PM -0800, Chun-Tse Shao wrote:
> Parse `owner_lock_stat` into a rb tree, and report owner lock stats with
> stack trace in order.
>
> Example output:
> $ sudo ~/linux/tools/perf/perf lock con -abvo -Y mutex-spin -E3 perf bench sched pipe
> ...
> contended total wait max wait avg wait type caller
>
> 171 1.55 ms 20.26 us 9.06 us mutex pipe_read+0x57
> 0xffffffffac6318e7 pipe_read+0x57
> 0xffffffffac623862 vfs_read+0x332
> 0xffffffffac62434b ksys_read+0xbb
> 0xfffffffface604b2 do_syscall_64+0x82
> 0xffffffffad00012f entry_SYSCALL_64_after_hwframe+0x76
> 36 193.71 us 15.27 us 5.38 us mutex pipe_write+0x50
> 0xffffffffac631ee0 pipe_write+0x50
> 0xffffffffac6241db vfs_write+0x3bb
> 0xffffffffac6244ab ksys_write+0xbb
> 0xfffffffface604b2 do_syscall_64+0x82
> 0xffffffffad00012f entry_SYSCALL_64_after_hwframe+0x76
> 4 51.22 us 16.47 us 12.80 us mutex do_epoll_wait+0x24d
> 0xffffffffac691f0d do_epoll_wait+0x24d
> 0xffffffffac69249b do_epoll_pwait.part.0+0xb
> 0xffffffffac693ba5 __x64_sys_epoll_pwait+0x95
> 0xfffffffface604b2 do_syscall_64+0x82
> 0xffffffffad00012f entry_SYSCALL_64_after_hwframe+0x76
>
> === owner stack trace ===
>
> 3 31.24 us 15.27 us 10.41 us mutex pipe_read+0x348
> 0xffffffffac631bd8 pipe_read+0x348
> 0xffffffffac623862 vfs_read+0x332
> 0xffffffffac62434b ksys_read+0xbb
> 0xfffffffface604b2 do_syscall_64+0x82
> 0xffffffffad00012f entry_SYSCALL_64_after_hwframe+0x76
> ...
>
> Signed-off-by: Chun-Tse Shao <ctshao@...gle.com>
> ---
> tools/perf/builtin-lock.c | 20 ++++++++++++-
> tools/perf/util/bpf_lock_contention.c | 41 +++++++++++++++++++++++++++
> tools/perf/util/lock-contention.h | 2 ++
> 3 files changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index f9b7620444c0..0dfec175b25b 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -42,6 +42,7 @@
> #include <linux/zalloc.h>
> #include <linux/err.h>
> #include <linux/stringify.h>
> +#include <linux/rbtree.h>
>
> static struct perf_session *session;
> static struct target target;
> @@ -1926,6 +1927,23 @@ static void print_contention_result(struct lock_contention *con)
> break;
> }
>
> + if (con->owner && con->save_callstack) {
> + struct rb_root root = RB_ROOT;
> +
> +
A duplicate new line.
> + if (symbol_conf.field_sep)
> + fprintf(lock_output, "# owner stack trace:\n");
> + else
> + fprintf(lock_output, "\n=== owner stack trace ===\n\n");
> + while ((st = pop_owner_stack_trace(con)))
> + insert_to(&root, st, compare);
> +
> + while ((st = pop_from(&root))) {
> + print_lock_stat(con, st);
> + zfree(st);
> + }
> + }
> +
> if (print_nr_entries) {
> /* update the total/bad stats */
> while ((st = pop_from_result())) {
> @@ -2071,7 +2089,7 @@ static int check_lock_contention_options(const struct option *options,
> }
> }
>
> - if (show_lock_owner)
> + if (show_lock_owner && !verbose)
> show_thread_stats = true;
No, I don't think -v should control this. Let's change the semantic not
to imply --threads anymore. I think you can simply delete this code.
Users need to specify -t/--threads option for the old behavior. Hmm..
maybe we can show some warnings.
if (show_lock_owner && !show_thread_stats) {
pr_warning("Now -o try to show owner's callstack instead of pid and comm.\n");
pr_warning("Please use -t option too to keep the old behavior.\n");
}
Can you please update the documentation of the -o/--lock-owner option
too?
>
> return 0;
> diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
> index c9c58f243ceb..a63d5ffac386 100644
> --- a/tools/perf/util/bpf_lock_contention.c
> +++ b/tools/perf/util/bpf_lock_contention.c
> @@ -414,6 +414,47 @@ static const char *lock_contention_get_name(struct lock_contention *con,
> return name_buf;
> }
>
> +struct lock_stat *pop_owner_stack_trace(struct lock_contention *con)
> +{
> + int fd;
> + u64 *stack_trace;
> + struct contention_data data = {};
> + size_t stack_size = con->max_stack * sizeof(*stack_trace);
> + struct lock_stat *st;
> + char name[KSYM_NAME_LEN];
> +
> + fd = bpf_map__fd(skel->maps.owner_lock_stat);
> +
> + stack_trace = zalloc(stack_size);
> + if (stack_trace == NULL)
> + return NULL;
> +
> + if (bpf_map_get_next_key(fd, NULL, stack_trace))
> + return NULL;
> +
> + bpf_map_lookup_elem(fd, stack_trace, &data);
> + st = zalloc(sizeof(struct lock_stat));
You need to check the error and handle it properly.
> +
> + strcpy(name, stack_trace[0] ? lock_contention_get_name(con, NULL,
> + stack_trace, 0) :
> + "unknown");
> +
> + st->name = strdup(name);
Why do you copy and strdup()? Anyway please check the error.
> + st->flags = data.flags;
> + st->nr_contended = data.count;
> + st->wait_time_total = data.total_time;
> + st->wait_time_max = data.max_time;
> + st->wait_time_min = data.min_time;
> + st->callstack = memdup(stack_trace, stack_size);
Why not just give it to the 'st'?
> +
> + if (data.count)
> + st->avg_wait_time = data.total_time / data.count;
> +
> + bpf_map_delete_elem(fd, stack_trace);
> + free(stack_trace);
Then you don't need to free it here.
> +
> + return st;
> +}
> int lock_contention_read(struct lock_contention *con)
> {
> int fd, stack, err = 0;
> diff --git a/tools/perf/util/lock-contention.h b/tools/perf/util/lock-contention.h
> index 1a7248ff3889..83b400a36137 100644
> --- a/tools/perf/util/lock-contention.h
> +++ b/tools/perf/util/lock-contention.h
> @@ -156,6 +156,8 @@ int lock_contention_stop(void);
> int lock_contention_read(struct lock_contention *con);
> int lock_contention_finish(struct lock_contention *con);
>
> +struct lock_stat *pop_owner_stack_trace(struct lock_contention *con);
> +
> #else /* !HAVE_BPF_SKEL */
I think you need to define it here as well.
Thanks,
Namhyung
>
> static inline int lock_contention_prepare(struct lock_contention *con __maybe_unused)
> --
> 2.47.1.688.g23fc6f90ad-goog
>
Powered by blists - more mailing lists