Re: [PATCH v17 bpf-next 12/23] bpf: add multi-buff support to the bpf_xdp_adjust_tail() API

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> On Thu,  4 Nov 2021 18:35:32 +0100 Lorenzo Bianconi wrote:
> > This change adds support for tail growing and shrinking for XDP multi-buff.
> > 
> > When called on a multi-buffer packet with a grow request, it will always
> > work on the last fragment of the packet. So the maximum grow size is the
> > last fragments tailroom, i.e. no new buffer will be allocated.
> > 
> > When shrinking, it will work from the last fragment, all the way down to
> > the base buffer depending on the shrinking size. It's important to mention
> > that once you shrink down the fragment(s) are freed, so you can not grow
> > again to the original size.
> 
> > +static int bpf_xdp_mb_increase_tail(struct xdp_buff *xdp, int offset)
> > +{
> > +	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> > +	skb_frag_t *frag = &sinfo->frags[sinfo->nr_frags - 1];
> > +	int size, tailroom;
> > +
> > +	tailroom = xdp->frame_sz - skb_frag_size(frag) - skb_frag_off(frag);
> 
> I know I complained about this before but the assumption that we can
> use all the space up to xdp->frame_sz makes me uneasy.
> 
> Drivers may not expect the idea that core may decide to extend the 
> last frag.. I don't think the skb path would ever do this.
> 
> How do you feel about any of these options: 
>  - dropping this part for now (return an error for increase)
>  - making this an rxq flag or reading the "reserved frag size"
>    from rxq (so that drivers explicitly opt-in)
>  - adding a test that can be run on real NICs
> ?

I think this has been added to be symmetric with bpf_xdp_adjust_tail().
I do think there is a real use-case for it so far so I am fine to just
support the shrink part.

@Eelco, Jesper, Toke: any comments on it?

> 
> > +static int bpf_xdp_mb_shrink_tail(struct xdp_buff *xdp, int offset)
> > +{
> > +	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> > +	int i, n_frags_free = 0, len_free = 0, tlen_free = 0;
> > +
> > +	if (unlikely(offset > ((int)xdp_get_buff_len(xdp) - ETH_HLEN)))
> 
> nit: outer parens unnecessary

ack, I will fix it.

> 
> > +		return -EINVAL;
> 
> 
> > @@ -371,6 +371,7 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
> >  		break;
> >  	}
> >  }
> > +EXPORT_SYMBOL_GPL(__xdp_return);
> 
> Why the export?

ack, I will remove it

Regards,
Lorenzo

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