Message-ID: <ZvYNranHf9X5ZjK-@mini-arch>
Date: Thu, 26 Sep 2024 18:43:09 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: Lorenzo Bianconi <lorenzo@...nel.org>
Cc: Toke Høiland-Jørgensen <toke@...hat.com>,
	Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
	Jesper Dangaard Brouer <hawk@...nel.org>,
	Arthur Fabre <afabre@...udflare.com>,
	Jakub Sitnicki <jakub@...udflare.com>,
	Alexander Lobakin <aleksander.lobakin@...el.com>,
	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

On 09/26, Lorenzo Bianconi wrote:
> > 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.
> 
> yes, what I wanted to say is I have the feeling we will end up 90% of the
> times in the same fields architecture and the cases where we can save some
> space seem very limited. Anyway, I am fine to discuss about a common
> architecture.
> 
> > 
> > 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.
> 
> Even if I like the idea of having a common approach for this kernel feature,
> hw metadata seems to me quite a corner case with respect of 'user-defined
> metadata', since:
> - I do not think it is a common scenario to disable hw offload capabilities
>   (e.g checksum offload in production)
> - I guess it is not just enough to disable them via bpf, but the user/sysadmin
>   will need even to configure the NIC via ethtool (so a 2-steps process).
> 
> I think we should pay attention to not overcomplicate something that is 99%
> enabled and just need to be fast. E.g I can see an issue of putting the hw rx
> metadata in metadata field since metadata grows backward and we will probably
> end up putting them in a different cacheline with respect to xdp_frame
> (xdp_headroom is usually 256B).
> 
> > 
> > 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))
> 
> yes, my plan is to add dedicated bpf kfuncs to store hw metadata in
> xdp_frame/xdp_buff
> 
> > 
> > 
> > ...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 use what we Stanislav have already added (maybe removing xdp
> prefix):
> 
> enum xdp_rx_metadata {
> 	XDP_METADATA_KFUNC_RX_TIMESTAMP,
> 	XDP_METADATA_KFUNC_RX_HASH,
> 	XDP_METADATA_KFUNC_RX_VLAN_TAG
> };

I think it was one of the ideas floating around back then for the
xdp->skb case (including redirection): have an extra kfunc that the bpf
program can call and make this kfunc store the metadata (in the metadata area)
in some fixed format. Then the kernel can consume it.

Powered by blists - more mailing lists