On Tue, Jun 4, 2019 at 2:07 PM Stanislav Fomichev <sdf@xxxxxxxxxxx> wrote: > > On 06/04, Andrii Nakryiko wrote: > > On Tue, Jun 4, 2019 at 6:45 AM Stanislav Fomichev <sdf@xxxxxxxxxxx> 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.
