From: Lorenz Bauer <lmb@cloudflare.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Andrii Nakryiko <andriin@fb.com>,
Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
Alexei Starovoitov <ast@fb.com>,
Daniel Borkmann <daniel@iogearbox.net>,
Kernel Team <kernel-team@fb.com>
Subject: Re: [RFC PATCH bpf-next 6/8] libbpf: allow specifying map definitions using BTF
Date: Mon, 17 Jun 2019 10:07:13 +0100 [thread overview]
Message-ID: <CACAyw9_Yr=pmvCRYsVHoQBrH7qBwmcaXZezmqafwJTxaCmDf6A@mail.gmail.com> (raw)
In-Reply-To: <CAEf4BzamSjSa-7ddzyVsqygbtT6WSwsWpCFGX-4Rav4Aev8UsA@mail.gmail.com>
On Thu, 6 Jun 2019 at 23:35, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Jun 6, 2019 at 9:43 AM Lorenz Bauer <lmb@cloudflare.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@fb.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
next prev parent reply other threads:[~2019-06-17 9:07 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-31 20:21 [RFC PATCH bpf-next 0/8] BTF-defined BPF map definitions Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 1/8] libbpf: add common min/max macro to libbpf_internal.h Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 2/8] libbpf: extract BTF loading and simplify ELF parsing logic Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 3/8] libbpf: refactor map initialization Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 4/8] libbpf: identify maps by section index in addition to offset Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 5/8] libbpf: split initialization and loading of BTF Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 6/8] libbpf: allow specifying map definitions using BTF Andrii Nakryiko
2019-05-31 21:28 ` Stanislav Fomichev
2019-05-31 22:58 ` Andrii Nakryiko
2019-06-03 0:33 ` Jakub Kicinski
2019-06-03 21:54 ` Andrii Nakryiko
2019-06-03 23:34 ` Jakub Kicinski
2019-06-03 16:32 ` Stanislav Fomichev
2019-06-03 22:03 ` Andrii Nakryiko
2019-06-04 1:02 ` Stanislav Fomichev
2019-06-04 1:07 ` Alexei Starovoitov
2019-06-04 4:29 ` Stanislav Fomichev
2019-06-04 13:45 ` Stanislav Fomichev
2019-06-04 17:31 ` Andrii Nakryiko
2019-06-04 21:07 ` Stanislav Fomichev
2019-06-04 21:22 ` Andrii Nakryiko
2019-06-06 21:09 ` Daniel Borkmann
2019-06-06 23:02 ` Andrii Nakryiko
2019-06-06 23:27 ` Alexei Starovoitov
2019-06-07 0:10 ` Jakub Kicinski
2019-06-07 0:27 ` Alexei Starovoitov
2019-06-07 1:02 ` Jakub Kicinski
2019-06-10 1:17 ` explicit maps. Was: " Alexei Starovoitov
2019-06-10 21:15 ` Jakub Kicinski
2019-06-10 23:48 ` Andrii Nakryiko
2019-06-03 22:34 ` Andrii Nakryiko
2019-06-06 16:42 ` Lorenz Bauer
2019-06-06 22:34 ` Andrii Nakryiko
2019-06-17 9:07 ` Lorenz Bauer [this message]
2019-06-17 20:59 ` Andrii Nakryiko
2019-06-20 9:27 ` Lorenz Bauer
2019-06-21 4:05 ` Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 7/8] selftests/bpf: add test for BTF-defined maps Andrii Nakryiko
2019-05-31 20:21 ` [RFC PATCH bpf-next 8/8] selftests/bpf: switch tests to BTF-defined map definitions Andrii Nakryiko
2019-06-11 4:34 [RFC PATCH bpf-next 0/8] BTF-defined BPF " Andrii Nakryiko
2019-06-11 4:35 ` [RFC PATCH bpf-next 6/8] libbpf: allow specifying map definitions using BTF Andrii Nakryiko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CACAyw9_Yr=pmvCRYsVHoQBrH7qBwmcaXZezmqafwJTxaCmDf6A@mail.gmail.com' \
--to=lmb@cloudflare.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andriin@fb.com \
--cc=ast@fb.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).