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 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?