From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Howard Chu <howardchu95@gmail.com>
Cc: peterz@infradead.org, mingo@redhat.com, namhyung@kernel.org,
mark.rutland@arm.com, alexander.shishkin@linux.intel.com,
jolsa@kernel.org, irogers@google.com, adrian.hunter@intel.com,
kan.liang@linux.intel.com, mic@digikod.net, gnoack@google.com,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH v4] perf trace: BTF-based enum pretty printing
Date: Thu, 13 Jun 2024 09:46:58 -0300 [thread overview]
Message-ID: <ZmrqQs64TvAt8XjK@x1> (raw)
In-Reply-To: <20240613042747.3770204-1-howardchu95@gmail.com>
On Thu, Jun 13, 2024 at 12:27:47PM +0800, Howard Chu wrote:
> changes in v4:
>
> - Add enum support to tracepoint arguments
That is cool, but see below the comment as having this as a separate
patch.
Also please, on the patch that introduces ! syscall tracepoint enum args
BTF augmentation include examples of tracepoints being augmented. I'll
try here while testing the patch as-is.
More comments below.
> - For tracing syscalls, move trace__load_vmlinux_btf() to
> syscall__set_arg_fmts() to make it more elegant
>
> changes in v3:
>
> - Fixed another awkward formatting issue in trace__load_vmlinux_btf()
>
> changes in v2:
>
> - Fix formatting issues
<SNIP>
> before:
>
> ```
> perf $ ./perf trace -e landlock_add_rule
> 0.000 ( 0.008 ms): ldlck-test/438194 landlock_add_rule(rule_type: 2) = -1 EBADFD (File descriptor in bad state)
> 0.010 ( 0.001 ms): ldlck-test/438194 landlock_add_rule(rule_type: 1) = -1 EBADFD (File descriptor in bad state)
> ```
>
> after:
>
> ```
> perf $ ./perf trace -e landlock_add_rule
> 0.000 ( 0.029 ms): ldlck-test/438194 landlock_add_rule(rule_type: LANDLOCK_RULE_NET_PORT) = -1 EBADFD (File descriptor in bad state)
> 0.036 ( 0.004 ms): ldlck-test/438194 landlock_add_rule(rule_type: LANDLOCK_RULE_PATH_BENEATH) = -1 EBADFD (File descriptor in bad state)
> ```
>
> Signed-off-by: Howard Chu <howardchu95@gmail.com>
> ---
> tools/perf/builtin-trace.c | 121 ++++++++++++++++++++++++++++++++++---
> 1 file changed, 111 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 5cbe1748911d..0a168cb9b0c2 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -19,6 +19,7 @@
> #ifdef HAVE_LIBBPF_SUPPORT
> #include <bpf/bpf.h>
> #include <bpf/libbpf.h>
> +#include <bpf/btf.h>
> #ifdef HAVE_BPF_SKEL
> #include "bpf_skel/augmented_raw_syscalls.skel.h"
> #endif
> @@ -110,6 +111,11 @@ struct syscall_arg_fmt {
> const char *name;
> u16 nr_entries; // for arrays
> bool show_zero;
> + bool is_enum;
> + struct {
> + void *entry;
> + u16 nr_entries;
> + } btf_entry;
> };
>
> struct syscall_fmt {
> @@ -140,6 +146,7 @@ struct trace {
> #ifdef HAVE_BPF_SKEL
> struct augmented_raw_syscalls_bpf *skel;
> #endif
> + struct btf *btf;
> struct record_opts opts;
> struct evlist *evlist;
> struct machine *host;
> @@ -887,6 +894,56 @@ static size_t syscall_arg__scnprintf_getrandom_flags(char *bf, size_t size,
>
> #define SCA_GETRANDOM_FLAGS syscall_arg__scnprintf_getrandom_flags
>
> +static int btf_enum_find_entry(struct btf *btf, char *type, struct syscall_arg_fmt *arg_fmt)
> +{
> + const struct btf_type *bt;
> + char enum_prefix[][16] = { "enum", "const enum" }, *ep;
> + int id;
> + size_t i;
> +
> + for (i = 0; i < ARRAY_SIZE(enum_prefix); i++) {
> + ep = enum_prefix[i];
> + if (strlen(type) > strlen(ep) + 1 && strstarts(type, ep))
> + type += strlen(ep) + 1;
> + }
> +
> + id = btf__find_by_name(btf, type);
> + if (id < 0)
> + return -1;
> +
> + bt = btf__type_by_id(btf, id);
> + if (bt == NULL)
> + return -1;
> +
> + arg_fmt->btf_entry.entry = btf_enum(bt);
> + arg_fmt->btf_entry.nr_entries = btf_vlen(bt);
> +
> + return 0;
> +}
> +
> +static size_t btf_enum_scnprintf(char *bf, size_t size, int val, struct btf *btf, char *type,
> + struct syscall_arg_fmt *arg_fmt)
> +{
> + struct btf_enum *be;
> + int i;
> +
> + /* if btf_entry is NULL, find and save it to arg_fmt */
> + if (arg_fmt->btf_entry.entry == NULL)
> + if (btf_enum_find_entry(btf, type, arg_fmt))
> + return 0;
> +
> + be = (struct btf_enum *)arg_fmt->btf_entry.entry;
> +
> + for (i = 0; i < arg_fmt->btf_entry.nr_entries; ++i, ++be) {
> + if (be->val == val) {
> + return scnprintf(bf, size, "%s",
> + btf__name_by_offset(btf, be->name_off));
> + }
> + }
> +
> + return 0;
> +}
> +
> #define STRARRAY(name, array) \
> { .scnprintf = SCA_STRARRAY, \
> .strtoul = STUL_STRARRAY, \
> @@ -1238,6 +1295,7 @@ struct syscall {
> bool is_exit;
> bool is_open;
> bool nonexistent;
> + bool use_btf;
> struct tep_format_field *args;
> const char *name;
> const struct syscall_fmt *fmt;
> @@ -1699,6 +1757,15 @@ static void trace__symbols__exit(struct trace *trace)
> symbol__exit();
> }
>
> +static void trace__load_vmlinux_btf(struct trace *trace)
> +{
> + trace->btf = btf__load_vmlinux_btf();
> + if (verbose > 0) {
> + fprintf(trace->output, trace->btf ? "vmlinux BTF loaded\n" :
> + "Failed to load vmlinux BTF\n");
> + }
> +}
> +
> static int syscall__alloc_arg_fmts(struct syscall *sc, int nr_args)
> {
> int idx;
> @@ -1744,7 +1811,8 @@ static const struct syscall_arg_fmt *syscall_arg_fmt__find_by_name(const char *n
> }
>
> static struct tep_format_field *
> -syscall_arg_fmt__init_array(struct syscall_arg_fmt *arg, struct tep_format_field *field)
> +syscall_arg_fmt__init_array(struct syscall_arg_fmt *arg, struct tep_format_field *field,
> + bool *use_btf)
> {
> struct tep_format_field *last_field = NULL;
> int len;
> @@ -1756,6 +1824,7 @@ syscall_arg_fmt__init_array(struct syscall_arg_fmt *arg, struct tep_format_field
> continue;
>
> len = strlen(field->name);
> + arg->is_enum = false;
>
> if (strcmp(field->type, "const char *") == 0 &&
> ((len >= 4 && strcmp(field->name + len - 4, "name") == 0) ||
> @@ -1782,6 +1851,8 @@ syscall_arg_fmt__init_array(struct syscall_arg_fmt *arg, struct tep_format_field
> * 7 unsigned long
> */
> arg->scnprintf = SCA_FD;
> + } else if (strstr(field->type, "enum") && use_btf != NULL) {
> + *use_btf = arg->is_enum = true;
> } else {
> const struct syscall_arg_fmt *fmt =
> syscall_arg_fmt__find_by_name(field->name);
> @@ -1796,9 +1867,14 @@ syscall_arg_fmt__init_array(struct syscall_arg_fmt *arg, struct tep_format_field
> return last_field;
> }
>
> -static int syscall__set_arg_fmts(struct syscall *sc)
> +static int syscall__set_arg_fmts(struct trace *trace, struct syscall *sc)
See the comment about evsel__init_tp_arg_scnprintf() below. Also please
do patches on top of previous work, i.e. the v3 patch should be a
separate patch and this v4 should add the extra functionality, i.e. the
support for !syscall tracepoint enum BTF augmentation.
> {
> - struct tep_format_field *last_field = syscall_arg_fmt__init_array(sc->arg_fmt, sc->args);
> + bool use_btf;
> + struct tep_format_field *last_field = syscall_arg_fmt__init_array(sc->arg_fmt, sc->args,
> + &use_btf);
> +
> + if (use_btf && trace->btf == NULL)
> + trace__load_vmlinux_btf(trace);
>
> if (last_field)
> sc->args_size = last_field->offset + last_field->size;
> @@ -1883,15 +1959,20 @@ static int trace__read_syscall_info(struct trace *trace, int id)
> sc->is_exit = !strcmp(name, "exit_group") || !strcmp(name, "exit");
> sc->is_open = !strcmp(name, "open") || !strcmp(name, "openat");
>
> - return syscall__set_arg_fmts(sc);
> + return syscall__set_arg_fmts(trace, sc);
> }
>
> -static int evsel__init_tp_arg_scnprintf(struct evsel *evsel)
> +static int evsel__init_tp_arg_scnprintf(struct trace *trace, struct evsel *evsel)
The convention here is that evsel__ is the "class" name, so the first
arg is a 'struct evsel *', if you really were transforming this into a
'struct trace' specific "method" you would change the name of the C
function to 'trace__init_tp_arg_scnprintf'.
But in this case instead of passing the 'struct trace' pointer all the
way down we should instead pass a 'bool *use_btf' argument, making it:
static int evsel__init_tp_arg_scnprintf(struct evsel *evsel, bool *use_btf)
Then, when evlist__set_syscall_tp_fields(evlist, &use_btf) returns,
check that use_btf to check if we need to call
trace__load_vmlinux_btf(trace).
I'll test the patch as is now.
- Arnaldo
> {
> struct syscall_arg_fmt *fmt = evsel__syscall_arg_fmt(evsel);
> + bool use_btf;
>
> if (fmt != NULL) {
> - syscall_arg_fmt__init_array(fmt, evsel->tp_format->format.fields);
> + syscall_arg_fmt__init_array(fmt, evsel->tp_format->format.fields, &use_btf);
> +
> + if (use_btf && trace->btf == NULL)
> + trace__load_vmlinux_btf(trace);
> +
> return 0;
> }
>
> @@ -2103,6 +2184,16 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
> if (trace->show_arg_names)
> printed += scnprintf(bf + printed, size - printed, "%s: ", field->name);
>
> + if (sc->arg_fmt[arg.idx].is_enum && trace->btf) {
> + size_t p = btf_enum_scnprintf(bf + printed, size - printed, val,
> + trace->btf, field->type,
> + &sc->arg_fmt[arg.idx]);
> + if (p) {
> + printed += p;
> + continue;
> + }
> + }
> +
> printed += syscall_arg_fmt__scnprintf_val(&sc->arg_fmt[arg.idx],
> bf + printed, size - printed, &arg, val);
> }
> @@ -2791,7 +2882,7 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel,
> val = syscall_arg_fmt__mask_val(arg, &syscall_arg, val);
>
> /* Suppress this argument if its value is zero and show_zero property isn't set. */
> - if (val == 0 && !trace->show_zeros && !arg->show_zero)
> + if (val == 0 && !trace->show_zeros && !arg->show_zero && !arg->is_enum)
> continue;
>
> printed += scnprintf(bf + printed, size - printed, "%s", printed ? ", " : "");
> @@ -2799,6 +2890,15 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel,
> if (trace->show_arg_names)
> printed += scnprintf(bf + printed, size - printed, "%s: ", field->name);
>
> + if (arg->is_enum && trace->btf) {
> + size_t p = btf_enum_scnprintf(bf + printed, size - printed, val, trace->btf,
> + field->type, arg);
> + if (p) {
> + printed += p;
> + continue;
> + }
> + }
> +
> printed += syscall_arg_fmt__scnprintf_val(arg, bf + printed, size - printed, &syscall_arg, val);
> }
>
> @@ -4461,8 +4561,9 @@ static void evsel__set_syscall_arg_fmt(struct evsel *evsel, const char *name)
> }
> }
>
> -static int evlist__set_syscall_tp_fields(struct evlist *evlist)
> +static int evlist__set_syscall_tp_fields(struct trace *trace)
> {
> + struct evlist *evlist = trace->evlist;
> struct evsel *evsel;
>
> evlist__for_each_entry(evlist, evsel) {
> @@ -4470,7 +4571,7 @@ static int evlist__set_syscall_tp_fields(struct evlist *evlist)
> continue;
>
> if (strcmp(evsel->tp_format->system, "syscalls")) {
> - evsel__init_tp_arg_scnprintf(evsel);
> + evsel__init_tp_arg_scnprintf(trace, evsel);
> continue;
> }
>
> @@ -4949,7 +5050,7 @@ int cmd_trace(int argc, const char **argv)
>
> if (trace.evlist->core.nr_entries > 0) {
> evlist__set_default_evsel_handler(trace.evlist, trace__event_handler);
> - if (evlist__set_syscall_tp_fields(trace.evlist)) {
> + if (evlist__set_syscall_tp_fields(&trace)) {
> perror("failed to set syscalls:* tracepoint fields");
> goto out;
> }
> --
> 2.45.2
>
next prev parent reply other threads:[~2024-06-13 12:47 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-06-13 4:27 [PATCH v4] perf trace: BTF-based enum pretty printing Howard Chu
2024-06-13 12:46 ` Arnaldo Carvalho de Melo [this message]
2024-06-13 12:59 ` Arnaldo Carvalho de Melo
2024-06-13 15:50 ` Howard Chu
2024-06-14 18:41 ` Arnaldo Carvalho de Melo
2024-06-13 14:10 ` Arnaldo Carvalho de Melo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZmrqQs64TvAt8XjK@x1 \
--to=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=bpf@vger.kernel.org \
--cc=gnoack@google.com \
--cc=howardchu95@gmail.com \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mic@digikod.net \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.