[Qemu-devel] [PATCH v3 0/2] virtio: improve virtio devices initialization time

[Qemu-devel] [PATCH v3 0/2] virtio: improve virtio devices initialization time

[Gal Hammer]

Using a cleanup callback function (Version 2 of this patches) in order to
support transactions fails when the same event notifier fd was used by more
than one Memory Region.

This time I ask the caller to do the event notifier cleanup by himself.

Gal Hammer (2):
  virtio: remove event notifier cleanup call on de-assign
  virtio: improve virtio devices initialization time

 hw/block/dataplane/virtio-blk.c |  2 ++
 hw/scsi/virtio-scsi-dataplane.c |  2 ++
 hw/virtio/vhost.c               |  2 ++
 hw/virtio/virtio-bus.c          | 14 ++++++++++----
 hw/virtio/virtio.c              | 22 +++++++++++++++++++++-
 include/hw/virtio/virtio-bus.h  |  2 ++
 6 files changed, 39 insertions(+), 5 deletions(-)

-- 
2.7.5

[Qemu-devel] [PATCH v3 1/2] virtio: remove event notifier cleanup call on de-assign

[Gal Hammer]

The virtio_bus_set_host_notifier function no longer calls
event_notifier_cleanup when a event notifier is removed.

The commit updates the code to match the new behavior and calls
virtio_bus_cleanup_host_notifier after the notifier was de-assign
and no longer in use.

This change is a preparation to allow executing the
virtio_bus_set_host_notifier function in a memory region
transaction.

Signed-off-by: Gal Hammer <ghammer@redhat.com>
---
 hw/block/dataplane/virtio-blk.c |  2 ++
 hw/scsi/virtio-scsi-dataplane.c |  2 ++
 hw/virtio/vhost.c               |  2 ++
 hw/virtio/virtio-bus.c          | 14 ++++++++++----
 hw/virtio/virtio.c              |  2 ++
 include/hw/virtio/virtio-bus.h  |  2 ++
 6 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index f6fc639..2cb9909 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -192,6 +192,7 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
             fprintf(stderr, "virtio-blk failed to set host notifier (%d)\n", r);
             while (i--) {
                 virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+                virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
             }
             goto fail_guest_notifiers;
         }
@@ -267,6 +268,7 @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
 
     for (i = 0; i < nvqs; i++) {
         virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
     }
 
     /* Clean up guest notifier (irq) */
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index add4b3f..1c33322 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -175,6 +175,7 @@ fail_vrings:
     aio_context_release(s->ctx);
     for (i = 0; i < vs->conf.num_queues + 2; i++) {
         virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
     }
     k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
 fail_guest_notifiers:
@@ -213,6 +214,7 @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
 
     for (i = 0; i < vs->conf.num_queues + 2; i++) {
         virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
     }
 
     /* Clean up guest notifier (irq) */
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 386aef8..2ee1f37 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1416,6 +1416,7 @@ fail_vq:
             error_report("vhost VQ %d notifier cleanup error: %d", i, -r);
         }
         assert (e >= 0);
+        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i);
     }
     virtio_device_release_ioeventfd(vdev);
 fail:
@@ -1439,6 +1440,7 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
             error_report("vhost VQ %d notifier cleanup failed: %d", i, -r);
         }
         assert (r >= 0);
+        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i);
     }
     virtio_device_release_ioeventfd(vdev);
 }
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 3042232..f9bc9ea 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -283,20 +283,26 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
         r = k->ioeventfd_assign(proxy, notifier, n, true);
         if (r < 0) {
             error_report("%s: unable to assign ioeventfd: %d", __func__, r);
-            goto cleanup_event_notifier;
+            virtio_bus_cleanup_host_notifier(bus, n);
         }
-        return 0;
     } else {
         k->ioeventfd_assign(proxy, notifier, n, false);
     }
 
