Remove tx_timer from virtio_net

Avi and I were talking this afternoon and he suggested that we should remove the tx_timer from the virtio_net front-end and replace it with a tx_timer in the backend.

Since the backend can suppress notifications, this is appealing since it gives much more flexibility to the backend in determining how to do tx batching. I've done a quick implementation and performance is pretty good.

We may need an ABI change, however. When the backend disables notifications, there is absolutely no way for the frontend to notify anymore. In the case where the queue fills up, it cannot flush since the backend has disabled notifications. To work around this, I had to least notifications enabled and check for the case where the queue fills up manually.

I think a possible solution to this would be to differentiate between a soft and hard notify. We would introduce a VRING_USED_F_NOTIFY_ON_FULL and a VRING_AVAIL_F_NOTIFY_ON_FULL.

The NO_NOTIFY variants would indicate that the other end never sends a notify, whereas NOTIFY_ON_FULL would indicate that the other end never sends a notify unless the queue fills up.

Regards,

Anthony Liguori

index 3940743..1006dee 100644
@@ -14,6 +14,7 @@
 #include "virtio.h"
 #include "net.h"
 #include "pc.h"
+#include "qemu-timer.h"
 
 /* from Linux's virtio_net.h */
 

@@ -28,6 +29,10 @@
 #define VIRTIO_NET_F_TSO6	4
 #define VIRTIO_NET_F_MAC	5
 
+#define USE_TX_TIMER
+
+#define TX_TIMER_INTERVAL (1000 / 500)
+
 /* The config defining mac address (6 bytes) */
 struct virtio_net_config
 {

@@ -63,6 +68,8 @@ typedef struct VirtIONet
     int tap_fd;
     struct VirtIONet *next;
     int do_notify;
+    QEMUTimer *tx_timer;
+    int tx_timer_active;
 } VirtIONet;
 
 static VirtIONet *VirtIONetHead = NULL;

@@ -222,10 +229,10 @@ again:
 }
 
 /* TX */
-static void virtio_net_handle_tx(VirtIODevice *vdev, VirtQueue *vq)
+static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
 {
-    VirtIONet *n = to_virtio_net(vdev);
     VirtQueueElement elem;
+    int count = 0;
 
     if (!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK))
         return;

@@ -241,11 +248,36 @@ static void virtio_net_handle_tx(VirtIODevice *vdev, VirtQueue *vq)
 	    len += elem.out_sg[i].iov_len;
 	}
 
+	count++;
+
 	virtqueue_push(vq, &elem, sizeof(struct virtio_net_hdr) + len);
 	virtio_notify(&n->vdev, vq);
     }
 }
 
+static void virtio_net_handle_tx(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIONet *n = to_virtio_net(vdev);
+
+    if (n->tx_timer_active &&
+	(vq->vring.avail->idx - vq->last_avail_idx) == 64) {
+	qemu_del_timer(n->tx_timer);
+	n->tx_timer_active = 0;
+	virtio_net_flush_tx(n, vq);
+    } else {
+	qemu_mod_timer(n->tx_timer, qemu_get_clock(vm_clock) + TX_TIMER_INTERVAL);
+	n->tx_timer_active = 1;
+    }
+}
+
+static void virtio_net_tx_timer(void *opaque)
+{
+    VirtIONet *n = opaque;
+
+    n->tx_timer_active = 0;
+    virtio_net_flush_tx(n, n->tx_vq);
+}
+
 void *virtio_net_init(PCIBus *bus, NICInfo *nd, int devfn)
 {
     VirtIONet *n;

@@ -270,5 +302,8 @@ void *virtio_net_init(PCIBus *bus, NICInfo *nd, int devfn)
         VirtIONetHead = n;
     }
 
+    n->tx_timer = qemu_new_timer(vm_clock, virtio_net_tx_timer, n);
+    n->tx_timer_active = 0;
+
     return &n->vdev;
 }

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Hi Anthony,

I really like this (in proportion to my discomfort with the hrtimers hack,
in fact). The good news is that I don't think we need a significant ABI
change: I think we should have the vring_get_buf() failure path kick the other end unconditionally.

This makes sense: the NO_INTERRUPT flag is really a "you don't need to
interrupt me" hint; you're allowed to interrupt even if it's set. So it should be at worst harmless. It should work in the inter-guest case, for example.

Rather than apply this, I will just drop the hrtimer patches, and do a separate "kick on queue full" patch.

Thanks!
Rusty.

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace