XDP hardware hints discussion mail archive
 help / color / mirror / Atom feed
From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
To: Jesper Dangaard Brouer <jbrouer@redhat.com>
Cc: "Toke Høiland-Jørgensen" <toke@redhat.com>,
	brouer@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: Tue, 7 Sep 2021 02:27:29 -0400	[thread overview]
Message-ID: <YTcGUbRpvWK+633g@localhost.localdomain> (raw)
In-Reply-To: <190d8d21-f11d-bb83-58aa-08e86e0006d9@redhat.com>

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?

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.

> > 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.

> --Jesper
> 1

  reply	other threads:[~2021-09-07 10:21 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 [this message]
2021-09-08 13:28                                             ` Jesper Dangaard Brouer
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=YTcGUbRpvWK+633g@localhost.localdomain \
    --to=michal.swiatkowski@linux.intel.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=bpf@vger.kernel.org \
    --cc=brouer@redhat.com \
    --cc=jbrouer@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=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