[Qemu-devel] Re: [PATCH 2/5] virtio-net: Limit number of packets sent per TX flush

[Michael S. Tsirkin]

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 <address@hidden>

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 address@hidden
> More majordomo info at  http://vger.kernel.org/majordomo-info.html