| 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: <Z5ps3wb8lN_N5pCH@google.com>
Date: Wed, 29 Jan 2025 10:01:03 -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, nathan@...nel.org,
ndesaulniers@...gle.com, morbo@...gle.com, justinstitt@...gle.com,
linux-perf-users@...r.kernel.org, bpf@...r.kernel.org,
llvm@...ts.linux.dev
Subject: Re: [PATCH v3 1/5] perf lock: Add bpf maps for owner stack tracing
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@...gle.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
>
Powered by blists - more mailing lists