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

On Tue, Jun 4, 2019 at 2:07 PM Stanislav Fomichev <sdf@...ichev.me> wrote:
>
> On 06/04, 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);
> So prog_array tail call info would be encoded in the magic struct instead of
> a __section_tail(whatever) macros that iproute2 is using? Does it

Yes. It will be C-style array initialization (where value is address
of a function, corresponding to a BPF program).

> mean that the programs that target iproute2 would have to be rewritten?
> Or we don't have a goal to provide source-level compatibility?

As outlined in separate email I sent out yesterday, my goal was making
sure we have very easy transition path not changing the semantics
(field renaming for common case, functionally-equivalent, but
different syntax for tail call prog array initialization, etc). Let's
see what folks working on Cilium think about this.


>
> In general, supporting iproute2 seems like the most compelling
> reason to use BTF given current state of llvm+btf adoption.
> BPF_ANNOTATE_KV_PAIR and map-in-map syntax while ugly, is not the major
> paint point (imho); but I agree, with BTF both of those things
> look much better.
>
> That's why I was trying to understand whether we can start with using
> BTF to support _existing_ iproute2 format and then, once it's working,
> generalize it (and kill bpf_map_def or make it a subset of generic BTF).
> That way we are not implementing another way to support pinning/tail
> calls, but enabling iproute2 to use libbpf.

We currently don't have a good way (except for programmatic API) to do
either tail call or map-in-map declaratively in libbpf, so the hope is
this approach will allow us to address that lack, and preferrably in a
bit more intuitive way, than iproute2 support today. Given it's simple
to convert iproute2 approach to BTF-based one, I'd vote for not
back-porting that logic into libbpf, if possible.

>
> But feel free to ignore all my nonsense above; I don't really have any
> major concerns with the new generic format rather than discoverability
> (the docs might help) and a mandate that everyone switches to it immediately.

No, thanks for feedback! For documentation, I think we might want to
add description to https://docs.cilium.io/en/v1.4/bpf/ (though
timing-wise it would be better to do after iproute2 starts using
libbpf, so a bit of a chicken-and-egg problem). If you have better
suggestions where to put it, let me know.

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