Received-SPF: pass (google.com: domain of linux-kernel+bounces-210779-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240610131032.516323-1-howardchu95@gmail.com> <ZmiftHw-66m3WymK@x1>
In-Reply-To: <ZmiftHw-66m3WymK@x1>
From: Howard Chu <howardchu95@gmail.com>
Date: Wed, 12 Jun 2024 09:51:45 +0800
Message-ID: <CAH0uvoigeXvLPXKz4Yp=jHHm637bkEvDdSUKW7MYOj5Pu7WNbw@mail.gmail.com>
Subject: Re: [PATCH v1] perf trace: BTF-based enum pretty printing
To: Arnaldo Carvalho de Melo <acme@kernel.org>
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
Content-Type: text/plain; charset="UTF-8"

[Resend because of the HTML error]

Hello Arnaldo,
Thanks a lot for the review, I guess you call it v1 for a reason. :)
> > +
> > +     id = btf__find_by_name(btf, type);
>
>         int id = ...

Do you want me to do the initialization in the middle of the function
body sir? A little reminder, char* pointer 'type' has to be shifted to
the first non-enum-prefix location to do the btf__find_by_name().

>
> > +     if (id < 0)
>
> Shouldn't we have some warning here? ok, I see you do it later, in
> trace__read_syscall_info().

I'm sorry, could you be more specific please? To my understanding, it
is syscall__scnprintf_args() who called btf_enum_scnprintf(), and I
did the error handling(or fallback) by calling
syscall_arg_fmt__scnprintf_val(). It's like:

if btf_enum_scnprintf() returns non-0 // success
        continue;
else // error
        syscall_arg_fmt__scnprintf_val()

So we fall back to just printing the long value.

>
> Also I looked at the btf_enum_scnprintf() caller and if this isn't found
> nothing is printed, it is better to fallback to printing the integer
> value, as done in other parts, see:

Do you think the code below could be seen as a sort of fallback
mechanism? If nothing is printed, btf_enum_scnprintf() returns a 0, we
continue to do a syscall_arg_fmt__scnprintf_val() as the fallback. I
tested it by putting return 0 at the top of btf_enum_scnprintf(), and
it works, although not so straightforward... Maybe I should put the
fallback straight in btf_enum_scnprintf().

> > +                     if (sc->arg_fmt[arg.idx].is_enum == true && trace->btf) {
> > +                             p = btf_enum_scnprintf(bf + printed, size - printed, val,
> > +                                                    trace->btf, field->type);
> > +                             if (p) {
> > +                                     printed += p;
> > +                                     continue;
> > +                             }
> > +                     }
> > +
> >                       printed += syscall_arg_fmt__scnprintf_val(&sc->arg_fmt[arg.idx],
> >                                                                 bf + printed, size - printed, &arg, val);

>
> size_t strarray__scnprintf(struct strarray *sa, char *bf, size_t size, const char *intfmt, bool show_prefix, int val)
>
> That intfmt is configurable to show hex or decimal and is used when the
> 'val' isn't found in the strarray, so we should use the same approach
> with BTF.
>
> > +             return 0;
> > +
> > +     bt = btf__type_by_id(btf, id);
> > +     e = btf_enum(bt);
>
> Declare 'bt' and 'e' here
>
> > +
> > +     for (int i = 0; i < btf_vlen(bt); i++, e++) {
> > +             if (e->val == val)
> > +                     return scnprintf(bf, size, "%s",
> > +                                      btf__name_by_offset(btf, e->name_off));

you mean doing it like this?
```
for (bt = btf__type_by_id(btf, id), e = btf_enum(bt), i = 0;
     i < btf_vlen(bt); i++, e++) {
        if (e->val == val) {
                return scnprintf(bf, size, "%s",
                btf__name_by_offset(btf, e->name_off));
        }
}
```

> This is shaping up super nicely, great!

:) Thank you so much sir.

>
> I'm pushing the simplified first patch to my tmp.perf-tools-next branch
> in my tree at:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tmp.perf-tools-next
>
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=tmp.perf-tools-next

Sure, I'll pull from it and build the enum support on top of that.

>
> Some more comments below.
>
> >       if (last_field)
> >               sc->args_size = last_field->offset + last_field->size;
> > @@ -1811,6 +1854,7 @@ static int trace__read_syscall_info(struct trace *trace, int id)
> >       char tp_name[128];
> >       struct syscall *sc;
> >       const char *name = syscalltbl__name(trace->sctbl, id);
> > +     int err;
> >
> >  #ifdef HAVE_SYSCALL_TABLE_SUPPORT
> >       if (trace->syscalls.table == NULL) {
> > @@ -1883,7 +1927,17 @@ 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);
> > +     err = syscall__set_arg_fmts(sc);
>
> > +     /* after calling syscall__set_arg_fmts() we'll know whether use_btf is true */
> > +     if (sc->use_btf && trace->btf == NULL) {
> > +             trace->btf = btf__load_vmlinux_btf();
> > +             if (verbose > 0)
> > +                     fprintf(trace->output, trace->btf ? "vmlinux BTF loaded\n" :
> > +                                                         "Failed to load vmlinux BTF\n");
> > +     }
>
> One suggestion here is to get the body of the above if and have it in a
> trace__load_vmlinux_btf(), as that call and the test under verbose will
> be used in other places, for instance, when supporting using BTF to
> pretty print non-syscall tracepoints.
>
> This function probably will grow to support detached BTF, possibly that
> idea about generating BTF from the scrape scripts, etc.

Sure.

Thank you very much for reviewing this patch, v2 is coming up.

Thanks,
Howard

>
> Thanks,
>
> - Arnaldo
>
> > +     return err;
> >  }
> >
> >  static int evsel__init_tp_arg_scnprintf(struct evsel *evsel)
> > @@ -2050,7 +2104,7 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
> >                                     unsigned char *args, void *augmented_args, int augmented_args_size,
> >                                     struct trace *trace, struct thread *thread)
> >  {
> > -     size_t printed = 0;
> > +     size_t printed = 0, p;
> >       unsigned long val;
> >       u8 bit = 1;
> >       struct syscall_arg arg = {
> > @@ -2103,6 +2157,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);
> >
> >               }
> > --
> > 2.45.2