On Thu,  4 Nov 2021 18:35:32 +0100 Lorenzo Bianconi wrote:

quoted

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.

quoted

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

quoted

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

quoted

+		return -EINVAL;

quoted

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