> > > On 21/09/2024 22.17, Alexander Lobakin wrote: > > From: Lorenzo Bianconi <lorenzo@xxxxxxxxxx> > > Date: Sat, 21 Sep 2024 18:52:56 +0200 > > > > > This series introduces the xdp_rx_meta struct in the xdp_buff/xdp_frame > > > > &xdp_buff is on the stack. > > &xdp_frame consumes headroom. > > > > IOW they're size-sensitive and putting metadata directly there might > > play bad; if not now, then later. > > > > Our idea (me + Toke) was as follows: > > > > - new BPF kfunc to build generic meta. If called, the driver builds a > > generic meta with hash, csum etc., in the data_meta area. > > I do agree that it should be the XDP prog (via a new BPF kfunc) that > decide if xdp_frame should be updated to contain a generic meta struct. > *BUT* I think we should use the xdp_frame area, and not the > xdp->data_meta area. ack, I will add a new kfunc for it. > > A details is that I think this kfunc should write data directly into > xdp_frame area, even then we are only operating on the xdp_buff, as we > do have access to the area xdp_frame will be created in. this would avoid to copy it when we convert from xdp_buff to xdp_frame, nice :) > > > When using data_meta area, then netstack encap/decap needs to move the > data_meta area (extra cycles). The xdp_frame area (live in top) don't > have this issue. > > It is easier to allow xdp_frame area to survive longer together with the > SKB. Today we "release" this xdp_frame area to be used by SKB for extra > headroom (see xdp_scrub_frame). I can imagine that we can move SKB > fields to this area, and reduce the size of the SKB alloc. (This then > becomes the mini-SKB we discussed a couple of years ago). > > > > Yes, this also consumes headroom, but only when the corresponding func > > is called. Introducing new fields like you're doing will consume it > > unconditionally; > > We agree on the kfunc call marks area as consumed/in-use. We can extend > xdp_frame statically like Lorenzo does (with struct xdp_rx_meta), but > xdp_frame->flags can be used for marking this area as used or not. the only downside with respect to a TLV approach would be to consume all the xdp_rx_meta as soon as we set a single xdp rx hw hint in it, right? The upside is it is easier and it requires less instructions. > > > > - when &xdp_frame gets converted to sk_buff, the function checks whether > > data_meta contains a generic structure filled with hints. > > > > Agree, but take data from xdp_frame->xdp_rx_meta. > > When XDP returns XDP_PASS, then I also want to see this data applied to > the SKB. In patchset[1] Yan called this xdp_frame_fixup_skb_offloading() > and xdp_buff_fixup_skb_offloading(). (Perhaps "fixup" isn't the right > term, "apply" is perhaps better). Having this generic-name allow us to > extend with newer offloads, and eventually move members out of SKB. > > We called it "fixup", because our use-case is that our XDP load-balancer > (Unimog) XDP_TX bounce packets with in GRE header encap, and on the > receiving NIC (due to encap) we lost the HW hash/csum, which we want to > transfer from the original NIC, decap in XDP and apply the original HW > hash/csum via this "fixup" call. I already set skb metadata converting xdp_frame into a skb in __xdp_build_skb_from_frame() but I can collect all this logic in a single routine. Regards, Lorenzo > > --Jesper > > [1] https://lore.kernel.org/all/cover.1718919473.git.yan@xxxxxxxxxxxxxx/ > > > We also thought about &skb_shared_info, but it's also size-sensitive as > > it consumes tailroom. > > > > > one as a container to store the already supported xdp rx hw hints (rx_hash > > > and rx_vlan, rx_timestamp will be stored in skb_shared_info area) when the > > > eBPF program running on the nic performs XDP_REDIRECT. Doing so, we are able > > > to set the skb metadata converting the xdp_buff/xdp_frame to a skb. > > > > Thanks, > > Olek >
Attachment:
signature.asc
Description: PGP signature
