virtio-net: More configurability and bh handling for tx

hw/virtio-net.c +2 -1

index 075f72d..9ef29f0 100644
@@ -36,6 +36,7 @@ typedef struct VirtIONet
     VirtQueue *ctrl_vq;
     NICState *nic;
     QEMUTimer *tx_timer;
+    uint32_t tx_timeout;
     int tx_timer_active;
     uint32_t has_vnet_hdr;
     uint8_t has_ufo;

@@ -702,7 +703,7 @@ static void virtio_net_handle_tx(VirtIODevice *vdev, VirtQueue *vq)
         virtio_net_flush_tx(n, vq);
     } else {
         qemu_mod_timer(n->tx_timer,
-                       qemu_get_clock(vm_clock) + TX_TIMER_INTERVAL);
+                       qemu_get_clock(vm_clock) + n->tx_timeout);
         n->tx_timer_active = 1;
         virtio_queue_set_notification(vq, 0);
     }

Quoted 23 lines · * Alex Williamson ([email protected]) wrote:

hw/virtio-net.c +2 -1

index 075f72d..9ef29f0 100644
@@ -36,6 +36,7 @@ typedef struct VirtIONet
     VirtQueue *ctrl_vq;
     NICState *nic;
     QEMUTimer *tx_timer;
+    uint32_t tx_timeout;
     int tx_timer_active;
     uint32_t has_vnet_hdr;
     uint8_t has_ufo;

@@ -702,7 +703,7 @@ static void virtio_net_handle_tx(VirtIODevice *vdev, VirtQueue *vq)
         virtio_net_flush_tx(n, vq);
     } else {
         qemu_mod_timer(n->tx_timer,
-                       qemu_get_clock(vm_clock) + TX_TIMER_INTERVAL);
+                       qemu_get_clock(vm_clock) + n->tx_timeout);
         n->tx_timer_active = 1;
         virtio_queue_set_notification(vq, 0);
     }

@@ -842,7 +843,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
 

hw/virtio-net.c static int virtio_net_load(QEMUFile *f, void *opaque, int version_id) · line 846

Quoted 4 lines

hw/virtio-net.c +1 -1

     if (n->tx_timer_active) {
         qemu_mod_timer(n->tx_timer,
-                       qemu_get_clock(vm_clock) + TX_TIMER_INTERVAL);
+                       qemu_get_clock(vm_clock) + n->tx_timeout);

I think I'm missing where this is stored? Looks like migration would revert a changed tx_timeout back to 150us.

Quoted 24 lines · Chris Wright

Quoted 23 lines · * Alex Williamson ([email protected]) wrote:

hw/virtio-net.c +2 -1

index 075f72d..9ef29f0 100644
@@ -36,6 +36,7 @@ typedef struct VirtIONet
     VirtQueue *ctrl_vq;
     NICState *nic;
     QEMUTimer *tx_timer;
+    uint32_t tx_timeout;
     int tx_timer_active;
     uint32_t has_vnet_hdr;
     uint8_t has_ufo;

@@ -702,7 +703,7 @@ static void virtio_net_handle_tx(VirtIODevice *vdev, VirtQueue *vq)
         virtio_net_flush_tx(n, vq);
     } else {
         qemu_mod_timer(n->tx_timer,
-                       qemu_get_clock(vm_clock) + TX_TIMER_INTERVAL);
+                       qemu_get_clock(vm_clock) + n->tx_timeout);
         n->tx_timer_active = 1;
         virtio_queue_set_notification(vq, 0);
     }

@@ -842,7 +843,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
 

Quoted 7 lines

hw/virtio-net.c static int virtio_net_load(QEMUFile *f, void *opaque, int version_id) · line 846

Quoted 4 lines

hw/virtio-net.c +1 -1

     if (n->tx_timer_active) {
         qemu_mod_timer(n->tx_timer,
-                       qemu_get_clock(vm_clock) + TX_TIMER_INTERVAL);
+                       qemu_get_clock(vm_clock) + n->tx_timeout);

I think I'm missing where this is stored? Looks like migration would revert a changed tx_timeout back to 150us.

It's not stored, it can be instantiated on the migration target any way you please and we can migrate between different values or even different TX mitigation strategies. If a non-default value is used on the source and you want to maintain the same behavior, the target needs to be started the same way.

If virtio_net_flush_tx is called with notification disabled, we can race with the guest, processing packets at the same rate as they get produced. The trouble is that this means we have no guaranteed exit condition from the function and can spend minutes in there. Currently flush_tx is only called with notification on, which seems to limit us to one pass through the queue per call. An upcoming patch changes this.

One pass through the queue (256) seems to be a good default value for this, balancing latency with throughput. We use a signed int for txburst because 2^31 packets in a burst would take many, many minutes to process and it allows us to easily return a negative value value from virtio_net_flush_tx() to indicate a back-off or error condition.

Signed-off-by Alex Williamson <[email protected]>

---

 hw/s390-virtio-bus.c |    4 +++-
 hw/s390-virtio-bus.h |    2 ++
 hw/syborg_virtio.c   |    6 +++++-
 hw/virtio-net.c      |   23 ++++++++++++++++-------
 hw/virtio-pci.c      |    6 +++++-
 hw/virtio.h          |    2 +-
 6 files changed, 32 insertions(+), 11 deletions(-)

hw/s390-virtio-bus.c +3 -1

index 4da0b40..1483362 100644
@@ -110,7 +110,8 @@ static int s390_virtio_net_init(VirtIOS390Device *dev)
 {
     VirtIODevice *vdev;
 
-    vdev = virtio_net_init((DeviceState *)dev, &dev->nic, dev->txtimer);
+    vdev = virtio_net_init((DeviceState *)dev, &dev->nic,
+                           dev->txtimer, dev->txburst);
     if (!vdev) {
         return -1;
     }

@@ -328,6 +329,7 @@ static VirtIOS390DeviceInfo s390_virtio_net = {
     .qdev.props = (Property[]) {
         DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic),
         DEFINE_PROP_UINT32("txtimer", VirtIOS390Device, txtimer, 1),
+        DEFINE_PROP_INT32("txburst", VirtIOS390Device, txburst, 256),
         DEFINE_PROP_END_OF_LIST(),
     },
 };

hw/s390-virtio-bus.h +2 -0

index 922daf2..808aea0 100644
@@ -45,6 +45,8 @@ typedef struct VirtIOS390Device {
     uint32_t max_virtserial_ports;
     /* Timeout value for virtio-net txtimer, 1 = default, >1 = ns timeout */
     uint32_t txtimer;
+    /* Max tx packets for virtio-net to burst at a time */
+    int32_t txburst;
 } VirtIOS390Device;
 
 typedef struct VirtIOS390Bus {

hw/syborg_virtio.c +5 -1

index c8d731a..7b76972 100644
@@ -70,6 +70,8 @@ typedef struct {
     uint32_t host_features;
     /* Timeout value for virtio-net txtimer, 1 = default, >1 = ns timeout */
     uint32_t txtimer;
+    /* Max tx packets for virtio-net to burst at a time */
+    int32_t txburst;
 } SyborgVirtIOProxy;
 
 static uint32_t syborg_virtio_readl(void *opaque, target_phys_addr_t offset)

@@ -286,7 +288,8 @@ static int syborg_virtio_net_init(SysBusDevice *dev)
     VirtIODevice *vdev;
     SyborgVirtIOProxy *proxy = FROM_SYSBUS(SyborgVirtIOProxy, dev);
 
-    vdev = virtio_net_init(&dev->qdev, &proxy->nic, proxy->txtimer);
+    vdev = virtio_net_init(&dev->qdev, &proxy->nic,
+                           proxy->txtimer, proxy->txburst);
     return syborg_virtio_init(proxy, vdev);
 }
 

