Linux Virtualization - May 2020 - [PATCH net-next 2/2] virtio-net: fix the XDP truesize calculation for mergeable buffers

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.

Cc: Jesper Dangaard Brouer <brouer at redhat.com>
Signed-off-by: Jason Wang <jasowang at 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 11f722460513..98dd75b665a5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -684,8 +684,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.data_end = xdp.data + len;
 		xdp.data_meta = xdp.data;
 		xdp.rxq = &rq->xdp_rxq;
@@ -845,7 +845,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.data_end = xdp.data + (len - vi->hdr_len);
 		xdp.data_meta = xdp.data;
-- 
2.20.1

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.

Cc: Jesper Dangaard Brouer <brouer at redhat.com>
Signed-off-by: Jason Wang <jasowang at 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 98dd75b665a5..3f3aa8308918 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1184,7 +1184,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
@@ -1194,6 +1194,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);
@@ -1205,11 +1206,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

On Wed, May 06, 2020 at 02:16:33PM +0800, Jason Wang
wrote:
> 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.
> 
> Cc: Jesper Dangaard Brouer <brouer at redhat.com>
> Signed-off-by: Jason Wang <jasowang at redhat.com>
Seems too aggressive, we do not use up the whole page for the size.


> ---
>  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 98dd75b665a5..3f3aa8308918 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1184,7 +1184,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
> @@ -1194,6 +1194,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);
Is this really just on the XDP path? Looks like a confusing way to
detect that.

> @@ -1205,11 +1206,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

On Wed, May 06, 2020 at 02:16:32PM +0800, Jason Wang
wrote:
> 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.
What do you mean? Doesn't XDP_PASS use the header in the buffer?
> So let's just not reserve space for vnet header in this case.
In any case, we can find out XDP does head adjustments
if we need to.

> Cc: Jesper Dangaard Brouer <brouer at redhat.com>
> Signed-off-by: Jason Wang <jasowang at 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 11f722460513..98dd75b665a5 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -684,8 +684,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.data_end = xdp.data + len;
>  		xdp.data_meta = xdp.data;
>  		xdp.rxq = &rq->xdp_rxq;
> @@ -845,7 +845,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.data_end = xdp.data + (len - vi->hdr_len);
>  		xdp.data_meta = xdp.data;
> -- 
> 2.20.1

On 2020/5/6 ??3:53, Michael S. Tsirkin wrote:
> On Wed, May 06, 2020 at 02:16:32PM +0800, Jason Wang wrote:
>> 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.
> What do you mean? Doesn't XDP_PASS use the header in the buffer?

We don't, see 436c9453a1ac0 ("virtio-net: keep vnet header zeroed after
processing XDP")

If there's other place, it should be a bug.

>
>> So let's just not reserve space for vnet header in this case.
> In any case, we can find out XDP does head adjustments
> if we need to.

But XDP program can modify the packets without adjusting headers.

Thanks

>
>
>> Cc: Jesper Dangaard Brouer <brouer at redhat.com>
>> Signed-off-by: Jason Wang <jasowang at 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 11f722460513..98dd75b665a5 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -684,8 +684,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.data_end = xdp.data + len;
>>   		xdp.data_meta = xdp.data;
>>   		xdp.rxq = &rq->xdp_rxq;
>> @@ -845,7 +845,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.data_end = xdp.data + (len - vi->hdr_len);
>>   		xdp.data_meta = xdp.data;
>> -- 
>> 2.20.1

On Wed,  6 May 2020 14:16:32 +0800
Jason Wang <jasowang at redhat.com> wrote:
> 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
IMHO above statements are wrong. XDP cannot access memory before
xdp.data_hard_start. Thus, it is safe to store a vnet headers before
xdp.data_hard_start. (The sfc driver also use this "before" area).
> there's no way for XDP to know the existence of the vnet header
currently.
It is true that XDP is unaware of this area, which is the way it
should be.  Currently the area will survive after calling BPF/XDP.
After your change it will be overwritten in xdp_frame cases.

