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

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

On 07/09/2021 08.27, Michal Swiatkowski wrote:
> On Thu, Sep 02, 2021 at 11:17:43AM +0200, Jesper Dangaard Brouer wrote:
>>
>>
>> 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
>>
> Thanks a lot. I tested Your CO-RE example. For defines that are located
> in vmlinux everything works fine (like for sk_buff). When I tried to get
> btf id of structures defined in module (loaded module, structure can be
> find in /sys/kerne/btf/module_name) I always get 0 (not found). Do You
> know if bpf_core_type_id_kernel() should also support modules btf?

This might be caused by git-submodule libbpf being too old in 
xdp-project/bpf-examples repo.  I will investigate closer.


I did notice my bpftool (on my devel box) were tool old, as bpftool 
could not dump info in /sys/kerne/btf/module_name

   # bpftool btf dump file /sys/kernel/btf/igc format raw
   Error: failed to load BTF from /sys/kernel/btf/igc: Unknown error -22

After updating bpftool it does work.

> Based on:
> [1] https://lore.kernel.org/bpf/20201110011932.3201430-5-andrii@kernel.org/
> I assume that modules btfs also are marked as in-kernel, but I can't
> make it works with bpf_core_type_id_kernel(). My clang version is
> 12.0.1, so changes needed by modules btf should be there
> [2] https://reviews.llvm.org/D91489
> 
>>> 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/
>>
> 
> As 'struct btf' object do You mean one module btf with all definitions
> or specific structure btf object?
> 
> In case of Your example [1]. If in driver side there will be call to get
> btf id of sk_buff:
> id = btf_find_by_name_kind(vmlinux_btf, "sk_buff", BTF_KIND_STRUCT);
> this id will be the same as id from Your ktrace01 program. I think this
> is id that we are looking for.

Yes, I think this is the ID we should use.

The 'struct btf' object ID is something else I suspect? Or it is also a 
valid BTF ID?

I wanted to ask Andrii if the IDs are unique across vmlinux and modules?

I suspect as it looks like kern uses the IDR infra[0]:
  [0] https://www.kernel.org/doc/html/latest/core-api/idr.html


>>> 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()
>>
> Thanks, this is what I needed.