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

[PATCH v2 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 v2 2/3] perf lock: Add --map-nr-entries option

[Namhyung Kim]

The --map-nr-entries 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 |  3 +++
 tools/perf/builtin-lock.c              | 23 ++++++++++++++++++++++-
 tools/perf/util/bpf_lock_contention.c  |  3 +++
 tools/perf/util/lock-contention.h      |  1 +
 4 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
index 7949d2e6891b..193c5d8b8db9 100644
--- a/tools/perf/Documentation/perf-lock.txt
+++ b/tools/perf/Documentation/perf-lock.txt
@@ -145,6 +145,9 @@ CONTENTION OPTIONS
 --tid=::
         Record events on existing thread ID (comma separated list).
 
+--map-nr-entries::
+	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..4c70cd1b9006 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_entries = 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_nr_entries = bpf_map_entries,
 	};
 
 	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_entry(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(0, "map-nr-entries", &bpf_map_entries, "num",
+		     "Max number of BPF map entries", parse_map_entry),
 	OPT_PARENT(lock_options)
 	};
 
diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
index f5e2b4f19a72..0556cf4469ff 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_nr_entries);
+	bpf_map__set_max_entries(skel->maps.lock_stat, con->map_nr_entries);
+
 	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..8de8550f5ef8 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_nr_entries;
 };
 
 #ifdef HAVE_BPF_SKEL
-- 
2.37.1.455.g008518b4e5-goog

[PATCH v2 3/3] perf lock: Print the number of lost entries for BPF

[Namhyung Kim]

Like the normal perf lock contention output, it'd print the number of
lost entries for BPF 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 --map-nr-entries 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 4c70cd1b9006..a8550f06cbfc 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 0556cf4469ff..c591a66733ef 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 8de8550f5ef8..2146efc33396 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_nr_entries;
+	unsigned long lost;
 };
 
 #ifdef HAVE_BPF_SKEL
-- 
2.37.1.455.g008518b4e5-goog

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

[Arnaldo Carvalho de Melo]

Em Tue, Aug 02, 2022 at 12:10:02PM -0700, Namhyung Kim escreveu:
> The lock_contention struct is to carry related fields together and to
> minimize the change when we add new config options.


Thanks, applied. Forgot the cover letter? :-)

- Arnaldo

 
> 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

-- 

- Arnaldo

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

[Namhyung Kim]

On Tue, Aug 2, 2022 at 2:04 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> Em Tue, Aug 02, 2022 at 12:10:02PM -0700, Namhyung Kim escreveu:
> > The lock_contention struct is to carry related fields together and to
> > minimize the change when we add new config options.
>
>
> Thanks, applied. Forgot the cover letter? :-)

Thank you!

I thought it's a small change that doesn't require a cover letter.
But if you prefer seeing it for small changes too, I'd write a
short letter next time.

Thanks,
Namhyung

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

[Arnaldo Carvalho de Melo]

Em Tue, Aug 02, 2022 at 04:47:02PM -0700, Namhyung Kim escreveu:
> On Tue, Aug 2, 2022 at 2:04 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> >
> > Em Tue, Aug 02, 2022 at 12:10:02PM -0700, Namhyung Kim escreveu:
> > > The lock_contention struct is to carry related fields together and to
> > > minimize the change when we add new config options.
> >
> >
> > Thanks, applied. Forgot the cover letter? :-)
> 
> Thank you!
> 
> I thought it's a small change that doesn't require a cover letter.
> But if you prefer seeing it for small changes too, I'd write a
> short letter next time.

Yeah, just felt unusual, but then, b4 with any of the message-ids works
just fine, so up to you, I'm adjusting.

- Arnaldo

Re: [PATCH v2 3/3] perf lock: Print the number of lost entries for BPF

[Jiri Slaby]

Hi,

On 02. 08. 22, 21:10, Namhyung Kim wrote:
> Like the normal perf lock contention output, it'd print the number of
> lost entries for BPF if exists or -v option is passed.  Currently it
> uses BROKEN_CONTENDED stat for the lost count (due to full stack maps).
...
> --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c
> +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c
...
> @@ -73,6 +73,9 @@ int enabled;
>   int has_cpu;
>   int has_task;
>   
> +/* error stat */
> +unsigned long lost;

I don't know how all this is generated into lock_contention.skel.h. But 
I believe this patch breaks perf build on 32bit:
> [  375s] In file included from util/bpf_lock_contention.c:13:
> [  375s] util/bpf_skel/lock_contention.skel.h: In function 'lock_contention_bpf__assert':
> [  375s] util/bpf_skel/lock_contention.skel.h:537:9: error: static assertion failed: "unexpected size of \'lost\'"
> [  375s]   537 |         _Static_assert(sizeof(s->bss->lost) == 8, "unexpected size of 'lost'");
> [  375s]       |         ^~~~~~~~~~~~~~

Should the above (and below too) be __u64?

> --- 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_nr_entries;
> +	unsigned long lost;

thanks,
-- 
js
suse labs

Re: [PATCH v2 3/3] perf lock: Print the number of lost entries for BPF

[Namhyung Kim]

Hello,

On Mon, Sep 26, 2022 at 1:05 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> Hi,
>
> On 02. 08. 22, 21:10, Namhyung Kim wrote:
> > Like the normal perf lock contention output, it'd print the number of
> > lost entries for BPF if exists or -v option is passed.  Currently it
> > uses BROKEN_CONTENDED stat for the lost count (due to full stack maps).
> ...
> > --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c
> > +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c
> ...
> > @@ -73,6 +73,9 @@ int enabled;
> >   int has_cpu;
> >   int has_task;
> >
> > +/* error stat */
> > +unsigned long lost;
>
> I don't know how all this is generated into lock_contention.skel.h. But
> I believe this patch breaks perf build on 32bit:
> > [  375s] In file included from util/bpf_lock_contention.c:13:
> > [  375s] util/bpf_skel/lock_contention.skel.h: In function 'lock_contention_bpf__assert':
> > [  375s] util/bpf_skel/lock_contention.skel.h:537:9: error: static assertion failed: "unexpected size of \'lost\'"
> > [  375s]   537 |         _Static_assert(sizeof(s->bss->lost) == 8, "unexpected size of 'lost'");
> > [  375s]       |         ^~~~~~~~~~~~~~
>
> Should the above (and below too) be __u64?

Oops, it seems BPF ABI uses 8-byte long even on 32-bit systems.
Sorry for the inconvenience.  Will change.

Thanks,
Namhyung


>
> > --- 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_nr_entries;
> > +     unsigned long lost;
>
> thanks,
> --
> js
> suse labs
>