Re: [PATCH] virtio: don't enable notifications during polling

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Dec 09, 2019 at 09:09:57PM +0000, Stefan Hajnoczi wrote:
> Virtqueue notifications are not necessary during polling, so we disable
> them.  This allows the guest driver to avoid MMIO vmexits.
> Unfortunately the virtio-blk and virtio-scsi handler functions re-enable
> notifications, defeating this optimization.
> 
> Fix virtio-blk and virtio-scsi emulation so they leave notifications
> disabled.  The key thing to remember for correctness is that polling
> always checks one last time after ending its loop, therefore it's safe
> to lose the race when re-enabling notifications at the end of polling.
> 
> There is a measurable performance improvement of 5-10% with the null-co
> block driver.  Real-life storage configurations will see a smaller
> improvement because the MMIO vmexit overhead contributes less to
> latency.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>


Thanks! I'll queue it for merge after the release. If possible please ping me
after the release to help make sure it didn't get dropped.


> ---
>  hw/block/virtio-blk.c      |  9 +++++++--
>  hw/scsi/virtio-scsi.c      |  9 +++++++--
>  hw/virtio/virtio.c         | 12 ++++++------
>  include/hw/virtio/virtio.h |  1 +
>  4 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 4c357d2928..c4e55fb3de 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -764,13 +764,16 @@ bool virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq)
>  {
>      VirtIOBlockReq *req;
>      MultiReqBuffer mrb = {};
> +    bool suppress_notifications = virtio_queue_get_notification(vq);
>      bool progress = false;
>  
>      aio_context_acquire(blk_get_aio_context(s->blk));
>      blk_io_plug(s->blk);
>  
>      do {
> -        virtio_queue_set_notification(vq, 0);
> +        if (suppress_notifications) {
> +            virtio_queue_set_notification(vq, 0);
> +        }
>  
>          while ((req = virtio_blk_get_request(s, vq))) {
>              progress = true;
> @@ -781,7 +784,9 @@ bool virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq)
>              }
>          }
>  
> -        virtio_queue_set_notification(vq, 1);
> +        if (suppress_notifications) {
> +            virtio_queue_set_notification(vq, 1);
> +        }
>      } while (!virtio_queue_empty(vq));
>  
>      if (mrb.num_reqs) {
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index e8b2b64d09..f080545f48 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -597,12 +597,15 @@ bool virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
>  {
>      VirtIOSCSIReq *req, *next;
>      int ret = 0;
> +    bool suppress_notifications = virtio_queue_get_notification(vq);
>      bool progress = false;
>  
>      QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);
>  
>      do {
> -        virtio_queue_set_notification(vq, 0);
> +        if (suppress_notifications) {
> +            virtio_queue_set_notification(vq, 0);
> +        }
>  
>          while ((req = virtio_scsi_pop_req(s, vq))) {
>              progress = true;
> @@ -622,7 +625,9 @@ bool virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
>              }
>          }
>  
> -        virtio_queue_set_notification(vq, 1);
> +        if (suppress_notifications) {
> +            virtio_queue_set_notification(vq, 1);
> +        }
>      } while (ret != -EINVAL && !virtio_queue_empty(vq));
>  
>      QTAILQ_FOREACH_SAFE(req, &reqs, next, next) {
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 762df12f4c..78e5852296 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -431,6 +431,11 @@ static void virtio_queue_packed_set_notification(VirtQueue *vq, int enable)
>      }
>  }
>  
> +bool virtio_queue_get_notification(VirtQueue *vq)
> +{
> +    return vq->notification;
> +}
> +
>  void virtio_queue_set_notification(VirtQueue *vq, int enable)
>  {
>      vq->notification = enable;
> @@ -3382,17 +3387,12 @@ static bool virtio_queue_host_notifier_aio_poll(void *opaque)
>  {
>      EventNotifier *n = opaque;
>      VirtQueue *vq = container_of(n, VirtQueue, host_notifier);
> -    bool progress;
>  
>      if (!vq->vring.desc || virtio_queue_empty(vq)) {
>          return false;
>      }
>  
> -    progress = virtio_queue_notify_aio_vq(vq);
> -
> -    /* In case the handler function re-enabled notifications */
> -    virtio_queue_set_notification(vq, 0);
> -    return progress;
> +    return virtio_queue_notify_aio_vq(vq);
>  }
>  
>  static void virtio_queue_host_notifier_aio_poll_end(EventNotifier *n)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 3448d67d2a..8ee93873a4 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -224,6 +224,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id);
>  
>  void virtio_notify_config(VirtIODevice *vdev);
>  
> +bool virtio_queue_get_notification(VirtQueue *vq);
>  void virtio_queue_set_notification(VirtQueue *vq, int enable);
>  
>  int virtio_queue_ready(VirtQueue *vq);
> -- 
> 2.23.0