Re: [PATCH net-next 21/33] virtio_net: add XDP frame size in two code paths

[Jason Wang <jasowang@redhat.com>]

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


On 2020/4/27 下午10:32, Jesper Dangaard Brouer wrote:
> On Mon, 27 Apr 2020 15:21:02 +0800
> Jason Wang<jasowang@redhat.com>  wrote:
>
>> On 2020/4/23 上午12:09, Jesper Dangaard Brouer wrote:
>>> The virtio_net driver is running inside the guest-OS. There are two
>>> XDP receive code-paths in virtio_net, namely receive_small() and
>>> receive_mergeable(). The receive_big() function does not support XDP.
>>>
>>> In receive_small() the frame size is available in buflen. The buffer
>>> backing these frames are allocated in add_recvbuf_small() with same
>>> size, except for the headroom, but tailroom have reserved room for
>>> skb_shared_info. The headroom is encoded in ctx pointer as a value.
>>>
>>> In receive_mergeable() the frame size is more dynamic. There are two
>>> basic cases: (1) buffer size is based on a exponentially weighted
>>> moving average (see DECLARE_EWMA) of packet length. Or (2) in case
>>> virtnet_get_headroom() have any headroom then buffer size is
>>> PAGE_SIZE. The ctx pointer is this time used for encoding two values;
>>> the buffer len "truesize" and headroom. In case (1) if the rx buffer
>>> size is underestimated, the packet will have been split over more
>>> buffers (num_buf info in virtio_net_hdr_mrg_rxbuf placed in top of
>>> buffer area). If that happens the XDP path does a xdp_linearize_page
>>> operation.
>>>
>>> Cc: Jason Wang<jasowang@redhat.com>
>>> Signed-off-by: Jesper Dangaard Brouer<brouer@redhat.com>
>>> ---
>>>    drivers/net/virtio_net.c |   15 ++++++++++++---
>>>    1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 11f722460513..1df3676da185 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -689,6 +689,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>    		xdp.data_end = xdp.data + len;
>>>    		xdp.data_meta = xdp.data;
>>>    		xdp.rxq = &rq->xdp_rxq;
>>> +		xdp.frame_sz = buflen;
>>>    		orig_data = xdp.data;
>>>    		act = bpf_prog_run_xdp(xdp_prog, &xdp);
>>>    		stats->xdp_packets++;
>>> @@ -797,10 +798,11 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>>>    	int offset = buf - page_address(page);
>>>    	struct sk_buff *head_skb, *curr_skb;
>>>    	struct bpf_prog *xdp_prog;
>>> -	unsigned int truesize;
>>> +	unsigned int truesize = mergeable_ctx_to_truesize(ctx);
>>>    	unsigned int headroom = mergeable_ctx_to_headroom(ctx);
>>> -	int err;
>>>    	unsigned int metasize = 0;
>>> +	unsigned int frame_sz;
>>> +	int err;
>>>    
>>>    	head_skb = NULL;
>>>    	stats->bytes += len - vi->hdr_len;
>>> @@ -821,6 +823,11 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>>>    		if (unlikely(hdr->hdr.gso_type))
>>>    			goto err_xdp;
>>>    
>>> +		/* Buffers with headroom use PAGE_SIZE as alloc size,
>>> +		 * see add_recvbuf_mergeable() + get_mergeable_buf_len()
>>> +		 */
>>> +		frame_sz = headroom ? PAGE_SIZE : truesize;
>>> +
>>>    		/* This happens when rx buffer size is underestimated
>>>    		 * or headroom is not enough because of the buffer
>>>    		 * was refilled before XDP is set. This should only
>>> @@ -834,6 +841,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>>>    						      page, offset,
>>>    						      VIRTIO_XDP_HEADROOM,
>>>    						      &len);
>>> +			frame_sz = PAGE_SIZE;
>> Should this be PAGE_SIZE -  SKB_DATA_ALIGN(sizeof(struct skb_shared_info))?
> No, frame_sz include the SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) length.


Ok, consider mergeable buffer path depends on the truesize which is 
encoded in ctx.

It looks to the the calculation in add_recvfbuf_mergeable() is wrong, we 
need count both headroom and tailroom there.

We probably need the attached 2 patches to fix this.

(untested, will test it tomorrow).

Thanks



[-- Attachment #2: 0002-virtio-net-fix-the-XDP-truesize-calculation-for-merg.patch --]
[-- Type: text/x-patch, Size: 1825 bytes --]

From c2778eb8ee4b7558bccb53f2fc7f1b0aaf1fcb58 Mon Sep 17 00:00:00 2001
From: Jason Wang <jasowang@redhat.com>
Date: Tue, 28 Apr 2020 11:37:39 +0800
Subject: [PATCH 2/2] virtio-net: fix the XDP truesize calculation for
 mergeable buffers

We should not exclude headroom and tailroom when XDP is set. So this
patch fixes this by initializing the truesize from PAGE_SIZE when XDP
is set.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9bdaf2425e6e..f9ba5275e447 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1172,7 +1172,7 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
 	char *buf;
 	void *ctx;
 	int err;
-	unsigned int len, hole;
+	unsigned int len, hole, truesize;
 
 	/* Extra tailroom is needed to satisfy XDP's assumption. This
 	 * means rx frags coalescing won't work, but consider we've
@@ -1182,6 +1182,7 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
 	if (unlikely(!skb_page_frag_refill(len + room, alloc_frag, gfp)))
 		return -ENOMEM;
 
+	truesize = headroom ? PAGE_SIZE: len;
 	buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
 	buf += headroom; /* advance address leaving hole at front of pkt */
 	get_page(alloc_frag->page);
