bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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

  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).