Re: XDP-hints: Howto support multiple BTF types per packet basis?

[Magnus Karlsson <magnus.karlsson@gmail.com>]

On Thu, Jun 24, 2021 at 2:23 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Michal Swiatkowski <michal.swiatkowski@linux.intel.com> writes:
>
> > On Tue, Jun 22, 2021 at 01:53:33PM +0200, Toke Høiland-Jørgensen wrote:
> >> Michal Swiatkowski <michal.swiatkowski@linux.intel.com> writes:
> >>
> >> > On Wed, Jun 02, 2021 at 09:18:37AM -0700, Jakub Kicinski wrote:
> >> >> On Tue, 01 Jun 2021 17:22:51 -0700 John Fastabend wrote:
> >> >> > > If we do this, the BPF program obviously needs to know which fields are
> >> >> > > valid and which are not. AFAICT you're proposing that this should be
> >> >> > > done out-of-band (i.e., by the system administrator manually ensuring
> >> >> > > BPF program config fits system config)? I think there are a couple of
> >> >> > > problems with this:
> >> >> > >
> >> >> > > - It requires the system admin to coordinate device config with all of
> >> >> > >   their installed XDP applications. This is error-prone, especially as
> >> >> > >   the number of applications grows (say if different containers have
> >> >> > >   different XDP programs installed on their virtual devices).
> >> >> >
> >> >> > A complete "system" will need to be choerent. If I forward into a veth
> >> >> > device the orchestration component needs to ensure program sending
> >> >> > bits there is using the same format the program installed there expects.
> >> >> >
> >> >> > If I tailcall/fentry into another program that program the callee and
> >> >> > caller need to agree on the metadata protocol.
> >> >> >
> >> >> > I don't see any way around this. Someone has to manage the network.
> >> >>
> >> >> FWIW I'd like to +1 Toke's concerns.
> >> >>
> >> >> In large deployments there won't be a single arbiter. Saying there
> >> >> is seems to contradict BPF maintainers' previous stand which lead
> >> >> to addition of bpf_links for XDP.
> >> >>
> >> >> In practical terms person rolling out an NTP config change may not
> >> >> be aware that in some part of the network some BPF program expects
> >> >> descriptor not to contain time stamps. Besides features may depend
> >> >> or conflict so the effects of feature changes may not be obvious
> >> >> across multiple drivers in a heterogeneous environment.
> >> >>
> >> >> IMO guarding from obvious mis-configuration provides obvious value.
> >> >
> >> > Hi,
> >> >
> >> > Thanks for a lot of usefull information about CO-RE. I have read
> >> > recommended articles, but still don't understand everything, so sorry if
> >> > my questions are silly.
> >> >
> >> > As introduction, I wrote small XDP example using CO-RE (autogenerated
> >> > vmlinux.h and getting rid of skeleton etc.) based on runqslower
> >> > implementation. Offset reallocation of hints works great, I built CO-RE
> >> > application, added new field to hints struct, changed struct layout and
> >> > without rebuilding application everything still works fine. Is it worth
> >> > to add XDP sample using CO-RE in kernel or this isn't good place for
> >> > this kind of sample?
> >> >
> >> > First question not stricte related to hints. How to get rid of #define
> >> > and macro when I am using generated vmlinux.h? For example I wanted to
> >> > use htons macro and ethtype definition. They are located in headers that
> >> > also contains few struct definition. Because of that I have redefinition
> >> > error when I am trying to include them (redefinition in vmlinux.h and
> >> > this included file). What can I do with this besides coping definitions
> >> > to bpf code?
> >>
> >> One way is to only include the structs you actually need from vmlinux.h.
> >> You can even prune struct members, since CO-RE works just fine with
> >> partial struct definitions as long as the member names match.
> >>
> >> Jesper has an example on how to handle this here:
> >> https://github.com/netoptimizer/bpf-examples/blob/ktrace01-CO-RE.public/headers/vmlinux_local.h
> >>
> >
> > I see, thanks, I will take a look at other examples.
> >
> >> > I defined hints struct in driver code, is it right place for that? All
> >> > vendors will define their own hints struct or the idea is to have one
> >> > big hints struct with flags informing about availability of each fields?
> >> >
> >> > For me defining it in driver code was easier because I can have used
> >> > module btf to generate vmlinux.h with hints struct inside. However this
> >> > break portability if other vendors will have different struct name etc,
> >> > am I right?
> >>
> >> I would expect the easiest is for drivers to just define their own
> >> structs and maybe have some infrastructure in the core to let userspace
> >> discover the right BTF IDs to use for a particular netdev. However, as
> >> you say it's not going to work if every driver just invents their own
> >> field names, so we'll need to coordinate somehow. We could do this by
> >> convention, though, it'll need manual intervention to make sure the
> >> semantics of identically-named fields match anyway.
> >>
> >> Cf the earlier discussion with how many BTF IDs each driver might
> >> define, I think we *also* need a way to have flags that specify which
> >> fields of a given BTF ID are currently used; and having some common
> >> infrastructure for that would be good...
> >>
> >
> > Sounds good.
> >
> > Sorry, but I feel that I don't fully understand the idea. Correct me if
> > I am wrong:
> >
> > In building CO-RE application step we can defined big struct with
> > all possible fields or even empty struct (?) and use
> > bpf_core_field_exists.
> >
> > bpf_core_field_exists will be resolve before loading program by libbpf
> > code. In normal case libbpf will look for btf with hints name in vmlinux
> > of running kernel and do offset rewrite and exsistence check. But as the
> > same hints struct will be define in multiple modules we want to add more
> > logic to libbpf to discover correct BTF ID based on netdev on which program
> > will be loaded?
>
> I would expect that the program would decide ahead-of-time which BTF IDs
> it supports, by something like including the relevant structs from
> vmlinux.h. And then we need the BTF ID encoded into the packet metadata
> as well, so that it is possible to check at run-time which driver the
> packet came from (since a packet can be redirected, so you may end up
> having to deal with multiple formats in the same XDP program).
>
> Which would allow you to write code like:
>
> if (ctx->has_driver_meta) {
>   /* this should be at a well-known position, like first (or last) in meta area */
>   __u32 *meta_btf_id = ctx->data_meta;
>
>   if (*meta_btf_id == BTF_ID_MLX5) {
>     struct meta_mlx5 *meta = ctx->data_meta;
>     /* do something with meta */
>   } else if (meta_btf_id == BTF_ID_I40E) {
>     struct meta_i40e *meta = ctx->data_meta;
>     /* do something with meta */
>   } /* etc */
> }
>
> and libbpf could do relocations based on the different meta structs,
> even removing the code for the ones that don't exist on the running
> kernel.

Just wondering how this will carry over to user-space and AF_XDP since
it sees the same metadata area as XDP? AFAIK, dynamic linkers today
cannot relocate structs or remove members, but I am not up-to-date
with the latest here so might be completely wrong. And it would be
good not to have to recompile a user-space binary just because a new
NIC came out with a new BTF ID and layout, but with the same metadata
member name and format as previous NICs/BTF IDs. But I do not know how
to solve these things in user-space at the moment (except to have
fixed locations for a common set of metadata, but that is what we are
trying to avoid), so any hints and suggestions are highly appreciated.

> -Toke
>