[PATCH net-next 1/2] virtio-net: don't reserve space for vnet header for XDP

[PATCH net-next 1/2] virtio-net: don't reserve space for vnet header for XDP

[Jason Wang]

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@redhat.com>
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 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

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

[Jason Wang]

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@redhat.com>
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 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

Re: [PATCH net-next 2/2] virtio-net: fix the XDP truesize calculation for mergeable buffers

[Michael S. Tsirkin]

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@redhat.com>
> Signed-off-by: Jason Wang <jasowang@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

Re: [PATCH net-next 2/2] virtio-net: fix the XDP truesize calculation for mergeable buffers

[Jason Wang]

On 2020/5/6 下午3:37, Michael S. Tsirkin wrote:
> 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@redhat.com>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
> Seems too aggressive, we do not use up the whole page for the size.
>
>
>

For XDP yes, we do:

static unsigned int get_mergeable_buf_len(struct receive_queue *rq,
                       struct ewma_pkt_len *avg_pkt_len,
                       unsigned int room)
{
     const size_t hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
     unsigned int len;

     if (room)
         return PAGE_SIZE - room;

...

Thanks

Re: [PATCH net-next 2/2] virtio-net: fix the XDP truesize calculation for mergeable buffers

[Michael S. Tsirkin]

On Wed, May 06, 2020 at 04:21:15PM +0800, Jason Wang wrote:
> 
> On 2020/5/6 下午3:37, Michael S. Tsirkin wrote:
> > 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@redhat.com>
> > > Signed-off-by: Jason Wang<jasowang@redhat.com>
> > Seems too aggressive, we do not use up the whole page for the size.
> > 
> > 
> > 
> 
> For XDP yes, we do:
> 
> static unsigned int get_mergeable_buf_len(struct receive_queue *rq,
>                       struct ewma_pkt_len *avg_pkt_len,
>                       unsigned int room)
> {
>     const size_t hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>     unsigned int len;
> 
>     if (room)
>         return PAGE_SIZE - room;
> 
> ...
> 
> Thanks

Hmm. But that's only for new buffers. Buffers that were outstanding
before xdp was attached don't use the whole page, do they?




Also, with TCP smallqueues blocking the queue like that might be a problem.
Could you try and check performance impact of this?
I looked at what other drivers do and I see they tend to copy the skb
in XDP_PASS case. ATM we don't normally - but should we?

-- 
MST

Re: [PATCH net-next 2/2] virtio-net: fix the XDP truesize calculation for mergeable buffers

[Jason Wang]

On 2020/5/6 下午8:08, Michael S. Tsirkin wrote:
> On Wed, May 06, 2020 at 04:21:15PM +0800, Jason Wang wrote:
>> On 2020/5/6 下午3:37, Michael S. Tsirkin wrote:
>>> 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@redhat.com>
>>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>>> Seems too aggressive, we do not use up the whole page for the size.
>>>
>>>
>>>
>> For XDP yes, we do:
>>
>> static unsigned int get_mergeable_buf_len(struct receive_queue *rq,
>>                        struct ewma_pkt_len *avg_pkt_len,
>>                        unsigned int room)
>> {
>>      const size_t hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>>      unsigned int len;
>>
>>      if (room)
>>          return PAGE_SIZE - room;
>>
>> ...
>>
>> Thanks
> Hmm. But that's only for new buffers. Buffers that were outstanding
> before xdp was attached don't use the whole page, do they?


They don't and in either case, we've encoded truesize in the ctx. Any 
issue you saw?


>
>
>
>
> Also, with TCP smallqueues blocking the queue like that might be a problem.
> Could you try and check performance impact of this?


I'm not sure I get you, TCP small queue is more about TX I guess. And 
since we've invalidated the vnet header, the performance of XDP_PASS 
won't be good.


> I looked at what other drivers do and I see they tend to copy the skb
> in XDP_PASS case. ATM we don't normally - but should we?


My understanding is XDP runs before skb, so I don't get here. Or maybe 
you can point me the driver you mentioned here? I've checked i40e and 
mlx5e, both of them build skb after XDP.

Thanks

>

Re: [PATCH net-next 1/2] virtio-net: don't reserve space for vnet header for XDP

[Michael S. Tsirkin]

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@redhat.com>
> 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 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

Re: [PATCH net-next 1/2] virtio-net: don't reserve space for vnet header for XDP

[Jason Wang]

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@redhat.com>
>> 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 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

Re: [PATCH net-next 1/2] virtio-net: don't reserve space for vnet header for XDP

[Michael S. Tsirkin]

On Wed, May 06, 2020 at 04:19:40PM +0800, Jason Wang wrote:
> 
> 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

Then what's the problem?

> 
> > 
> > 
> > > Cc: Jesper Dangaard Brouer <brouer@redhat.com>
> > > 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 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

Re: [PATCH net-next 1/2] virtio-net: don't reserve space for vnet header for XDP

[Jason Wang]

On 2020/5/6 下午5:54, Michael S. Tsirkin wrote:
> On Wed, May 06, 2020 at 04:19:40PM +0800, Jason Wang wrote:
>> 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
> Then what's the problem?


Then we can't do anything more than just invalidating vnet header since 
we don't know whether or not the packet has been modified or not.

Technically, XDP can give the driver some hint about whether or not the 
packet has been modified, but AFAIK we haven't implemented this yet.

Thanks


>

Re: [PATCH net-next 1/2] virtio-net: don't reserve space for vnet header for XDP

[Jesper Dangaard Brouer]

On Wed,  6 May 2020 14:16:32 +0800
Jason Wang <jasowang@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@redhat.com>
> 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 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 */
};

Re: [PATCH net-next 1/2] virtio-net: don't reserve space for vnet header for XDP

[Jason Wang]

On 2020/5/6 下午4:21, Jesper Dangaard Brouer wrote:
> On Wed,  6 May 2020 14:16:32 +0800
> Jason Wang <jasowang@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@redhat.com>
>> 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 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;
>
>

Re: [PATCH net-next 1/2] virtio-net: don't reserve space for vnet header for XDP

[Michael S. Tsirkin]

On Wed, May 06, 2020 at 04:34:36PM +0800, Jason Wang wrote:
> 
> On 2020/5/6 下午4:21, Jesper Dangaard Brouer wrote:
> > On Wed,  6 May 2020 14:16:32 +0800
> > Jason Wang <jasowang@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

That's likely to be somewhat expensive.


> 
> > 
> > 
> > > Cc: Jesper Dangaard Brouer <brouer@redhat.com>
> > > 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 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;
> > 
> > 

Re: [PATCH net-next 1/2] virtio-net: don't reserve space for vnet header for XDP

[Jason Wang]

On 2020/5/6 下午5:46, Michael S. Tsirkin wrote:
>>> 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
> That's likely to be somewhat expensive.


Any better approach? Note that it's not the issue that is introduced in 
this patch. Anyhow the header adjustment may just overwrite the vnet 
header even without this patch.

Thanks