Re: [PATCH bpf-next 11/15] bpftool: add skeleton codegen command

[Andrii Nakryiko]

On Thu, Dec 12, 2019 at 1:46 PM Stanislav Fomichev <sdf@xxxxxxxxxxx> wrote:
>
> On 12/12, Alexei Starovoitov wrote:
> > On Thu, Dec 12, 2019 at 10:43:34AM -0800, Jakub Kicinski wrote:
> > One more point from Stan's email:
> >
> > > You can replace "our build system" with some other project you care about,
> > > like systemd. They'd have the same problem with vendoring in recent enough
> >
> > we've been working with systemd folks for ~8 month to integrate libbpf into
> > their build that is using meson build system and their CI that is github based.
> > So we're well aware about systemd requirements for libbpf and friends.
> Just curious (searching on systemd github for bpftool/libbpf doesn't
> show up any code/issues): are you saying that there will be another ~8 months
> to bring in bpftool or that it's already being worked on as part of
> libbpf integration?
>
> > > bpftool or waiting for every distro to do it. And all this work is
> > > because you think that doing:
> > >
> > >        my_obj->rodata->my_var = 123;
> > >
> > > Is easier / more type safe than doing:
> > >        int *my_var = bpf_object__rodata_lookup(obj, "my_var");
> > >        *my_var = 123;
> >
> > Stan, you conveniently skipped error checking. It should have been:
> >     int *my_var = bpf_object__rodata_lookup(obj, "my_var");
> >     if (IS_ERROR_NULL(my_var))
> >         goto out_cleanup;
> >      *my_var = 123;
> Yeah, but you have a choice, right? You can choose to check the error
> and support old programs that don't export some global var and a new
> program that has it. Or you can skip the error checks and rely on null
> deref crash which is sometimes an option.

So your point is that between

A. Skeleton way:

my_obj->rodata->my_var = 123;

and

B. look up way:

int *my_var = bpf_object__rodata_lookup(obj, "my_var");
*my_var = 123;

B is **better**, because **if you drop error checking** (which
apparently is a choice within "Google distro") then it comes really
close to succinctness of skeleton, is that right?

Let's follow this through to the end with two pretty typical situations.

1. Someone renames my_var into my_var1.
  A. Skeleton: compilation error.
  B. Lookup: NULL deref, core dump. If there is helpful in-program
infra, it will dump stack trace and it should be pretty easy to
pinpoint which variable look up failed (provided binary is built with
debug info). If not - gdb is your friend, you'll figure this out
pretty quickly.

2. Someone changes the type of my_var from u64 to u32.
  A. Skeleton: compiler will warn on too large constant assignment,
but otherwise it's C, baby. But at least we are only going to write
(potentially truncated) 4 bytes.
  B. Lookup: we are happily overwriting 4 bytes of some other variable
(best case -- just padding), without anyone realizing this. Good if we
have exhaustive correctness testing for program logic.

I do think there is a clear winner, it just seems like we disagree which one.

>
> (might be not relevant with the introduction of EMBED_FILE which you
> seem to be using more and more; ideally, we still would like to be able to
> distribute bpf.o and userspace binary separately).

I don't even know what BPF_EMBED_OBJ has to do with skeleton, tbh.
BPF_EMBED_OBJ is just one of the ways to get contents of BPF object
file. You can just as well instantiate skeleton from separate .o file,
if you read its content in memory and create struct bpf_embed_data
with pointer to it (or just mmap() it, which would be even easier). If
this is typical case, we can generate an extra function to instantiate
skeleton from file, of course.

>
> > Take a look at Andrii's patch 13/15:
> > 5 files changed, 149 insertions(+), 249 deletions(-)
> > Those are simple selftests, yet code removal is huge. Bigger project benefits
> > even more.
> Excluding fentry/fexit tests (where new find_program_by_title+attach
> helper and mmap might help), it looks like the majority of those gains come
> from the fact that the patch in question doesn't do any error checking.
> You can drop all the CHECK() stuff for existing
> find_map_by_name/find_prog_by_name instead and get the same gains.

See above. The fact we are not doing error checking with skeleton is
because it can't fail. If programmer screwed up name of variable,
program, or variable -- that's compilation error! That's the reason we
don't check errors, not because it's a cowboy-style "You can choose to
check the error" approach.

>
> [as usual, feel free to ignore me, I don't want to keep flaming about
> it, but it's hard not to reply]

likewise