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

[Ian Rogers]

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

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.

Thanks,
Ian

> Thanks,
>
> - Arnaldo
>
> [root@quaco perf]# perf trace -e /home/acme/git/perf/tools/perf/examples/bpf/augmented_raw_syscalls.c,connect* --max-events 10
>      0.000 ( 0.074 ms): fetchmail/175578 connect(fd: 3, uservaddr: { .family: LOCAL, path: /run/systemd/resolve/io.systemd.Resolve }, addrlen: 42) = 0
>      0.397 ( 0.011 ms): systemd-resolv/973 connect(fd: 23, uservaddr: { .family: INET, port: 53, addr: 127.0.0.1 }, addrlen: 16) = 0
>      0.532 ( 0.006 ms): systemd-resolv/973 connect(fd: 24, uservaddr: { .family: INET, port: 53, addr: 127.0.0.1 }, addrlen: 16) = 0
>      0.910 ( 0.007 ms): systemd-resolv/973 connect(fd: 23, uservaddr: { .family: INET, port: 53, addr: 127.0.0.1 }, addrlen: 16) = 0
>      0.962 ( 0.003 ms): systemd-resolv/973 connect(fd: 24, uservaddr: { .family: INET, port: 53, addr: 127.0.0.1 }, addrlen: 16) = 0
>      1.337 ( 0.007 ms): fetchmail/175578 connect(fd: 3, uservaddr: { .family: INET, port: 0, addr: 67.195.176.151 }, addrlen: 16) = 0
>      1.348 ( 0.014 ms): fetchmail/175578 connect(fd: 3, uservaddr: { .family: UNSPEC }, addrlen: 16)           = 0
>      1.363 ( 0.003 ms): fetchmail/175578 connect(fd: 3, uservaddr: { .family: INET, port: 0, addr: 76.13.33.33 }, addrlen: 16) = 0
>      1.527 ( 0.014 ms): fetchmail/175578 connect(fd: 3, uservaddr: { .family: LOCAL, path: /run/systemd/resolve/io.systemd.Resolve }, addrlen: 42) = 0
>      1.726 ( 0.008 ms): systemd-resolv/973 connect(fd: 23, uservaddr: { .family: INET, port: 53, addr: 127.0.0.1 }, addrlen: 16) = 0
> [root@quaco perf]#