> So let's just not reserve space for vnet header in this case.
I think this is a wrong approach!

We are working on supporting GRO multi-buffer for XDP.  The vnet header
contains GRO information (see pahole below sign). It is currently not
used in the XDP case, but we will be working towards using it.  There
are a lot of unanswered questions on how this will be implemented.
Thus, I cannot layout how we are going to leverage this info yet, but
your patch are killing this info, which IHMO is going in the wrong
direction.

> Cc: Jesper Dangaard Brouer <brouer at redhat.com>
> Signed-off-by: Jason Wang <jasowang at 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 11f722460513..98dd75b665a5 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -684,8 +684,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.data_end = xdp.data + len;
>  		xdp.data_meta = xdp.data;
>  		xdp.rxq = &rq->xdp_rxq;
> @@ -845,7 +845,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.data_end = xdp.data + (len - vi->hdr_len);
>  		xdp.data_meta = xdp.data;


-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer



$ pahole -C virtio_net_hdr_mrg_rxbuf drivers/net/virtio_net.o
struct virtio_net_hdr_mrg_rxbuf {
	struct virtio_net_hdr hdr;                       /*     0    10 */
	__virtio16                 num_buffers;          /*    10     2 */

	/* size: 12, cachelines: 1, members: 2 */
	/* last cacheline: 12 bytes */
};


$ pahole -C virtio_net_hdr drivers/net/virtio_net.o
struct virtio_net_hdr {
	__u8                       flags;                /*     0     1 */
	__u8                       gso_type;             /*     1     1 */
	__virtio16                 hdr_len;              /*     2     2 */
	__virtio16                 gso_size;             /*     4     2 */
	__virtio16                 csum_start;           /*     6     2 */
	__virtio16                 csum_offset;          /*     8     2 */

	/* size: 10, cachelines: 1, members: 6 */
	/* last cacheline: 10 bytes */
};

On 2020/5/6 ??4:21, Jesper Dangaard Brouer wrote:
> On Wed,  6 May 2020 14:16:32 +0800
> Jason Wang <jasowang at redhat.com> wrote:
>
>> 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
> IMHO above statements are wrong. XDP cannot access memory before
> xdp.data_hard_start. Thus, it is safe to store a vnet headers before
> xdp.data_hard_start. (The sfc driver also use this "before"
area).

The problem is if we place vnet header before data_hard_start, 
virtio-net will fail any header adjustment.

Or do you mean to copy vnet header before data_hard_start before 
processing XDP?

>
>> there's no way for XDP to know the existence of the vnet header
currently.
> It is true that XDP is unaware of this area, which is the way it
> should be.  Currently the area will survive after calling BPF/XDP.
> After your change it will be overwritten in xdp_frame cases.
>
>
>> So let's just not reserve space for vnet header in this case.
> I think this is a wrong approach!
>
> We are working on supporting GRO multi-buffer for XDP.  The vnet header
> contains GRO information (see pahole below sign).

Another note is that since we need reserve room for skb_shared_info, GRO 
for XDP may probably lead more frag list.

>   It is currently not
> used in the XDP case, but we will be working towards using it.

Good to know that, but I think it can only work when the packet is not 
modified by XDP?

> There
> are a lot of unanswered questions on how this will be implemented.
> Thus, I cannot layout how we are going to leverage this info yet, but
> your patch are killing this info, which IHMO is going in the wrong
> direction.

I can copy vnet header ahead of data_hard_start, does it work for you?

Thanks

>
>
>> Cc: Jesper Dangaard Brouer <brouer at redhat.com>
>> Signed-off-by: Jason Wang <jasowang at 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 11f722460513..98dd75b665a5 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -684,8 +684,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.data_end = xdp.data + len;
>>   		xdp.data_meta = xdp.data;
>>   		xdp.rxq = &rq->xdp_rxq;
>> @@ -845,7 +845,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.data_end = xdp.data + (len - vi->hdr_len);
>>   		xdp.data_meta = xdp.data;
>
>