From: Arnaldo Carvalho de Melo <acme@redhat.com>

Hi,

	This is an attempt at paving the way for the remaining parts of
Howard's work on this years GSoC to be merged, as we want to keep the
existing pretty printers for the few structs we have, that have special
characteristics (see the commit logs) while having the generic BPF + BTF
collector/pretty printer, using the libbpf BPF dumper (see more ideas on
the commit log on how to improve it, maybe even getting
tools/perf/trace/beauty/ into tools/lib/beauty/ to get reused by
libbpf).

	I plan to work on the weekend to plugging his latest series on
top of these patches so that we can get it merged in the next merge
window.

	Any comment/test is more than welcome,

- Arnaldo

Arnaldo Carvalho de Melo (2):
  perf trace augmented_syscalls.bpf: Move the renameat augmenter to
    renameat2, temporarily
  perf trace: Use a common encoding for augmented arguments, with size +
    error + payload

 tools/perf/trace/beauty/perf_event_open.c     |   2 +-
 tools/perf/trace/beauty/sockaddr.c            |   2 +-
 tools/perf/trace/beauty/timespec.c            |   2 +-
 .../bpf_skel/augmented_raw_syscalls.bpf.c     | 108 +++++++++++-------
 4 files changed, 68 insertions(+), 46 deletions(-)

-- 
2.46.0

On 06/09/2024 20:50, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> Hi,
> 
> 	This is an attempt at paving the way for the remaining parts of
> Howard's work on this years GSoC to be merged, as we want to keep the
> existing pretty printers for the few structs we have, that have special
> characteristics (see the commit logs) while having the generic BPF + BTF
> collector/pretty printer, using the libbpf BPF dumper (see more ideas on
> the commit log on how to improve it, maybe even getting
> tools/perf/trace/beauty/ into tools/lib/beauty/ to get reused by
> libbpf).
>

hey Arnaldo

Absolutely, finding the common ground here would be great! I took a
quick look at some of the beautify scripts, and it struck me that some
of what's missing today - and what makes this hard - is that we don't
have easy access to numeric macro name -> value mappings for things like
arch-specific errno values (at least not without DWARF).

In another context, we've seen pain for BPF program writers who have to
cut+paste macro values into their BPF code.  We've sort of solved this
in a few specific cases by converting some values to enumerations. They
then get BTF representations, and can benefit from Compile Once - Run
Everywhere when the macro value is used in the BPF context.

But it seems to me that what both these problems suggest is that it
would be nice to more systematically represent numeric macro values such
that they would be more easily available. I talked a bit about this a
few years back at Plumbers for macros as a whole, but I wonder if a
tweak to pahole that does something like

1. check if a macro has a valid numeric representation;
2. if so, convert it to a singleton anonymous BTF enumerated type that
will not clash with the real macro name (so is safe to use when headers
containing that macro are also included)

This would allow BPF program writers to reference macro-defined flag
values and get the CO-RE benefits they get from enums and presence in a
generated vmlinux.h. It might also help here for beautify where you
could establish name-value mappings for things like arch-specific errnos.

We've been talking about having a loadable module of vmlinux BTF extras,
so it seems like numeric macro representations would be helpful there
too. What do you think? Thanks!

Alan


> 	I plan to work on the weekend to plugging his latest series on
> top of these patches so that we can get it merged in the next merge
> window.
> 
> 	Any comment/test is more than welcome,
> 
> - Arnaldo
> 
> Arnaldo Carvalho de Melo (2):
>   perf trace augmented_syscalls.bpf: Move the renameat augmenter to
>     renameat2, temporarily
>   perf trace: Use a common encoding for augmented arguments, with size +
>     error + payload
> 
>  tools/perf/trace/beauty/perf_event_open.c     |   2 +-
>  tools/perf/trace/beauty/sockaddr.c            |   2 +-
>  tools/perf/trace/beauty/timespec.c            |   2 +-
>  .../bpf_skel/augmented_raw_syscalls.bpf.c     | 108 +++++++++++-------
>  4 files changed, 68 insertions(+), 46 deletions(-)
>

