Re: [PATCH bpf-next 2/2] perf: stop using deprecated bpf__object_next() API

[Jiri Olsa <jolsa@redhat.com>]

On Tue, Dec 21, 2021 at 01:58:14PM -0800, Andrii Nakryiko wrote:
> On Tue, Dec 21, 2021 at 12:23 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Thu, Dec 16, 2021 at 02:21:08PM -0800, Christy Lee wrote:
> > > bpf__object_next is deprecated, track bpf_objects directly in
> > > perf instead.
> > >
> > > Signed-off-by: Christy Lee <christylee@fb.com>
> > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > ---
> > >  tools/perf/util/bpf-loader.c | 72 +++++++++++++++++++++++++++---------
> > >  tools/perf/util/bpf-loader.h |  1 +
> > >  2 files changed, 55 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> > > index 528aeb0ab79d..9e3988fd719a 100644
> > > --- a/tools/perf/util/bpf-loader.c
> > > +++ b/tools/perf/util/bpf-loader.c
> > > @@ -29,9 +29,6 @@
> > >
> > >  #include <internal/xyarray.h>
> > >
> > > -/* temporarily disable libbpf deprecation warnings */
> > > -#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> > > -
> > >  static int libbpf_perf_print(enum libbpf_print_level level __attribute__((unused)),
> > >                             const char *fmt, va_list args)
> > >  {
> > > @@ -49,6 +46,36 @@ struct bpf_prog_priv {
> > >       int *type_mapping;
> > >  };
> > >
> > > +struct bpf_perf_object {
> > > +     struct bpf_object *obj;
> > > +     struct list_head list;
> > > +};
> > > +
> > > +static LIST_HEAD(bpf_objects_list);
> >
> > hum, so this duplicates libbpf's bpf_objects_list,
> > how do objects get on this list?
> 
> yep, this list needs to be updated on perf side each time
> bpf_object__open() (and any variant of open) is called.
> 
> >
> > could you please put more comments in changelog
> > and share how you tested this?
> 
> I actually have no idea how to test this as well, can you please share
> some ideas?
> 

I don't use it, I just know it's there.. that's why I asked ;-)

it's possible to specify bpf program on the perf command line
to be attached to event, like:

      # cat tools/perf/examples/bpf/hello.c
      #include <stdio.h>
    
      int syscall_enter(openat)(void *args)
      {
              puts("Hello, world\n");
              return 0;
      }
    
      license(GPL);
      #
      # perf trace -e openat,tools/perf/examples/bpf/hello.c cat /etc/passwd > /dev/null
         0.016 (         ): __bpf_stdout__:Hello, world
         0.018 ( 0.010 ms): cat/9079 openat(dfd: CWD, filename: /etc/ld.so.cache, flags: CLOEXEC) = 3
         0.057 (         ): __bpf_stdout__:Hello, world
         0.059 ( 0.011 ms): cat/9079 openat(dfd: CWD, filename: /lib64/libc.so.6, flags: CLOEXEC) = 3
         0.417 (         ): __bpf_stdout__:Hello, world
         0.419 ( 0.009 ms): cat/9079 openat(dfd: CWD, filename: /etc/passwd) = 3
      #

I took that example from commit message

> 
> BTW, while we are at it, Jiri, do you have any good ideas on how to
> remove perf's usage of bpf_program__set_priv() and
> bpf_program__set_prep() APIs in perf code base? These APIs are
> deprecated as well, but seems like perf relies on them pretty heavily.
> What would be the best way to stop using them?
> 
> For set_priv(), I think it should be totally fine to maintain a
> separate lookup hash table by `struct bpf_program *` or its name, that
> shouldn't be hard.

ok, so there's no alternative api for this one then

> 
> But for set_prep(), what does perf use it for? I assume for cloning
> BPF programs, right? Anything else besides that? If it's just for
> cloning, libbpf provides bpf_program__insns() API to get access to
> underlying bpf_insn array, do you think it's possible to switch perf
> to that instead?

look like it's used to generate prologs for programs:
  a08357d8dc7d perf bpf: Generate prologue for BPF programs

the author Wang Nan haven't touched that for some time,
I'm cc-ing other folks that were involved..  Arnaldo? ;-)

if nobody volunteers, I can check on that

jirka