From: Namhyung Kim <namhyung@kernel.org>
To: Chun-Tse Shao <ctshao@google.com>
Cc: linux-kernel@vger.kernel.org, peterz@infradead.org,
mingo@redhat.com, acme@kernel.org, mark.rutland@arm.com,
alexander.shishkin@linux.intel.com, jolsa@kernel.org,
irogers@google.com, adrian.hunter@intel.com,
kan.liang@linux.intel.com, nick.forrington@arm.com,
linux-perf-users@vger.kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH v6 4/4] perf lock: Report owner stack in usermode
Date: Sun, 23 Feb 2025 22:07:36 -0800 [thread overview]
Message-ID: <Z7wMqNUPbFoJA4RR@google.com> (raw)
In-Reply-To: <20250219214400.3317548-5-ctshao@google.com>
Hello,
On Wed, Feb 19, 2025 at 01:40:03PM -0800, Chun-Tse Shao wrote:
> This patch parses `owner_lock_stat` into a RB tree, enabling ordered
> reporting of owner lock statistics with stack traces. It also updates
> the documentation for the `-o` option in contention mode, decouples `-o`
> from `-t`, and issues a warning to inform users about the new behavior
> of `-ov`.
>
> 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@google.com>
> ---
> tools/perf/Documentation/perf-lock.txt | 6 +--
> tools/perf/builtin-lock.c | 22 +++++++++-
> tools/perf/util/bpf_lock_contention.c | 58 ++++++++++++++++++++++++++
> tools/perf/util/lock-contention.h | 7 ++++
> 4 files changed, 88 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
> index d3793054f7d3..255e4f3e9d2b 100644
> --- a/tools/perf/Documentation/perf-lock.txt
> +++ b/tools/perf/Documentation/perf-lock.txt
> @@ -140,7 +140,6 @@ CONTENTION OPTIONS
> --use-bpf::
> Use BPF program to collect lock contention stats instead of
> using the input data.
> -
> -a::
> --all-cpus::
> System-wide collection from all CPUs.
> @@ -179,8 +178,9 @@ CONTENTION OPTIONS
>
> -o::
> --lock-owner::
> - Show lock contention stat by owners. Implies --threads and
> - requires --use-bpf.
> + Show lock contention stat by owners. This option can be combined with -t,
> + which shows owner's per thread lock stats, or -v, which shows owner's
> + stacktrace. Requires --use-bpf.
>
> -Y::
> --type-filter=<value>::
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index 9bebc186286f..34cffa3c7cad 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -1817,6 +1817,22 @@ static void print_contention_result(struct lock_contention *con)
> break;
> }
>
> + if (con->owner && con->save_callstack && verbose > 0) {
> + struct rb_root root = RB_ROOT;
> +
> + 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);
I think it should be 'zfree(&st)' or 'free(st)'.
> + }
> + }
> +
> if (print_nr_entries) {
> /* update the total/bad stats */
> while ((st = pop_from_result())) {
> @@ -1962,8 +1978,10 @@ static int check_lock_contention_options(const struct option *options,
> }
> }
>
> - if (show_lock_owner)
> - show_thread_stats = true;
> + 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");
> + }
>
> return 0;
> }
> diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
> index 76542b86e83f..226ec7a06ab1 100644
> --- a/tools/perf/util/bpf_lock_contention.c
> +++ b/tools/perf/util/bpf_lock_contention.c
> @@ -549,6 +549,64 @@ 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 stacks_fd, stat_fd;
> + u64 *stack_trace = NULL;
> + s32 stack_id;
> + struct contention_key ckey = {};
> + struct contention_data cdata = {};
> + size_t stack_size = con->max_stack * sizeof(*stack_trace);
> + struct lock_stat *st = NULL;
> +
> + stacks_fd = bpf_map__fd(skel->maps.owner_stacks);
> + stat_fd = bpf_map__fd(skel->maps.owner_stat);
> + if (!stacks_fd || !stat_fd)
> + goto out_err;
> +
> + stack_trace = zalloc(stack_size);
> + if (stack_trace == NULL)
> + goto out_err;
> +
> + if (bpf_map_get_next_key(stacks_fd, NULL, stack_trace))
> + goto out_err;
> +
> + bpf_map_lookup_elem(stacks_fd, stack_trace, &stack_id);
> + ckey.stack_id = stack_id;
> + bpf_map_lookup_elem(stat_fd, &ckey, &cdata);
> +
> + st = zalloc(sizeof(struct lock_stat));
> + if (!st)
> + goto out_err;
> +
> + st->name = strdup(stack_trace[0] ? lock_contention_get_name(con, NULL, stack_trace, 0) :
> + "unknown");
> + if (!st->name)
> + goto out_err;
> +
> + st->flags = cdata.flags;
> + st->nr_contended = cdata.count;
> + st->wait_time_total = cdata.total_time;
> + st->wait_time_max = cdata.max_time;
> + st->wait_time_min = cdata.min_time;
> + st->callstack = stack_trace;
> +
> + if (cdata.count)
> + st->avg_wait_time = cdata.total_time / cdata.count;
> +
> + bpf_map_delete_elem(stacks_fd, stack_trace);
> + bpf_map_delete_elem(stat_fd, &ckey);
> +
> + return st;
> +
> +out_err:
> + if (stack_trace)
> + free(stack_trace);
> + if (st)
> + free(st);
No need to check NULL before calling free().
Thanks,
Namhyung
> + return NULL;
> +}
> +
> 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 a09f7fe877df..97fd33c57f17 100644
> --- a/tools/perf/util/lock-contention.h
> +++ b/tools/perf/util/lock-contention.h
> @@ -168,6 +168,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 */
>
> static inline int lock_contention_prepare(struct lock_contention *con __maybe_unused)
> @@ -187,6 +189,11 @@ static inline int lock_contention_read(struct lock_contention *con __maybe_unuse
> return 0;
> }
>
> +struct lock_stat *pop_owner_stack_trace(struct lock_contention *con)
> +{
> + return NULL;
> +}
> +
> #endif /* HAVE_BPF_SKEL */
>
> #endif /* PERF_LOCK_CONTENTION_H */
> --
> 2.48.1.601.g30ceb7b040-goog
>
prev parent reply other threads:[~2025-02-24 6:07 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-02-19 21:39 [PATCH v6 0/4] Tracing contention lock owner call stack Chun-Tse Shao
2025-02-19 21:40 ` [PATCH v6 1/4] perf lock: Add bpf maps for owner stack tracing Chun-Tse Shao
2025-02-19 21:40 ` [PATCH v6 2/4] perf lock: Retrieve owner callstack in bpf program Chun-Tse Shao
2025-02-21 6:46 ` Namhyung Kim
2025-02-24 18:25 ` Chun-Tse Shao
2025-02-19 21:40 ` [PATCH v6 3/4] perf lock: Make rb_tree helper functions generic Chun-Tse Shao
2025-02-19 21:40 ` [PATCH v6 4/4] perf lock: Report owner stack in usermode Chun-Tse Shao
2025-02-24 6:07 ` Namhyung Kim [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Z7wMqNUPbFoJA4RR@google.com \
--to=namhyung@kernel.org \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=bpf@vger.kernel.org \
--cc=ctshao@google.com \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=nick.forrington@arm.com \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox