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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Jun 20, 2019 at 2:28 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Mon, 17 Jun 2019 at 22:00, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> > > 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.
> >
> > That would work in ideal world, where kernel is updated frequently
> > (and BTF is self-describing, which it is not). In practice, though,
> > libbpf is far more up-to-date and lends its hand on "sanitizing" .BTF
> > from kernel-unsupported features (so far we manage to pull this off
> > very reasonably). If you have a good proposal how to make .BTF
> > self-describing, that would be great!
>
> I think sanitizing is going to become a problem, but we've been around
> that argument a few times :)

Yep :)

>
> Making .BTF self describing need at least adding length to certain fields,
> as I mentioned in another thread. Plus an interface to interrogate the
> kernel about a loaded BTF blob.
>
> > > 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.
> >
> > So let's lay out how it will be done in practice:
> >
> > 1. Simple map w/ custom key/value
> >
> > struct my_key { ... };
> > struct my_value { ... };
> >
> > struct {
> >     __u32 type;
> >     __u32 max_entries;
> >     struct my_key *key;
> >     struct my_value *value;
> > } my_simple_map BPF_MAP = {
> >     .type = BPF_MAP_TYPE_ARRAY,
> >     .max_entries = 16,
> > };
> >
> > 2. Now map-in-map:
> >
> > struct {
> >     __u32 type;
> >     __u32 max_entries;
> >     struct my_key *key;
> >     struct {
> >         __u32 type;
> >         __u32 max_entries;
> >         __u64 *key;
> >         struct my_value *value;
> >     } value;
> > } my_map_in_map BPF_MAP = {
> >     .type = BPF_MAP_TYPE_HASH_OF_MAPS,
> >     .max_entries = 16,
> >     .value = {
> >         .type = BPF_MAP_TYPE_ARRAY,
> >         .max_entries = 100,
> >     },
> > };
> >
> > It's clearly hard to misinterpret inner map definition for a custom
> > anonymous struct type, right?
>
> That's not what I'm concerned about. My point is: sometimes you
> have to use a pointer, sometimes you don't. Every user has to learn this.
> Chance is, they'll probably get it wrong first. Is there a way to give a
> reasonable error message for this?

Right now pointer is always required. My initial intent for map-in-map
was to not use pointer, but since then I've proposed a slightly
different approach, which eliminates all these concerns you mentioned.
As for messaging, yeah, that the simplest part, which can always be
improved.

>
> > > I kind of assumed that BTF support for those maps would at some point
> > > appear, maybe I should have checked that.
> >
> > It will. Current situation with maps not supporting specifying BTF for
> > key and/or value looks more like a bug, than feature and we should fix
> > that. But even if we fix it today, kernels are updated much slower
> > than libbpf, so by not supporting key_size/value_size, we force people
> > to get stuck with legacy bpf_map_def for a really long time.
>
> OK.
>
> I'll go and look at the newest revision of the patch set now :o)
>
> --
> Lorenz Bauer  |  Systems Engineer
> 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
>
> www.cloudflare.com