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

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Thu, 12 Dec 2019 13:28:00 -0800, Alexei Starovoitov wrote:
> On Thu, Dec 12, 2019 at 12:21:15PM -0800, Jakub Kicinski wrote:
> > > > It can be a separate tool like
> > > >   libbpf-skel-gen or libbpf-c-skel or something, distributed with libbpf.
> > > >   That way you can actually soften the backward compat. In case people
> > > >   become dependent on it they can carry that little tool on their own.    
> > > 
> > > Jakub,
> > > 
> > > Could you please consider Andrii's reply to your comment from two days ago:
> > > https://lore.kernel.org/bpf/CAEf4BzbeZbmCTOOo2uQXjm0GL0WDu7aLN6fdUk18Nv2g0kfwVg@mail.gmail.com/
> > > "we are trying to make users lives easier by having major distributions
> > > distribute bpftool and libbpf properly. Adding extra binaries to
> > > distribute around doesn't seem to be easing any of users pains."  
> > 
> > Last time we argued I heard how GH makes libbpf packaging easier.
> > Only to have that dis-proven once the people in Europe who do distro
> > packaging woke up:
> > 
> > https://lkml.org/lkml/2019/12/5/101
> > https://lkml.org/lkml/2019/12/5/312  
> 
> I think you missed the point of these two comments. It was about packaging
> bpftool and libbpf together. Regardless how bpftool is packaged. I still
> strongly suggest to use github/libbpf to package libbpf. It's something that is
> actually tested whereas libbpf in the kernel tree has unit test coverage only.

I disagree.

> > > My opinion is the following.
> > > bpftool is necessary to write bpf programs already. It's necessary to produce
> > > vmlinux.h for bpf programs to include it. It's part of build process. I can
> > > relate to Stan's complains that he needs to update clang and pahole. He missed
> > > the fact that he needs to update bpftool too if he wants to use all features of
> > > CO-RE. Same thing for skeleton generation. If people need to run the latest
> > > selftest/bpf on the latest kernel they need to upgrade to the latest clang,
> > > pahole, libbpf, bpftool. Nothing new here.  
> > 
> > They have to update libbpf, so why can't the code gen tool be part of
> > libbpf?   
> 
> I'm not sure why two answers were not enough.
> No idea how to answer this question differently for the third time.

I'm just presenting what I consider to be a cleaner solution.

> > > Backwards compat is the same concern for skeleton generation and for vmlinux.h
> > > generation. Obviously no one wants to introduce something that will keep
> > > changing. Is vmlinux.h generation stable? I like to believe so. Same with
> > > skeleton. I wouldn't want to see it changing, but in both cases such chance
> > > exists.   
> > 
> > vmlinux.h is pretty stable, there isn't much wiggle room there.  
> 
> Do you have experience working with vmlinux.h? I bet the answer is no.
> While we have and identified few things that needs improvement.
> They require vmlinux.h to be generated differently.
> 
> > It's more of a conversion tool, if you will.
> > 
> > Skeleton OTOH is supposed to make people's lives easier, so it's a
> > completely different beast. It should be malleable so that users can
> > improve and hack on it. Baking it into as system tool is counter
> > productive. Users should be able to grab the skel tool single-file
> > source and adjust for their project's needs. Distributing your own copy
> > of bpftool because you want to adjust skel is a heavy lift.  
> 
> Adjust generator for their custom needs? essentially fork it for
> private use? I'd rather prevent such possibility.
> When people start using it I'd prefer they come back to this mailing
> list with patches than do 'easy fork'.
> 
> > > Now consider if vmlinux.h and skeleton generation is split out of bpftool into
> > > new tool. Effectively it would mean a fork of bpftool. Two binaries doing bpf
> > > elf file processing without clear distinction between them is going to be very
> > > confusing.  
> > 
> > To be clear I'm suggesting skel gen is a separate tool, vmlinux and
> > Quentin's header gen work on the running system, they are not pure
> > build env tools.  
> 
> You meant to say Andrii's header generator that is based on Quentin's man page
> generator. Its output bpf_helper_defs.h makes sense as a part of libbpf
> package. The generator script itself doesn't need to be included with any package.
> bpftool vmlinux gen consumes vmlinux elf files and is a part of the build.
> bpftool skeleton gen consumes bpf elf files and is a part of the same build.

I said what I meant to say tools/bpf/bpftool/feature.c