XDP hardware hints discussion mail archive
 help / color / mirror / Atom feed
From: "Toke Høiland-Jørgensen" <toke@redhat.com>
To: Stanislav Fomichev <sdf@google.com>
Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
	andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
	yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org,
	haoluo@google.com, jolsa@kernel.org,
	David Ahern <dsahern@gmail.com>, Jakub Kicinski <kuba@kernel.org>,
	Willem de Bruijn <willemb@google.com>,
	Jesper Dangaard Brouer <brouer@redhat.com>,
	Anatoly Burakov <anatoly.burakov@intel.com>,
	Alexander Lobakin <alexandr.lobakin@intel.com>,
	Magnus Karlsson <magnus.karlsson@gmail.com>,
	Maryam Tahhan <mtahhan@redhat.com>,
	xdp-hints@xdp-project.net, netdev@vger.kernel.org
Subject: [xdp-hints] Re: [PATCH bpf-next v2 2/8] bpf: XDP metadata RX kfuncs
Date: Mon, 28 Nov 2022 23:25:22 +0100	[thread overview]
Message-ID: <87y1ruzpgt.fsf@toke.dk> (raw)
In-Reply-To: <CAKH8qBvp+4MjRpwMeG3G_duC6RCoJurswMFuC9ynf-F9-is0+g@mail.gmail.com>

Stanislav Fomichev <sdf@google.com> writes:

> On Mon, Nov 28, 2022 at 10:53 AM Stanislav Fomichev <sdf@google.com> wrote:
>>
>>  s
>>
>> On Fri, Nov 25, 2022 at 9:53 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >
>> > Stanislav Fomichev <sdf@google.com> writes:
>> >
>> > > There is an ndo handler per kfunc, the verifier replaces a call to the
>> > > generic kfunc with a call to the per-device one.
>> > >
>> > > For XDP, we define a new kfunc set (xdp_metadata_kfunc_ids) which
>> > > implements all possible metatada kfuncs. Not all devices have to
>> > > implement them. If kfunc is not supported by the target device,
>> > > the default implementation is called instead.
>> >
>> > BTW, this "the default implementation is called instead" bit is not
>> > included in this version... :)
>>
>> fixup_xdp_kfunc_call should return 0 when the device doesn't have a
>> kfunc defined and should fallback to the default kfunc implementation,
>> right?
>> Or am I missing something?
>>
>> > [...]
>> >
>> > > +#ifdef CONFIG_DEBUG_INFO_BTF
>> > > +BTF_SET8_START(xdp_metadata_kfunc_ids)
>> > > +#define XDP_METADATA_KFUNC(name, str) BTF_ID_FLAGS(func, str, 0)
>> > > +XDP_METADATA_KFUNC_xxx
>> > > +#undef XDP_METADATA_KFUNC
>> > > +BTF_SET8_END(xdp_metadata_kfunc_ids)
>> > > +
>> > > +static const struct btf_kfunc_id_set xdp_metadata_kfunc_set = {
>> > > +     .owner = THIS_MODULE,
>> > > +     .set   = &xdp_metadata_kfunc_ids,
>> > > +};
>> > > +
>> > > +u32 xdp_metadata_kfunc_id(int id)
>> > > +{
>> > > +     return xdp_metadata_kfunc_ids.pairs[id].id;
>> > > +}
>> > > +EXPORT_SYMBOL_GPL(xdp_metadata_kfunc_id);
>> >
>> > So I was getting some really weird values when testing (always getting a
>> > timestamp value of '1'), and it turns out to be because this way of
>> > looking up the ID doesn't work: The set is always sorted by the BTF ID,
>> > not the order it was defined. Which meant that the mapping code got the
>> > functions mixed up, and would call a different one instead (so the
>> > timestamp value I was getting was really the return value of
>> > rx_hash_enabled()).
>> >
>> > I fixed it by building a secondary lookup table as below; feel free to
>> > incorporate that (or if you can come up with a better way, go ahead!).
>>
>> Interesting, will take a closer look. I took this pattern from
>> BTF_SOCK_TYPE_xxx, which means that 'sorting by btf-id' is something
>> BTF_SET8_START specific...
>> But if it's sorted, probably easier to do a bsearch over this table
>> than to build another one?
>
> Ah, I see, there is no place to store an index :-( Maybe the following
> is easier still?
>
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index e43f7d4ef4cf..8240805bfdb7 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -743,9 +743,15 @@ static const struct btf_kfunc_id_set
> xdp_metadata_kfunc_set = {
>         .set   = &xdp_metadata_kfunc_ids,
>  };
>
> +BTF_ID_LIST(xdp_metadata_kfunc_ids_unsorted)
> +#define XDP_METADATA_KFUNC(name, str) BTF_ID(func, str)
> +XDP_METADATA_KFUNC_xxx
> +#undef XDP_METADATA_KFUNC
> +
>  u32 xdp_metadata_kfunc_id(int id)
>  {
> -       return xdp_metadata_kfunc_ids.pairs[id].id;
> +       /* xdp_metadata_kfunc_ids is sorted and can't be used */
> +       return xdp_metadata_kfunc_ids_unsorted[id];
>  }
>  EXPORT_SYMBOL_GPL(xdp_metadata_kfunc_id);

Right, as long as having that extra list isn't problematic (does it make
things show up twice somewhere or something like that? not really sure
how that works), that is certainly simpler than what I came up with :)