@@ -298,6 +301,7 @@ static SysBusDeviceInfo syborg_virtio_net_info = {
         DEFINE_NIC_PROPERTIES(SyborgVirtIOProxy, nic),
         DEFINE_VIRTIO_NET_FEATURES(SyborgVirtIOProxy, host_features),
         DEFINE_PROP_UINT32("txtimer", SyborgVirtIOProxy, txtimer, 1),
+        DEFINE_PROP_INT32("txburst", SyborgVirtIOProxy, txburst, 256),
         DEFINE_PROP_END_OF_LIST(),
     }
 };

hw/virtio-net.c +16 -7

index 9ef29f0..ac4aa8f 100644
@@ -37,6 +37,7 @@ typedef struct VirtIONet
     NICState *nic;
     QEMUTimer *tx_timer;
     uint32_t tx_timeout;
+    int32_t tx_burst;
     int tx_timer_active;
     uint32_t has_vnet_hdr;
     uint8_t has_ufo;

@@ -620,7 +621,7 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
     return size;
 }
 
-static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq);
+static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq);
 
 static void virtio_net_tx_complete(VLANClientState *nc, ssize_t len)
 {

@@ -636,16 +637,18 @@ static void virtio_net_tx_complete(VLANClientState *nc, ssize_t len)
 }
 
 /* TX */
-static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
+static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
 {
     VirtQueueElement elem;
+    int32_t num_packets = 0;
 
-    if (!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK))
-        return;
+    if (!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+        return num_packets;
+    }
 
     if (n->async_tx.elem.out_num) {
         virtio_queue_set_notification(n->tx_vq, 0);
-        return;
+        return num_packets;
     }
 
     while (virtqueue_pop(vq, &elem)) {

@@ -682,14 +685,19 @@ static void virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
             virtio_queue_set_notification(n->tx_vq, 0);
             n->async_tx.elem = elem;
             n->async_tx.len  = len;
-            return;
+            return -EBUSY;
         }
 
         len += ret;
 
         virtqueue_push(vq, &elem, len);
         virtio_notify(&n->vdev, vq);
+
+        if (++num_packets >= n->tx_burst) {
+            break;
+        }
     }
+    return num_packets;
 }
 
 static void virtio_net_handle_tx(VirtIODevice *vdev, VirtQueue *vq)

@@ -905,7 +913,7 @@ static void virtio_net_vmstate_change(void *opaque, int running, int reason)
 }
 
 VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
-                              uint32_t txtimer)
+                              uint32_t txtimer, int32_t txburst)
 {
     VirtIONet *n;
 

@@ -942,6 +950,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
             n->tx_timeout = txtimer;
         }
     }
+    n->tx_burst = txburst;
     n->mergeable_rx_bufs = 0;
     n->promisc = 1; /* for compatibility */
 

hw/virtio-pci.c +5 -1

index e3b9897..e025c09 100644
@@ -109,6 +109,8 @@ typedef struct {
     uint32_t max_virtserial_ports;
     /* Timeout value for virtio-net txtimer, 1 = default, >1 = ns timeout */
     uint32_t txtimer;
+    /* Max tx packets for virtio-net to burst at a time */
+    int32_t txburst;
 } VirtIOPCIProxy;
 
 /* virtio device */

@@ -615,7 +617,8 @@ static int virtio_net_init_pci(PCIDevice *pci_dev)
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
     VirtIODevice *vdev;
 
-    vdev = virtio_net_init(&pci_dev->qdev, &proxy->nic, proxy->txtimer);
+    vdev = virtio_net_init(&pci_dev->qdev, &proxy->nic,
+                           proxy->txtimer, proxy->txburst);
 
     vdev->nvectors = proxy->nvectors;
     virtio_init_pci(proxy, vdev,

@@ -693,6 +696,7 @@ static PCIDeviceInfo virtio_info[] = {
             DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
             DEFINE_NIC_PROPERTIES(VirtIOPCIProxy, nic),
             DEFINE_PROP_UINT32("txtimer", VirtIOPCIProxy, txtimer, 1),
+            DEFINE_PROP_INT32("txburst", VirtIOPCIProxy, txburst, 256),
             DEFINE_PROP_END_OF_LIST(),
         },
         .qdev.reset = virtio_pci_reset,

hw/virtio.h +1 -2

index 77d05e0..4051889 100644
@@ -186,7 +186,7 @@ void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding,
 /* Base devices.  */
 VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf);
 VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
-                              uint32_t txtimer);
+                              uint32_t txtimer, int32_t txburst);
 VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports);
 VirtIODevice *virtio_balloon_init(DeviceState *dev);
 #ifdef CONFIG_LINUX
 
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Based on a patch from Mark McLoughlin, this patch introduces a new bottom half packet transmitter that avoids the latency imposed by the tx_timer approach. Rather than scheduling a timer when a TX packet comes in, schedule a bottom half to be run from the iothread. The bottom half handler first attempts to flush the queue with notification disabled (this is where we could race with a guest without txburst). If we flush a full burst, reschedule immediately. If we send short of a full burst, try to re-enable notification. To avoid a race with TXs that may have occurred, we must then flush again. If we find some packets to send, the guest it probably active, so we can reschedule again.

tx_timer and tx_bh are mutually exclusive, so we can re-use the tx_waiting flag to indicate one or the other needs to be setup. This allows us to seamlessly migrate between timer and bh TX handling.

Signed-off-by Alex Williamson <[email protected]>

---

 hw/virtio-net.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 68 insertions(+), 13 deletions(-)

hw/virtio-net.c +37 -8

index 8b652f2..3288c77 100644
@@ -36,6 +36,7 @@ typedef struct VirtIONet
     VirtQueue *ctrl_vq;
     NICState *nic;
     QEMUTimer *tx_timer;
+    QEMUBH *tx_bh;
     uint32_t tx_timeout;
     int32_t tx_burst;
     int tx_waiting;

@@ -704,16 +705,25 @@ static void virtio_net_handle_tx(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIONet *n = to_virtio_net(vdev);
 
-    if (n->tx_waiting) {
-        virtio_queue_set_notification(vq, 1);
-        qemu_del_timer(n->tx_timer);
-        n->tx_waiting = 0;
-        virtio_net_flush_tx(n, vq);
+    if (n->tx_timer) {
+        if (n->tx_waiting) {
+            virtio_queue_set_notification(vq, 1);
+            qemu_del_timer(n->tx_timer);
+            n->tx_waiting = 0;
+            virtio_net_flush_tx(n, vq);
+        } else {
+            qemu_mod_timer(n->tx_timer,
+                           qemu_get_clock(vm_clock) + n->tx_timeout);
+            n->tx_waiting = 1;
+            virtio_queue_set_notification(vq, 0);
+        }
     } else {
-        qemu_mod_timer(n->tx_timer,
-                       qemu_get_clock(vm_clock) + n->tx_timeout);
+        if (unlikely(n->tx_waiting)) {
+            return;
+        }
+        virtio_queue_set_notification(n->tx_vq, 0);
+        qemu_bh_schedule(n->tx_bh);
         n->tx_waiting = 1;
-        virtio_queue_set_notification(vq, 0);
     }
 }
 

@@ -731,6 +741,41 @@ static void virtio_net_tx_timer(void *opaque)
     virtio_net_flush_tx(n, n->tx_vq);
 }
 
