Re: [PATCH v1 6/7] perf trace: 5sec fix libbpf 1.0+ compatibility

[Arnaldo Carvalho de Melo]

Em Thu, Nov 03, 2022 at 03:01:21PM -0700, Ian Rogers escreveu:
> On Thu, Nov 3, 2022 at 2:35 PM Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> wrote:
> >
> > With this:
> >
> > diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> > index d3d3c13a9f25b55c..067a6e56eeacc9fc 100644
> > --- a/tools/perf/Makefile.config
> > +++ b/tools/perf/Makefile.config
> > @@ -1239,7 +1239,7 @@ includedir = $(abspath $(prefix)/$(includedir_relative))
> >  mandir = share/man
> >  infodir = share/info
> >  perfexecdir = libexec/perf-core
> > -perf_include_dir = lib/perf/include
> > +perf_include_dir = /usr/include
> >  perf_examples_dir = lib/perf/examples
> >  sharedir = $(prefix)/share
> >  template_dir = share/perf-core/templates
> > diff --git a/tools/perf/examples/bpf/augmented_raw_syscalls.c b/tools/perf/examples/bpf/augmented_raw_syscalls.c
> > index 13c72fd602c307e4..98a2731c011339ba 100644
> > --- a/tools/perf/examples/bpf/augmented_raw_syscalls.c
> > +++ b/tools/perf/examples/bpf/augmented_raw_syscalls.c
> > @@ -17,8 +17,9 @@
> >  #include <linux/bpf.h>
> >  #include <bpf/bpf_helpers.h>
> >  #include <linux/limits.h>
> > -#include <stdbool.h>
> > -#include <sys/socket.h>
> > +
> > +typedef char bool;
> > +typedef int pid_t;
> >
> >  /* bpf-output associated map */
> >  struct __augmented_syscalls__ {
> > @@ -94,6 +95,30 @@ struct pids_filtered {
> >         __uint(max_entries, 64);
> >  } pids_filtered SEC(".maps");
> >
> > +/*
> > + * Desired design of maximum size and alignment (see RFC2553)
> > + */
> > +#define _K_SS_MAXSIZE   128     /* Implementation specific max size */
> > +
> > +typedef unsigned short sa_family_t;
> > +
> > +/*
> > + * The definition uses anonymous union and struct in order to control the
> > + * default alignment.
> > + */
> > +struct sockaddr_storage {
> > +        union {
> > +                struct {
> > +                        sa_family_t    ss_family; /* address family */
> > +                        /* Following field(s) are implementation specific */
> > +                        char __data[_K_SS_MAXSIZE - sizeof(unsigned short)];
> > +                                /* space to achieve desired size, */
> > +                                /* _SS_MAXSIZE value minus size of ss_family */
> > +                };
> > +                void *__align; /* implementation specific desired alignment */
> > +        };
> > +};
> > +
> >  struct augmented_args_payload {
> >         struct syscall_enter_args args;
> >         union {
> > diff --git a/tools/perf/examples/bpf/empty.c b/tools/perf/examples/bpf/empty.c
> > index 3e296c0c53d7d8e2..e4872c48a484f218 100644
> > --- a/tools/perf/examples/bpf/empty.c
> > +++ b/tools/perf/examples/bpf/empty.c
> > @@ -7,6 +7,6 @@ struct syscall_enter_args;
> >  SEC("raw_syscalls:sys_enter")
> >  int sys_enter(struct syscall_enter_args *args)
> >  {
> > -       return 0;
> > +       return 1;
> >  }
> >  char _license[] SEC("license") = "GPL";
> > diff --git a/tools/perf/util/llvm-utils.c b/tools/perf/util/llvm-utils.c
> > index 2dc7970074196ca8..a5cac85783d8711f 100644
> > --- a/tools/perf/util/llvm-utils.c
> > +++ b/tools/perf/util/llvm-utils.c
> > @@ -495,7 +495,7 @@ int llvm__compile_bpf(const char *path, void **p_obj_buf,
> >
> >         snprintf(linux_version_code_str, sizeof(linux_version_code_str),
> >                  "0x%x", kernel_version);
> > -       if (asprintf(&perf_bpf_include_opts, "-I%s/bpf", perf_include_dir) < 0)
> > +       if (asprintf(&perf_bpf_include_opts, "-I%s/", perf_include_dir) < 0)
> >                 goto errout;
> >         force_set_env("NR_CPUS", nr_cpus_avail_str);
> >         force_set_env("LINUX_VERSION_CODE", linux_version_code_str);
> >
> >
> > The connect calls gets served, tomorrow, if you don't beat me I'll apply
> > the series after adding these minimal changes so that we have this
> > working with libbpf 1.0 and then we can move from there, with a switch
> > to a BPF skel, simplest things first, then deal with faults at pointer
> > payload copy, which is another avenue, AFAIK with solutions already.
> 
> So I was trying to be clean and not redefine too much. My clang
> command line was:
> 
> clang -target bpf -O2 -g -c -I/usr/include/x86_64-linux-gnu
> -D__NR_CPUS__=16 -D__x86_64__=1

Sure, I haven't even checked why that is needed, maybe its not anymore.

I just tried the first hunch about the header files, those other
variables maybe were needed long ago, maybe its just more stuff to trow
out in the direction of doing it the modern way, BPF skels.
 
> It'd be nice to just drop the need for __NR_CPUS__ and have it be
> dynamic, the skeleton approach would require this. Not sure how to
> workaround the x86 define and path :-/ Perhaps send out your changes
> for review and I can look at and test them.

Done deal, early tomorrow I'll send it and wait for your review.

- Arnaldo