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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Lorenzo Bianconi <lorenzo@kernel.org> writes:

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

Well, tail adjust is useful for things like encapsulations that need to
add a trailer. Don't see why that wouldn't be something people would
want to do for jumboframes as well?

Not sure I get what the issue is with this either? But having a test
that can be run to validate this on hardware would be great in any case,
I suppose - we've been discussing more general "compliance tests" for
XDP before...

-Toke