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

[Jesper Dangaard Brouer <jbrouer@redhat.com>]

On 02/09/2021 04.49, Michal Swiatkowski wrote:
> On Fri, Jul 09, 2021 at 12:57:08PM +0200, Toke Høiland-Jørgensen wrote:
>> Michal Swiatkowski <michal.swiatkowski@linux.intel.com> writes:
>>
>>>> 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.
>>>
>>> This looks nice. In this case we need defintions of struct meta_mlx5 and
>>> struct meta_i40e at build time. How are we going to deliver this to bpf
>>> core app? This will be available in /sys/kernel/btf/mlx5 and
>>> /sys/kernel/btf/i40e (if drivers are loaded). Should we dump this to
>>> vmlinux.h? Or a developer of the xdp program should add this definition
>>> to his code?
>>
>> Well, if the driver just defines the struct, the BTF for it will be
>> automatically part of the driver module BTF. BPF program developers
>> would need to include this in their programs somehow (similar to how
>> you'll need to get the type definitions from vmlinux.h today to use
>> CO-RE); how they do this is up to them. Since this is a compile-time
>> thing it will probably depend on the project (for instance, BCC includes
>> a copy of vmlinux.h in their source tree, but you can also just pick out
>> the structs you need).
>>
>>> Maybe create another /sys/kernel/btf/hints with vmlinux and hints from
>>> all drivers which support hints?
>>
>> It may be useful to have a way for the kernel to export all the hints
>> currently loaded, so libbpf can just use that when relocating. The
>> problem of course being that this will only include drivers that are
>> actually loaded, so users need to make sure to load all their network
>> drivers before loading any XDP programs. I think it would be better if
>> the loader could discover all modules *available* on the system, but I'm
>> not sure if there's a good way to do that.
>>
>>> Previously in this thread someone mentioned this ___ use case in libbpf
>>> and proposed creating something like mega xdp hints structure with all
>>> available fields across all drivers. As I understand this could solve
>>> the problem about defining correct structure at build time. But how will
>>> it work when there will be more than one structures with the same name
>>> before ___? I mean:
>>> struct xdp_hints___mega defined only in core app
>>> struct xdp_hints___mlx5 available when mlx5 driver is loaded
>>> struct xdp_hints___i40e available when i40e driver is loaded
>>>
>>> When there will be only one driver loaded should libbpf do correct
>>> reallocation of fields? What will happen when both of the drivers are
>>> loaded?
>>
>> I think we definitely need to make this easy for developers so they
>> don't have to go and manually track down the driver structs and write
>> the disambiguation code etc. I.e., the example code I included above
>> that checks the frame BTF ID and does the loading based on it should be
>> auto-generated. We already have some precedence for auto-generated code
>> in vmlinux.h and the bpftool skeletons. So maybe we could have a command
>> like 'bpftool gen_xdp_meta <fields>' which would go and lookup all the
>> available driver structs and generate a code helper function that will
>> extract the driver structs and generate the loader code? So that if,
>> say, you're interested in rxhash and tstamp you could do:
>>
>> bpftool gen_xdp_meta rxhash tstamp > my_meta.h
>>
>> which would then produce my_meta.h with content like:
>>
>> struct my_meta { /* contains fields specified on the command line */
>>    u32 rxhash;
>>    u32 tstamp;
>> }
>>
>> struct meta_mlx5 {/*generated from kernel BTF */};
>> struct meta_i40e {/*generated from kernel BTF */};
>>
>> static inline int get_xdp_meta(struct xdp_md *ctx, struct my_meta *meta)
>> {
>>   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;
>>       my_meta->rxhash = meta->rxhash;
>>       my_meta->tstamp = meta->tstamp;
>>       return 0;
>>     } else if (meta_btf_id == BTF_ID_I40E) {
>>       struct meta_i40e *meta = ctx->data_meta;
>>       my_meta->rxhash = meta->rxhash;
>>       my_meta->tstamp = meta->tstamp;
>>       return 0;
>>     } /* etc */
>>   }
>>   return -ENOENT;
>> }
> 
> According to meta_btf_id. 

In BPF-prog (that gets loaded by libbpf), the BTF_ID_MLX5 and 
BTF_ID_I40E should be replaced by bpf_core_type_id_kernel() calls.

I have a code example here[1], where I use the triple-underscore to 
lookup btf_id = bpf_core_type_id_kernel(struct sk_buff___local).

AFAIK (Andrii correctly me if I'm wrong) It is libbpf that does the 
bpf_id lookup before loading the BPF-prog into the kernel.

For AF_XDP we need to code our own similar lookup of the btf_id. (In 
that process I imagine that userspace tools could/would read the BTF 
member offsets and check it against what they expected).


  [1] 
https://github.com/xdp-project/bpf-examples/blob/master/ktrace-CO-RE/ktrace01_kern.c#L34-L57

> Do You have an idea how driver should fill this field?

(Andrii please correctly me as this is likely wrong:)
I imagine that driver will have a pointer to a 'struct btf' object and 
the btf_id can be read via btf_obj_id() (that just return btf->id).
As this also allows driver to take refcnt on the btf-object.
Much like Ederson did in [2].

Maybe I misunderstood the use of the 'struct btf' object ?

Maybe it is the wrong approach? As the patchset[2] exports btf_obj_id() 
and introduced helper functions that can register/unregister btf 
objects[3], which I sense might not be needed today, as modules can get 
their own BTF info automatically today.
Maybe this (btf->id) is not the ID we are looking for?

[2] 
https://lore.kernel.org/all/20210803010331.39453-11-ederson.desouza@intel.com/
[3] 
https://lore.kernel.org/all/20210803010331.39453-2-ederson.desouza@intel.com/

> hints->btf_id = btf_id_by_name("struct meta_i40e"); /* fill btf id at
> config time */

Yes, at config time the btf_id can change (and maybe we want to cache 
the btf_obj_id() lookup to avoid a function call).

> btf_id_by_name will get module btf (or vmlinux btf) and search for
> correct name for each ids. Does this look correct?
 >
> Is there any way in kernel to get btf id based on name or we have to
> write functions for this? I haven't seen code for this case, but maybe I
> missed it.

There is a function named: btf_find_by_name_kind()

--Jesper