Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 13 Dec 2019 09:47:05 -0800
From:   Jakub Kicinski <jakub.kicinski@netronome.com>
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc:     Alexei Starovoitov <alexei.starovoitov@gmail.com>,
        Stanislav Fomichev <sdf@fomichev.me>,
        Andrii Nakryiko <andriin@fb.com>,
        LKML <linux-kernel@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        Networking <netdev@vger.kernel.org>,
        Alexei Starovoitov <ast@fb.com>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next 11/15] bpftool: add skeleton codegen command
Message-ID: <20191213094705.486101a0@cakuba.netronome.com>
In-Reply-To: <CAEf4BzaG95dxgSBSm7m8c3gJ-XeL97=N4srS5fR7JRfcjaMwTw@mail.gmail.com>
References: <CAEf4Bzb+3b-ypP8YJVA=ogQgp1KXx2xPConOswA0EiGXsmfJow@mail.gmail.com>
        <20191211191518.GD3105713@mini-arch>
        <CAEf4BzYofFFjSAO3O-G37qyeVHE6FACex=yermt8bF8mXksh8g@mail.gmail.com>
        <20191211200924.GE3105713@mini-arch>
        <CAEf4BzaE0Q7LnPOa90p1RX9qSbOA_8hkT=6=7peP9C88ErRumQ@mail.gmail.com>
        <20191212025735.GK3105713@mini-arch>
        <CAEf4BzY2KHK4h5e40QgGt4GzJ6c+rm-vtbyEdM41vUSqcs=txA@mail.gmail.com>
        <20191212162953.GM3105713@mini-arch>
        <CAEf4BzYJHvuFbBM-xvCCsEa+Pg-bG1tprGMbCDtsbGHdv7KspA@mail.gmail.com>
        <20191212104334.222552a1@cakuba.netronome.com>
        <20191212195415.ubnuypco536rp6mu@ast-mbp.dhcp.thefacebook.com>
        <20191212122115.612bb13b@cakuba.netronome.com>
        <CAEf4BzaG95dxgSBSm7m8c3gJ-XeL97=N4srS5fR7JRfcjaMwTw@mail.gmail.com>
Organization: Netronome Systems, Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 12 Dec 2019 22:48:10 -0800, Andrii Nakryiko wrote:
> > 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.  
> 
> Skeleton is auto-generated code, it's not supposed to be tweaked or
> "adjusted" by hand. 

Obviously not, I said adjusting the codegen tool, not the output.

> Because next time you do tiny change to your BPF
> source code (just swap order of two global variables), skeleton
> changes. If someone is not satisfied with the way skeleton generation
> looks like, they should propose changes and contribute to common
> algorithm. Or, of course, they can just go and re-implement it on
> their own, if struct bpf_object_skeleton suits them still (which is
> what libbpf works with). Then they can do it in Python, Go, Scala,
> Java, Perl, whatnot. But somehow I doubt anyone would want to do that.
> 
> > And maybe one day we do have Python/Go/whatever bindings, and we can
> > convert the skel tool to a higher level language with modern templating.  
> 
> Because high-level implementation is going to be so much simpler and
> shorter, really? Is it that complicated in C right now? What's the
> real benefit of waiting to be able to do it in "higher level" language
> beyond being the contrarian? 

I did not say wait, I said do C and convert to something else once easy.
You really gotta read responses more carefully :/

> Apart from \n\ (which is mostly hidden
> from view), I don't think high-level templates are going to be much
> more clean.
> 
> > > We cannot and should not adopt kernel-like ABI guarantees to user space
> > > code. It will paralyze the development.  
> >
> > Discussion for another time :)  
> 
> If this "experimental" disclaimer is a real blocker for all of this, I
> don't mind making it a public API right now. bpf_object_skeleton is
> already designed to be backward/forward compatible with size of struct
> itself and all the sub-structs recorded during initialization. I
> didn't mean to create impression like this whole approach is so raw
> and untried that it will most certainly break and we are still unsure
> about it. It's not and it certainly improves set up code for
> real-world applications. We might need to add some extra option here
> and there, but the stuff that's there already will stay as is.

As explained the experimental disclaimer is fairly useless and it gives
people precedent for maybe not caring as hard as they should about
ironing the details out before sending code upstream.

I think we can just add a switch or option for improved generation when
needed. You already check there are not extra trailing arguments so we
should be good.

> Would moving all the skeleton-related stuff into libbpf.h and
> "stabilizing" it make all this more tolerable for you?

I think I'm too tired if this to have an option any more.