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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

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?