XDP hardware hints discussion mail archive
 help / color / mirror / Atom feed
From: Jesper Dangaard Brouer <jbrouer@redhat.com>
To: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>,
	Jesper Dangaard Brouer <jbrouer@redhat.com>
Cc: brouer@redhat.com, "Toke Høiland-Jørgensen" <toke@redhat.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"Andrii Nakryiko" <andrii.nakryiko@gmail.com>,
	BPF-dev-list <bpf@vger.kernel.org>,
	"Magnus Karlsson" <magnus.karlsson@gmail.com>,
	"William Tu" <u9012063@gmail.com>,
	xdp-hints@xdp-project.net,
	"Zaremba Larysa" <larysa.zaremba@intel.com>,
	"Jiri Olsa" <jolsa@redhat.com>
Subject: Re: XDP-hints: Howto support multiple BTF types per packet basis?
Date: Wed, 8 Sep 2021 15:28:21 +0200	[thread overview]
Message-ID: <936bfbdf-e194-b676-d28a-acf526120155@redhat.com> (raw)
In-Reply-To: <YTcGUbRpvWK+633g@localhost.localdomain>


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.


  reply	other threads:[~2021-09-08 13:28 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20210526125848.1c7adbb0@carbon>
     [not found] ` <CAEf4BzYXUDyQaBjZmb_Q5-z3jw1-Uvdgxm+cfcQjSwb9oRoXnQ@mail.gmail.com>
     [not found]   ` <60aeb01ebcd10_fe49208b8@john-XPS-13-9370.notmuch>
     [not found]     ` <CAEf4Bza3m5dwZ_d0=zAWR+18f5RUjzv9=1NbhTKAO1uzWg_fzQ@mail.gmail.com>
     [not found]       ` <60aeeb5252147_19a622085a@john-XPS-13-9370.notmuch>
     [not found]         ` <CAEf4Bzb1OZHpHYagbVs7s9tMSk4wrbxzGeBCCBHQ-qCOgdu6EQ@mail.gmail.com>
     [not found]           ` <60b08442b18d5_1cf8208a0@john-XPS-13-9370.notmuch>
2021-05-28  9:16             ` Toke Høiland-Jørgensen
2021-05-28 10:38               ` Alexander Lobakin
2021-05-28 14:35               ` John Fastabend
2021-05-28 15:33                 ` Toke Høiland-Jørgensen
2021-05-28 16:02                 ` Jesper Dangaard Brouer
2021-05-28 17:29                   ` John Fastabend
2021-05-30  3:27                     ` Andrii Nakryiko
2021-05-31 11:03                     ` Toke Høiland-Jørgensen
2021-05-31 13:17                       ` Jesper Dangaard Brouer
2021-06-02  0:22                       ` John Fastabend
2021-06-02 16:18                         ` Jakub Kicinski
2021-06-22  7:44                           ` Michal Swiatkowski
2021-06-22 11:53                             ` Toke Høiland-Jørgensen
2021-06-23  8:32                               ` Michal Swiatkowski
2021-06-24 12:23                                 ` Toke Høiland-Jørgensen
2021-06-24 13:07                                   ` Magnus Karlsson
2021-06-24 14:58                                     ` Alexei Starovoitov
2021-06-24 15:11                                   ` Zvi Effron
2021-06-24 16:04                                     ` Toke Høiland-Jørgensen
2021-06-24 16:32                                       ` Zvi Effron
2021-06-24 16:45                                       ` Jesper Dangaard Brouer
2021-07-08  8:32                                   ` Michal Swiatkowski
2021-07-09 10:57                                     ` Toke Høiland-Jørgensen
2021-09-02  2:49                                       ` Michal Swiatkowski
2021-09-02  9:17                                         ` Jesper Dangaard Brouer
2021-09-07  6:27                                           ` Michal Swiatkowski
2021-09-08 13:28                                             ` Jesper Dangaard Brouer [this message]
2021-09-09 18:19                                               ` Andrii Nakryiko
2021-09-10 11:16                                                 ` Jesper Dangaard Brouer
     [not found]   ` <20210526222023.44f9b3c6@carbon>
     [not found]     ` <CAEf4BzZ+VSemxx7WFanw7DfLGN7w42G6ZC4dvOSB1zAsUgRQaw@mail.gmail.com>
2021-05-28 11:16       ` Jesper Dangaard Brouer
2021-05-30  3:24         ` Andrii Nakryiko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://lists.xdp-project.net/postorius/lists/xdp-hints.xdp-project.net/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=936bfbdf-e194-b676-d28a-acf526120155@redhat.com \
    --to=jbrouer@redhat.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=bpf@vger.kernel.org \
    --cc=brouer@redhat.com \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@redhat.com \
    --cc=kuba@kernel.org \
    --cc=larysa.zaremba@intel.com \
    --cc=magnus.karlsson@gmail.com \
    --cc=michal.swiatkowski@linux.intel.com \
    --cc=toke@redhat.com \
    --cc=u9012063@gmail.com \
    --cc=xdp-hints@xdp-project.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox