Date:   Sun, 15 Dec 2019 16:30:54 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andriin@...com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org, ast@...com,
        daniel@...earbox.net, andrii.nakryiko@...il.com, kernel-team@...com
Subject: Re: [PATCH v4 bpf-next 00/17] Add code-generated BPF object skeleton
 support

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 ?

Powered by blists - more mailing lists