+static void virtio_net_tx_bh(void *opaque)
+{
+    VirtIONet *n = opaque;
+    int32_t ret;
+
+    n->tx_waiting = 0;
+
+    /* Just in case the driver is not ready on more */
+    if (unlikely(!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK)))
+        return;
+
+    ret = virtio_net_flush_tx(n, n->tx_vq);
+    if (ret == -EBUSY) {
+        return; /* Notification re-enable handled by tx_complete */
+    }
+
+    /* If we flush a full burst of packets, assume there are
+     * more coming and immediately reschedule */
+    if (ret >= n->tx_burst) {

hw/virtio-net.c +21 -6

+        virtio_queue_set_notification(n->tx_vq, 0);
+        qemu_bh_schedule(n->tx_bh);
+        n->tx_waiting = 1;
+    }
+}
+
 static void virtio_net_save(QEMUFile *f, void *opaque)
 {
     VirtIONet *n = opaque;

@@ -850,8 +895,12 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     n->mac_table.first_multi = i;
 
     if (n->tx_waiting) {
-        qemu_mod_timer(n->tx_timer,
-                       qemu_get_clock(vm_clock) + n->tx_timeout);
+        if (n->tx_timer) {
+            qemu_mod_timer(n->tx_timer,
+                           qemu_get_clock(vm_clock) + n->tx_timeout);
+        } else {
+            qemu_bh_schedule(n->tx_bh);
+        }
     }
     return 0;
 }

@@ -939,9 +988,9 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
 
     qemu_format_nic_info_str(&n->nic->nc, conf->macaddr.a);
 
-    n->tx_timer = qemu_new_timer(vm_clock, virtio_net_tx_timer, n);
     n->tx_waiting = 0;
     if (txtimer) {
+        n->tx_timer = qemu_new_timer(vm_clock, virtio_net_tx_timer, n);
         if (txtimer == 1) {
             /* For convenience, 1 = "on" = predefined default, anything else
              * specifies and actual timeout value */

@@ -949,6 +998,8 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
         } else {
             n->tx_timeout = txtimer;
         }
+    } else {
+        n->tx_bh = qemu_bh_new(virtio_net_tx_bh, n);
     }
     n->tx_burst = txburst;
     n->mergeable_rx_bufs = 0;

@@ -982,8 +1033,12 @@ void virtio_net_exit(VirtIODevice *vdev)
     qemu_free(n->mac_table.macs);
     qemu_free(n->vlans);
 
-    qemu_del_timer(n->tx_timer);
-    qemu_free_timer(n->tx_timer);
+    if (n->tx_timer) {
+        qemu_del_timer(n->tx_timer);
+        qemu_free_timer(n->tx_timer);
+    } else {
+        qemu_bh_delete(n->tx_bh);
+    }
 
     virtio_cleanup(&n->vdev);
     qemu_del_vlan_client(&n->nic->nc);
 
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 99 lines · Alex Williamson

Based on a patch from Mark McLoughlin, this patch introduces a new bottom half packet transmitter that avoids the latency imposed by the tx_timer approach. Rather than scheduling a timer when a TX packet comes in, schedule a bottom half to be run from the iothread. The bottom half handler first attempts to flush the queue with notification disabled (this is where we could race with a guest without txburst). If we flush a full burst, reschedule immediately. If we send short of a full burst, try to re-enable notification. To avoid a race with TXs that may have occurred, we must then flush again. If we find some packets to send, the guest it probably active, so we can reschedule again.

tx_timer and tx_bh are mutually exclusive, so we can re-use the tx_waiting flag to indicate one or the other needs to be setup. This allows us to seamlessly migrate between timer and bh TX handling.

Signed-off-by Alex Williamson <[email protected]>

---

 hw/virtio-net.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 68 insertions(+), 13 deletions(-)

hw/virtio-net.c +44 -8

index 8b652f2..3288c77 100644
@@ -36,6 +36,7 @@ typedef struct VirtIONet
     VirtQueue *ctrl_vq;
     NICState *nic;
     QEMUTimer *tx_timer;
+    QEMUBH *tx_bh;
     uint32_t tx_timeout;
     int32_t tx_burst;
     int tx_waiting;

@@ -704,16 +705,25 @@ static void virtio_net_handle_tx(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIONet *n = to_virtio_net(vdev);
 
-    if (n->tx_waiting) {
-        virtio_queue_set_notification(vq, 1);
-        qemu_del_timer(n->tx_timer);
-        n->tx_waiting = 0;
-        virtio_net_flush_tx(n, vq);
+    if (n->tx_timer) {
+        if (n->tx_waiting) {
+            virtio_queue_set_notification(vq, 1);
+            qemu_del_timer(n->tx_timer);
+            n->tx_waiting = 0;
+            virtio_net_flush_tx(n, vq);
+        } else {
+            qemu_mod_timer(n->tx_timer,
+                           qemu_get_clock(vm_clock) + n->tx_timeout);
+            n->tx_waiting = 1;
+            virtio_queue_set_notification(vq, 0);
+        }
     } else {
-        qemu_mod_timer(n->tx_timer,
-                       qemu_get_clock(vm_clock) + n->tx_timeout);
+        if (unlikely(n->tx_waiting)) {
+            return;
+        }
+        virtio_queue_set_notification(n->tx_vq, 0);
+        qemu_bh_schedule(n->tx_bh);
         n->tx_waiting = 1;
-        virtio_queue_set_notification(vq, 0);
     }
 }
 

@@ -731,6 +741,41 @@ static void virtio_net_tx_timer(void *opaque)
     virtio_net_flush_tx(n, n->tx_vq);
 }
 
+static void virtio_net_tx_bh(void *opaque)
+{
+    VirtIONet *n = opaque;
+    int32_t ret;
+
+    n->tx_waiting = 0;
+
+    /* Just in case the driver is not ready on more */
+    if (unlikely(!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK)))
+        return;
+
+    ret = virtio_net_flush_tx(n, n->tx_vq);
+    if (ret == -EBUSY) {
+        return; /* Notification re-enable handled by tx_complete */
+    }
+
+    /* If we flush a full burst of packets, assume there are
+     * more coming and immediately reschedule */
+    if (ret >= n->tx_burst) {
+        qemu_bh_schedule(n->tx_bh);
+        n->tx_waiting = 1;
+        return;
+    }
+
+    /* If less than a full burst, re-enable notification and flush
+     * anything that may have come in while we weren't looking.  If

hw/virtio-net.c static void virtio_net_tx_timer(void *opaque) · line 772

Quoted 3 lines

hw/virtio-net.c +3 -0

+     * we find something, assume the guest is still active and reschedule */
+    virtio_queue_set_notification(n->tx_vq, 1);
+    if (virtio_net_flush_tx(n, n->tx_vq) > 0) {

Shouldn't this be virtio_net_flush_tx(n, n->tx_vq) >= n->tx_burst? If we get less than tx_burst, the ring is empty now so no need to reschedule.
Right?

Quoted 64 lines

hw/virtio-net.c +21 -6

+        virtio_queue_set_notification(n->tx_vq, 0);
+        qemu_bh_schedule(n->tx_bh);
+        n->tx_waiting = 1;
+    }
+}
+
 static void virtio_net_save(QEMUFile *f, void *opaque)
 {
     VirtIONet *n = opaque;

@@ -850,8 +895,12 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     n->mac_table.first_multi = i;
 
     if (n->tx_waiting) {
-        qemu_mod_timer(n->tx_timer,
-                       qemu_get_clock(vm_clock) + n->tx_timeout);
+        if (n->tx_timer) {
+            qemu_mod_timer(n->tx_timer,
+                           qemu_get_clock(vm_clock) + n->tx_timeout);
+        } else {
+            qemu_bh_schedule(n->tx_bh);
+        }
     }
     return 0;
 }

