[PATCH 0/4] perf lock contention: Small random fixes

[PATCH 0/4] perf lock contention: Small random fixes

[Namhyung Kim]

Hello,

This is a small update for the perf lock contention command.

The first issue is from msan (memory sanitizer) related to uninitialized
memory access and use of variable length array.

The other is to increase the default stack skip count to find appropriate
callers in most cases.

You can get it from 'perf/lock-con-fix-v1' branch in

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung


Namhyung Kim (4):
  perf lock contention: Fix memory sanitizer issue
  perf lock contention: Check --max-stack option
  perf lock contention: Avoid variable length arrays
  perf lock contention: Increase default stack skip to 4

 tools/perf/builtin-lock.c             | 30 +++++++++++++++++--
 tools/perf/util/bpf_lock_contention.c | 43 ++++++++++++++++++---------
 tools/perf/util/lock-contention.h     |  2 +-
 3 files changed, 57 insertions(+), 18 deletions(-)


base-commit: a3a365655a28f12f07eddf4f3fd596987b175e1d
-- 
2.38.1.273.g43a17bfeac-goog

[PATCH 1/4] perf lock contention: Fix memory sanitizer issue

[Namhyung Kim]

The msan reported a use-of-uninitialized-value warning for the struct
lock_contention_data in lock_contention_read().  While it'd be filled
by bpf_map_lookup_elem(), let's just initialize it to silence the
warning.

  ==12524==WARNING: MemorySanitizer: use-of-uninitialized-value
  #0 0x562b0f16b1cd in lock_contention_read  util/bpf_lock_contention.c:139:7
  #1 0x562b0ef65ec6 in __cmd_contention  builtin-lock.c:1737:3
  #2 0x562b0ef65ec6 in cmd_lock  builtin-lock.c:1992:8
  #3 0x562b0ee7f50b in run_builtin  perf.c:322:11
  #4 0x562b0ee7efc1 in handle_internal_command  perf.c:376:8
  #5 0x562b0ee7e1e9 in run_argv  perf.c:420:2
  #6 0x562b0ee7e1e9 in main  perf.c:550:3
  #7 0x7f065f10e632 in __libc_start_main (/usr/lib64/libc.so.6+0x61632)
  #8 0x562b0edf2fa9 in _start (perf+0xfa9)
  SUMMARY: MemorySanitizer: use-of-uninitialized-value (perf+0xe15160) in lock_contention_read

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/bpf_lock_contention.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
index fc4d613cb979..06466da792e4 100644
--- a/tools/perf/util/bpf_lock_contention.c
+++ b/tools/perf/util/bpf_lock_contention.c
@@ -110,7 +110,7 @@ int lock_contention_read(struct lock_contention *con)
 {
 	int fd, stack;
 	s32 prev_key, key;
-	struct lock_contention_data data;
+	struct lock_contention_data data = {};
 	struct lock_stat *st;
 	struct machine *machine = con->machine;
 	u64 stack_trace[con->max_stack];
-- 
2.38.1.273.g43a17bfeac-goog

[PATCH 2/4] perf lock contention: Check --max-stack option

[Namhyung Kim]

The --max-stack option is used to allocate the BPF stack map and stack
trace array in the userspace.  Check the value properly before using.
Practically it cannot be greater than the sysctl_perf_event_max_stack.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-lock.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 9722d4ab2e55..f67db60f1de6 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -24,6 +24,7 @@
 #include "util/data.h"
 #include "util/string2.h"
 #include "util/map.h"
+#include "util/util.h"
 
 #include <sys/types.h>
 #include <sys/prctl.h>
@@ -1858,6 +1859,29 @@ static int parse_map_entry(const struct option *opt, const char *str,
 	return 0;
 }
 
+static int parse_max_stack(const struct option *opt, const char *str,
+			   int unset __maybe_unused)
+{
+	unsigned long *len = (unsigned long *)opt->value;
+	long val;
+	char *endptr;
+
+	errno = 0;
+	val = strtol(str, &endptr, 0);
+	if (*endptr != '\0' || errno != 0) {
+		pr_err("invalid max stack depth: %s\n", str);
+		return -1;
+	}
+
+	if (val < 0 || val > sysctl__max_stack()) {
+		pr_err("invalid max stack depth: %ld\n", val);
+		return -1;
+	}
+
+	*len = val;
+	return 0;
+}
+
 int cmd_lock(int argc, const char **argv)
 {
 	const struct option lock_options[] = {
@@ -1913,9 +1937,9 @@ int cmd_lock(int argc, const char **argv)
 		   "Trace on existing thread id (exclusive to --pid)"),
 	OPT_CALLBACK(0, "map-nr-entries", &bpf_map_entries, "num",
 		     "Max number of BPF map entries", parse_map_entry),
-	OPT_INTEGER(0, "max-stack", &max_stack_depth,
-		    "Set the maximum stack depth when collecting lock contention, "
-		    "Default: " __stringify(CONTENTION_STACK_DEPTH)),
+	OPT_CALLBACK(0, "max-stack", &max_stack_depth, "num",
+		     "Set the maximum stack depth when collecting lopck contention, "
+		     "Default: " __stringify(CONTENTION_STACK_DEPTH), parse_max_stack),
 	OPT_INTEGER(0, "stack-skip", &stack_skip,
 		    "Set the number of stack depth to skip when finding a lock caller, "
 		    "Default: " __stringify(CONTENTION_STACK_SKIP)),
-- 
2.38.1.273.g43a17bfeac-goog

[PATCH 3/4] perf lock contention: Avoid variable length arrays

[Namhyung Kim]

The msan also warns about the use of VLA for stack_trace variable.
We can dynamically allocate instead.  While at it, simplify the error
handle a bit (and fix bugs).

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/bpf_lock_contention.c | 41 ++++++++++++++++++---------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
index 06466da792e4..0deec1178778 100644
--- a/tools/perf/util/bpf_lock_contention.c
+++ b/tools/perf/util/bpf_lock_contention.c
@@ -108,28 +108,36 @@ int lock_contention_stop(void)
 
 int lock_contention_read(struct lock_contention *con)
 {
-	int fd, stack;
+	int fd, stack, err = 0;
 	s32 prev_key, key;
 	struct lock_contention_data data = {};
-	struct lock_stat *st;
+	struct lock_stat *st = NULL;
 	struct machine *machine = con->machine;
-	u64 stack_trace[con->max_stack];
+	u64 *stack_trace;
+	size_t stack_size = con->max_stack * sizeof(*stack_trace);
 
 	fd = bpf_map__fd(skel->maps.lock_stat);
 	stack = bpf_map__fd(skel->maps.stacks);
 
 	con->lost = skel->bss->lost;
 
+	stack_trace = zalloc(stack_size);
+	if (stack_trace == NULL)
+		return -1;
+
 	prev_key = 0;
 	while (!bpf_map_get_next_key(fd, &prev_key, &key)) {
 		struct map *kmap;
 		struct symbol *sym;
 		int idx = 0;
 
+		/* to handle errors in the loop body */
+		err = -1;
+
 		bpf_map_lookup_elem(fd, &key, &data);
 		st = zalloc(sizeof(*st));
 		if (st == NULL)
-			return -1;
+			break;
 
 		st->nr_contended = data.count;
 		st->wait_time_total = data.total_time;
@@ -163,25 +171,32 @@ int lock_contention_read(struct lock_contention *con)
 				st->name = strdup(sym->name);
 
 			if (ret < 0 || st->name == NULL)
-				return -1;
+				break;
 		} else if (asprintf(&st->name, "%#lx", (unsigned long)st->addr) < 0) {
-			free(st);
-			return -1;
+			break;
 		}
 
 		if (verbose) {
-			st->callstack = memdup(stack_trace, sizeof(stack_trace));
-			if (st->callstack == NULL) {
-				free(st);
-				return -1;
-			}
+			st->callstack = memdup(stack_trace, stack_size);
+			if (st->callstack == NULL)
+				break;
 		}
 
 		hlist_add_head(&st->hash_entry, con->result);
 		prev_key = key;
+
+		/* we're fine now, reset the values */
+		st = NULL;
+		err = 0;
 	}
 
-	return 0;
+	free(stack_trace);
+	if (st) {
+		free(st->name);
+		free(st);
+	}
+
+	return err;
 }
 
 int lock_contention_finish(void)
-- 
2.38.1.273.g43a17bfeac-goog

[PATCH 4/4] perf lock contention: Increase default stack skip to 4

[Namhyung Kim]

In most configurations, it works well with skipping 4 entries by
default.  If some systems still have 3 BPF internal stack frames,
the next frame should be in a lock function which will be skipped
later when it tries to find a caller.  So increasing to 4 won't
affect such systems too.

With --stack-skip=0, I can see something like this:

    24     49.84 us      7.41 us      2.08 us        mutex   bpf_prog_e1b85959d520446c_contention_begin+0x12e
                    0xffffffffc045040e  bpf_prog_e1b85959d520446c_contention_begin+0x12e
                    0xffffffffc045040e  bpf_prog_e1b85959d520446c_contention_begin+0x12e
                    0xffffffff82ea2071  bpf_trace_run2+0x51
                    0xffffffff82de775b  __bpf_trace_contention_begin+0xb
                    0xffffffff82c02045  __mutex_lock+0x245
                    0xffffffff82c019e3  __mutex_lock_slowpath+0x13
                    0xffffffff82c019c0  mutex_lock+0x20
                    0xffffffff830a083c  kernfs_iop_permission+0x2c

Cc: Song Liu <song@kernel.org>
Cc: bpf@vger.kernel.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/lock-contention.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/lock-contention.h b/tools/perf/util/lock-contention.h
index b8cb8830b7bc..e3c061b1795b 100644
--- a/tools/perf/util/lock-contention.h
+++ b/tools/perf/util/lock-contention.h
@@ -91,7 +91,7 @@ struct thread_stat {
  * Number of stack trace entries to skip when finding callers.
  * The first few entries belong to the locking implementation itself.
  */
-#define CONTENTION_STACK_SKIP  3
+#define CONTENTION_STACK_SKIP  4
 
 /*
  * flags for lock:contention_begin
-- 
2.38.1.273.g43a17bfeac-goog

Re: [PATCH 0/4] perf lock contention: Small random fixes

[Arnaldo Carvalho de Melo]

Em Fri, Oct 28, 2022 at 11:01:24AM -0700, Namhyung Kim escreveu:
> Hello,
> 
> This is a small update for the perf lock contention command.
> 
> The first issue is from msan (memory sanitizer) related to uninitialized
> memory access and use of variable length array.
> 
> The other is to increase the default stack skip count to find appropriate
> callers in most cases.
> 
> You can get it from 'perf/lock-con-fix-v1' branch in
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks, applied.

- Arnaldo

 
> Thanks,
> Namhyung
> 
> 
> Namhyung Kim (4):
>   perf lock contention: Fix memory sanitizer issue
>   perf lock contention: Check --max-stack option
>   perf lock contention: Avoid variable length arrays
>   perf lock contention: Increase default stack skip to 4
> 
>  tools/perf/builtin-lock.c             | 30 +++++++++++++++++--
>  tools/perf/util/bpf_lock_contention.c | 43 ++++++++++++++++++---------
>  tools/perf/util/lock-contention.h     |  2 +-
>  3 files changed, 57 insertions(+), 18 deletions(-)
> 
> 
> base-commit: a3a365655a28f12f07eddf4f3fd596987b175e1d
> -- 
> 2.38.1.273.g43a17bfeac-goog

-- 

- Arnaldo