-cleanup_event_notifier:
+    return r;
+}
+
+void virtio_bus_cleanup_host_notifier(VirtioBusState *bus, int n)
+{
+    VirtIODevice *vdev = virtio_bus_get_device(bus);
+    VirtQueue *vq = virtio_get_queue(vdev, n);
+    EventNotifier *notifier = virtio_queue_get_host_notifier(vq);
+
     /* Test and clear notifier after disabling event,
      * in case poll callback didn't have time to run.
      */
     virtio_queue_host_notifier_read(notifier);
     event_notifier_cleanup(notifier);
-    return r;
 }
 
 static char *virtio_bus_get_dev_path(DeviceState *dev)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index d6002ee..3667cd6 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2608,6 +2608,7 @@ assign_error:
         event_notifier_set_handler(&vq->host_notifier, NULL);
         r = virtio_bus_set_host_notifier(qbus, n, false);
         assert(r >= 0);
+        virtio_bus_cleanup_host_notifier(qbus, n);
     }
     return err;
 }
@@ -2634,6 +2635,7 @@ static void virtio_device_stop_ioeventfd_impl(VirtIODevice *vdev)
         event_notifier_set_handler(&vq->host_notifier, NULL);
         r = virtio_bus_set_host_notifier(qbus, n, false);
         assert(r >= 0);
+        virtio_bus_cleanup_host_notifier(qbus, n);
     }
 }
 
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index a63c1d2..ced3d2d 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -148,5 +148,7 @@ int virtio_bus_grab_ioeventfd(VirtioBusState *bus);
 void virtio_bus_release_ioeventfd(VirtioBusState *bus);
 /* Switch from/to the generic ioeventfd handler */
 int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign);
+/* Tell the bus that the ioeventfd handler is no longer required. */
+void virtio_bus_cleanup_host_notifier(VirtioBusState *bus, int n);
 
 #endif /* VIRTIO_BUS_H */
-- 
2.7.5

[Qemu-devel] [PATCH v3 2/2] virtio: improve virtio devices initialization time

[Gal Hammer]

The loading time of a VM is quite significant when its virtio
devices use a large amount of virt-queues (e.g. a virtio-serial
device with max_ports=511). Most of the time is spend in the
creation of all the required event notifiers (ioeventfd and memory
regions).

This patch pack all the changes to the memory regions in a
single memory transaction.

Reported-by: Sitong Liu <address@hidden>
Reported-by: Xiaoling Gao <address@hidden>
Signed-off-by: Gal Hammer <ghammer@redhat.com>
---
 hw/virtio/virtio.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 3667cd6..006d3d1 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2572,8 +2572,9 @@ static Property virtio_properties[] = {
 static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
 {
     VirtioBusState *qbus = VIRTIO_BUS(qdev_get_parent_bus(DEVICE(vdev)));
-    int n, r, err;
+    int i, n, r, err;
 
+    memory_region_transaction_begin();
     for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
         VirtQueue *vq = &vdev->vq[n];
         if (!virtio_queue_get_num(vdev, n)) {
@@ -2596,9 +2597,11 @@ static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
         }
         event_notifier_set(&vq->host_notifier);
     }
+    memory_region_transaction_commit();
     return 0;
 
 assign_error:
+    i = n; /* save n for a second iteration after transaction is committed. */
     while (--n >= 0) {
         VirtQueue *vq = &vdev->vq[n];
         if (!virtio_queue_get_num(vdev, n)) {
@@ -2608,7 +2611,14 @@ assign_error:
         event_notifier_set_handler(&vq->host_notifier, NULL);
         r = virtio_bus_set_host_notifier(qbus, n, false);
         assert(r >= 0);
-        virtio_bus_cleanup_host_notifier(qbus, n);
+    }
+    memory_region_transaction_commit();
+
+    while (--i >= 0) {
+        if (!virtio_queue_get_num(vdev, i)) {
+            continue;
+        }
+        virtio_bus_cleanup_host_notifier(qbus, i);
     }
     return err;
 }
@@ -2626,6 +2636,7 @@ static void virtio_device_stop_ioeventfd_impl(VirtIODevice *vdev)
     VirtioBusState *qbus = VIRTIO_BUS(qdev_get_parent_bus(DEVICE(vdev)));
     int n, r;
 
+    memory_region_transaction_begin();
     for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
         VirtQueue *vq = &vdev->vq[n];
 
@@ -2635,6 +2646,13 @@ static void virtio_device_stop_ioeventfd_impl(VirtIODevice *vdev)
         event_notifier_set_handler(&vq->host_notifier, NULL);
         r = virtio_bus_set_host_notifier(qbus, n, false);
         assert(r >= 0);
+    }
+    memory_region_transaction_commit();
+
+    for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
+        if (!virtio_queue_get_num(vdev, n)) {
+            continue;
+        }
         virtio_bus_cleanup_host_notifier(qbus, n);
     }
 }