@@ -939,9 +988,9 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
 
     qemu_format_nic_info_str(&n->nic->nc, conf->macaddr.a);
 
-    n->tx_timer = qemu_new_timer(vm_clock, virtio_net_tx_timer, n);
     n->tx_waiting = 0;
     if (txtimer) {
+        n->tx_timer = qemu_new_timer(vm_clock, virtio_net_tx_timer, n);
         if (txtimer == 1) {
             /* For convenience, 1 = "on" = predefined default, anything else
              * specifies and actual timeout value */

@@ -949,6 +998,8 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
         } else {
             n->tx_timeout = txtimer;
         }
+    } else {
+        n->tx_bh = qemu_bh_new(virtio_net_tx_bh, n);
     }
     n->tx_burst = txburst;
     n->mergeable_rx_bufs = 0;

@@ -982,8 +1033,12 @@ void virtio_net_exit(VirtIODevice *vdev)
     qemu_free(n->mac_table.macs);
     qemu_free(n->vlans);
 
-    qemu_del_timer(n->tx_timer);
-    qemu_free_timer(n->tx_timer);
+    if (n->tx_timer) {
+        qemu_del_timer(n->tx_timer);
+        qemu_free_timer(n->tx_timer);
+    } else {
+        qemu_bh_delete(n->tx_bh);
+    }
 
     virtio_cleanup(&n->vdev);
     qemu_del_vlan_client(&n->nic->nc);
 
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 100 lines · Michael S. Tsirkin

Quoted 99 lines · Alex Williamson

Based on a patch from Mark McLoughlin, this patch introduces a new bottom half packet transmitter that avoids the latency imposed by the tx_timer approach. Rather than scheduling a timer when a TX packet comes in, schedule a bottom half to be run from the iothread. The bottom half handler first attempts to flush the queue with notification disabled (this is where we could race with a guest without txburst). If we flush a full burst, reschedule immediately. If we send short of a full burst, try to re-enable notification. To avoid a race with TXs that may have occurred, we must then flush again. If we find some packets to send, the guest it probably active, so we can reschedule again.

tx_timer and tx_bh are mutually exclusive, so we can re-use the tx_waiting flag to indicate one or the other needs to be setup. This allows us to seamlessly migrate between timer and bh TX handling.

Signed-off-by Alex Williamson <[email protected]>

---

 hw/virtio-net.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 68 insertions(+), 13 deletions(-)

hw/virtio-net.c +44 -8

index 8b652f2..3288c77 100644
@@ -36,6 +36,7 @@ typedef struct VirtIONet
     VirtQueue *ctrl_vq;
     NICState *nic;
     QEMUTimer *tx_timer;
+    QEMUBH *tx_bh;
     uint32_t tx_timeout;
     int32_t tx_burst;
     int tx_waiting;

@@ -704,16 +705,25 @@ static void virtio_net_handle_tx(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIONet *n = to_virtio_net(vdev);
 
-    if (n->tx_waiting) {
-        virtio_queue_set_notification(vq, 1);
-        qemu_del_timer(n->tx_timer);
-        n->tx_waiting = 0;
-        virtio_net_flush_tx(n, vq);
+    if (n->tx_timer) {
+        if (n->tx_waiting) {
+            virtio_queue_set_notification(vq, 1);
+            qemu_del_timer(n->tx_timer);
+            n->tx_waiting = 0;
+            virtio_net_flush_tx(n, vq);
+        } else {
+            qemu_mod_timer(n->tx_timer,
+                           qemu_get_clock(vm_clock) + n->tx_timeout);
+            n->tx_waiting = 1;
+            virtio_queue_set_notification(vq, 0);
+        }
     } else {
-        qemu_mod_timer(n->tx_timer,
-                       qemu_get_clock(vm_clock) + n->tx_timeout);
+        if (unlikely(n->tx_waiting)) {
+            return;
+        }
+        virtio_queue_set_notification(n->tx_vq, 0);
+        qemu_bh_schedule(n->tx_bh);
         n->tx_waiting = 1;
-        virtio_queue_set_notification(vq, 0);
     }
 }
 

@@ -731,6 +741,41 @@ static void virtio_net_tx_timer(void *opaque)
     virtio_net_flush_tx(n, n->tx_vq);
 }
 
+static void virtio_net_tx_bh(void *opaque)
+{
+    VirtIONet *n = opaque;
+    int32_t ret;
+
+    n->tx_waiting = 0;
+
+    /* Just in case the driver is not ready on more */
+    if (unlikely(!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK)))
+        return;
+
+    ret = virtio_net_flush_tx(n, n->tx_vq);
+    if (ret == -EBUSY) {
+        return; /* Notification re-enable handled by tx_complete */
+    }
+
+    /* If we flush a full burst of packets, assume there are
+     * more coming and immediately reschedule */
+    if (ret >= n->tx_burst) {
+        qemu_bh_schedule(n->tx_bh);
+        n->tx_waiting = 1;
+        return;
+    }
+
+    /* If less than a full burst, re-enable notification and flush
+     * anything that may have come in while we weren't looking.  If

Quoted 8 lines

hw/virtio-net.c static void virtio_net_tx_timer(void *opaque) · line 772

Quoted 3 lines

hw/virtio-net.c +3 -0

+     * we find something, assume the guest is still active and reschedule */
+    virtio_queue_set_notification(n->tx_vq, 1);
+    if (virtio_net_flush_tx(n, n->tx_vq) > 0) {

Shouldn't this be virtio_net_flush_tx(n, n->tx_vq) >= n->tx_burst? If we get less than tx_burst, the ring is empty now so no need to reschedule.
Right?

I suppose it depends on how aggressive we want to be. If the guest put something on the queue between the first flush and this one, then it might be actively transmitting, and if we want to optimize latency, we anticipate that it might continue to transmit and re-schedule. This is taken straight from markmc's rhel5 patch. I wouldn't argue that it's wrong to not reschedule here, but it's clearly less aggressive. Thanks,

Alex

Quoted 64 lines

Quoted 64 lines

hw/virtio-net.c +21 -6

+        virtio_queue_set_notification(n->tx_vq, 0);
+        qemu_bh_schedule(n->tx_bh);
+        n->tx_waiting = 1;
+    }
+}
+
 static void virtio_net_save(QEMUFile *f, void *opaque)
 {
     VirtIONet *n = opaque;

@@ -850,8 +895,12 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
     n->mac_table.first_multi = i;
 
     if (n->tx_waiting) {
-        qemu_mod_timer(n->tx_timer,
-                       qemu_get_clock(vm_clock) + n->tx_timeout);
+        if (n->tx_timer) {
+            qemu_mod_timer(n->tx_timer,
+                           qemu_get_clock(vm_clock) + n->tx_timeout);
+        } else {
+            qemu_bh_schedule(n->tx_bh);
+        }
     }
     return 0;
 }

@@ -939,9 +988,9 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
 
     qemu_format_nic_info_str(&n->nic->nc, conf->macaddr.a);
 
-    n->tx_timer = qemu_new_timer(vm_clock, virtio_net_tx_timer, n);
     n->tx_waiting = 0;
     if (txtimer) {
+        n->tx_timer = qemu_new_timer(vm_clock, virtio_net_tx_timer, n);
         if (txtimer == 1) {
             /* For convenience, 1 = "on" = predefined default, anything else
              * specifies and actual timeout value */

@@ -949,6 +998,8 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
         } else {
             n->tx_timeout = txtimer;
         }
+    } else {
+        n->tx_bh = qemu_bh_new(virtio_net_tx_bh, n);
     }
     n->tx_burst = txburst;
     n->mergeable_rx_bufs = 0;