@@ -1193,11 +1194,12 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
 		 * the current buffer.
 		 */
 		len += hole;
+		truesize += hole;
 		alloc_frag->offset += hole;
 	}
 
 	sg_init_one(rq->sg, buf, len);
-	ctx = mergeable_len_to_ctx(len, headroom);
+	ctx = mergeable_len_to_ctx(truesize, headroom);
 	err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
 	if (err < 0)
 		put_page(virt_to_head_page(buf));
-- 
2.20.1


[-- Attachment #3: 0001-virtio-net-don-t-reserve-space-for-vnet-header-for-X.patch --]
[-- Type: text/x-patch, Size: 1738 bytes --]

From 307ac87e823fde059be3bb5a7bdd3ffd3b18521d Mon Sep 17 00:00:00 2001
From: Jason Wang <jasowang@redhat.com>
Date: Tue, 28 Apr 2020 11:31:47 +0800
Subject: [PATCH 1/2] virtio-net: don't reserve space for vnet header for XDP

We tried to reserve space for vnet header before
xdp.data_hard_start. But this is useless since the packet could be
modified by XDP which may invalidate the information stored in the
header and there's no way for XDP to know the existence of the vnet
header currently.

So let's just not reserve space for vnet header in this case.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 2fe7a3188282..9bdaf2425e6e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -681,8 +681,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
 			page = xdp_page;
 		}
 
-		xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
-		xdp.data = xdp.data_hard_start + xdp_headroom;
+		xdp.data_hard_start = buf + VIRTNET_RX_PAD;
+		xdp.data = xdp.data_hard_start + xdp_headroom + vi->hdr_len;;
 		xdp_set_data_meta_invalid(&xdp);
 		xdp.data_end = xdp.data + len;
 		xdp.rxq = &rq->xdp_rxq;
@@ -837,7 +837,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 		 * the descriptor on if we get an XDP_TX return code.
 		 */
 		data = page_address(xdp_page) + offset;
-		xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
+		xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM;
 		xdp.data = data + vi->hdr_len;
 		xdp_set_data_meta_invalid(&xdp);
 		xdp.data_end = xdp.data + (len - vi->hdr_len);
-- 
2.20.1