Date:   Thu, 6 Jun 2019 23:09:18 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Stanislav Fomichev <sdf@...ichev.me>
Cc:     Alexei Starovoitov <ast@...com>, Andrii Nakryiko <andriin@...com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>, yhs@...com
Subject: Re: [RFC PATCH bpf-next 6/8] libbpf: allow specifying map definitions
 using BTF

On 06/04/2019 07:31 PM, Andrii Nakryiko wrote:
> On Tue, Jun 4, 2019 at 6:45 AM Stanislav Fomichev <sdf@...ichev.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
derive key/val sizes etc. The SEC() could be dropped as well as map attribute
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
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.

Powered by blists - more mailing lists