Re: [RFC PATCH bpf-next 6/8] libbpf: allow specifying map definitions using BTF

From: Lorenz Bauer <lmb@cloudflare.com>
To: Andrii Nakryiko <andriin@fb.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	Networking <netdev@vger.kernel.org>,
	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: Thu, 6 Jun 2019 17:42:49 +0100	[thread overview]
Message-ID: <CACAyw99wD+7mXXeger6WoBTTu3aYHDW8EJV9_tP7MfXOnT0ODg@mail.gmail.com> (raw)
In-Reply-To: <20190531202132.379386-7-andriin@fb.com>

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.

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

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.

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.

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

> 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?

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

www.cloudflare.com