On Fri, Aug 27, 2010 at 04:37:18PM -0600, Alex Williamson wrote:
> 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 <alex.williamson@xxxxxxxxxx>
It might be better not to make this configurable, and simply set
txburst = vq->num in code in a single place, than the magic
256 constant in multiuple places.
Alternatively, let's put a macro in a .h file.
It might also be a good idea to put the above motivation in comments in the code.
> ---
>
> 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(-)
>
> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
> index 4da0b40..1483362 100644
> --- a/hw/s390-virtio-bus.c
> +++ b/hw/s390-virtio-bus.c
> @@ -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(),
> },
> };
> diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
> index 922daf2..808aea0 100644
> --- a/hw/s390-virtio-bus.h
> +++ b/hw/s390-virtio-bus.h
> @@ -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 {
> diff --git a/hw/syborg_virtio.c b/hw/syborg_virtio.c
> index c8d731a..7b76972 100644
> --- a/hw/syborg_virtio.c
> +++ b/hw/syborg_virtio.c
> @@ -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(),
> }
> };
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 9ef29f0..ac4aa8f 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -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 */
>
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index e3b9897..e025c09 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -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,
> diff --git a/hw/virtio.h b/hw/virtio.h
> index 77d05e0..4051889 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -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 majordomo@xxxxxxxxxxxxxxx
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
