Re: [xdp-hints] [PATCH bpf-next v2 2/8] bpf: XDP metadata RX kfuncs - Toke Høiland-Jørgensen

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: Re: [xdp-hints] [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