Re: [PATCH v2 4/4] perf lock: Report owner stack in usermode

[Namhyung Kim <namhyung@kernel.org>]

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@google.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
>