Re: [PATCH v3 4/8] perf trace: Pretty print struct data

[Arnaldo Carvalho de Melo]

On Fri, Aug 30, 2024 at 08:16:38AM +0800, Howard Chu wrote:
> Hello Arnaldo,
> 
> On Thu, Aug 29, 2024 at 5:05 AM Arnaldo Carvalho de Melo
> <acme@xxxxxxxxxx> wrote:
> >
> > On Sun, Aug 25, 2024 at 12:33:18AM +0800, Howard Chu wrote:
> > > Change the arg->augmented.args to arg->augmented.args->value to skip the
> > > header for customized pretty printers, since we collect data in BPF
> > > using the general augment_sys_enter(), which always adds the header.
> > >
> > > Use btf_dump API to pretty print augmented struct pointer.
> > >
> > > Prefer existed pretty-printer than btf general pretty-printer.
> > >
> > > set compact = true and skip_names = true, so that no newline character
> > > and argument name are printed.
> > >
> > > Committer notes:
> > >
> > > Simplified the btf_dump_snprintf callback to avoid using multiple
> > > buffers, as discussed in the thread accessible via the Link tag below.
> > >
> > > Signed-off-by: Howard Chu <howardchu95@xxxxxxxxx>
> > > Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> > > Cc: Ian Rogers <irogers@xxxxxxxxxx>
> > > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > Cc: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> > > Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> > > Link: https://lore.kernel.org/r/20240815013626.935097-7-howardchu95@xxxxxxxxx
> > > Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > > ---
> > >  tools/perf/builtin-trace.c                | 65 +++++++++++++++++++++--
> > >  tools/perf/trace/beauty/perf_event_open.c |  2 +-
> > >  tools/perf/trace/beauty/sockaddr.c        |  2 +-
> > >  tools/perf/trace/beauty/timespec.c        |  2 +-
> > >  4 files changed, 63 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > > index 43b1f63415b4..048bcb92624c 100644
> > > --- a/tools/perf/builtin-trace.c
> > > +++ b/tools/perf/builtin-trace.c
> > > @@ -990,6 +990,54 @@ static size_t btf_enum_scnprintf(const struct btf_type *type, struct btf *btf, c
> > >       return 0;
> > >  }
> > >
> > > +struct trace_btf_dump_snprintf_ctx {
> > > +     char   *bf;
> > > +     size_t printed, size;
> > > +};
> > > +
> > > +static void trace__btf_dump_snprintf(void *vctx, const char *fmt, va_list args)
> > > +{
> > > +     struct trace_btf_dump_snprintf_ctx *ctx = vctx;
> > > +
> > > +     ctx->printed += vscnprintf(ctx->bf + ctx->printed, ctx->size - ctx->printed, fmt, args);
> > > +}
> > > +
> > > +static size_t btf_struct_scnprintf(const struct btf_type *type, struct btf *btf, char *bf, size_t size, struct syscall_arg *arg)
> > > +{
> > > +     struct trace_btf_dump_snprintf_ctx ctx = {
> > > +             .bf   = bf,
> > > +             .size = size,
> > > +     };
> > > +     struct augmented_arg *augmented_arg = arg->augmented.args;
> > > +     int type_id = arg->fmt->type_id, consumed;
> > > +     struct btf_dump *btf_dump;
> > > +
> > > +     LIBBPF_OPTS(btf_dump_opts, dump_opts);
> > > +     LIBBPF_OPTS(btf_dump_type_data_opts, dump_data_opts);
> > > +
> > > +     if (arg == NULL || arg->augmented.args == NULL)
> > > +             return 0;
> > > +
> > > +     dump_data_opts.compact     = true;
> > > +     dump_data_opts.skip_names  = true;
> > > +
> > > +     btf_dump = btf_dump__new(btf, trace__btf_dump_snprintf, &ctx, &dump_opts);
> > > +     if (btf_dump == NULL)
> > > +             return 0;
> > > +
> > > +     /* pretty print the struct data here */
> > > +     if (btf_dump__dump_type_data(btf_dump, type_id, arg->augmented.args->value, type->size, &dump_data_opts) == 0)
> > > +             return 0;
> > > +
> > > +     consumed = sizeof(*augmented_arg) + augmented_arg->size;
> > > +     arg->augmented.args = ((void *)arg->augmented.args) + consumed;
> > > +     arg->augmented.size -= consumed;
> > > +
> > > +     btf_dump__free(btf_dump);
> > > +
> > > +     return ctx.printed;
> > > +}
> > > +
> > >  static size_t trace__btf_scnprintf(struct trace *trace, struct syscall_arg *arg, char *bf,
> > >                                  size_t size, int val, char *type)
> > >  {
> > > @@ -1009,6 +1057,8 @@ static size_t trace__btf_scnprintf(struct trace *trace, struct syscall_arg *arg,
> > >
> > >       if (btf_is_enum(arg_fmt->type))
> > >               return btf_enum_scnprintf(arg_fmt->type, trace->btf, bf, size, val);
> > > +     else if (btf_is_struct(arg_fmt->type))
> > > +             return btf_struct_scnprintf(arg_fmt->type, trace->btf, bf, size, arg);
> > >
> > >       return 0;
> > >  }
> > > @@ -2222,6 +2272,7 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
> > >               .show_string_prefix = trace->show_string_prefix,
> > >       };
> > >       struct thread_trace *ttrace = thread__priv(thread);
> > > +     void *default_scnprintf;
> > >
> > >       /*
> > >        * Things like fcntl will set this in its 'cmd' formatter to pick the
> > > @@ -2263,11 +2314,15 @@ 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);
> > >
> > > -                     btf_printed = trace__btf_scnprintf(trace, &arg, bf + printed,
> > > -                                                        size - printed, val, field->type);
> > > -                     if (btf_printed) {
> > > -                             printed += btf_printed;
> > > -                             continue;
> > > +                     default_scnprintf = sc->arg_fmt[arg.idx].scnprintf;
> > > +
> > > +                     if (default_scnprintf == NULL || default_scnprintf == SCA_PTR) {
> > > +                             btf_printed = trace__btf_scnprintf(trace, &arg, bf + printed,
> > > +                                                                size - printed, val, field->type);
> > > +                             if (btf_printed) {
> > > +                                     printed += btf_printed;
> > > +                                     continue;
> > > +                             }
> >
> > Ok, we agreed on this one, and you noted that that in this cset comment,
> > good.
> >
> > Next time make a note after the cset commit log message and before the
> > actual patch, something like:
> >
> > vN: prefer pre-existing userspace scnprintf if explicitely specified,
> > only fallbacking to the generic BTF one when none is specified.
> >
> > >                       }
> > >
> > >                       printed += syscall_arg_fmt__scnprintf_val(&sc->arg_fmt[arg.idx],
> > > diff --git a/tools/perf/trace/beauty/perf_event_open.c b/tools/perf/trace/beauty/perf_event_open.c
> > > index 01ee15fe9d0c..632237128640 100644
> > > --- a/tools/perf/trace/beauty/perf_event_open.c
> > > +++ b/tools/perf/trace/beauty/perf_event_open.c
> > > @@ -76,7 +76,7 @@ static size_t perf_event_attr___scnprintf(struct perf_event_attr *attr, char *bf
> >
> > But this part will work if we use the old collectors in the BPF skel?
> >
> 
> You are right, it won't. The one scenario is that when a vmlinux file
> is presented, the general collector (the new one, based on BTF) will
> fallback to the old collectors, and now we have new
> consumers(ptr->value). This will break.
> 
> > I.e. isn't this a change in the protocol of the BPF colector with the
> > userpace augmented snprintf routines?
> >
> > If I remember we discussed that first you make this change in the
> > protocol, test it with the pre-existing BPF collector, it works. Ok, now
> > we have a new protocol and we then use it in the generic BTF-based BPF
> > collector. This way that option of using the BTF-based collector or the
> > preexisting BPF collector works.
> >
> > I'll try to do this.
> 
> Thank you so much.

Now that I have:

6b22c2b502a1c21b (x1/perf-tools-next, perf-tools-next/tmp.perf-tools-next, acme/tmp.perf-tools-next) perf trace: Use a common encoding for augmented arguments, with size + error + payload
5d9cd24924f57066 perf trace augmented_syscalls.bpf: Move the renameat aumenter to renameat2, temporarily

And also:

7bedcbaefdf5d4f7 perf trace: Pass the richer 'struct syscall_arg' pointer to trace__btf_scnprintf()
8df1d8c6cbd6825b perf trace: Fix perf trace -p <PID>

I can move to this patch, but then it doesn't build as it uses things
that will only be available when we get to adding the new BPF components
in a later patch:

builtin-trace.c: In function ‘trace__init_syscalls_bpf_prog_array_maps’:
builtin-trace.c:3725:58: error: ‘struct <anonymous>’ has no member named ‘beauty_map_enter’
 3725 |         int beauty_map_fd = bpf_map__fd(trace->skel->maps.beauty_map_enter);
      |                                                          ^
make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:105: /tmp/build/perf-tools-next/builtin-trace.o] Error 1
make[3]: *** Waiting for unfinished jobs....

It will only come when we add:

Subject: [PATCH v3 6/8] perf trace: Collect augmented data using BPF

Where we have:

+struct beauty_map_enter {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, int);
+	__type(value, __u32[6]);
+	__uint(max_entries, 512);
+} beauty_map_enter SEC(".maps");


So I'll try to combine the addition of the map with the code that uses
it in the builtin-trace.c codebase.

- Arnaldo