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

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

On Thu, Jun 6, 2019 at 2:09 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 06/04/2019 07:31 PM, Andrii Nakryiko wrote:
> > On Tue, Jun 4, 2019 at 6:45 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >> On 06/03, Stanislav Fomichev wrote:
> >>>> BTF is mandatory for _any_ new feature.
> >>> If something is easy to support without asking everyone to upgrade to
> >>> a bleeding edge llvm, why not do it?
> >>> So much for backwards compatibility and flexibility.
> >>>
> >>>> It's for introspection and debuggability in the first place.
> >>>> Good debugging is not optional.
> >>> Once llvm 8+ is everywhere, sure, but we are not there yet (I'm talking
> >>> about upstream LTS distros like ubuntu/redhat).
> >> But putting this aside, one thing that I didn't see addressed in the
> >> cover letter is: what is the main motivation for the series?
> >> Is it to support iproute2 map definitions (so cilium can switch to libbpf)?
> >
> > In general, the motivation is to arrive at a way to support
> > declaratively defining maps in such a way, that:
> > - captures type information (for debuggability/introspection) in
> > coherent and hard-to-screw-up way;
> > - allows to support missing useful features w/ good syntax (e.g.,
> > natural map-in-map case vs current completely manual non-declarative
> > way for libbpf);
> > - ultimately allow iproute2 to use libbpf as unified loader (and thus
> > the need to support its existing features, like
> > BPF_MAP_TYPE_PROG_ARRAY initialization, pinning, map-in-map);
>
> Thanks for working on this & sorry for jumping in late! Generally, I like
> the approach of using BTF to make sense out of the individual members and
> to have extensibility, so overall I think it's a step in the right direction.
> Going back to the example where others complained that the k/v NULL
> initialization feels too much magic from a C pov:
>
> struct {
>         int type;
>         int max_entries;
>         int *key;
>         struct my_value *value;
> } my_map SEC(".maps") = {
>         .type = BPF_MAP_TYPE_ARRAY,
>         .max_entries = 16,
> };
>
> Given LLVM is in charge of emitting BTF plus given gcc/clang seem /both/
> to support *target* specific attributes [0], how about something along these
> lines where the type specific info is annotated as a variable BPF target
> attribute, like:
>
> struct {
>         int type;
>         int max_entries;
> } my_map __attribute__((map(int,struct my_value))) = {
>         .type = BPF_MAP_TYPE_ARRAY,
>         .max_entries = 16,
> };
>
> Of course this would need BPF backend support, but at least that approach
> would be more C like. Thus this would define types where we can automatically

I guess it's technically possible (not a compiler guru, but I don't
see why it wouldn't be possible). But it will require at least two
things:
1. Compiler support, obviously, as you mentioned.
2. BTF specification on how to describe attributes and how to describe
what entities (variable in this case) it is attached to.

2. is not straightforward, as attributes in general is a collection of
values of vastly different types: some values could be integers, some
strings, some, like in this case, would be a reference another BTF
type. It seems like a powerful and potentially useful addition to BTF,
of course, but it's very unclear at this point what's the best way to
represent them.

I'm not relating with "non idiomatic C" motive, though, so all that
seems like unnecessarily heavy-weight way to get something that we can
get today w/o compiler support in a clean, succinct and familiar C
syntax, that to me doesn't look like magic at all.

And if anything, attribute feels just as much magic to me. But here's
very similarly looking macro-trick:

#define MAP_KEY_VALUE_META(KEY, VALUE) KEY *key; VALUE *value;

struct {
       MAP_KEY_VALUE_META(int, struct my_value)
       int type;
       int max_entries;
} my_map SEC(".maps") = {
       .type = BPF_MAP_TYPE_ARRAY,
       .max_entries = 16,
};

Or even:

#define MAP_DEF(KEY, VALUE) struct { KEY *key; VALUE *value; int type;
int max_entries; }

MAP_DEF(int, struct my_value) my_map SEC(".maps") = {
       .type = BPF_MAP_TYPE_ARRAY,
       .max_entries = 16,
};

> derive key/val sizes etc. The SEC() could be dropped as well as map attribute

I think we should at least have an ability to override ELF section
name, just in case we add support to have maps in multiple sections
(e.g., shared library with its own set of maps, or whatever).

> would imply it for LLVM to do the right thing underneath. The normal/actual members
> from the struct has a base set of well-known names that are minimally required
> but there could be custom stuff as well where libbpf would query some user
> defined callback that can handle these. Anyway, main point, what do you think

So regarding callback. I find it hard to imagine how that could be
implemented interface-wise. As each field can have very different
value (it could be another embedded custom struct, not just integer;
or it could be char array of fixed size, etc), which is determined by
BTF, I don't know how I would expose that to custom callback in C type
system.

If I absolutely had to do it, though, how about this approach. We
either add BTF type id of a defining struct to bpf_map_def or add
bpf_map__btf_def() API, which returns it, so:

struct bpf_map *map = bpf_object__find_map_by_name(obj, "my_fancy_map");
struct btf *btf = bpf_object__btf(obj);
__u32 def_id = bpf_map__btf_map_def_type_id(map);
const void *def_data = bpf_map__btf_map_def_data(map);
struct btf_type *t = btf__type_by_id(btf, def_id);

Then application can do whatever parsing it wants on BTF map
definition and extract values in whatever manner suits it. This way
it's just a bunch of very straightforward APIs, instead of callbacks
w/ unclear interface (i.e., you'd still need to expose field_name,
field's type_id, raw pointer to data).

Does this make sense?

But having said that, what are the use cases you have in mind that
require application to put custom stuff into a standardized map
definition?

> about the __attribute__ approach instead? I think this feels cleaner to me at
> least iff feasible.
>
> Thanks,
> Daniel
>
>   [0] https://clang.llvm.org/docs/AttributeReference.html
>       https://gcc.gnu.org/onlinedocs/gcc/Variable-Attributes.html
>
> > The only missing feature that can be supported reasonably with
> > bpf_map_def is pinning (as it's just another int field), but all the
> > other use cases requires awkward approach of matching arbitrary IDs,
> > which feels like a bad way forward.
> >
> >> If that's the case, maybe explicitly focus on that? Once we have
> >> proof-of-concept working for iproute2 mode, we can extend it to everything.
>