Re: [Virtio-for-kvm] [PATCH 7/7] userspace virtio

[Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

I think we should hold off on this sort of patch at first.  I know it 
improves performance, but it's very hack-ish.  I have a similar patch[1] 
that improves performance more but is even more hack-ish.

I think we have to approach this by not special cases virtio-net to know 
about the tap fd, but to figure out the interface that virtio-net would 
need to be efficient, and then refactor the net interface to look like 
that.  Then we can still support user, pcap, and the other network 
transports.

[1] http://hg.codemonkey.ws/qemu-virtio/file/75cefe566cea/aio-net.diff

Regards,

Anthony Liguori

Dor Laor wrote:
> From f244bcad756c4f761627557bb7f315b1d8f22fb2 Mon Sep 17 00:00:00 2001
> From: Dor Laor <dor.laor-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
> Date: Thu, 20 Dec 2007 13:26:30 +0200
> Subject: [PATCH] [VIRTIO-NET] Rx performance improvement
> The current performance are not good enough, the problem lies
> in qemu tap handling code that caused to pass packets one at
> a time and also to copy them to a temporal buffer.
>
> This patch prevents qemu handlers from reading the tap and instead
> it selects the tap descriptors for virtio devices.
> This eliminates copies and also batch guest notifications (interrupts).
>
> Using this patch the rx performance reaches 800Mbps.
> The patch does not follow qemu's api since the intention is first to have
> a better io in kvm and then to polish it correctly.
>
> Signed-off-by: Dor Laor <dor.laor-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
> ---
> qemu/hw/pc.h         |    2 +-
> qemu/hw/virtio-net.c |  114 
> +++++++++++++++++++++++++++++++++++--------------
> qemu/sysemu.h        |    3 +
> qemu/vl.c            |   23 ++++++++++-
> 4 files changed, 107 insertions(+), 35 deletions(-)
>
> diff --git a/qemu/hw/pc.h b/qemu/hw/pc.h
> index 95471f3..5d4c747 100644
> --- a/qemu/hw/pc.h
> +++ b/qemu/hw/pc.h
> @@ -145,7 +145,7 @@ void isa_ne2000_init(int base, qemu_irq irq, 
> NICInfo *nd);
> /* virtio-net.c */
>
> void *virtio_net_init(PCIBus *bus, NICInfo *nd, int devfn);
> -
> +void virtio_net_poll(void);
>
> /* virtio-blk.h */
> void *virtio_blk_init(PCIBus *bus, uint16_t vendor, uint16_t device,
> diff --git a/qemu/hw/virtio-net.c b/qemu/hw/virtio-net.c
> index f6f1f28..b955a5e 100644
> --- a/qemu/hw/virtio-net.c
> +++ b/qemu/hw/virtio-net.c
> @@ -60,8 +60,13 @@ typedef struct VirtIONet
>     VirtQueue *tx_vq;
>     VLANClientState *vc;
>     int can_receive;
> +    int tap_fd;
> +    struct VirtIONet *next;
> +    int do_notify;
> } VirtIONet;
>
> +static VirtIONet *VirtIONetHead = NULL;
> +
> static VirtIONet *to_virtio_net(VirtIODevice *vdev)
> {
>     return (VirtIONet *)vdev;
> @@ -96,42 +101,81 @@ static int virtio_net_can_receive(void *opaque)
>     return (n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) && 
> n->can_receive;
> }
>
> -static void virtio_net_receive(void *opaque, const uint8_t *buf, int 
> size)
> +void virtio_net_poll(void)
> {
> -    VirtIONet *n = opaque;
> +    VirtIONet *vnet;
> +    int len;
> +    fd_set rfds;
> +    struct timeval tv;
> +    int max_fd = -1;
>     VirtQueueElement elem;
>     struct virtio_net_hdr *hdr;
> -    int offset, i;
> -
> -    /* FIXME: the drivers really need to set their status better */
> -    if (n->rx_vq->vring.avail == NULL) {
> -    n->can_receive = 0;
> -    return;
> -    }
> -
> -    if (virtqueue_pop(n->rx_vq, &elem) == 0) {
> -    /* wait until the guest adds some rx bufs */
> -    n->can_receive = 0;
> -    return;
> -    }
> -
> -    hdr = (void *)elem.in_sg[0].iov_base;
> -    hdr->flags = 0;
> -    hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE;
> -
> -    /* copy in packet.  ugh */
> -    offset = 0;
> -    i = 1;
> -    while (offset < size && i < elem.in_num) {
> -    int len = MIN(elem.in_sg[i].iov_len, size - offset);
> -    memcpy(elem.in_sg[i].iov_base, buf + offset, len);
> -    offset += len;
> -    i++;
> -    }
> +    int did_notify;
> +
> +    FD_ZERO(&rfds);
> +    tv.tv_sec = 0;
> +    tv.tv_usec = 0;
> +
> +    while (1) {
> +
> +         // Prepare the set of device to select from
> +        for (vnet = VirtIONetHead; vnet; vnet = vnet->next) {
> +
> +             vnet->do_notify = 0;
> +             //first check if the driver is ok
> +             if (!virtio_net_can_receive(vnet))
> +                 continue;
> +
> +             /* FIXME: the drivers really need to set their status 
> better */
> +             if (vnet->rx_vq->vring.avail == NULL) {
> +                vnet->can_receive = 0;
> +                continue;
> +             }
> +
> +             FD_SET(vnet->tap_fd, &rfds);
> +             if (max_fd < vnet->tap_fd) max_fd = vnet->tap_fd;
> +        }
> +       +        if (select(max_fd + 1, &rfds, NULL, NULL, &tv) <= 0)
> +            break;
> +
> +       // Now check who has data pending in the tap
> +        for (vnet = VirtIONetHead; vnet; vnet = vnet->next) {
> +
> +             if (!FD_ISSET(vnet->tap_fd, &rfds))
> +                 continue;
> +   +             if (virtqueue_pop(vnet->rx_vq, &elem) == 0) {
> +                vnet->can_receive = 0;
> +                 continue;
> +             }
> +
> +             hdr = (void *)elem.in_sg[0].iov_base;
> +             hdr->flags = 0;
> +             hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE;
> +again:
> +             len = readv(vnet->tap_fd, &elem.in_sg[1], elem.in_num - 1);
> +             if (len == -1)
> +                 if (errno == EINTR || errno == EAGAIN)
> +                     goto again;
> +                 else
> +                     fprintf(stderr, "reading network error %d", len);
> +
> +            virtqueue_push(vnet->rx_vq, &elem, sizeof(*hdr) + len);
> +            vnet->do_notify = 1;
> +        }
> +
> +        /* signal other side */
> +        did_notify = 0;
> +        for (vnet = VirtIONetHead; vnet; vnet = vnet->next)
> +            if (vnet->do_notify) {
> +                virtio_notify(&vnet->vdev, vnet->rx_vq);
> +                did_notify++;
> +            }
> +        if (!did_notify)
> +            break;
> +     }
>
> -    /* signal other side */
> -    virtqueue_push(n->rx_vq, &elem, sizeof(*hdr) + offset);
> -    virtio_notify(&n->vdev, n->rx_vq);
> }
>
> /* TX */
> @@ -174,8 +218,12 @@ void *virtio_net_init(PCIBus *bus, NICInfo *nd, 
> int devfn)
>     n->tx_vq = virtio_add_queue(&n->vdev, 128, virtio_net_handle_tx);
>     n->can_receive = 0;
>     memcpy(n->mac, nd->macaddr, 6);
> -    n->vc = qemu_new_vlan_client(nd->vlan, virtio_net_receive,
> +    n->vc = qemu_new_vlan_client(nd->vlan, NULL,
>                  virtio_net_can_receive, n);
> +    n->tap_fd = get_tap_fd(n->vc->vlan->first_client->opaque);
> +    n->next = VirtIONetHead;
> +    //push the device on top of the list
> +    VirtIONetHead = n;
>
>     return &n->vdev;
> }
> diff --git a/qemu/sysemu.h b/qemu/sysemu.h
> index e20159d..4bedd11 100644
> --- a/qemu/sysemu.h
> +++ b/qemu/sysemu.h
> @@ -66,6 +66,9 @@ void qemu_del_wait_object(HANDLE handle, 
> WaitObjectFunc *func, void *opaque);
> /* TAP win32 */
> int tap_win32_init(VLANState *vlan, const char *ifname);
>
> +/* virtio hack for zero copy receive */
> +int get_tap_fd(void *opaque);
> +
> /* SLIRP */
> void do_info_slirp(void);
>
> diff --git a/qemu/vl.c b/qemu/vl.c
> index 26055a4..eef602a 100644
> --- a/qemu/vl.c
> +++ b/qemu/vl.c
> @@ -3885,8 +3885,26 @@ typedef struct TAPState {
>     VLANClientState *vc;
>     int fd;
>     char down_script[1024];
> +    int no_poll;
> } TAPState;
>
> +int get_tap_fd(void *opaque)
> +{
> +    TAPState *s = opaque;
> +
> +    if (s) {
> +       s->no_poll = 1;
> +       return s->fd;
> +    }
> +    return -1;
> +}
> +
> +static int tap_read_poll(void *opaque)
> +{
> +   TAPState *s = opaque;
> +    return (!s->no_poll);
> +}
> +
> static void tap_receive(void *opaque, const uint8_t *buf, int size)
> {
>     TAPState *s = opaque;
> @@ -3930,9 +3948,10 @@ static TAPState *net_tap_fd_init(VLANState 
> *vlan, int fd)
>     if (!s)
>         return NULL;
>     s->fd = fd;
> +    s->no_poll = 0;
>     enable_sigio_timer(fd);
>     s->vc = qemu_new_vlan_client(vlan, tap_receive, NULL, s);
> -    qemu_set_fd_handler(s->fd, tap_send, NULL, s);
> +    qemu_set_fd_handler2(s->fd, tap_read_poll, tap_send, NULL, s);
>     snprintf(s->vc->info_str, sizeof(s->vc->info_str), "tap: fd=%d", fd);
>     return s;
> }
> @@ -7717,6 +7736,8 @@ void main_loop_wait(int timeout)
>         slirp_select_poll(&rfds, &wfds, &xfds);
>     }
> #endif
> +    virtio_net_poll();
> +
>     qemu_aio_poll();
>
>     if (vm_running) {


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/