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, nathan@kernel.org,
ndesaulniers@google.com, morbo@google.com,
justinstitt@google.com, linux-perf-users@vger.kernel.org,
bpf@vger.kernel.org, llvm@lists.linux.dev
Subject: Re: [PATCH v3 1/5] perf lock: Add bpf maps for owner stack tracing
Date: Wed, 29 Jan 2025 10:01:03 -0800 [thread overview]
Message-ID: <Z5ps3wb8lN_N5pCH@google.com> (raw)
In-Reply-To: <20250129001905.619859-2-ctshao@google.com>
On Tue, Jan 28, 2025 at 04:14:57PM -0800, Chun-Tse Shao wrote:
> Add a struct and few bpf maps in order to tracing owner stack.
> `struct owner_tracing_data`: Contains owner's pid, stack id, timestamp for
> when the owner acquires lock, and the count of lock waiters.
> `stack_buf`: Percpu buffer for retrieving owner stacktrace.
> `owner_stacks`: For tracing owner stacktrace to customized owner stack id.
> `owner_data`: For tracing lock_address to `struct owner_tracing_data` in
> bpf program.
> `owner_stat`: For reporting owner stacktrace in usermode.
>
> Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> ---
> tools/perf/util/bpf_lock_contention.c | 16 +++++--
> .../perf/util/bpf_skel/lock_contention.bpf.c | 42 ++++++++++++++++---
> tools/perf/util/bpf_skel/lock_data.h | 7 ++++
> 3 files changed, 57 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
> index fc8666222399..795e2374facc 100644
> --- a/tools/perf/util/bpf_lock_contention.c
> +++ b/tools/perf/util/bpf_lock_contention.c
> @@ -131,9 +131,19 @@ int lock_contention_prepare(struct lock_contention *con)
> else
> bpf_map__set_max_entries(skel->maps.task_data, 1);
>
> - if (con->save_callstack)
> - bpf_map__set_max_entries(skel->maps.stacks, con->map_nr_entries);
> - else
> + if (con->save_callstack) {
> + bpf_map__set_max_entries(skel->maps.stacks,
> + con->map_nr_entries);
It can be on the same line as it used to be.
> + if (con->owner) {
> + bpf_map__set_value_size(skel->maps.stack_buf, con->max_stack * sizeof(u64));
> + bpf_map__set_key_size(skel->maps.owner_stacks,
> + con->max_stack * sizeof(u64));
> + bpf_map__set_max_entries(skel->maps.owner_stacks, con->map_nr_entries);
> + bpf_map__set_max_entries(skel->maps.owner_data, con->map_nr_entries);
> + bpf_map__set_max_entries(skel->maps.owner_stat, con->map_nr_entries);
> + skel->rodata->max_stack = con->max_stack;
> + }
> + } else
Please add parenthesis even for single line when the matching if block
has parenthesis.
> bpf_map__set_max_entries(skel->maps.stacks, 1);
>
> if (target__has_cpu(target)) {
> diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> index 6533ea9b044c..b4961dd86222 100644
> --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c
> +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> @@ -19,13 +19,37 @@
> #define LCB_F_PERCPU (1U << 4)
> #define LCB_F_MUTEX (1U << 5)
>
> -/* callstack storage */
> + /* buffer for owner stacktrace */
> struct {
> - __uint(type, BPF_MAP_TYPE_STACK_TRACE);
> + __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
> __uint(key_size, sizeof(__u32));
> __uint(value_size, sizeof(__u64));
> - __uint(max_entries, MAX_ENTRIES);
> -} stacks SEC(".maps");
> + __uint(max_entries, 1);
> +} stack_buf SEC(".maps");
> +
> +/* a map for tracing owner stacktrace to owner stack id */
> +struct {
> + __uint(type, BPF_MAP_TYPE_HASH);
> + __uint(key_size, sizeof(__u64)); // owner stacktrace
> + __uint(value_size, sizeof(__u64)); // owner stack id
> + __uint(max_entries, 1);
> +} owner_stacks SEC(".maps");
> +
> +/* a map for tracing lock address to owner data */
> +struct {
> + __uint(type, BPF_MAP_TYPE_HASH);
> + __uint(key_size, sizeof(__u64)); // lock address
> + __uint(value_size, sizeof(struct owner_tracing_data));
> + __uint(max_entries, 1);
> +} owner_data SEC(".maps");
> +
> +/* a map for contention_key (stores owner stack id) to contention data */
> +struct {
> + __uint(type, BPF_MAP_TYPE_HASH);
> + __uint(key_size, sizeof(struct contention_key));
> + __uint(value_size, sizeof(struct contention_data));
> + __uint(max_entries, 1);
> +} owner_stat SEC(".maps");
>
> /* maintain timestamp at the beginning of contention */
> struct {
> @@ -43,6 +67,14 @@ struct {
> __uint(max_entries, 1);
> } tstamp_cpu SEC(".maps");
>
> +/* callstack storage */
> +struct {
> + __uint(type, BPF_MAP_TYPE_STACK_TRACE);
> + __uint(key_size, sizeof(__u32));
> + __uint(value_size, sizeof(__u64));
> + __uint(max_entries, MAX_ENTRIES);
> +} stacks SEC(".maps");
Can you please keep this on top of the maps so that it can minimize the
diff? I think it's better to move the new owner related stuff to
bottom.
> +
> /* actual lock contention statistics */
> struct {
> __uint(type, BPF_MAP_TYPE_HASH);
> @@ -143,6 +175,7 @@ const volatile int needs_callstack;
> const volatile int stack_skip;
> const volatile int lock_owner;
> const volatile int use_cgroup_v2;
> +const volatile int max_stack;
>
> /* determine the key of lock stat */
> const volatile int aggr_mode;
> @@ -466,7 +499,6 @@ int contention_end(u64 *ctx)
> return 0;
> need_delete = true;
> }
> -
Not needed.
Thanks,
Namhyung
> duration = bpf_ktime_get_ns() - pelem->timestamp;
> if ((__s64)duration < 0) {
> __sync_fetch_and_add(&time_fail, 1);
> diff --git a/tools/perf/util/bpf_skel/lock_data.h b/tools/perf/util/bpf_skel/lock_data.h
> index c15f734d7fc4..15f5743bd409 100644
> --- a/tools/perf/util/bpf_skel/lock_data.h
> +++ b/tools/perf/util/bpf_skel/lock_data.h
> @@ -3,6 +3,13 @@
> #ifndef UTIL_BPF_SKEL_LOCK_DATA_H
> #define UTIL_BPF_SKEL_LOCK_DATA_H
>
> +struct owner_tracing_data {
> + u32 pid; // Who has the lock.
> + u32 count; // How many waiters for this lock.
> + u64 timestamp; // The time while the owner acquires lock and contention is going on.
> + s32 stack_id; // Identifier for `owner_stat`, which stores as value in `owner_stacks`
> +};
> +
> struct tstamp_data {
> u64 timestamp;
> u64 lock;
> --
> 2.48.1.262.g85cc9f2d1e-goog
>
next prev parent reply other threads:[~2025-01-29 18:01 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-29 0:14 [PATCH v3 0/5] Tracing contention lock owner call stack Chun-Tse Shao
2025-01-29 0:14 ` [PATCH v3 1/5] perf lock: Add bpf maps for owner stack tracing Chun-Tse Shao
2025-01-29 18:01 ` Namhyung Kim [this message]
2025-01-29 0:14 ` [PATCH v3 2/5] perf lock: Retrieve owner callstack in bpf program Chun-Tse Shao
2025-01-29 18:48 ` Namhyung Kim
2025-01-29 0:14 ` [PATCH v3 3/5] perf lock: Make rb_tree helper functions generic Chun-Tse Shao
2025-01-29 0:15 ` [PATCH v3 4/5] perf lock: Report owner stack in usermode Chun-Tse Shao
2025-01-29 0:15 ` [PATCH v3 5/5] perf lock: Update documentation for -o option in contention mode Chun-Tse Shao
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=Z5ps3wb8lN_N5pCH@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=justinstitt@google.com \
--cc=kan.liang@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=llvm@lists.linux.dev \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=morbo@google.com \
--cc=nathan@kernel.org \
--cc=ndesaulniers@google.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;
as well as URLs for NNTP newsgroup(s).