Re: [PATCH net-next] xdp: add multi-buff support for xdp running in generic mode

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3630 bytes --]

> Lorenzo Bianconi <lorenzo@kernel.org> writes:
> 
> > 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@kernel.org>
> 
> With one nit below:
> 
> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> > ---
> >  net/core/dev.c | 28 +++++++++++++++++++++++-----
> >  1 file changed, 23 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 3950ced396b5..5a58f3e28657 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -4853,6 +4853,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;
> > @@ -4882,6 +4888,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) ||
> > @@ -4927,9 +4941,9 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
> >  	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.
> >  	 */
> >  	if (skb_cloned(skb) || skb_is_nonlinear(skb) ||
> >  	    skb_headroom(skb) < XDP_PACKET_HEADROOM) {
> > @@ -4943,8 +4957,12 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
> >  				     hroom > 0 ? ALIGN(hroom, NET_SKB_PAD) : 0,
> >  				     troom > 0 ? troom + 128 : 0, GFP_ATOMIC))
> >  			goto do_drop;
> 
> There's a comment above the pskb_expand_head() call that isn't quite
> part of the context here, that should also be adjusted now that we don't
> always linearise:
> 
> 		/* In case we have to go down the path and also linearize,
> 		 * then lets do the pskb_expand_head() work just once here.
> 		 */
> 
> Actually, I think maybe just dropping that comment entirely with this
> change would make sense?

ack, I will get rid of it. Thx.

Regards,
Lorenzo

> 
> 
> > -		if (skb_linearize(skb))
> > -			goto do_drop;
> > +
> > +		/* XDP does not support fraglist */
> > +		if (skb_has_frag_list(skb) || !xdp_prog->aux->xdp_has_frags) {
> > +			if (skb_linearize(skb))
> > +				goto do_drop;
> > +		}
> >  	}
> >  
> >  	act = bpf_prog_run_generic_xdp(skb, xdp, xdp_prog);
> > -- 
> > 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]