Message-ID: <874j62u1lb.fsf@toke.dk>
Date: Thu, 26 Sep 2024 12:54:40 +0200
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
Cc: Jesper Dangaard Brouer <hawk@...nel.org>, Arthur Fabre
 <afabre@...udflare.com>, Jakub Sitnicki <jakub@...udflare.com>, Alexander
 Lobakin <aleksander.lobakin@...el.com>, Lorenzo Bianconi
 <lorenzo@...nel.org>, bpf@...r.kernel.org, netdev@...r.kernel.org,
 ast@...nel.org, daniel@...earbox.net, davem@...emloft.net,
 kuba@...nel.org, john.fastabend@...il.com, edumazet@...gle.com,
 pabeni@...hat.com, sdf@...ichev.me, tariqt@...dia.com, saeedm@...dia.com,
 anthony.l.nguyen@...el.com, przemyslaw.kitszel@...el.com,
 intel-wired-lan@...ts.osuosl.org, mst@...hat.com, jasowang@...hat.com,
 mcoquelin.stm32@...il.com, alexandre.torgue@...s.st.com, kernel-team
 <kernel-team@...udflare.com>, Yan Zhai <yan@...udflare.com>
Subject: Re: [RFC bpf-next 0/4] Add XDP rx hw hints support performing
 XDP_REDIRECT

Lorenzo Bianconi <lorenzo.bianconi@...hat.com> writes:

>> I'm hinting at some complications here (with the EFAULT return) that
>> needs to be resolved: there is no guarantee that a given packet will be
>> in sync with the current status of the registered metadata, so we need
>> explicit checks for this. If metadata entries are de-registered again
>> this also means dealing with holes and/or reshuffling the metadata
>> layout to reuse the released space (incidentally, this is the one place
>> where a TLV format would have advantages).
>
> I like this approach but it seems to me more suitable for 'sw' metadata
> (this is main Arthur and Jakub use case iiuc) where the userspace would
> enable/disable these functionalities system-wide.
> Regarding device hw metadata (e.g. checksum offload) I can see some issues
> since on a system we can have multiple NICs with different capabilities.
> If we consider current codebase, stmmac driver supports only rx timestamp,
> while mlx5 supports all of them. In a theoretical system with these two
> NICs, since pkt_metadata_registry is global system-wide, we will end-up
> with quite a lot of holes for the stmmac, right? (I am not sure if this
> case is relevant or not). In other words, we will end-up with a fixed
> struct for device rx hw metadata (like xdp_rx_meta). So I am wondering
> if we really need all this complexity for xdp rx hw metadata?

Well, the "holes" will be there anyway (in your static struct approach).
They would just correspond to parts of the "struct xdp_rx_meta" being
unset.

What the "userspace can turn off the fields system wide" would
accomplish is to *avoid* the holes if you know that you will never need
them. E.g., say a system administrator know that they have no networks
that use (offloaded) VLANs. They could then disable the vlan offload
field system-wide, and thus reclaim the four bytes taken up by the
"vlan" member of struct xdp_rx_meta, freeing that up for use by
application metadata.

However, it may well be that the complexity of allowing fields to be
turned off is not worth the gains. At least as long as there are only
the couple of HW metadata fields that we have currently. Having the
flexibility to change our minds later would be good, though, which is
mostly a matter of making the API exposed to BPF and/or userspace
flexible enough to allow us to move things around in memory in the
future. Which was basically my thought with the API I sketched out in
the previous email. I.e., you could go:

ret = bpf_get_packet_metadata_field(pkt, METADATA_ID_HW_HASH,
                                    &my_hash_vlaue, sizeof(u32))


...and the METADATA_ID_HW_HASH would be a constant defined by the
kernel, for which the bpf_get_packet_metadata_field() kfunc just has a
hardcoded lookup into struct xdp_rx_meta. And then, if we decide to move
the field in the future, we just change the kfunc implementation, with
no impact to the BPF programs calling it.

> Maybe we can start with a simple approach for xdp rx hw metadata
> putting the struct in xdp_frame as suggested by Jesper and covering
> the most common use-cases. We can then integrate this approach with
> Arthur/Jakub's solution without introducing any backward compatibility
> issue since these field are not visible to userspace.

Yes, this is basically the gist of my suggestion (as I hopefully managed
to clarify above): Expose the fields via an API that is flexible enough
that we can move things around should the need arise, *and* which can
co-exist with the user-defined application metadata.

-Toke

Powered by blists - more mailing lists