[Qemu-devel] Re: [PATCH 4/5] virtio-net: Introduce a new bottom half packet TX

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 2010-08-31 at 23:14 +0300, Michael S. Tsirkin wrote:
> On Fri, Aug 27, 2010 at 04:37:36PM -0600, Alex Williamson wrote:
> > 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 <alex.williamson@redhat.com>
> > ---
> > 
> >  hw/virtio-net.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++---------
> >  1 files changed, 68 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index 8b652f2..3288c77 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -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
> > +     * 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

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