-- 
2.7.5

Re: [Qemu-devel] [PATCH v3 2/2] virtio: improve virtio devices initialization time

[Michael S. Tsirkin]

On Mon, Jan 29, 2018 at 04:20:57PM +0200, Gal Hammer wrote:
> The loading time of a VM is quite significant when its virtio
> devices use a large amount of virt-queues (e.g. a virtio-serial
> device with max_ports=511). Most of the time is spend in the
> creation of all the required event notifiers (ioeventfd and memory
> regions).
> 
> This patch pack all the changes to the memory regions in a
> single memory transaction.
> 
> Reported-by: Sitong Liu <address@hidden>
> Reported-by: Xiaoling Gao <address@hidden>

Just a small note: the way to hide the address is not to put it in:

Reported-by: Sitong Liu
Reported-by: Xiaoling Gao

a stub email isn't needed, it merely confuses tools.

I've fixed this up for now, no need to worry.

> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> ---
>  hw/virtio/virtio.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 3667cd6..006d3d1 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2572,8 +2572,9 @@ static Property virtio_properties[] = {
>  static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
>  {
>      VirtioBusState *qbus = VIRTIO_BUS(qdev_get_parent_bus(DEVICE(vdev)));
> -    int n, r, err;
> +    int i, n, r, err;
>  
> +    memory_region_transaction_begin();
>      for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
>          VirtQueue *vq = &vdev->vq[n];
>          if (!virtio_queue_get_num(vdev, n)) {
> @@ -2596,9 +2597,11 @@ static int virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
>          }
>          event_notifier_set(&vq->host_notifier);
>      }
> +    memory_region_transaction_commit();
>      return 0;
>  
>  assign_error:
> +    i = n; /* save n for a second iteration after transaction is committed. */
>      while (--n >= 0) {
>          VirtQueue *vq = &vdev->vq[n];
>          if (!virtio_queue_get_num(vdev, n)) {
> @@ -2608,7 +2611,14 @@ assign_error:
>          event_notifier_set_handler(&vq->host_notifier, NULL);
>          r = virtio_bus_set_host_notifier(qbus, n, false);
>          assert(r >= 0);
> -        virtio_bus_cleanup_host_notifier(qbus, n);
> +    }
> +    memory_region_transaction_commit();
> +
> +    while (--i >= 0) {
> +        if (!virtio_queue_get_num(vdev, i)) {
> +            continue;
> +        }
> +        virtio_bus_cleanup_host_notifier(qbus, i);
>      }
>      return err;
>  }
> @@ -2626,6 +2636,7 @@ static void virtio_device_stop_ioeventfd_impl(VirtIODevice *vdev)
>      VirtioBusState *qbus = VIRTIO_BUS(qdev_get_parent_bus(DEVICE(vdev)));
>      int n, r;
>  
> +    memory_region_transaction_begin();
>      for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
>          VirtQueue *vq = &vdev->vq[n];
>  
> @@ -2635,6 +2646,13 @@ static void virtio_device_stop_ioeventfd_impl(VirtIODevice *vdev)
>          event_notifier_set_handler(&vq->host_notifier, NULL);
>          r = virtio_bus_set_host_notifier(qbus, n, false);
>          assert(r >= 0);
> +    }
> +    memory_region_transaction_commit();
> +
> +    for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
> +        if (!virtio_queue_get_num(vdev, n)) {
> +            continue;
> +        }
>          virtio_bus_cleanup_host_notifier(qbus, n);
>      }
>  }
> -- 
> 2.7.5