@@ -982,8 +1033,12 @@ void virtio_net_exit(VirtIODevice *vdev)
     qemu_free(n->mac_table.macs);
     qemu_free(n->vlans);
 
-    qemu_del_timer(n->tx_timer);
-    qemu_free_timer(n->tx_timer);
+    if (n->tx_timer) {
+        qemu_del_timer(n->tx_timer);
+        qemu_free_timer(n->tx_timer);
+    } else {
+        qemu_bh_delete(n->tx_bh);
+    }
 
     virtio_cleanup(&n->vdev);
     qemu_del_vlan_client(&n->nic->nc);
 
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

The bottom half handler shows big improvements over the timer with few downsides, default to it when the iothread is enabled.

Using the following tests, with the guest and host connected via tap+bridge:

guest> netperf -t TCP_STREAM -H $HOST
host> netperf -t TCP_STREAM -H $GUEST
guest> netperf -t UDP_STREAM -H $HOST
host> netperf -t UDP_STREAM -H $GUEST
guest> netperf -t TCP_RR -H $HOST

Results: base throughput, exits/throughput ->
patched throughput, exits/throughput

--enable-io-thread

TCP guest->host 2737.77, 47.82  -> 6767.09, 29.15 = 247%, 61%
TCP host->guest 2231.33, 74.00  -> 4125.80, 67.61 = 185%, 91%
UDP guest->host 6281.68, 14.66  -> 12569.27, 1.98 = 200%, 14%

UDP host->guest 275.91, 289.22 -> 264.80, 293.53 = 96%, 101%
interations/s 1949.65, 82.97 -> 7417.56, 84.31 = 380%, 102%

No --enable-io-thread
TCP guest->host 3041.57, 55.11 -> 1038.93, 517.57 = 34%, 939%

TCP host->guest 2416.03, 76.67 -> 5655.92, 55.52  = 234%, 72%
UDP guest->host 12255.82, 6.11 -> 7775.87, 31.32  = 63%, 513%
UDP host->guest 587.92, 245.95 -> 611.88, 239.92  = 104%, 98%
interations/s   1975.59, 83.21 -> 8935.50, 88.18  = 452%, 106%

Signed-off-by Alex Williamson <[email protected]>

---

 hw/s390-virtio-bus.c |    3 ++-
 hw/syborg_virtio.c   |    3 ++-
 hw/virtio-pci.c      |    3 ++-
 hw/virtio.h          |    6 ++++++
 4 files changed, 12 insertions(+), 3 deletions(-)

hw/s390-virtio-bus.c +2 -1

index 1483362..985f99a 100644
@@ -328,7 +328,8 @@ static VirtIOS390DeviceInfo s390_virtio_net = {
     .qdev.size = sizeof(VirtIOS390Device),
     .qdev.props = (Property[]) {
         DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic),
-        DEFINE_PROP_UINT32("txtimer", VirtIOS390Device, txtimer, 1),
+        DEFINE_PROP_UINT32("txtimer", VirtIOS390Device, txtimer,
+                           TXTIMER_DEFAULT),
         DEFINE_PROP_INT32("txburst", VirtIOS390Device, txburst, 256),
         DEFINE_PROP_END_OF_LIST(),
     },

hw/syborg_virtio.c +2 -1

index 7b76972..ee5746d 100644
@@ -300,7 +300,8 @@ static SysBusDeviceInfo syborg_virtio_net_info = {
     .qdev.props = (Property[]) {
         DEFINE_NIC_PROPERTIES(SyborgVirtIOProxy, nic),
         DEFINE_VIRTIO_NET_FEATURES(SyborgVirtIOProxy, host_features),
-        DEFINE_PROP_UINT32("txtimer", SyborgVirtIOProxy, txtimer, 1),
+        DEFINE_PROP_UINT32("txtimer", SyborgVirtIOProxy, txtimer,
+                           TXTIMER_DEFAULT),
         DEFINE_PROP_INT32("txburst", SyborgVirtIOProxy, txburst, 256),
         DEFINE_PROP_END_OF_LIST(),
     }

hw/virtio-pci.c +2 -1

index e025c09..9740f57 100644
@@ -695,7 +695,8 @@ static PCIDeviceInfo virtio_info[] = {
             DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
             DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
             DEFINE_NIC_PROPERTIES(VirtIOPCIProxy, nic),
-            DEFINE_PROP_UINT32("txtimer", VirtIOPCIProxy, txtimer, 1),
+            DEFINE_PROP_UINT32("txtimer", VirtIOPCIProxy, txtimer,
+                               TXTIMER_DEFAULT),
             DEFINE_PROP_INT32("txburst", VirtIOPCIProxy, txburst, 256),
             DEFINE_PROP_END_OF_LIST(),
         },

hw/virtio.h +0 -0

index 4051889..a1a17a2 100644

hw/virtio.h +0 -1

 /* Base devices.  */
 VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf);
 VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
 
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 29 lines · Alex Williamson

The bottom half handler shows big improvements over the timer with few downsides, default to it when the iothread is enabled.

Using the following tests, with the guest and host connected via tap+bridge:

guest> netperf -t TCP_STREAM -H $HOST
host> netperf -t TCP_STREAM -H $GUEST
guest> netperf -t UDP_STREAM -H $HOST
host> netperf -t UDP_STREAM -H $GUEST
guest> netperf -t TCP_RR -H $HOST

Results: base throughput, exits/throughput ->
patched throughput, exits/throughput

--enable-io-thread

TCP guest->host 2737.77, 47.82  -> 6767.09, 29.15 = 247%, 61%
TCP host->guest 2231.33, 74.00  -> 4125.80, 67.61 = 185%, 91%
UDP guest->host 6281.68, 14.66  -> 12569.27, 1.98 = 200%, 14%

UDP host->guest 275.91, 289.22 -> 264.80, 293.53 = 96%, 101%
interations/s 1949.65, 82.97 -> 7417.56, 84.31 = 380%, 102%

No --enable-io-thread
TCP guest->host 3041.57, 55.11 -> 1038.93, 517.57 = 34%, 939%

TCP host->guest 2416.03, 76.67 -> 5655.92, 55.52  = 234%, 72%
UDP guest->host 12255.82, 6.11 -> 7775.87, 31.32  = 63%, 513%
UDP host->guest 587.92, 245.95 -> 611.88, 239.92  = 104%, 98%
interations/s   1975.59, 83.21 -> 8935.50, 88.18  = 452%, 106%

Quoted 1 line

Signed-off-by Alex Williamson <[email protected]>

parameter having different settings based on config options might surprise some users. I don't think we really need a parameter here ...

Quoted 55 lines

---

 hw/s390-virtio-bus.c |    3 ++-
 hw/syborg_virtio.c   |    3 ++-
 hw/virtio-pci.c      |    3 ++-
 hw/virtio.h          |    6 ++++++
 4 files changed, 12 insertions(+), 3 deletions(-)

hw/s390-virtio-bus.c +2 -1

index 1483362..985f99a 100644
@@ -328,7 +328,8 @@ static VirtIOS390DeviceInfo s390_virtio_net = {
     .qdev.size = sizeof(VirtIOS390Device),
     .qdev.props = (Property[]) {
         DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic),
-        DEFINE_PROP_UINT32("txtimer", VirtIOS390Device, txtimer, 1),
+        DEFINE_PROP_UINT32("txtimer", VirtIOS390Device, txtimer,
+                           TXTIMER_DEFAULT),
         DEFINE_PROP_INT32("txburst", VirtIOS390Device, txburst, 256),
         DEFINE_PROP_END_OF_LIST(),
     },

