Re: [PATCH v2 bpf-next 09/16] libbpf: Support for fd_idx

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Mon, Apr 26, 2021 at 7:53 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Apr 26, 2021 at 10:14:45AM -0700, Andrii Nakryiko wrote:
> > On Thu, Apr 22, 2021 at 5:27 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > From: Alexei Starovoitov <ast@kernel.org>
> > >
> > > Add support for FD_IDX make libbpf prefer that approach to loading programs.
> > >
> > > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > > ---
> > >  tools/lib/bpf/bpf.c             |  1 +
> > >  tools/lib/bpf/libbpf.c          | 70 +++++++++++++++++++++++++++++----
> > >  tools/lib/bpf/libbpf_internal.h |  1 +
> > >  3 files changed, 65 insertions(+), 7 deletions(-)
> > >
> >

[...]

> > >         for (i = 0; i < obj->nr_programs; i++) {
> > >                 prog = &obj->programs[i];
> > >                 if (prog_is_subprog(obj, prog))
> > > @@ -7256,10 +7308,14 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
> > >                         continue;
> > >                 }
> > >                 prog->log_level |= log_level;
> > > +               prog->fd_array = fd_array;
> >
> > you are not freeing this memory on success, as far as I can see.
>
> hmm. there is free on success below.

right, my bad, I somehow understood as if it was only for error case

>
> > And
> > given multiple programs are sharing fd_array, it's a bit problematic
> > for prog to have fd_array. This is per-object properly, so let's add
> > it at bpf_object level and clean it up on bpf_object__close()? And by
> > assigning to obj->fd_array at malloc() site, you won't need to do all
> > the error-handling free()s below.
>
> hmm. that sounds worse.
> why add another 8 byte to bpf_object that won't be used
> until this last step of bpf_object__load_progs.
> And only for the duration of this loading.
> It's cheaper to have this alloc here with two free()s below.

So if you care about extra 8 bytes, then it's even more efficient to
have just one obj->fd_array rather than N prog->fd_array, no? And it's
also not very clean that prog->fd_array will have a dangling pointer
to deallocated memory after bpf_object__load_progs().

But that brings the entire question of why use fd_array at all here?
Commit description doesn't explain why libbpf has to use fd_array and
why it should be preferred. What are the advantages justifying added
complexity and extra memory allocation/clean up? It also reduces test
coverage of the "old ways" that offer the same capabilities. I think
this should be part of the commit description, if we agree that
fd_array has to be used outside of the auto-generated loader program.


>
> >
> > >                 err = bpf_program__load(prog, obj->license, obj->kern_version);
> > > -               if (err)
> > > +               if (err) {
> > > +                       free(fd_array);
> > >                         return err;
> > > +               }
> > >         }
> > > +       free(fd_array);
> > >         return 0;
> > >  }
> > >
> > > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > > index 6017902c687e..9114c7085f2a 100644
> > > --- a/tools/lib/bpf/libbpf_internal.h
> > > +++ b/tools/lib/bpf/libbpf_internal.h
> > > @@ -204,6 +204,7 @@ struct bpf_prog_load_params {
> > >         __u32 log_level;
> > >         char *log_buf;
> > >         size_t log_buf_sz;
> > > +       int *fd_array;
> > >  };
> > >
> > >  int libbpf__bpf_prog_load(const struct bpf_prog_load_params *load_attr);
> > > --
> > > 2.30.2
> > >
>
> --