Date: Wed, 20 Dec 2023 17:01:37 +0100
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: Lorenzo Bianconi <lorenzo@...nel.org>, netdev@...r.kernel.org
Cc: lorenzo.bianconi@...hat.com, davem@...emloft.net, kuba@...nel.org,
 edumazet@...gle.com, pabeni@...hat.com, bpf@...r.kernel.org,
 toke@...hat.com, willemdebruijn.kernel@...il.com, jasowang@...hat.com,
 sdf@...gle.com, Yan Zhai <yan@...udflare.com>
Subject: Re: [PATCH v5 net-next 3/3] xdp: add multi-buff support for xdp
 running in generic mode



On 14/12/2023 15.29, Lorenzo Bianconi wrote:
> Similar to native xdp, do not always linearize the skb in
> netif_receive_generic_xdp routine but create a non-linear xdp_buff to be
> processed by the eBPF program. This allow to add  multi-buffer support
> for xdp running in generic mode.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> ---
>   net/core/dev.c | 153 +++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 134 insertions(+), 19 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d7857de03dba..47164acc3268 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4854,6 +4854,12 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
>   	xdp_init_buff(xdp, frame_sz, &rxqueue->xdp_rxq);
>   	xdp_prepare_buff(xdp, hard_start, skb_headroom(skb) - mac_len,
>   			 skb_headlen(skb) + mac_len, true);
> +	if (skb_is_nonlinear(skb)) {
> +		skb_shinfo(skb)->xdp_frags_size = skb->data_len;
> +		xdp_buff_set_frags_flag(xdp);
> +	} else {
> +		xdp_buff_clear_frags_flag(xdp);
> +	}
>   
>   	orig_data_end = xdp->data_end;
>   	orig_data = xdp->data;
> @@ -4883,6 +4889,14 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
>   		skb->len += off; /* positive on grow, negative on shrink */
>   	}
>   
> +	/* XDP frag metadata (e.g. nr_frags) are updated in eBPF helpers
> +	 * (e.g. bpf_xdp_adjust_tail), we need to update data_len here.
> +	 */
> +	if (xdp_buff_has_frags(xdp))
> +		skb->data_len = skb_shinfo(skb)->xdp_frags_size;
> +	else
> +		skb->data_len = 0;
> +
>   	/* check if XDP changed eth hdr such SKB needs update */
>   	eth = (struct ethhdr *)xdp->data;
>   	if ((orig_eth_type != eth->h_proto) ||
> @@ -4916,12 +4930,118 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
>   	return act;
>   }
>   
> +static int netif_skb_segment_for_xdp(struct sk_buff **pskb,

This function "...segment_for_xdp" always reallocate SKB and copies all
bits over.
Should it have been named "skb_realloc_for_xdp" ?

I was really hopeing we can find a design to avoid doing this realloc.

If the BPF-prog doesn't write into any of the fragments, then we can
avoid this realloc (+copy) dance. We designed XDP multi-buff to have
exactly the same layout+location as SKB in skb_shared_info, exactly to
avoid having to reallocated.

More comments inline below...

> +				     struct bpf_prog *prog)
> +{
> +#if IS_ENABLED(CONFIG_PAGE_POOL)
> +	struct softnet_data *sd = this_cpu_ptr(&softnet_data);
> +	u32 size, truesize, len, max_head_size, off;
> +	struct sk_buff *skb = *pskb, *nskb;
> +	int err, i, head_off;
> +	void *data;
> +
> +	/* XDP does not support fraglist so we need to linearize
> +	 * the skb.
> +	 */
> +	if (skb_has_frag_list(skb) || !prog->aux->xdp_has_frags)
> +		return -EOPNOTSUPP;
> +
> +	max_head_size = SKB_WITH_OVERHEAD(PAGE_SIZE - XDP_PACKET_HEADROOM);
> +	if (skb->len > max_head_size + MAX_SKB_FRAGS * PAGE_SIZE)
> +		return -ENOMEM;
> +
> +	size = min_t(u32, skb->len, max_head_size);
> +	truesize = SKB_HEAD_ALIGN(size) + XDP_PACKET_HEADROOM;
> +	data = page_pool_dev_alloc_va(sd->page_pool, &truesize);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	nskb = napi_build_skb(data, truesize);
> +	if (!nskb) {
> +		page_pool_free_va(sd->page_pool, data, true);
> +		return -ENOMEM;
> +	}
> +
> +	skb_reserve(nskb, XDP_PACKET_HEADROOM);
> +	skb_copy_header(nskb, skb);
> +	skb_mark_for_recycle(nskb);
> +
> +	err = skb_copy_bits(skb, 0, nskb->data, size);

This will likely copy part of the "frags" into the SKB "head" area.

Especially for netstack generated TCP packets, this will change the
segmentation layout significantly.  I wonder what (performance) effects
this will have on further handling of these SKBs.



> +	if (err) {
> +		consume_skb(nskb);
> +		return err;
> +	}
> +	skb_put(nskb, size);
> +
> +	head_off = skb_headroom(nskb) - skb_headroom(skb);
> +	skb_headers_offset_update(nskb, head_off);
> +
> +	off = size;
> +	len = skb->len - off;
> +	for (i = 0; i < MAX_SKB_FRAGS && off < skb->len; i++) {
> +		struct page *page;
> +		u32 page_off;
> +
> +		size = min_t(u32, len, PAGE_SIZE);
> +		truesize = size;
> +
> +		page = page_pool_dev_alloc(sd->page_pool, &page_off,
> +					   &truesize);
> +		if (!data) {
> +			consume_skb(nskb);
> +			return -ENOMEM;
> +		}
> +
> +		skb_add_rx_frag(nskb, i, page, page_off, size, truesize);
> +		err = skb_copy_bits(skb, off, page_address(page) + page_off,
> +				    size);

I think it is correct, but we can easily endup with the new SKB (nskb)
having a different nskb->nr_frags.


> +		if (err) {
> +			consume_skb(nskb);
> +			return err;
> +		}
> +
> +		len -= size;
> +		off += size;
> +	}
> +
> +	consume_skb(skb);
> +	*pskb = nskb;
> +
> +	return 0;
> +#else
> +	return -EOPNOTSUPP;
> +#endif
> +}
> +
> +static int netif_skb_check_for_xdp(struct sk_buff **pskb,
> +				   struct bpf_prog *prog)
> +{
> +	struct sk_buff *skb = *pskb;
> +	int err, hroom, troom;
> +
> +	if (!netif_skb_segment_for_xdp(pskb, prog))
> +		return 0;

IMHO the code call logic, does not make it easy to add cases where we
can avoid the realloc.  With this patch, it feels like the realloc+copy
code path is the "main" code path for XDP-generic.

Our goal should be to avoid realloc.

My goal for XDP multi-buff was/is that it can co-exist with GSO/GRO
packets.  This patchset is a step in the direction of enabling GRO on
devices with XDP (generic) loaded.  And I was really excited about this,
but the overhead is going to be massive compared to normal GRO (without
realloc+copy) that XDP end-users are going to be disappointed.


> +
> +	/* In case we have to go down the path and also linearize,
> +	 * then lets do the pskb_expand_head() work just once here.
> +	 */
> +	hroom = XDP_PACKET_HEADROOM - skb_headroom(skb);
> +	troom = skb->tail + skb->data_len - skb->end;
> +	err = pskb_expand_head(skb,
> +			       hroom > 0 ? ALIGN(hroom, NET_SKB_PAD) : 0,
> +			       troom > 0 ? troom + 128 : 0, GFP_ATOMIC);
> +	if (err)
> +		return err;
> +
> +	return skb_linearize(skb);
> +}
> +
>   static u32 netif_receive_generic_xdp(struct sk_buff **pskb,
>   				     struct xdp_buff *xdp,
>   				     struct bpf_prog *xdp_prog)
>   {
>   	struct sk_buff *skb = *pskb;
> -	u32 act = XDP_DROP;
> +	u32 mac_len, act = XDP_DROP;
>   
>   	/* Reinjected packets coming from act_mirred or similar should
>   	 * not get XDP generic processing.
> @@ -4929,41 +5049,36 @@ static u32 netif_receive_generic_xdp(struct sk_buff **pskb,
>   	if (skb_is_redirected(skb))
>   		return XDP_PASS;
>   
> -	/* XDP packets must be linear and must have sufficient headroom
> -	 * of XDP_PACKET_HEADROOM bytes. This is the guarantee that also
> -	 * native XDP provides, thus we need to do it here as well.
> +	/* XDP packets must have sufficient headroom of XDP_PACKET_HEADROOM
> +	 * bytes. This is the guarantee that also native XDP provides,
> +	 * thus we need to do it here as well.

Some "native" XDP provider only have 192 bytes as HEADROOM and XDP code
can this not being static (256 bytes).  So, perhaps it is time to allow
XDP generic to only require 192 bytes?

>   	 */
> +	mac_len = skb->data - skb_mac_header(skb);
> +	__skb_push(skb, mac_len);
> +
>   	if (skb_cloned(skb) || skb_is_nonlinear(skb) ||
>   	    skb_headroom(skb) < XDP_PACKET_HEADROOM) {
> -		int hroom = XDP_PACKET_HEADROOM - skb_headroom(skb);
> -		int troom = skb->tail + skb->data_len - skb->end;
> -
> -		/* In case we have to go down the path and also linearize,
> -		 * then lets do the pskb_expand_head() work just once here.
> -		 */
> -		if (pskb_expand_head(skb,
> -				     hroom > 0 ? ALIGN(hroom, NET_SKB_PAD) : 0,
> -				     troom > 0 ? troom + 128 : 0, GFP_ATOMIC))
> -			goto do_drop;
> -		if (skb_linearize(skb))
> +		if (netif_skb_check_for_xdp(pskb, xdp_prog))
>   			goto do_drop;
>   	}
>   
> -	act = bpf_prog_run_generic_xdp(skb, xdp, xdp_prog);
> +	__skb_pull(*pskb, mac_len);
> +
> +	act = bpf_prog_run_generic_xdp(*pskb, xdp, xdp_prog);
>   	switch (act) {
>   	case XDP_REDIRECT:
>   	case XDP_TX:
>   	case XDP_PASS:
>   		break;
>   	default:
> -		bpf_warn_invalid_xdp_action(skb->dev, xdp_prog, act);
> +		bpf_warn_invalid_xdp_action((*pskb)->dev, xdp_prog, act);
>   		fallthrough;
>   	case XDP_ABORTED:
> -		trace_xdp_exception(skb->dev, xdp_prog, act);
> +		trace_xdp_exception((*pskb)->dev, xdp_prog, act);
>   		fallthrough;
>   	case XDP_DROP:
>   	do_drop:
> -		kfree_skb(skb);
> +		kfree_skb(*pskb);
>   		break;
>   	}
>   

Powered by blists - more mailing lists