2022-08-02 19:12:50

by Namhyung Kim

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

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 <[email protected]>
---
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



2022-08-02 19:13:00

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH v2 2/3] perf lock: Add --map-nr-entries option

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

Signed-off-by: Namhyung Kim <[email protected]>
---
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


2022-08-02 19:20:07

by Namhyung Kim

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

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 <[email protected]>
---
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


2022-08-02 21:12:46

by Arnaldo Carvalho de Melo

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

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 <[email protected]>
> ---
> 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

2022-08-03 00:57:00

by Namhyung Kim

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

On Tue, Aug 2, 2022 at 2:04 PM Arnaldo Carvalho de Melo <[email protected]> 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

2022-08-03 15:20:02

by Arnaldo Carvalho de Melo

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

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 <[email protected]> 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

2022-09-26 08:46:12

by Jiri Slaby

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

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

2022-09-26 22:16:33

by Namhyung Kim

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

Hello,

On Mon, Sep 26, 2022 at 1:05 AM Jiri Slaby <[email protected]> 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
>