Message-ID: <CACAyw9_Yr=pmvCRYsVHoQBrH7qBwmcaXZezmqafwJTxaCmDf6A@mail.gmail.com>
Date:   Mon, 17 Jun 2019 10:07:13 +0100
From:   Lorenz Bauer <lmb@...udflare.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [RFC PATCH bpf-next 6/8] libbpf: allow specifying map definitions
 using BTF

On Thu, 6 Jun 2019 at 23:35, Andrii Nakryiko <andrii.nakryiko@...il.com> wrote:
>
> On Thu, Jun 6, 2019 at 9:43 AM Lorenz Bauer <lmb@...udflare.com> wrote:
> >
> > Thanks for sending this RFC! For me, the biggest draw is that map-in-map
> > would be so much nicer to use, plus automatic dumping of map values.
> >
> > Others on the thread have raised this point already: not everybody lives
> > on the bleeding edge or can control all of their dependencies. To me this means
> > that having a good compatibility story is paramount. I'd like to have very clear
> > rules how the presence / absence of fields is handled.
>
> I think that discussion was more about selftests being switched to
> BTF-defined maps rather than BPF users having to switch to latest
> compiler. struct bpf_map_def is still supported for those who can't
> use clang that supports BTF_KIND_VAR/BTF_KIND_DATASEC.
> So I don't think this enforces anyone to switch compiler, but
> certainly incentivizes them :)
>
> >
> > For example:
> > - Fields that are present but not understood are an error. This makes
> > sense because
> >   the user can simply omit the field in their definition if they do
> > not use it. It's also necessary
> >   to preserve the freedom to add new fields in the future without
> > risking user breakage.
>
> So you are arguing for strict-by-default behavior. It's fine by me,
> but exactly that strict-by-default behavior is the problem with BTF
> extensivility, that you care a lot about. You are advocating for
> skipping unknown BTF types (if it was possible), which is directly
> opposite to strict-by-default behavior. I have no strong preference
> here, but given amount of problem (and how many times we missed this
> problem in the past) w/ introducing new BTF feature and then
> forgetting about doing something for older kernels, kind of makes me
> lean towards skip-and-log behavior. But I'm happy to support both
> (through flags) w/ strict by default.

In my mind, BPF loaders should be able to pass through BTF to the kernel
as a binary blob as much as possible. That's why I want the format to
be "self describing". Compatibility then becomes a question of: what
feature are you using on which kernel. The kernel itself can then still be
strict-by-default or what have you.

>
> > - If libbpf adds support for a new field, it must be optional. Seems
> > like this is what current
> >   map extensions already do, so maybe a no-brainer.
>
> Yeah, of course.
>
> >
> > Somewhat related to this: I really wish that BTF was self-describing,
> > e.g. possible
> > to parse without understanding all types. I mentioned this in another
> > thread of yours,
> > but the more we add features where BTF is required the more important it becomes
> > IMO.
>
> I relate, but have no new and better solution than previously
> discussed :) We should try to add new stuff to .BTF.ext as much as
> possible, which is self-describing.
>
> >
> > Finally, some nits inline:
> >
> > On Fri, 31 May 2019 at 21:22, Andrii Nakryiko <andriin@...com> wrote:
> > >
> > > The outline of the new map definition (short, BTF-defined maps) is as follows:
> > > 1. All the maps should be defined in .maps ELF section. It's possible to
> > >    have both "legacy" map definitions in `maps` sections and BTF-defined
> > >    maps in .maps sections. Everything will still work transparently.
> >
> > I'd prefer using a new map section "btf_maps" or whatever. No need to
> > worry about code that deals with either type.
>
> We do use new map section. Its ".maps" vs "maps". Difference is
> subtle, but ".maps" looks a bit more "standardized" than "btf_maps" to
> me (and hopefully, eventually no one will use "maps" anymore :) ).

Phew, spotting that difference is night impossible IMO.

>
> >
> > > 3. Key/value fields should be **a pointer** to a type describing
> > >    key/value. The pointee type is assumed (and will be recorded as such
> > >    and used for size determination) to be a type describing key/value of
> > >    the map. This is done to save excessive amounts of space allocated in
> > >    corresponding ELF sections for key/value of big size.
> >
> > My biggest concern with the pointer is that there are cases when we want
> > to _not_ use a pointer, e.g. your proposal for map in map and tail calling.
> > There we need value to be a struct, an array, etc. The burden on the user
> > for this is very high.
>
> Well, map-in-map is still a special case and whichever syntax we go
> with, it will need to be of slightly different syntax to distinguish
> between those cases. Initialized maps fall into similar category,
> IMHO.

I agree with you, the syntax probably has to be different. I'd just like it to
differ by more than a "*" in the struct definition, because that is too small
to notice.

>
> Embedding full value just to capture type info/size is unacceptable,
> as we have use cases that cause too big ELF size increase, which will
> prevent users from switching to this.
>
> >
> > > 4. As some maps disallow having BTF type ID associated with key/value,
> > >    it's possible to specify key/value size explicitly without
> > >    associating BTF type ID with it. Use key_size and value_size fields
> > >    to do that (see example below).
> >
> > Why not just make them use the legacy map?
>
> For completeness' sake at the least. E.g., what if you want to use
> map-in-map, where inner map is stackmap or something like that, which
> requires key_size/value_size? I think we all agree that it's better if
> application uses just one style, instead of a mix of both, right?

I kind of assumed that BTF support for those maps would at some point
appear, maybe I should have checked that.

> Btw, for map cases where map key can be arbitrary, but value is FD or
> some other opaque value, libbpf can automatically "derive" value size
> and still capture key type. I haven't done that, but it's very easy to
> do (and also we can keep adding per-map-type checks/niceties, to help
> users understand what's wrong with their map definition, instead of
> getting EINVAL from kernel on map creation).
>
> >
> > --
> > Lorenz Bauer  |  Systems Engineer
> > 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
> >
> > www.cloudflare.com



-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Powered by blists - more mailing lists