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>
---
 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

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

On Wed, Dec 11, 2019 at 10:58:43AM -0500, Michael S. Tsirkin wrote:
> 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.

Great, thanks!

Stefan

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>
> ---
>  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(-)

Post-release ping :)

> 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
>