Re: [PATCH v4 bpf-next 00/17] Add code-generated BPF object skeleton support

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

On Sun, Dec 15, 2019 at 8:45 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sun, Dec 15, 2019 at 06:01:16PM -0800, Andrii Nakryiko wrote:
> > On Sun, Dec 15, 2019 at 4:30 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Fri, Dec 13, 2019 at 05:43:24PM -0800, Andrii Nakryiko wrote:
> > > > This patch set introduces an alternative and complimentary to existing libbpf
> > > > API interface for working with BPF objects, maps, programs, and global data
> > > > from userspace side. This approach is relying on code generation. bpftool
> > > > produces a struct (a.k.a. skeleton) tailored and specific to provided BPF
> > > > object file. It includes hard-coded fields and data structures for every map,
> > > > program, link, and global data present.
> > > >
> > > > Altogether this approach significantly reduces amount of userspace boilerplate
> > > > code required to open, load, attach, and work with BPF objects. It improves
> > > > attach/detach story, by providing pre-allocated space for bpf_links, and
> > > > ensuring they are properly detached on shutdown. It allows to do away with by
> > > > name/title lookups of maps and programs, because libbpf's skeleton API, in
> > > > conjunction with generated code from bpftool, is filling in hard-coded fields
> > > > with actual pointers to corresponding struct bpf_map/bpf_program/bpf_link.
> > > >
> > > > Also, thanks to BPF array mmap() support, working with global data (variables)
> > > > from userspace is now as natural as it is from BPF side: each variable is just
> > > > a struct field inside skeleton struct. Furthermore, this allows to have
> > > > a natural way for userspace to pre-initialize global data (including
> > > > previously impossible to initialize .rodata) by just assigning values to the
> > > > same per-variable fields. Libbpf will carefully take into account this
> > > > initialization image, will use it to pre-populate BPF maps at creation time,
> > > > and will re-mmap() BPF map's contents at exactly the same userspace memory
> > > > address such that it can continue working with all the same pointers without
> > > > any interruptions. If kernel doesn't support mmap(), global data will still be
> > > > successfully initialized, but after map creation global data structures inside
> > > > skeleton will be NULL-ed out. This allows userspace application to gracefully
> > > > handle lack of mmap() support, if necessary.
> > > >
> > > > A bunch of selftests are also converted to using skeletons, demonstrating
> > > > significant simplification of userspace part of test and reduction in amount
> > > > of code necessary.
> > > >
> > > > v3->v4:
> > > > - add OPTS_VALID check to btf_dump__emit_type_decl (Alexei);
> > > > - expose skeleton as LIBBPF_API functions (Alexei);
> > > > - copyright clean up, update internal map init refactor (Alexei);
> > >
> > > Applied. Thanks.
> > >
> > > I really liked how much more concise test_fentry_fexit() test has become.
> > > I also liked how renaming global variable s/test1_result/_test1_result/
> > > in bpf program became a build time error for user space part:
> > > ../prog_tests/fentry_fexit.c:49:35: error: ‘struct fentry_test__bss’ has no member named ‘test1_result’; did you mean ‘_test1_result’?
> > >   printf("%lld\n", fentry_skel->bss->test1_result);
> > > Working with global variables is so much easier now.
> > >
> > > I'd like you to consider additional feature request.
> > > The following error:
> > > -BPF_EMBED_OBJ(fentry, "fentry_test.o");
> > > -BPF_EMBED_OBJ(fexit, "fexit_test.o");
> > > +BPF_EMBED_OBJ(fexit, "fentry_test.o");
> > > +BPF_EMBED_OBJ(fentry, "fexit_test.o");
> > > will not be caught.
> > > I think skeleton should get smarter somehow to catch that too.
> > >
> > > One option would be to do BPF_EMBED_OBJ() as part of *.skel.h but then
> > > accessing the same embedded .o from multiple tests will not be possible and
> > > what stacktrace_build_id.c and stacktrace_build_id_nmi.c are doing won't work
> > > anymore. Some sort of build-id/sha1 of .o can work, but it will be caught
> > > in run-time. I think build time would be better.
> > > May be generate new macro in skel.h that user space can instantiate
> > > instead of using common BPF_EMBED_OBJ ?
> > >
> >
> > All those issues are actually very easy to solve. As part of bla.skel.h:
> >
> > ....
> >
> > #ifndef __BLA__SKEL_EMBEDDED
> > #define __BLA__SKEL_EMBEDDED
> > BPF_EMBED_OBJ(<some_identifier>, <path_to_.o>);
> > #endif
> >
> > extern struct bpf_embed_data <some_identifier>_embed;
> >
> > /* we can have a variant of bla__create_skeleton() that just uses
> > above <some_identifier>_embed */
> >
> > ....
> >
> >
> > That seems to solve all the problems you mentioned. But it creates the
> > problem of knowing/specifying <some_identifier> and <path_to_.o>.
> > While we can "dictate" <some_identifier> (e.g., based on object file
> > name), <path_to_.o> sometimes might need to be overridden, depending
> > on specifics of build system.
> >
> >
> > But I guess we can follow convention-driven way, and in addition to
> > above do something like:
> >
> >
> > #ifndef __BLA__SKEL__OBJ_PATH
> > #define __BLA__SKEL__OBJ_PATH "<whatever path was provided to bpftool
> > to generate skeleton>"
> > #endif
> >
> >
> > /* then just use __BLA__SKEL__OBJ_PATH for BPF_EMBED_OBJ,
> >  * which user can override before including skeleton on userspace side
> >  */
> >
> > WDYT?
>
> Another idea...
> How about __weak definition of BPF_EMBED_OBJ ?
> via generated macro inside .skel.h ?
> With another method like test_pkt_access__open_and_load() that
> doesn't take _embed ?
> Then BPF_EMBED_OBJ_DECLARE() can be removed?
>

So I like the idea of not having to do BPF_EMBED_OBJ. It can't be done
cleanly with BPF_EMBED_OBJ, but skeleton can auto-generate array with
embedded contents of object file used for skeleton generation. That
will solve all the issues of keeping skeleton and object file contents
in sync. I'll post a follow up patch with that change.