hw/syborg_virtio.c +2 -1

index 7b76972..ee5746d 100644
@@ -300,7 +300,8 @@ static SysBusDeviceInfo syborg_virtio_net_info = {
     .qdev.props = (Property[]) {
         DEFINE_NIC_PROPERTIES(SyborgVirtIOProxy, nic),
         DEFINE_VIRTIO_NET_FEATURES(SyborgVirtIOProxy, host_features),
-        DEFINE_PROP_UINT32("txtimer", SyborgVirtIOProxy, txtimer, 1),
+        DEFINE_PROP_UINT32("txtimer", SyborgVirtIOProxy, txtimer,
+                           TXTIMER_DEFAULT),
         DEFINE_PROP_INT32("txburst", SyborgVirtIOProxy, txburst, 256),
         DEFINE_PROP_END_OF_LIST(),
     }

hw/virtio-pci.c +2 -1

index e025c09..9740f57 100644
@@ -695,7 +695,8 @@ static PCIDeviceInfo virtio_info[] = {
             DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
             DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
             DEFINE_NIC_PROPERTIES(VirtIOPCIProxy, nic),
-            DEFINE_PROP_UINT32("txtimer", VirtIOPCIProxy, txtimer, 1),
+            DEFINE_PROP_UINT32("txtimer", VirtIOPCIProxy, txtimer,
+                               TXTIMER_DEFAULT),
             DEFINE_PROP_INT32("txburst", VirtIOPCIProxy, txburst, 256),
             DEFINE_PROP_END_OF_LIST(),
         },

hw/virtio.h +0 -0

index 4051889..a1a17a2 100644

hw/virtio.h void virtio_update_irq(VirtIODevice *vdev); · line 191

Quoted 9 lines

hw/virtio.h +6 -0

@@ -183,6 +183,12 @@ void virtio_update_irq(VirtIODevice *vdev);
 void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding,
                         void *opaque);
 
+#ifdef CONFIG_IOTHREAD
+ #define TXTIMER_DEFAULT 0
+#else
+ #define TXTIMER_DEFAULT 1
+#endif
+

Add a comment explaning that this is just a performance optimization?

Quoted 8 lines

hw/virtio.h +0 -1

 /* Base devices.  */
 VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf);
 VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
 
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 31 lines · Michael S. Tsirkin

Quoted 29 lines · Alex Williamson

The bottom half handler shows big improvements over the timer with few downsides, default to it when the iothread is enabled.

Using the following tests, with the guest and host connected via tap+bridge:

guest> netperf -t TCP_STREAM -H $HOST
host> netperf -t TCP_STREAM -H $GUEST
guest> netperf -t UDP_STREAM -H $HOST
host> netperf -t UDP_STREAM -H $GUEST
guest> netperf -t TCP_RR -H $HOST

Results: base throughput, exits/throughput ->
patched throughput, exits/throughput

--enable-io-thread

TCP guest->host 2737.77, 47.82  ->  6767.09, 29.15 = 247%, 61%
TCP host->guest 2231.33, 74.00  ->  4125.80, 67.61 = 185%, 91%
UDP guest->host 6281.68, 14.66  ->  12569.27, 1.98 = 200%, 14%
UDP host->guest 275.91,  289.22 ->  264.80, 293.53 = 96%, 101%
interations/s   1949.65, 82.97  ->  7417.56, 84.31 = 380%, 102%

No --enable-io-thread

TCP guest->host 3041.57, 55.11 ->  1038.93, 517.57 = 34%, 939%
TCP host->guest 2416.03, 76.67 ->  5655.92, 55.52  = 234%, 72%
UDP guest->host 12255.82, 6.11 ->  7775.87, 31.32  = 63%, 513%
UDP host->guest 587.92, 245.95 ->  611.88, 239.92  = 104%, 98%
interations/s   1975.59, 83.21 ->  8935.50, 88.18  = 452%, 106%

Quoted 6 lines

Quoted 2 lines

Signed-off-by Alex Williamson<[email protected]>

parameter having different settings based on config options might surprise some users. I don't think we really need a parameter here ...

I'm not a bit fan of this either, but I'd also prefer not to introduce a regression for a performance difference we know about in advance. It gets even more complicated when we factor in qemu-kvm, as it doesn't build with iothread enabled, but seems to get and even better boost in performance across the board thanks largely to the kvm-irqchip. Should we instead make this a configure option? --enable-virtio-net-txbh?

Thanks,

Alex

Quoted 77 lines

Quoted 55 lines

---

  hw/s390-virtio-bus.c |    3 ++-
  hw/syborg_virtio.c   |    3 ++-
  hw/virtio-pci.c      |    3 ++-
  hw/virtio.h          |    6 ++++++
  4 files changed, 12 insertions(+), 3 deletions(-)

hw/s390-virtio-bus.c +2 -1

index 1483362..985f99a 100644
@@ -328,7 +328,8 @@ static VirtIOS390DeviceInfo s390_virtio_net = {
      .qdev.size = sizeof(VirtIOS390Device),
      .qdev.props = (Property[]) {
          DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic),
-        DEFINE_PROP_UINT32("txtimer", VirtIOS390Device, txtimer, 1),
+        DEFINE_PROP_UINT32("txtimer", VirtIOS390Device, txtimer,
+                           TXTIMER_DEFAULT),
          DEFINE_PROP_INT32("txburst", VirtIOS390Device, txburst, 256),
          DEFINE_PROP_END_OF_LIST(),
      },

hw/syborg_virtio.c +2 -1

index 7b76972..ee5746d 100644
@@ -300,7 +300,8 @@ static SysBusDeviceInfo syborg_virtio_net_info = {
      .qdev.props = (Property[]) {
          DEFINE_NIC_PROPERTIES(SyborgVirtIOProxy, nic),
          DEFINE_VIRTIO_NET_FEATURES(SyborgVirtIOProxy, host_features),
-        DEFINE_PROP_UINT32("txtimer", SyborgVirtIOProxy, txtimer, 1),
+        DEFINE_PROP_UINT32("txtimer", SyborgVirtIOProxy, txtimer,
+                           TXTIMER_DEFAULT),
          DEFINE_PROP_INT32("txburst", SyborgVirtIOProxy, txburst, 256),
          DEFINE_PROP_END_OF_LIST(),
      }

hw/virtio-pci.c +2 -1

index e025c09..9740f57 100644
@@ -695,7 +695,8 @@ static PCIDeviceInfo virtio_info[] = {
              DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
              DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
              DEFINE_NIC_PROPERTIES(VirtIOPCIProxy, nic),
-            DEFINE_PROP_UINT32("txtimer", VirtIOPCIProxy, txtimer, 1),
+            DEFINE_PROP_UINT32("txtimer", VirtIOPCIProxy, txtimer,
+                               TXTIMER_DEFAULT),
              DEFINE_PROP_INT32("txburst", VirtIOPCIProxy, txburst, 256),
              DEFINE_PROP_END_OF_LIST(),
          },

hw/virtio.h +0 -0

index 4051889..a1a17a2 100644

hw/virtio.h void virtio_update_irq(VirtIODevice *vdev); · line 192

Quoted 10 lines

hw/virtio.h +6 -0

@@ -183,6 +183,12 @@ void virtio_update_irq(VirtIODevice *vdev);
  void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding,
                          void *opaque);
 
+#ifdef CONFIG_IOTHREAD
+ #define TXTIMER_DEFAULT 0
+#else
+ #define TXTIMER_DEFAULT 1
+#endif
+
 