On Mon, Sep 09, 2024 at 04:46:15PM +0100, Alan Maguire wrote:
> On 06/09/2024 20:50, Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> > Hi,
> > 
> > 	This is an attempt at paving the way for the remaining parts of
> > Howard's work on this years GSoC to be merged, as we want to keep the
> > existing pretty printers for the few structs we have, that have special
> > characteristics (see the commit logs) while having the generic BPF + BTF
> > collector/pretty printer, using the libbpf BPF dumper (see more ideas on
> > the commit log on how to improve it, maybe even getting
> > tools/perf/trace/beauty/ into tools/lib/beauty/ to get reused by
> > libbpf).
> >
> 
> hey Arnaldo
> 
> Absolutely, finding the common ground here would be great! I took a
> quick look at some of the beautify scripts, and it struck me that some
> of what's missing today - and what makes this hard - is that we don't
> have easy access to numeric macro name -> value mappings for things like
> arch-specific errno values (at least not without DWARF).

Even with DWARF that is not always available, IIRC it depends on
compiler flags how much DWARF is generated.
 
> In another context, we've seen pain for BPF program writers who have to
> cut+paste macro values into their BPF code.  We've sort of solved this
> in a few specific cases by converting some values to enumerations. They
> then get BTF representations, and can benefit from Compile Once - Run
> Everywhere when the macro value is used in the BPF context.

Right, that is something we discussed in this GSoC cycle, to have extra
BTF generated from the scraper scripts, not in the kernel's BTF, but
with perf, but then that would work for ABIs, and not for internal
kernel stuff, where, as you rightly described, CO-RE would come to save
the day.

Having some extra BTF info, not necessarily in a kernel module or things
like that, but from a debuginfo server, in an extra rpm package,
whatever, but accessible via a build-id lookup somehow would be a great
improvement.
 
> But it seems to me that what both these problems suggest is that it
> would be nice to more systematically represent numeric macro values such
> that they would be more easily available. I talked a bit about this a
> few years back at Plumbers for macros as a whole, but I wonder if a
> tweak to pahole that does something like

We need to keep talking about this to make it a reality :-)
 
> 1. check if a macro has a valid numeric representation;
> 2. if so, convert it to a singleton anonymous BTF enumerated type that
> will not clash with the real macro name (so is safe to use when headers
> containing that macro are also included)
 
> This would allow BPF program writers to reference macro-defined flag
> values and get the CO-RE benefits they get from enums and presence in a
> generated vmlinux.h. It might also help here for beautify where you
> could establish name-value mappings for things like arch-specific errnos.
> 
> We've been talking about having a loadable module of vmlinux BTF extras,
> so it seems like numeric macro representations would be helpful there
> too. What do you think? Thanks!

See above, maybe we don't really need to have it as a loadable kernel
module.

- Arnaldo
 
> Alan
> 
> 
> > 	I plan to work on the weekend to plugging his latest series on
> > top of these patches so that we can get it merged in the next merge
> > window.

No work in the weekend, but took most of today :-)

I'm pushing what I have to the tmp.perf-tools-next branch at:

https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git

> > 	Any comment/test is more than welcome,
> > 
> > - Arnaldo
> > 
> > Arnaldo Carvalho de Melo (2):
> >   perf trace augmented_syscalls.bpf: Move the renameat augmenter to
> >     renameat2, temporarily
> >   perf trace: Use a common encoding for augmented arguments, with size +
> >     error + payload
> > 
> >  tools/perf/trace/beauty/perf_event_open.c     |   2 +-
> >  tools/perf/trace/beauty/sockaddr.c            |   2 +-
> >  tools/perf/trace/beauty/timespec.c            |   2 +-
> >  .../bpf_skel/augmented_raw_syscalls.bpf.c     | 108 +++++++++++-------
> >  4 files changed, 68 insertions(+), 46 deletions(-)
> >