[Qemu-devel] [PATCH] virtio-net: keep the packet layout intact

[Qemu-devel] [PATCH] virtio-net: keep the packet layout intact

[Wei Wang]

The current implementation may change the packet layout when
vnet_hdr needs an endianness swap. The layout change causes
one more iov to be added to the iov[] passed from the guest, which
is a barrier to making the TX queue size 1024 due to the possible
off-by-one issue.

This patch changes the implementation to remain the packet layout
intact. In this case, the number of iov[] passed to writev will be
equal to the number obtained from the guest.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 hw/net/virtio-net.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 7d091c9..a51e9a8 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1287,8 +1287,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
     for (;;) {
         ssize_t ret;
         unsigned int out_num;
-        struct iovec sg[VIRTQUEUE_MAX_SIZE], sg2[VIRTQUEUE_MAX_SIZE + 1], *out_sg;
-        struct virtio_net_hdr_mrg_rxbuf mhdr;
+        struct iovec sg[VIRTQUEUE_MAX_SIZE], mhdr, *out_sg;
 
         elem = virtqueue_pop(q->tx_vq, sizeof(VirtQueueElement));
         if (!elem) {
@@ -1305,7 +1304,10 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
         }
 
         if (n->has_vnet_hdr) {
-            if (iov_to_buf(out_sg, out_num, 0, &mhdr, n->guest_hdr_len) <
+            /* Buffer to copy vnet_hdr and the possible adjacent data */
+            mhdr.iov_len = out_sg[0].iov_len;
+            mhdr.iov_base = g_malloc0(mhdr.iov_len);
+            if (iov_to_buf(out_sg, out_num, 0, mhdr.iov_base, mhdr.iov_len) <
                 n->guest_hdr_len) {
                 virtio_error(vdev, "virtio-net header incorrect");
                 virtqueue_detach_element(q->tx_vq, elem, 0);
@@ -1313,17 +1315,12 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
                 return -EINVAL;
             }
             if (n->needs_vnet_hdr_swap) {
-                virtio_net_hdr_swap(vdev, (void *) &mhdr);
-                sg2[0].iov_base = &mhdr;
-                sg2[0].iov_len = n->guest_hdr_len;
-                out_num = iov_copy(&sg2[1], ARRAY_SIZE(sg2) - 1,
-                                   out_sg, out_num,
-                                   n->guest_hdr_len, -1);
-                if (out_num == VIRTQUEUE_MAX_SIZE) {
-                    goto drop;
-		}
-                out_num += 1;
-                out_sg = sg2;
+                virtio_net_hdr_swap(vdev, mhdr.iov_base);
+                /* Copy the first iov where the vnet_hdr resides in */
+                out_num = iov_copy(sg, 1, &mhdr, 1, 0, mhdr.iov_len);
+                out_num += iov_copy(sg + 1, ARRAY_SIZE(sg) - 1, out_sg + 1,
+                                    elem->out_num - 1, 0, -1);
+                out_sg = sg;
 	    }
         }
         /*
@@ -1345,13 +1342,15 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
 
         ret = qemu_sendv_packet_async(qemu_get_subqueue(n->nic, queue_index),
                                       out_sg, out_num, virtio_net_tx_complete);
+        if (n->has_vnet_hdr) {
+            g_free(mhdr.iov_base);
+        }
         if (ret == 0) {
             virtio_queue_set_notification(q->tx_vq, 0);
             q->async_tx.elem = elem;
             return -EBUSY;
         }
 
-drop:
         virtqueue_push(q->tx_vq, elem, 0);
         virtio_notify(vdev, q->tx_vq);
         g_free(elem);
-- 
2.7.4

Re: [Qemu-devel] [PATCH] virtio-net: keep the packet layout intact

[Wei Wang]

Ping for comments, thanks.

On 05/11/2017 12:57 PM, Wei Wang wrote:
> The current implementation may change the packet layout when
> vnet_hdr needs an endianness swap. The layout change causes
> one more iov to be added to the iov[] passed from the guest, which
> is a barrier to making the TX queue size 1024 due to the possible
> off-by-one issue.
>
> This patch changes the implementation to remain the packet layout
> intact. In this case, the number of iov[] passed to writev will be
> equal to the number obtained from the guest.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>   hw/net/virtio-net.c | 29 ++++++++++++++---------------
>   1 file changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 7d091c9..a51e9a8 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1287,8 +1287,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>       for (;;) {
>           ssize_t ret;
>           unsigned int out_num;
> -        struct iovec sg[VIRTQUEUE_MAX_SIZE], sg2[VIRTQUEUE_MAX_SIZE + 1], *out_sg;
> -        struct virtio_net_hdr_mrg_rxbuf mhdr;
> +        struct iovec sg[VIRTQUEUE_MAX_SIZE], mhdr, *out_sg;
>   
>           elem = virtqueue_pop(q->tx_vq, sizeof(VirtQueueElement));
>           if (!elem) {
> @@ -1305,7 +1304,10 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>           }
>   
>           if (n->has_vnet_hdr) {
> -            if (iov_to_buf(out_sg, out_num, 0, &mhdr, n->guest_hdr_len) <
> +            /* Buffer to copy vnet_hdr and the possible adjacent data */
> +            mhdr.iov_len = out_sg[0].iov_len;
> +            mhdr.iov_base = g_malloc0(mhdr.iov_len);
> +            if (iov_to_buf(out_sg, out_num, 0, mhdr.iov_base, mhdr.iov_len) <
>                   n->guest_hdr_len) {
>                   virtio_error(vdev, "virtio-net header incorrect");
>                   virtqueue_detach_element(q->tx_vq, elem, 0);
> @@ -1313,17 +1315,12 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>                   return -EINVAL;
>               }
>               if (n->needs_vnet_hdr_swap) {
> -                virtio_net_hdr_swap(vdev, (void *) &mhdr);
> -                sg2[0].iov_base = &mhdr;
> -                sg2[0].iov_len = n->guest_hdr_len;
> -                out_num = iov_copy(&sg2[1], ARRAY_SIZE(sg2) - 1,
> -                                   out_sg, out_num,
> -                                   n->guest_hdr_len, -1);
> -                if (out_num == VIRTQUEUE_MAX_SIZE) {
> -                    goto drop;
> -		}
> -                out_num += 1;
> -                out_sg = sg2;
> +                virtio_net_hdr_swap(vdev, mhdr.iov_base);
> +                /* Copy the first iov where the vnet_hdr resides in */
> +                out_num = iov_copy(sg, 1, &mhdr, 1, 0, mhdr.iov_len);
> +                out_num += iov_copy(sg + 1, ARRAY_SIZE(sg) - 1, out_sg + 1,
> +                                    elem->out_num - 1, 0, -1);
> +                out_sg = sg;
>   	    }
>           }
>           /*
> @@ -1345,13 +1342,15 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>   
>           ret = qemu_sendv_packet_async(qemu_get_subqueue(n->nic, queue_index),
>                                         out_sg, out_num, virtio_net_tx_complete);
> +        if (n->has_vnet_hdr) {
> +            g_free(mhdr.iov_base);
> +        }
>           if (ret == 0) {
>               virtio_queue_set_notification(q->tx_vq, 0);
>               q->async_tx.elem = elem;
>               return -EBUSY;
>           }
>   
> -drop:
>           virtqueue_push(q->tx_vq, elem, 0);
>           virtio_notify(vdev, q->tx_vq);
>           g_free(elem);

Re: [Qemu-devel] [PATCH] virtio-net: keep the packet layout intact

[Michael S. Tsirkin]

On Mon, May 15, 2017 at 05:29:15PM +0800, Wei Wang wrote:
> Ping for comments, thanks.
> 
> On 05/11/2017 12:57 PM, Wei Wang wrote:
> > The current implementation may change the packet layout when
> > vnet_hdr needs an endianness swap. The layout change causes
> > one more iov to be added to the iov[] passed from the guest, which
> > is a barrier to making the TX queue size 1024 due to the possible
> > off-by-one issue.

It blocks making it 512 but I don't think we can make it 1024
as entries might cross page boundaries and get split.


> > 
> > This patch changes the implementation to remain the packet layout
> > intact. In this case, the number of iov[] passed to writev will be
> > equal to the number obtained from the guest.
> > 
> > Signed-off-by: Wei Wang <wei.w.wang@intel.com>

As this is at the cost of a full data copy, I don't think
this makes sense. We could limit this when sg list does not fit
in 1024.

But I really think we should just add a max s/g field to virtio
and then we'll be free to increase the ring size.

> > ---
> >   hw/net/virtio-net.c | 29 ++++++++++++++---------------
> >   1 file changed, 14 insertions(+), 15 deletions(-)
> > 
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 7d091c9..a51e9a8 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -1287,8 +1287,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> >       for (;;) {
> >           ssize_t ret;
> >           unsigned int out_num;
> > -        struct iovec sg[VIRTQUEUE_MAX_SIZE], sg2[VIRTQUEUE_MAX_SIZE + 1], *out_sg;
> > -        struct virtio_net_hdr_mrg_rxbuf mhdr;
> > +        struct iovec sg[VIRTQUEUE_MAX_SIZE], mhdr, *out_sg;
> >           elem = virtqueue_pop(q->tx_vq, sizeof(VirtQueueElement));
> >           if (!elem) {
> > @@ -1305,7 +1304,10 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> >           }
> >           if (n->has_vnet_hdr) {
> > -            if (iov_to_buf(out_sg, out_num, 0, &mhdr, n->guest_hdr_len) <
> > +            /* Buffer to copy vnet_hdr and the possible adjacent data */
> > +            mhdr.iov_len = out_sg[0].iov_len;
> > +            mhdr.iov_base = g_malloc0(mhdr.iov_len);
> > +            if (iov_to_buf(out_sg, out_num, 0, mhdr.iov_base, mhdr.iov_len) <
> >                   n->guest_hdr_len) {
> >                   virtio_error(vdev, "virtio-net header incorrect");
> >                   virtqueue_detach_element(q->tx_vq, elem, 0);
> > @@ -1313,17 +1315,12 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> >                   return -EINVAL;
> >               }
> >               if (n->needs_vnet_hdr_swap) {
> > -                virtio_net_hdr_swap(vdev, (void *) &mhdr);
> > -                sg2[0].iov_base = &mhdr;
> > -                sg2[0].iov_len = n->guest_hdr_len;
> > -                out_num = iov_copy(&sg2[1], ARRAY_SIZE(sg2) - 1,
> > -                                   out_sg, out_num,
> > -                                   n->guest_hdr_len, -1);
> > -                if (out_num == VIRTQUEUE_MAX_SIZE) {
> > -                    goto drop;
> > -		}
> > -                out_num += 1;
> > -                out_sg = sg2;
> > +                virtio_net_hdr_swap(vdev, mhdr.iov_base);
> > +                /* Copy the first iov where the vnet_hdr resides in */
> > +                out_num = iov_copy(sg, 1, &mhdr, 1, 0, mhdr.iov_len);
> > +                out_num += iov_copy(sg + 1, ARRAY_SIZE(sg) - 1, out_sg + 1,
> > +                                    elem->out_num - 1, 0, -1);
> > +                out_sg = sg;
> >   	    }
> >           }
> >           /*



> > @@ -1345,13 +1342,15 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> >           ret = qemu_sendv_packet_async(qemu_get_subqueue(n->nic, queue_index),
> >                                         out_sg, out_num, virtio_net_tx_complete);
> > +        if (n->has_vnet_hdr) {
> > +            g_free(mhdr.iov_base);
> > +        }
> >           if (ret == 0) {
> >               virtio_queue_set_notification(q->tx_vq, 0);
> >               q->async_tx.elem = elem;
> >               return -EBUSY;
> >           }
> > -drop:
> >           virtqueue_push(q->tx_vq, elem, 0);
> >           virtio_notify(vdev, q->tx_vq);
> >           g_free(elem);

Re: [Qemu-devel] [virtio-dev] Re: [PATCH] virtio-net: keep the packet layout intact

[Wei Wang]

On 05/15/2017 10:46 PM, Michael S. Tsirkin wrote:
> On Mon, May 15, 2017 at 05:29:15PM +0800, Wei Wang wrote:
>> Ping for comments, thanks.
>>
>> On 05/11/2017 12:57 PM, Wei Wang wrote:
>>> The current implementation may change the packet layout when
>>> vnet_hdr needs an endianness swap. The layout change causes
>>> one more iov to be added to the iov[] passed from the guest, which
>>> is a barrier to making the TX queue size 1024 due to the possible
>>> off-by-one issue.
> It blocks making it 512 but I don't think we can make it 1024
> as entries might cross page boundaries and get split.
>

I agree with the performance lose issue you mentioned
below, thanks. To understand more here, could you please
shed some light on "entries can't cross page boundaries"?

Seems the virtio spec doesn't mention that vring_desc entries
shouldn't be in two physically continuous pages. Also didn't
find an issue from the implementation.
On the device side, the writev manual does require the iov[]
array to be in one page only, and the limit to iovcnt is 1024.


>>> This patch changes the implementation to remain the packet layout
>>> intact. In this case, the number of iov[] passed to writev will be
>>> equal to the number obtained from the guest.
>>>
>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> As this is at the cost of a full data copy, I don't think
> this makes sense. We could limit this when sg list does not fit
> in 1024.

Yes, this would cause a performance loss with the layout that data
is adjacent with vnet_hdr. Since you prefer another solution below,
I will skip and not discuss my ideas to avoid that copy.

> But I really think we should just add a max s/g field to virtio
> and then we'll be free to increase the ring size.

Yes, that's also a good way to solve it. So, add a new device
property, "max_chain_size" and a feature bit to detect it?


Best,
Wei

Re: [Qemu-devel] [virtio-dev] Re: [PATCH] virtio-net: keep the packet layout intact

[Wei Wang]

On 05/16/2017 12:27 PM, Wei Wang wrote:
> On 05/15/2017 10:46 PM, Michael S. Tsirkin wrote:
>> On Mon, May 15, 2017 at 05:29:15PM +0800, Wei Wang wrote:
>>> Ping for comments, thanks.
>>>
>>> On 05/11/2017 12:57 PM, Wei Wang wrote:
>>>> The current implementation may change the packet layout when
>>>> vnet_hdr needs an endianness swap. The layout change causes
>>>> one more iov to be added to the iov[] passed from the guest, which
>>>> is a barrier to making the TX queue size 1024 due to the possible
>>>> off-by-one issue.
>> It blocks making it 512 but I don't think we can make it 1024
>> as entries might cross page boundaries and get split.
>>
>
> I agree with the performance lose issue you mentioned
> below, thanks. To understand more here, could you please
> shed some light on "entries can't cross page boundaries"?
>
> Seems the virtio spec doesn't mention that vring_desc entries
> shouldn't be in two physically continuous pages. Also didn't
> find an issue from the implementation.
> On the device side, the writev manual does 
typo - "does" -> "doesn't"


> require the iov[]
> array to be in one page only, and the limit to iovcnt is 1024.
>
>
>>>> This patch changes the implementation to remain the packet layout
>>>> intact. In this case, the number of iov[] passed to writev will be
>>>> equal to the number obtained from the guest.
>>>>
>>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> As this is at the cost of a full data copy, I don't think
>> this makes sense. We could limit this when sg list does not fit
>> in 1024.
>
> Yes, this would cause a performance loss with the layout that data
> is adjacent with vnet_hdr. Since you prefer another solution below,
> I will skip and not discuss my ideas to avoid that copy.
>
>> But I really think we should just add a max s/g field to virtio
>> and then we'll be free to increase the ring size.
>
> Yes, that's also a good way to solve it. So, add a new device
> property, "max_chain_size" and a feature bit to detect it?
>
>

Best,
Wei