> Lorenzo Bianconi <lorenzo.bianconi@xxxxxxxxxx> 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
};
> > 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.
ack
Regards,
Lorenzo
>
> -Toke
>
Attachment:
signature.asc
Description: PGP signature