Re: [Qemu-devel] [PATCH v3 0/2] virtio: improve virtio devices initialization time

[Greg Kurz]

On Mon, 29 Jan 2018 16:20:55 +0200
Gal Hammer <ghammer@redhat.com> wrote:

> Using a cleanup callback function (Version 2 of this patches) in order to
> support transactions fails when the same event notifier fd was used by more
> than one Memory Region.
> 
> This time I ask the caller to do the event notifier cleanup by himself.
> 

Much better ! QEMU no longer aborts. Also I could observe a speed-up when
booting a fedora27 guest with 5 virtio-serial devices (max_ports=511), from
1m40s down to 1m10s. Impressive. :)

For all the patches in this series:

Reviewed-by: Greg Kurz <groug@kaod.org>

and

Tested-by: Greg Kurz <groug@kaod.org>

> Gal Hammer (2):
>   virtio: remove event notifier cleanup call on de-assign
>   virtio: improve virtio devices initialization time
> 
>  hw/block/dataplane/virtio-blk.c |  2 ++
>  hw/scsi/virtio-scsi-dataplane.c |  2 ++
>  hw/virtio/vhost.c               |  2 ++
>  hw/virtio/virtio-bus.c          | 14 ++++++++++----
>  hw/virtio/virtio.c              | 22 +++++++++++++++++++++-
>  include/hw/virtio/virtio-bus.h  |  2 ++
>  6 files changed, 39 insertions(+), 5 deletions(-)
> 

Re: [Qemu-devel] [PATCH v3 0/2] virtio: improve virtio devices initialization time

[Gal Hammer]

Hi Greg,

On Mon, Jan 29, 2018 at 7:47 PM, Greg Kurz <groug@kaod.org> wrote:
> On Mon, 29 Jan 2018 16:20:55 +0200
> Gal Hammer <ghammer@redhat.com> wrote:
>
>> Using a cleanup callback function (Version 2 of this patches) in order to
>> support transactions fails when the same event notifier fd was used by more
>> than one Memory Region.
>>
>> This time I ask the caller to do the event notifier cleanup by himself.
>>
>
> Much better ! QEMU no longer aborts. Also I could observe a speed-up when
> booting a fedora27 guest with 5 virtio-serial devices (max_ports=511), from
> 1m40s down to 1m10s. Impressive. :)

Thanks for the quick review, testing, and the kudos ;-).

After spending some time trying to understand why the previous patch
fails I was able to figure it out. I was using
"disable-legacy=on,disable-modern=off", so I missed the fact that a
"transition" device registers two different memory regions with the
same event fd.

> For all the patches in this series:
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
> and
>
> Tested-by: Greg Kurz <groug@kaod.org>
>
>> Gal Hammer (2):
>>   virtio: remove event notifier cleanup call on de-assign
>>   virtio: improve virtio devices initialization time
>>
>>  hw/block/dataplane/virtio-blk.c |  2 ++
>>  hw/scsi/virtio-scsi-dataplane.c |  2 ++
>>  hw/virtio/vhost.c               |  2 ++
>>  hw/virtio/virtio-bus.c          | 14 ++++++++++----
>>  hw/virtio/virtio.c              | 22 +++++++++++++++++++++-
>>  include/hw/virtio/virtio-bus.h  |  2 ++
>>  6 files changed, 39 insertions(+), 5 deletions(-)
>>
>

BTW: There is a pending patch for kvm that will trim few more seconds
from the boot time...

    Gal.