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

[Zvi Effron <zeffron@riotgames.com>]

On Thu, Jun 24, 2021 at 9:05 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Zvi Effron via xdp-hints <xdp-hints@xdp-project.net> writes:
>
> > On Thu, Jun 24, 2021 at 5:23 AM 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:[..]
> >> >
> >> > 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.
> >>
> >> -Toke
> >>
> >
> > How does putting the BTF ID and the driver metadata into the XDP metadata
> > section interact with programs that are already using the metadata section
> > for other purposes. For example, programs that use the XDP metadata to pass
> > information through BPF tail calls?
> >
> > Would this break existing programs that aren't aware of the new driver
> > metadata? Do we need to make driver metadata opt-in at XDP program
> > load?
>
> Well, XDP applications would be free to just ignore the driver-provided
> metadata and overwrite it with its own data? And I guess any application
> that doesn't know about it will just implicitly do that? :)
>
> -Toke
>

Ah, right, because bpf_xdp_adjust_meta() moves ctx->data_meta earlier in
the buffer. That would mean that if the BTF ID were stored in the metadata
it would have to be the last position in the metadata or
bpf_xdp_adjust_meta() would make it impossible to find for subsequent
programs (specifically, tail calls).

Or, potentially, we could put the BTF ID into struct xdp_md. In your code
sample, there's already a new has_driver_meta field added to that struct.
I believe that could instead just be the BTF ID, and a value of 0 (I believe
that's an invalid BTF ID?) would indicate no driver metadata.

That would change your sample to:

__u32 meta_btf_id = ctx->driver_meta_btf_id;

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 */
} else if (meta_btf_id == BTF_ID_INVALID /* 0 */) {
  /* there is no driver metadata */
} /* etc */

The current limit on metadata size would also likely need to be adjusted
to allow for current uses (that could potentially be using all of the
metadata) as well as the driver metadata and BTF ID.

--Zvi