Add a comment explaning that this is just a performance optimization?

Quoted 9 lines

hw/virtio.h +0 -0

  /* Base devices.  */
  VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf);

VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 30 lines · Michael S. Tsirkin

Quoted 29 lines · Alex Williamson

The bottom half handler shows big improvements over the timer with few downsides, default to it when the iothread is enabled.

Using the following tests, with the guest and host connected via tap+bridge:

guest> netperf -t TCP_STREAM -H $HOST
host> netperf -t TCP_STREAM -H $GUEST
guest> netperf -t UDP_STREAM -H $HOST
host> netperf -t UDP_STREAM -H $GUEST
guest> netperf -t TCP_RR -H $HOST

Results: base throughput, exits/throughput -> patched throughput, exits/throughput

--enable-io-thread
TCP guest->host 2737.77, 47.82  -> 6767.09, 29.15 = 247%, 61% TCP host->guest 2231.33, 74.00  -> 4125.80, 67.61 = 185%, 91% UDP guest->host 6281.68, 14.66  -> 12569.27, 1.98 = 200%, 14% UDP host->guest 275.91,  289.22 -> 264.80, 293.53 = 96%, 101% interations/s   1949.65, 82.97  -> 7417.56, 84.31 = 380%, 102%

No --enable-io-thread
TCP guest->host 3041.57, 55.11 -> 1038.93, 517.57 = 34%, 939% TCP host->guest 2416.03, 76.67 -> 5655.92, 55.52  = 234%, 72% UDP guest->host 12255.82, 6.11 -> 7775.87, 31.32  = 63%, 513% UDP host->guest 587.92, 245.95 -> 611.88, 239.92  = 104%, 98% interations/s   1975.59, 83.21 -> 8935.50, 88.18  = 452%, 106%

Quoted 5 lines

Quoted 1 line

Signed-off-by Alex Williamson <[email protected]>

parameter having different settings based on config options might surprise some users. I don't think we really need a parameter here ...

I'm not a bit fan of this either, but I'd also prefer not to introduce a regression for a performance difference we know about in advance.  It gets even more complicated when we factor in qemu-kvm, as it doesn't build with iothread enabled, but seems to get and even better boost in performance across the board thanks largely to the kvm-irqchip.  Should we instead make this a configure option?  --enable-virtio-net-txbh? Thanks,

Alex

Quoted 59 lines

Quoted 59 lines

---

 hw/s390-virtio-bus.c |    3 ++-
 hw/syborg_virtio.c   |    3 ++-
 hw/virtio-pci.c      |    3 ++-
 hw/virtio.h          |    6 ++++++

 4 files changed, 12 insertions(+), 3 deletions(-)

hw/s390-virtio-bus.c +2 -1

index 1483362..985f99a 100644
@@ -328,7 +328,8 @@ static VirtIOS390DeviceInfo s390_virtio_net = {
      .qdev.size = sizeof(VirtIOS390Device),
      .qdev.props = (Property[]) {
          DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic),
-        DEFINE_PROP_UINT32("txtimer", VirtIOS390Device, txtimer, 1),
+        DEFINE_PROP_UINT32("txtimer", VirtIOS390Device, txtimer,
+                           TXTIMER_DEFAULT),
          DEFINE_PROP_INT32("txburst", VirtIOS390Device, txburst, 256),
          DEFINE_PROP_END_OF_LIST(),
      },

hw/syborg_virtio.c +2 -1

index 7b76972..ee5746d 100644
@@ -300,7 +300,8 @@ static SysBusDeviceInfo syborg_virtio_net_info = {
      .qdev.props = (Property[]) {
          DEFINE_NIC_PROPERTIES(SyborgVirtIOProxy, nic),
          DEFINE_VIRTIO_NET_FEATURES(SyborgVirtIOProxy, host_features),
-        DEFINE_PROP_UINT32("txtimer", SyborgVirtIOProxy, txtimer, 1),
+        DEFINE_PROP_UINT32("txtimer", SyborgVirtIOProxy, txtimer,
+                           TXTIMER_DEFAULT),
          DEFINE_PROP_INT32("txburst", SyborgVirtIOProxy, txburst, 256),
          DEFINE_PROP_END_OF_LIST(),
      }

hw/virtio-pci.c +2 -1

index e025c09..9740f57 100644
@@ -695,7 +695,8 @@ static PCIDeviceInfo virtio_info[] = {
              DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
              DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
              DEFINE_NIC_PROPERTIES(VirtIOPCIProxy, nic),
-            DEFINE_PROP_UINT32("txtimer", VirtIOPCIProxy, txtimer, 1),
+            DEFINE_PROP_UINT32("txtimer", VirtIOPCIProxy, txtimer,
+                               TXTIMER_DEFAULT),
              DEFINE_PROP_INT32("txburst", VirtIOPCIProxy, txburst, 256),
              DEFINE_PROP_END_OF_LIST(),
          },

hw/virtio.h +0 -0

index 4051889..a1a17a2 100644
@@ -183,6 +183,12 @@ void virtio_update_irq(VirtIODevice *vdev);
  void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding,
                          void *opaque);
 

Quoted 17 lines

hw/virtio.h void virtio_update_irq(VirtIODevice *vdev); · line 191

Quoted 6 lines

hw/virtio.h +6 -0

+#ifdef CONFIG_IOTHREAD
+ #define TXTIMER_DEFAULT 0
+#else
+ #define TXTIMER_DEFAULT 1
+#endif
+

Add a comment explaning that this is just a performance optimization?

Quoted 8 lines

hw/virtio.h +0 -1

  /* Base devices.  */
  VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf);
  VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
 
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 30 lines · Michael S. Tsirkin

Quoted 29 lines · Alex Williamson

The bottom half handler shows big improvements over the timer with few downsides, default to it when the iothread is enabled.

Using the following tests, with the guest and host connected via tap+bridge:

guest> netperf -t TCP_STREAM -H $HOST
host> netperf -t TCP_STREAM -H $GUEST
guest> netperf -t UDP_STREAM -H $HOST
host> netperf -t UDP_STREAM -H $GUEST
guest> netperf -t TCP_RR -H $HOST

Results: base throughput, exits/throughput ->
patched throughput, exits/throughput

--enable-io-thread

TCP guest->host 2737.77, 47.82  -> 6767.09, 29.15 = 247%, 61%
TCP host->guest 2231.33, 74.00  -> 4125.80, 67.61 = 185%, 91%
UDP guest->host 6281.68, 14.66  -> 12569.27, 1.98 = 200%, 14%

UDP host->guest 275.91, 289.22 -> 264.80, 293.53 = 96%, 101%
interations/s 1949.65, 82.97 -> 7417.56, 84.31 = 380%, 102%

No --enable-io-thread
TCP guest->host 3041.57, 55.11 -> 1038.93, 517.57 = 34%, 939%

TCP host->guest 2416.03, 76.67 -> 5655.92, 55.52  = 234%, 72%
UDP guest->host 12255.82, 6.11 -> 7775.87, 31.32  = 63%, 513%
UDP host->guest 587.92, 245.95 -> 611.88, 239.92  = 104%, 98%
interations/s   1975.59, 83.21 -> 8935.50, 88.18  = 452%, 106%

Quoted 5 lines

Quoted 1 line

Signed-off-by Alex Williamson <[email protected]>

parameter having different settings based on config options might surprise some users. I don't think we really need a parameter here ...

I'm not a bit fan of this either, but I'd also prefer not to introduce a regression for a performance difference we know about in advance.

It gets even more complicated when we factor in qemu-kvm, as it doesn't build with iothread enabled, but seems to get and even better boost in performance across the board thanks largely to the kvm-irqchip. Should we instead make this a configure option? --enable-virtio-net-txbh?

