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

["Michael S. Tsirkin" <mst@redhat.com>]

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);