[PATCH 1/3] perf lock: Introduce struct lock_contention

[PATCH 1/3] perf lock: Introduce struct lock_contention

[Namhyung Kim]

The lock_contention struct is to carry related fields together and to
minimize the change when we add new config options.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-lock.c             | 23 ++++++++++++++---------
 tools/perf/util/bpf_lock_contention.c |  9 ++++++---
 tools/perf/util/lock-contention.h     | 17 +++++++++++------
 3 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 7897a33fec1b..eef778b7d33d 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -1594,7 +1594,10 @@ static int __cmd_contention(int argc, const char **argv)
 		.mode  = PERF_DATA_MODE_READ,
 		.force = force,
 	};
-	struct evlist *evlist = NULL;
+	struct lock_contention con = {
+		.target = &target,
+		.result = &lockhash_table[0],
+	};
 
 	session = perf_session__new(use_bpf ? NULL : &data, &eops);
 	if (IS_ERR(session)) {
@@ -1620,24 +1623,26 @@ static int __cmd_contention(int argc, const char **argv)
 		signal(SIGCHLD, sighandler);
 		signal(SIGTERM, sighandler);
 
-		evlist = evlist__new();
-		if (evlist == NULL) {
+		con.machine = &session->machines.host;
+
+		con.evlist = evlist__new();
+		if (con.evlist == NULL) {
 			err = -ENOMEM;
 			goto out_delete;
 		}
 
-		err = evlist__create_maps(evlist, &target);
+		err = evlist__create_maps(con.evlist, &target);
 		if (err < 0)
 			goto out_delete;
 
 		if (argc) {
-			err = evlist__prepare_workload(evlist, &target,
+			err = evlist__prepare_workload(con.evlist, &target,
 						       argv, false, NULL);
 			if (err < 0)
 				goto out_delete;
 		}
 
-		if (lock_contention_prepare(evlist, &target) < 0) {
+		if (lock_contention_prepare(&con) < 0) {
 			pr_err("lock contention BPF setup failed\n");
 			goto out_delete;
 		}
@@ -1672,13 +1677,13 @@ static int __cmd_contention(int argc, const char **argv)
 	if (use_bpf) {
 		lock_contention_start();
 		if (argc)
-			evlist__start_workload(evlist);
+			evlist__start_workload(con.evlist);
 
 		/* wait for signal */
 		pause();
 
 		lock_contention_stop();
-		lock_contention_read(&session->machines.host, &lockhash_table[0]);
+		lock_contention_read(&con);
 	} else {
 		err = perf_session__process_events(session);
 		if (err)
@@ -1691,7 +1696,7 @@ static int __cmd_contention(int argc, const char **argv)
 	print_contention_result();
 
 out_delete:
-	evlist__delete(evlist);
+	evlist__delete(con.evlist);
 	lock_contention_finish();
 	perf_session__delete(session);
 	return err;
diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
index 16b7451b4b09..f5e2b4f19a72 100644
--- a/tools/perf/util/bpf_lock_contention.c
+++ b/tools/perf/util/bpf_lock_contention.c
@@ -27,10 +27,12 @@ struct lock_contention_data {
 	u32 flags;
 };
 
-int lock_contention_prepare(struct evlist *evlist, struct target *target)
+int lock_contention_prepare(struct lock_contention *con)
 {
 	int i, fd;
 	int ncpus = 1, ntasks = 1;
+	struct evlist *evlist = con->evlist;
+	struct target *target = con->target;
 
 	skel = lock_contention_bpf__open();
 	if (!skel) {
@@ -102,12 +104,13 @@ int lock_contention_stop(void)
 	return 0;
 }
 
-int lock_contention_read(struct machine *machine, struct hlist_head *head)
+int lock_contention_read(struct lock_contention *con)
 {
 	int fd, stack;
 	u32 prev_key, key;
 	struct lock_contention_data data;
 	struct lock_stat *st;
+	struct machine *machine = con->machine;
 	u64 stack_trace[CONTENTION_STACK_DEPTH];
 
 	fd = bpf_map__fd(skel->maps.lock_stat);
@@ -163,7 +166,7 @@ int lock_contention_read(struct machine *machine, struct hlist_head *head)
 			return -1;
 		}
 
-		hlist_add_head(&st->hash_entry, head);
+		hlist_add_head(&st->hash_entry, con->result);
 		prev_key = key;
 	}
 
diff --git a/tools/perf/util/lock-contention.h b/tools/perf/util/lock-contention.h
index 092c84441f9f..a0df5308cca4 100644
--- a/tools/perf/util/lock-contention.h
+++ b/tools/perf/util/lock-contention.h
@@ -107,18 +107,24 @@ struct evlist;
 struct machine;
 struct target;
 
+struct lock_contention {
+	struct evlist *evlist;
+	struct target *target;
+	struct machine *machine;
+	struct hlist_head *result;
+};
+
 #ifdef HAVE_BPF_SKEL
 
-int lock_contention_prepare(struct evlist *evlist, struct target *target);
+int lock_contention_prepare(struct lock_contention *con);
 int lock_contention_start(void);
 int lock_contention_stop(void);
-int lock_contention_read(struct machine *machine, struct hlist_head *head);
+int lock_contention_read(struct lock_contention *con);
 int lock_contention_finish(void);
 
 #else  /* !HAVE_BPF_SKEL */
 
-static inline int lock_contention_prepare(struct evlist *evlist __maybe_unused,
-					  struct target *target __maybe_unused)
+static inline int lock_contention_prepare(struct lock_contention *con __maybe_unused)
 {
 	return 0;
 }
@@ -127,8 +133,7 @@ static inline int lock_contention_start(void) { return 0; }
 static inline int lock_contention_stop(void) { return 0; }
 static inline int lock_contention_finish(void) { return 0; }
 
-static inline int lock_contention_read(struct machine *machine __maybe_unused,
-				       struct hlist_head *head __maybe_unused)
+static inline int lock_contention_read(struct lock_contention *con __maybe_unused)
 {
 	return 0;
 }
-- 
2.37.1.455.g008518b4e5-goog

[PATCH 2/3] perf lock: Add -m/--map-length option

[Namhyung Kim]

The -m/--map-length option is to control number of max entries in the
perf lock contention BPF maps.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-lock.txt |  4 ++++
 tools/perf/builtin-lock.c              | 23 ++++++++++++++++++++++-
 tools/perf/util/bpf_lock_contention.c  |  3 +++
 tools/perf/util/lock-contention.h      |  1 +
 4 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
index 7949d2e6891b..2101644785e0 100644
--- a/tools/perf/Documentation/perf-lock.txt
+++ b/tools/perf/Documentation/perf-lock.txt
@@ -145,6 +145,10 @@ CONTENTION OPTIONS
 --tid=::
         Record events on existing thread ID (comma separated list).
 
+-m::
+--map-length::
+	Maximum number of BPF map entries (default: 10240).
+
 
 SEE ALSO
 --------
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index eef778b7d33d..e32fdcd497e0 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -55,6 +55,7 @@ static struct rb_root		thread_stats;
 static bool combine_locks;
 static bool show_thread_stats;
 static bool use_bpf;
+static unsigned long bpf_map_len = 10240;
 
 static enum {
 	LOCK_AGGR_ADDR,
@@ -1597,6 +1598,7 @@ static int __cmd_contention(int argc, const char **argv)
 	struct lock_contention con = {
 		.target = &target,
 		.result = &lockhash_table[0],
+		.map_len = bpf_map_len,
 	};
 
 	session = perf_session__new(use_bpf ? NULL : &data, &eops);
@@ -1786,6 +1788,24 @@ static int __cmd_record(int argc, const char **argv)
 	return ret;
 }
 
+static int parse_map_length(const struct option *opt, const char *str,
+			    int unset __maybe_unused)
+{
+	unsigned long *len = (unsigned long *)opt->value;
+	unsigned long val;
+	char *endptr;
+
+	errno = 0;
+	val = strtoul(str, &endptr, 0);
+	if (*endptr != '\0' || errno != 0) {
+		pr_err("invalid BPF map length: %s\n", str);
+		return -1;
+	}
+
+	*len = val;
+	return 0;
+}
+
 int cmd_lock(int argc, const char **argv)
 {
 	const struct option lock_options[] = {
@@ -1835,9 +1855,10 @@ int cmd_lock(int argc, const char **argv)
 		    "List of cpus to monitor"),
 	OPT_STRING('p', "pid", &target.pid, "pid",
 		   "Trace on existing process id"),
-	/* TODO: Add short option -t after -t/--tracer can be removed. */
 	OPT_STRING(0, "tid", &target.tid, "tid",
 		   "Trace on existing thread id (exclusive to --pid)"),
+	OPT_CALLBACK('m', "map-length", &bpf_map_len, "len",
+		     "Max number of BPF map entries", parse_map_length),
 	OPT_PARENT(lock_options)
 	};
 
diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
index f5e2b4f19a72..26128e5bb659 100644
--- a/tools/perf/util/bpf_lock_contention.c
+++ b/tools/perf/util/bpf_lock_contention.c
@@ -40,6 +40,9 @@ int lock_contention_prepare(struct lock_contention *con)
 		return -1;
 	}
 
+	bpf_map__set_max_entries(skel->maps.stacks, con->map_len);
+	bpf_map__set_max_entries(skel->maps.lock_stat, con->map_len);
+
 	if (target__has_cpu(target))
 		ncpus = perf_cpu_map__nr(evlist->core.user_requested_cpus);
 	if (target__has_task(target))
diff --git a/tools/perf/util/lock-contention.h b/tools/perf/util/lock-contention.h
index a0df5308cca4..b09fd6eb978a 100644
--- a/tools/perf/util/lock-contention.h
+++ b/tools/perf/util/lock-contention.h
@@ -112,6 +112,7 @@ struct lock_contention {
 	struct target *target;
 	struct machine *machine;
 	struct hlist_head *result;
+	unsigned long map_len;
 };
 
 #ifdef HAVE_BPF_SKEL
-- 
2.37.1.455.g008518b4e5-goog

[PATCH 3/3] perf lock: Print lost entries at the end

[Namhyung Kim]

Like the normal perf lock report output, it'd print bad stats at the end
if exists or -v option is passed.  Currently it uses BROKEN_CONTENDED
stat for the lost count (due to full stack maps).

  $ sudo perf lock con -a -b -m 128 sleep 5
  ...
  === output for debug===

  bad: 43, total: 14903
  bad rate: 0.29 %
  histogram of events caused bad sequence
      acquire: 0
     acquired: 0
    contended: 43
      release: 0

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-lock.c                      | 8 +++++++-
 tools/perf/util/bpf_lock_contention.c          | 6 ++++--
 tools/perf/util/bpf_skel/lock_contention.bpf.c | 9 +++++++--
 tools/perf/util/lock-contention.h              | 1 +
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index e32fdcd497e0..8065f0268e55 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -1471,8 +1471,11 @@ static void print_contention_result(void)
 		pr_info("  %10s   %s\n\n", "type", "caller");
 
 	bad = total = 0;
+	if (use_bpf)
+		bad = bad_hist[BROKEN_CONTENDED];
+
 	while ((st = pop_from_result())) {
-		total++;
+		total += use_bpf ? st->nr_contended : 1;
 		if (st->broken)
 			bad++;
 
@@ -1686,6 +1689,9 @@ static int __cmd_contention(int argc, const char **argv)
 
 		lock_contention_stop();
 		lock_contention_read(&con);
+
+		/* abuse bad hist stats for lost entries */
+		bad_hist[BROKEN_CONTENDED] = con.lost;
 	} else {
 		err = perf_session__process_events(session);
 		if (err)
diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
index 26128e5bb659..65f51cc25236 100644
--- a/tools/perf/util/bpf_lock_contention.c
+++ b/tools/perf/util/bpf_lock_contention.c
@@ -16,7 +16,7 @@ static struct lock_contention_bpf *skel;
 
 /* should be same as bpf_skel/lock_contention.bpf.c */
 struct lock_contention_key {
-	u32 stack_id;
+	s32 stack_id;
 };
 
 struct lock_contention_data {
@@ -110,7 +110,7 @@ int lock_contention_stop(void)
 int lock_contention_read(struct lock_contention *con)
 {
 	int fd, stack;
-	u32 prev_key, key;
+	s32 prev_key, key;
 	struct lock_contention_data data;
 	struct lock_stat *st;
 	struct machine *machine = con->machine;
@@ -119,6 +119,8 @@ int lock_contention_read(struct lock_contention *con)
 	fd = bpf_map__fd(skel->maps.lock_stat);
 	stack = bpf_map__fd(skel->maps.stacks);
 
+	con->lost = skel->bss->lost;
+
 	prev_key = 0;
 	while (!bpf_map_get_next_key(fd, &prev_key, &key)) {
 		struct map *kmap;
diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c
index 67d46533e518..9e8b94eb6320 100644
--- a/tools/perf/util/bpf_skel/lock_contention.bpf.c
+++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c
@@ -12,7 +12,7 @@
 #define MAX_ENTRIES  10240
 
 struct contention_key {
-	__u32 stack_id;
+	__s32 stack_id;
 };
 
 struct contention_data {
@@ -27,7 +27,7 @@ struct tstamp_data {
 	__u64 timestamp;
 	__u64 lock;
 	__u32 flags;
-	__u32 stack_id;
+	__s32 stack_id;
 };
 
 /* callstack storage  */
@@ -73,6 +73,9 @@ int enabled;
 int has_cpu;
 int has_task;
 
+/* error stat */
+unsigned long lost;
+
 static inline int can_record(void)
 {
 	if (has_cpu) {
@@ -116,6 +119,8 @@ int contention_begin(u64 *ctx)
 	pelem->flags = (__u32)ctx[1];
 	pelem->stack_id = bpf_get_stackid(ctx, &stacks, BPF_F_FAST_STACK_CMP);
 
+	if (pelem->stack_id < 0)
+		lost++;
 	return 0;
 }
 
diff --git a/tools/perf/util/lock-contention.h b/tools/perf/util/lock-contention.h
index b09fd6eb978a..d9fc5f076567 100644
--- a/tools/perf/util/lock-contention.h
+++ b/tools/perf/util/lock-contention.h
@@ -113,6 +113,7 @@ struct lock_contention {
 	struct machine *machine;
 	struct hlist_head *result;
 	unsigned long map_len;
+	unsigned long lost;
 };
 
 #ifdef HAVE_BPF_SKEL
-- 
2.37.1.455.g008518b4e5-goog

Re: [PATCH 2/3] perf lock: Add -m/--map-length option

[Arnaldo Carvalho de Melo]

Em Tue, Aug 02, 2022 at 12:35:10AM -0700, Namhyung Kim escreveu:
> The -m/--map-length option is to control number of max entries in the
> perf lock contention BPF maps.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/Documentation/perf-lock.txt |  4 ++++
>  tools/perf/builtin-lock.c              | 23 ++++++++++++++++++++++-
>  tools/perf/util/bpf_lock_contention.c  |  3 +++
>  tools/perf/util/lock-contention.h      |  1 +
>  4 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
> index 7949d2e6891b..2101644785e0 100644
> --- a/tools/perf/Documentation/perf-lock.txt
> +++ b/tools/perf/Documentation/perf-lock.txt
> @@ -145,6 +145,10 @@ CONTENTION OPTIONS
>  --tid=::
>          Record events on existing thread ID (comma separated list).
>  
> +-m::
> +--map-length::
> +	Maximum number of BPF map entries (default: 10240).

--map-nr-entries?

I think we use this jargon "nr-entries" for arrays, lists, etc, better
try to stick to it.

Also what do you think about not using single letter options for things
that are not that used?

The map size has a default, one that seems generous, so changing it
should be something uncommon, and then, if it becomes common that more
entries are needed by default, we can change the default in the tool.

- Arnaldo

> +
>  
>  SEE ALSO
>  --------
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index eef778b7d33d..e32fdcd497e0 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -55,6 +55,7 @@ static struct rb_root		thread_stats;
>  static bool combine_locks;
>  static bool show_thread_stats;
>  static bool use_bpf;
> +static unsigned long bpf_map_len = 10240;
>  
>  static enum {
>  	LOCK_AGGR_ADDR,
> @@ -1597,6 +1598,7 @@ static int __cmd_contention(int argc, const char **argv)
>  	struct lock_contention con = {
>  		.target = &target,
>  		.result = &lockhash_table[0],
> +		.map_len = bpf_map_len,
>  	};
>  
>  	session = perf_session__new(use_bpf ? NULL : &data, &eops);
> @@ -1786,6 +1788,24 @@ static int __cmd_record(int argc, const char **argv)
>  	return ret;
>  }
>  
> +static int parse_map_length(const struct option *opt, const char *str,
> +			    int unset __maybe_unused)
> +{
> +	unsigned long *len = (unsigned long *)opt->value;
> +	unsigned long val;
> +	char *endptr;
> +
> +	errno = 0;
> +	val = strtoul(str, &endptr, 0);
> +	if (*endptr != '\0' || errno != 0) {
> +		pr_err("invalid BPF map length: %s\n", str);
> +		return -1;
> +	}
> +
> +	*len = val;
> +	return 0;
> +}
> +
>  int cmd_lock(int argc, const char **argv)
>  {
>  	const struct option lock_options[] = {
> @@ -1835,9 +1855,10 @@ int cmd_lock(int argc, const char **argv)
>  		    "List of cpus to monitor"),
>  	OPT_STRING('p', "pid", &target.pid, "pid",
>  		   "Trace on existing process id"),
> -	/* TODO: Add short option -t after -t/--tracer can be removed. */
>  	OPT_STRING(0, "tid", &target.tid, "tid",
>  		   "Trace on existing thread id (exclusive to --pid)"),
> +	OPT_CALLBACK('m', "map-length", &bpf_map_len, "len",
> +		     "Max number of BPF map entries", parse_map_length),
>  	OPT_PARENT(lock_options)
>  	};
>  
> diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
> index f5e2b4f19a72..26128e5bb659 100644
> --- a/tools/perf/util/bpf_lock_contention.c
> +++ b/tools/perf/util/bpf_lock_contention.c
> @@ -40,6 +40,9 @@ int lock_contention_prepare(struct lock_contention *con)
>  		return -1;
>  	}
>  
> +	bpf_map__set_max_entries(skel->maps.stacks, con->map_len);
> +	bpf_map__set_max_entries(skel->maps.lock_stat, con->map_len);
> +
>  	if (target__has_cpu(target))
>  		ncpus = perf_cpu_map__nr(evlist->core.user_requested_cpus);
>  	if (target__has_task(target))
> diff --git a/tools/perf/util/lock-contention.h b/tools/perf/util/lock-contention.h
> index a0df5308cca4..b09fd6eb978a 100644
> --- a/tools/perf/util/lock-contention.h
> +++ b/tools/perf/util/lock-contention.h
> @@ -112,6 +112,7 @@ struct lock_contention {
>  	struct target *target;
>  	struct machine *machine;
>  	struct hlist_head *result;
> +	unsigned long map_len;
>  };
>  
>  #ifdef HAVE_BPF_SKEL
> -- 
> 2.37.1.455.g008518b4e5-goog

-- 

- Arnaldo

Re: [PATCH 3/3] perf lock: Print lost entries at the end

[Arnaldo Carvalho de Melo]

Em Tue, Aug 02, 2022 at 12:35:11AM -0700, Namhyung Kim escreveu:
> Like the normal perf lock report output, it'd print bad stats at the end
> if exists or -v option is passed.  Currently it uses BROKEN_CONTENDED
 stat for the lost count (due to full stack maps).

"Print the number of lost entries in verbose mode"?
 
>   $ sudo perf lock con -a -b -m 128 sleep 5
>   ...
>   === output for debug===
> 
>   bad: 43, total: 14903
>   bad rate: 0.29 %
>   histogram of events caused bad sequence
>       acquire: 0
>      acquired: 0
>     contended: 43
>       release: 0
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-lock.c                      | 8 +++++++-
>  tools/perf/util/bpf_lock_contention.c          | 6 ++++--
>  tools/perf/util/bpf_skel/lock_contention.bpf.c | 9 +++++++--
>  tools/perf/util/lock-contention.h              | 1 +
>  4 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index e32fdcd497e0..8065f0268e55 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -1471,8 +1471,11 @@ static void print_contention_result(void)
>  		pr_info("  %10s   %s\n\n", "type", "caller");
>  
>  	bad = total = 0;
> +	if (use_bpf)
> +		bad = bad_hist[BROKEN_CONTENDED];
> +
>  	while ((st = pop_from_result())) {
> -		total++;
> +		total += use_bpf ? st->nr_contended : 1;
>  		if (st->broken)
>  			bad++;
>  
> @@ -1686,6 +1689,9 @@ static int __cmd_contention(int argc, const char **argv)
>  
>  		lock_contention_stop();
>  		lock_contention_read(&con);
> +
> +		/* abuse bad hist stats for lost entries */
> +		bad_hist[BROKEN_CONTENDED] = con.lost;
>  	} else {
>  		err = perf_session__process_events(session);
>  		if (err)
> diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
> index 26128e5bb659..65f51cc25236 100644
> --- a/tools/perf/util/bpf_lock_contention.c
> +++ b/tools/perf/util/bpf_lock_contention.c
> @@ -16,7 +16,7 @@ static struct lock_contention_bpf *skel;
>  
>  /* should be same as bpf_skel/lock_contention.bpf.c */
>  struct lock_contention_key {
> -	u32 stack_id;
> +	s32 stack_id;
>  };
>  
>  struct lock_contention_data {
> @@ -110,7 +110,7 @@ int lock_contention_stop(void)
>  int lock_contention_read(struct lock_contention *con)
>  {
>  	int fd, stack;
> -	u32 prev_key, key;
> +	s32 prev_key, key;
>  	struct lock_contention_data data;
>  	struct lock_stat *st;
>  	struct machine *machine = con->machine;
> @@ -119,6 +119,8 @@ int lock_contention_read(struct lock_contention *con)
>  	fd = bpf_map__fd(skel->maps.lock_stat);
>  	stack = bpf_map__fd(skel->maps.stacks);
>  
> +	con->lost = skel->bss->lost;
> +
>  	prev_key = 0;
>  	while (!bpf_map_get_next_key(fd, &prev_key, &key)) {
>  		struct map *kmap;
> diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> index 67d46533e518..9e8b94eb6320 100644
> --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c
> +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> @@ -12,7 +12,7 @@
>  #define MAX_ENTRIES  10240
>  
>  struct contention_key {
> -	__u32 stack_id;
> +	__s32 stack_id;
>  };
>  
>  struct contention_data {
> @@ -27,7 +27,7 @@ struct tstamp_data {
>  	__u64 timestamp;
>  	__u64 lock;
>  	__u32 flags;
> -	__u32 stack_id;
> +	__s32 stack_id;
>  };
>  
>  /* callstack storage  */
> @@ -73,6 +73,9 @@ int enabled;
>  int has_cpu;
>  int has_task;
>  
> +/* error stat */
> +unsigned long lost;
> +
>  static inline int can_record(void)
>  {
>  	if (has_cpu) {
> @@ -116,6 +119,8 @@ int contention_begin(u64 *ctx)
>  	pelem->flags = (__u32)ctx[1];
>  	pelem->stack_id = bpf_get_stackid(ctx, &stacks, BPF_F_FAST_STACK_CMP);
>  
> +	if (pelem->stack_id < 0)
> +		lost++;
>  	return 0;
>  }
>  
> diff --git a/tools/perf/util/lock-contention.h b/tools/perf/util/lock-contention.h
> index b09fd6eb978a..d9fc5f076567 100644
> --- a/tools/perf/util/lock-contention.h
> +++ b/tools/perf/util/lock-contention.h
> @@ -113,6 +113,7 @@ struct lock_contention {
>  	struct machine *machine;
>  	struct hlist_head *result;
>  	unsigned long map_len;
> +	unsigned long lost;
>  };
>  
>  #ifdef HAVE_BPF_SKEL
> -- 
> 2.37.1.455.g008518b4e5-goog

-- 

- Arnaldo

Re: [PATCH 2/3] perf lock: Add -m/--map-length option

[Namhyung Kim]

On Tue, Aug 2, 2022 at 10:43 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Tue, Aug 02, 2022 at 12:35:10AM -0700, Namhyung Kim escreveu:
> > The -m/--map-length option is to control number of max entries in the
> > perf lock contention BPF maps.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/Documentation/perf-lock.txt |  4 ++++
> >  tools/perf/builtin-lock.c              | 23 ++++++++++++++++++++++-
> >  tools/perf/util/bpf_lock_contention.c  |  3 +++
> >  tools/perf/util/lock-contention.h      |  1 +
> >  4 files changed, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
> > index 7949d2e6891b..2101644785e0 100644
> > --- a/tools/perf/Documentation/perf-lock.txt
> > +++ b/tools/perf/Documentation/perf-lock.txt
> > @@ -145,6 +145,10 @@ CONTENTION OPTIONS
> >  --tid=::
> >          Record events on existing thread ID (comma separated list).
> >
> > +-m::
> > +--map-length::
> > +     Maximum number of BPF map entries (default: 10240).
>
> --map-nr-entries?
>
> I think we use this jargon "nr-entries" for arrays, lists, etc, better
> try to stick to it.

Makes sense, will change.

>
> Also what do you think about not using single letter options for things
> that are not that used?
>
> The map size has a default, one that seems generous, so changing it
> should be something uncommon, and then, if it becomes common that more
> entries are needed by default, we can change the default in the tool.

Fair enough, I'll remove the single letter option.

Thanks,
Namhyung

Re: [PATCH 3/3] perf lock: Print lost entries at the end

[Namhyung Kim]

On Tue, Aug 2, 2022 at 10:45 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Tue, Aug 02, 2022 at 12:35:11AM -0700, Namhyung Kim escreveu:
> > Like the normal perf lock report output, it'd print bad stats at the end
> > if exists or -v option is passed.  Currently it uses BROKEN_CONTENDED
>  stat for the lost count (due to full stack maps).
>
> "Print the number of lost entries in verbose mode"?

Do you want me to change the commit description?

Note that it can print the numbers not in verbose mode.
It used to print unconditionally but I changed it to do
that only if there's a bad number.  The -v option can
help to keep the original behavior of printing it always.

Thanks,
Namhyung


>
> >   $ sudo perf lock con -a -b -m 128 sleep 5
> >   ...
> >   === output for debug===
> >
> >   bad: 43, total: 14903
> >   bad rate: 0.29 %
> >   histogram of events caused bad sequence
> >       acquire: 0
> >      acquired: 0
> >     contended: 43
> >       release: 0
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/builtin-lock.c                      | 8 +++++++-
> >  tools/perf/util/bpf_lock_contention.c          | 6 ++++--
> >  tools/perf/util/bpf_skel/lock_contention.bpf.c | 9 +++++++--
> >  tools/perf/util/lock-contention.h              | 1 +
> >  4 files changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> > index e32fdcd497e0..8065f0268e55 100644
> > --- a/tools/perf/builtin-lock.c
> > +++ b/tools/perf/builtin-lock.c
> > @@ -1471,8 +1471,11 @@ static void print_contention_result(void)
> >               pr_info("  %10s   %s\n\n", "type", "caller");
> >
> >       bad = total = 0;
> > +     if (use_bpf)
> > +             bad = bad_hist[BROKEN_CONTENDED];
> > +
> >       while ((st = pop_from_result())) {
> > -             total++;
> > +             total += use_bpf ? st->nr_contended : 1;
> >               if (st->broken)
> >                       bad++;
> >
> > @@ -1686,6 +1689,9 @@ static int __cmd_contention(int argc, const char **argv)
> >
> >               lock_contention_stop();
> >               lock_contention_read(&con);
> > +
> > +             /* abuse bad hist stats for lost entries */
> > +             bad_hist[BROKEN_CONTENDED] = con.lost;
> >       } else {
> >               err = perf_session__process_events(session);
> >               if (err)
> > diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
> > index 26128e5bb659..65f51cc25236 100644
> > --- a/tools/perf/util/bpf_lock_contention.c
> > +++ b/tools/perf/util/bpf_lock_contention.c
> > @@ -16,7 +16,7 @@ static struct lock_contention_bpf *skel;
> >
> >  /* should be same as bpf_skel/lock_contention.bpf.c */
> >  struct lock_contention_key {
> > -     u32 stack_id;
> > +     s32 stack_id;
> >  };
> >
> >  struct lock_contention_data {
> > @@ -110,7 +110,7 @@ int lock_contention_stop(void)
> >  int lock_contention_read(struct lock_contention *con)
> >  {
> >       int fd, stack;
> > -     u32 prev_key, key;
> > +     s32 prev_key, key;
> >       struct lock_contention_data data;
> >       struct lock_stat *st;
> >       struct machine *machine = con->machine;
> > @@ -119,6 +119,8 @@ int lock_contention_read(struct lock_contention *con)
> >       fd = bpf_map__fd(skel->maps.lock_stat);
> >       stack = bpf_map__fd(skel->maps.stacks);
> >
> > +     con->lost = skel->bss->lost;
> > +
> >       prev_key = 0;
> >       while (!bpf_map_get_next_key(fd, &prev_key, &key)) {
> >               struct map *kmap;
> > diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> > index 67d46533e518..9e8b94eb6320 100644
> > --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c
> > +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> > @@ -12,7 +12,7 @@
> >  #define MAX_ENTRIES  10240
> >
> >  struct contention_key {
> > -     __u32 stack_id;
> > +     __s32 stack_id;
> >  };
> >
> >  struct contention_data {
> > @@ -27,7 +27,7 @@ struct tstamp_data {
> >       __u64 timestamp;
> >       __u64 lock;
> >       __u32 flags;
> > -     __u32 stack_id;
> > +     __s32 stack_id;
> >  };
> >
> >  /* callstack storage  */
> > @@ -73,6 +73,9 @@ int enabled;
> >  int has_cpu;
> >  int has_task;
> >
> > +/* error stat */
> > +unsigned long lost;
> > +
> >  static inline int can_record(void)
> >  {
> >       if (has_cpu) {
> > @@ -116,6 +119,8 @@ int contention_begin(u64 *ctx)
> >       pelem->flags = (__u32)ctx[1];
> >       pelem->stack_id = bpf_get_stackid(ctx, &stacks, BPF_F_FAST_STACK_CMP);
> >
> > +     if (pelem->stack_id < 0)
> > +             lost++;
> >       return 0;
> >  }
> >
> > diff --git a/tools/perf/util/lock-contention.h b/tools/perf/util/lock-contention.h
> > index b09fd6eb978a..d9fc5f076567 100644
> > --- a/tools/perf/util/lock-contention.h
> > +++ b/tools/perf/util/lock-contention.h
> > @@ -113,6 +113,7 @@ struct lock_contention {
> >       struct machine *machine;
> >       struct hlist_head *result;
> >       unsigned long map_len;
> > +     unsigned long lost;
> >  };
> >
> >  #ifdef HAVE_BPF_SKEL
> > --
> > 2.37.1.455.g008518b4e5-goog
>
> --
>
> - Arnaldo

Re: [PATCH 2/3] perf lock: Add -m/--map-length option

[Arnaldo Carvalho de Melo]

Em Tue, Aug 02, 2022 at 11:05:09AM -0700, Namhyung Kim escreveu:
> On Tue, Aug 2, 2022 at 10:43 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > Em Tue, Aug 02, 2022 at 12:35:10AM -0700, Namhyung Kim escreveu:
> > > +-m::
> > > +--map-length::
> > > +     Maximum number of BPF map entries (default: 10240).
> >
> > --map-nr-entries?
> >
> > I think we use this jargon "nr-entries" for arrays, lists, etc, better
> > try to stick to it.
 
> Makes sense, will change.

Great.
 
> > Also what do you think about not using single letter options for things
> > that are not that used?
> >
> > The map size has a default, one that seems generous, so changing it
> > should be something uncommon, and then, if it becomes common that more
> > entries are needed by default, we can change the default in the tool.
> 
> Fair enough, I'll remove the single letter option.

Thanks!

- Arnaldo