Thanks,

Alex

Quoted 75 lines

Quoted 55 lines

---

 hw/s390-virtio-bus.c |    3 ++-
 hw/syborg_virtio.c   |    3 ++-
 hw/virtio-pci.c      |    3 ++-
 hw/virtio.h          |    6 ++++++
 4 files changed, 12 insertions(+), 3 deletions(-)

hw/s390-virtio-bus.c +2 -1

index 1483362..985f99a 100644
@@ -328,7 +328,8 @@ static VirtIOS390DeviceInfo s390_virtio_net = {
     .qdev.size = sizeof(VirtIOS390Device),
     .qdev.props = (Property[]) {
         DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic),
-        DEFINE_PROP_UINT32("txtimer", VirtIOS390Device, txtimer, 1),
+        DEFINE_PROP_UINT32("txtimer", VirtIOS390Device, txtimer,
+                           TXTIMER_DEFAULT),
         DEFINE_PROP_INT32("txburst", VirtIOS390Device, txburst, 256),
         DEFINE_PROP_END_OF_LIST(),
     },

hw/syborg_virtio.c +2 -1

index 7b76972..ee5746d 100644
@@ -300,7 +300,8 @@ static SysBusDeviceInfo syborg_virtio_net_info = {
     .qdev.props = (Property[]) {
         DEFINE_NIC_PROPERTIES(SyborgVirtIOProxy, nic),
         DEFINE_VIRTIO_NET_FEATURES(SyborgVirtIOProxy, host_features),
-        DEFINE_PROP_UINT32("txtimer", SyborgVirtIOProxy, txtimer, 1),
+        DEFINE_PROP_UINT32("txtimer", SyborgVirtIOProxy, txtimer,
+                           TXTIMER_DEFAULT),
         DEFINE_PROP_INT32("txburst", SyborgVirtIOProxy, txburst, 256),
         DEFINE_PROP_END_OF_LIST(),
     }

hw/virtio-pci.c +2 -1

index e025c09..9740f57 100644
@@ -695,7 +695,8 @@ static PCIDeviceInfo virtio_info[] = {
             DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
             DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
             DEFINE_NIC_PROPERTIES(VirtIOPCIProxy, nic),
-            DEFINE_PROP_UINT32("txtimer", VirtIOPCIProxy, txtimer, 1),
+            DEFINE_PROP_UINT32("txtimer", VirtIOPCIProxy, txtimer,
+                               TXTIMER_DEFAULT),
             DEFINE_PROP_INT32("txburst", VirtIOPCIProxy, txburst, 256),
             DEFINE_PROP_END_OF_LIST(),
         },

hw/virtio.h +0 -0

index 4051889..a1a17a2 100644

hw/virtio.h void virtio_update_irq(VirtIODevice *vdev); · line 191

Quoted 9 lines

hw/virtio.h +6 -0

@@ -183,6 +183,12 @@ void virtio_update_irq(VirtIODevice *vdev);
 void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding,
                         void *opaque);
 
+#ifdef CONFIG_IOTHREAD
+ #define TXTIMER_DEFAULT 0
+#else
+ #define TXTIMER_DEFAULT 1
+#endif
+

Add a comment explaning that this is just a performance optimization?

Quoted 8 lines

hw/virtio.h +0 -1

 /* Base devices.  */
 VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf);
 VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
 
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Add the ability to configure the tx_timer timeout and add a bottom half tx handler that typically shows a nice perf boost over the time based approach. See last patch for perf details. Make this the new default when the iothread is enabled. Thanks,

Alex

---

Alex Williamson (5):
virtio-net: Switch default to new bottom half TX handler for iothread
virtio-net: Introduce a new bottom half packet TX
virtio-net: Rename tx_timer_active to tx_waiting
virtio-net: Limit number of packets sent per TX flush
virtio-net: Make tx_timer timeout configurable

 hw/s390-virtio-bus.c |    6 ++
 hw/s390-virtio-bus.h |    4 ++
 hw/syborg_virtio.c   |   10 +++-
 hw/virtio-net.c      |  129 ++++++++++++++++++++++++++++++++++++++++----------
 hw/virtio-net.h      |    2 -
 hw/virtio-pci.c      |   10 +++-
 hw/virtio.h          |    9 +++
 7 files changed, 137 insertions(+), 33 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Add the ability to configure the tx_timer timeout and add a bottom half tx handler that typically shows a nice perf boost over the time based approach. See last patch for perf details. Make this the new default when the iothread is enabled. Thanks,

Alex

---

Alex Williamson (5):
virtio-net: Switch default to new bottom half TX handler for iothread
virtio-net: Introduce a new bottom half packet TX
virtio-net: Rename tx_timer_active to tx_waiting
virtio-net: Limit number of packets sent per TX flush
virtio-net: Make tx_timer timeout configurable

 hw/s390-virtio-bus.c |    6 ++
 hw/s390-virtio-bus.h |    4 ++
 hw/syborg_virtio.c   |   10 +++-
 hw/virtio-net.c      |  129 ++++++++++++++++++++++++++++++++++++++++----------
 hw/virtio-net.h      |    2 -
 hw/virtio-pci.c      |   10 +++-
 hw/virtio.h          |    9 +++
 7 files changed, 137 insertions(+), 33 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Alex Williamson

Add the ability to configure the tx_timer timeout and add a bottom half tx handler that typically shows a nice perf boost over the time based approach. See last patch for perf details. Make this the new default when the iothread is enabled. Thanks,

Quoted 2 lines

Alex

As a further thought, maybe it would help to have a separate namespace for unsupported, developer-only parameters. Thoughts?

Quoted 25 lines

---

Alex Williamson (5):
virtio-net: Switch default to new bottom half TX handler for iothread
virtio-net: Introduce a new bottom half packet TX
virtio-net: Rename tx_timer_active to tx_waiting
virtio-net: Limit number of packets sent per TX flush
virtio-net: Make tx_timer timeout configurable

  hw/s390-virtio-bus.c |    6 ++
  hw/s390-virtio-bus.h |    4 ++
  hw/syborg_virtio.c   |   10 +++-
  hw/virtio-net.c      |  129 ++++++++++++++++++++++++++++++++++++++++----------
  hw/virtio-net.h      |    2 -
  hw/virtio-pci.c      |   10 +++-
  hw/virtio.h          |    9 +++
  7 files changed, 137 insertions(+), 33 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · Michael S. Tsirkin

Quoted 7 lines · Alex Williamson

Add the ability to configure the tx_timer timeout and add a bottom half tx handler that typically shows a nice perf boost over the time based approach. See last patch for perf details. Make this the new default when the iothread is enabled. Thanks,

Alex

Quoted 4 lines

As a further thought, maybe it would help to have a separate namespace for unsupported, developer-only parameters. Thoughts?

We already have an undocumented one, just prefix with 'x-' to indicate that it's experimental.

Quoted 15 lines · Anthony Liguori

Quoted 13 lines · Michael S. Tsirkin

Quoted 7 lines · Alex Williamson

Add the ability to configure the tx_timer timeout and add a bottom half tx handler that typically shows a nice perf boost over the time based approach. See last patch for perf details. Make this the new default when the iothread is enabled. Thanks,

Alex

As a further thought, maybe it would help to have a separate namespace for unsupported, developer-only parameters. Thoughts?

Quoted 2 lines

We already have an undocumented one, just prefix with 'x-' to indicate that it's experimental.

Any objection then to x-txburst and x-txtimer then? Easy enough to rename. Thanks,

Alex