Re: [RFC bpf-next 0/4] Add XDP rx hw hints support performing XDP_REDIRECT

[Jesper Dangaard Brouer <hawk@kernel.org>]

On 21/09/2024 22.17, Alexander Lobakin wrote:
> From: Lorenzo Bianconi <lorenzo@kernel.org>
> 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.

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.


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.


> - 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.

--Jesper

[1] https://lore.kernel.org/all/cover.1718919473.git.yan@cloudflare.com/

> 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