-Toke


  reply	other threads:[~2022-11-28 22:25 UTC|newest]

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-21 18:25 [xdp-hints] [PATCH bpf-next v2 0/8] xdp: hints via kfuncs Stanislav Fomichev
2022-11-21 18:25 ` [xdp-hints] [PATCH bpf-next v2 1/8] bpf: Document XDP RX metadata Stanislav Fomichev
2022-11-21 18:25 ` [xdp-hints] [PATCH bpf-next v2 2/8] bpf: XDP metadata RX kfuncs Stanislav Fomichev
2022-11-21 23:31   ` [xdp-hints] " kernel test robot
2022-11-23  6:34   ` Martin KaFai Lau
2022-11-23 18:43     ` Stanislav Fomichev
2022-11-23 14:24   ` Toke Høiland-Jørgensen
2022-11-23 18:43     ` Stanislav Fomichev
2022-11-24  2:23   ` kernel test robot
2022-11-24 12:19   ` kernel test robot
2022-11-24 13:09   ` kernel test robot
2022-11-25 17:53   ` Toke Høiland-Jørgensen
2022-11-28 18:53     ` Stanislav Fomichev
2022-11-28 19:21       ` Stanislav Fomichev
2022-11-28 22:25         ` Toke Høiland-Jørgensen [this message]
2022-11-28 22:10       ` Toke Høiland-Jørgensen
2022-11-30 17:24   ` Larysa Zaremba
2022-11-30 19:06     ` Stanislav Fomichev
2022-11-30 20:17       ` Stanislav Fomichev
2022-12-01 13:52         ` Larysa Zaremba
2022-12-01 17:14           ` Stanislav Fomichev
2022-11-21 18:25 ` [xdp-hints] [PATCH bpf-next v2 3/8] veth: Introduce veth_xdp_buff wrapper for xdp_buff Stanislav Fomichev
2022-11-21 18:25 ` [xdp-hints] [PATCH bpf-next v2 4/8] veth: Support RX XDP metadata Stanislav Fomichev
2022-11-21 18:25 ` [xdp-hints] [PATCH bpf-next v2 5/8] selftests/bpf: Verify xdp_metadata xdp->af_xdp path Stanislav Fomichev
2022-11-29 10:06   ` [xdp-hints] " Anton Protopopov
2022-11-29 18:52     ` Stanislav Fomichev
2022-11-21 18:25 ` [xdp-hints] [PATCH bpf-next v2 6/8] mlx4: Introduce mlx4_xdp_buff wrapper for xdp_buff Stanislav Fomichev
2022-11-22 13:49   ` [xdp-hints] " Tariq Toukan
2022-11-22 18:08     ` Stanislav Fomichev
2022-11-23 14:33   ` Toke Høiland-Jørgensen
2022-11-23 18:26     ` Stanislav Fomichev
2022-11-23 19:14       ` Jakub Kicinski
2022-11-23 19:52         ` sdf
2022-11-23 21:54           ` Maciej Fijalkowski
2022-11-23 21:55           ` Toke Høiland-Jørgensen
2022-11-24  1:47             ` Jakub Kicinski
2022-11-24 14:39               ` Toke Høiland-Jørgensen
2022-11-24 15:17                 ` Maciej Fijalkowski
2022-11-24 16:11                   ` Maciej Fijalkowski
2022-11-25  0:36                     ` Toke Høiland-Jørgensen
2022-11-28 21:58                       ` Stanislav Fomichev
2022-11-28 22:11                         ` Toke Høiland-Jørgensen
2022-11-21 18:25 ` [xdp-hints] [PATCH bpf-next v2 7/8] mxl4: Support RX XDP metadata Stanislav Fomichev
2022-11-22 13:50   ` [xdp-hints] " Tariq Toukan
2022-11-22 18:08     ` Stanislav Fomichev
2022-11-21 18:25 ` [xdp-hints] [PATCH bpf-next v2 8/8] selftests/bpf: Simple program to dump XDP RX metadata Stanislav Fomichev
2022-11-23 14:26   ` [xdp-hints] " Toke Høiland-Jørgensen
2022-11-23 18:29     ` Stanislav Fomichev
2022-11-23 19:17       ` Jakub Kicinski
2022-11-23 19:54         ` Stanislav Fomichev
2022-11-23 14:46 ` [xdp-hints] [PATCH bpf-next 1/2] xdp: Add drv_priv pointer to struct xdp_buff Toke Høiland-Jørgensen
2022-11-23 14:46   ` [xdp-hints] [PATCH bpf-next 2/2] mlx5: Support XDP RX metadata Toke Høiland-Jørgensen
2022-11-23 22:29     ` [xdp-hints] " Saeed Mahameed
2022-11-23 22:44       ` Stanislav Fomichev

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=87y1ruzpgt.fsf@toke.dk \
    --to=toke@redhat.com \
    --cc=alexandr.lobakin@intel.com \
    --cc=anatoly.burakov@intel.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=brouer@redhat.com \
    --cc=daniel@iogearbox.net \
    --cc=dsahern@gmail.com \
    --cc=haoluo@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kpsingh@kernel.org \
    --cc=kuba@kernel.org \
    --cc=magnus.karlsson@gmail.com \
    --cc=martin.lau@linux.dev \
    --cc=mtahhan@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=sdf@google.com \
    --cc=song@kernel.org \
    --cc=willemb@google.com \
    --cc=xdp-hints@xdp-project.net \
    --cc=yhs@fb.com \
    /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