* [for-6.1 1/4] virtio-blk: Fix rollback path in virtio_blk_data_plane_start()
2021-04-07 14:34 [for-6.1 0/4] virtio: Improve boot time of virtio-scsi-pci and virtio-blk-pci Greg Kurz
@ 2021-04-07 14:34 ` Greg Kurz
2021-04-07 14:34 ` [for-6.1 2/4] virtio-blk: Configure all host notifiers in a single MR transaction Greg Kurz
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Greg Kurz @ 2021-04-07 14:34 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, qemu-block, Michael S. Tsirkin, Greg Kurz,
Max Reitz, Stefan Hajnoczi, Paolo Bonzini, David Gibson
When dataplane multiqueue support was added in QEMU 2.7, the path
that would rollback guest notifiers assignment in case of error
simply got dropped.
Later on, when Error was added to blk_set_aio_context() in QEMU 4.1,
another error path was introduced, but it ommits to rollback both
host and guest notifiers.
It seems cleaner to fix the rollback path in one go. The patch is
simple enough that it can be adjusted if backported to a pre-4.1
QEMU.
Fixes: 51b04ac5c6a6 ("virtio-blk: dataplane multiqueue support")
Cc: stefanha@redhat.com
Fixes: 97896a4887a0 ("block: Add Error to blk_set_aio_context()")
Cc: kwolf@redhat.com
Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/block/dataplane/virtio-blk.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index e9050c8987e7..d7b5c95d26d9 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -207,7 +207,7 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
}
- goto fail_guest_notifiers;
+ goto fail_host_notifiers;
}
}
@@ -221,7 +221,7 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
aio_context_release(old_context);
if (r < 0) {
error_report_err(local_err);
- goto fail_guest_notifiers;
+ goto fail_aio_context;
}
/* Process queued requests before the ones in vring */
@@ -245,6 +245,13 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
aio_context_release(s->ctx);
return 0;
+ fail_aio_context:
+ 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);
+ }
+ fail_host_notifiers:
+ k->set_guest_notifiers(qbus->parent, nvqs, false);
fail_guest_notifiers:
/*
* If we failed to set up the guest notifiers queued requests will be
--
2.26.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [for-6.1 2/4] virtio-blk: Configure all host notifiers in a single MR transaction
2021-04-07 14:34 [for-6.1 0/4] virtio: Improve boot time of virtio-scsi-pci and virtio-blk-pci Greg Kurz
2021-04-07 14:34 ` [for-6.1 1/4] virtio-blk: Fix rollback path in virtio_blk_data_plane_start() Greg Kurz
@ 2021-04-07 14:34 ` Greg Kurz
2021-05-05 10:14 ` Stefan Hajnoczi
2021-04-07 14:35 ` [for-6.1 3/4] virtio-scsi: Set host notifiers and callbacks separately Greg Kurz
` (3 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Greg Kurz @ 2021-04-07 14:34 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, qemu-block, Michael S. Tsirkin, Greg Kurz,
Max Reitz, Stefan Hajnoczi, Paolo Bonzini, David Gibson
This allows the virtio-blk-pci device to batch the setup of all its
host notifiers. This significantly improves boot time of VMs with a
high number of vCPUs, e.g. from 3m26.186s down to 0m58.023s for a
pseries machine with 384 vCPUs.
Note that memory_region_transaction_commit() must be called before
virtio_bus_cleanup_host_notifier() because the latter might close
ioeventfds that the transaction still assumes to be around when it
commits.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/block/dataplane/virtio-blk.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index d7b5c95d26d9..cd81893d1d01 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -198,19 +198,30 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
goto fail_guest_notifiers;
}
+ memory_region_transaction_begin();
+
/* Set up virtqueue notify */
for (i = 0; i < nvqs; i++) {
r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, true);
if (r != 0) {
+ int j = i;
+
fprintf(stderr, "virtio-blk failed to set host notifier (%d)\n", r);
while (i--) {
virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+ }
+
+ memory_region_transaction_commit();
+
+ while (j--) {
virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
}
goto fail_host_notifiers;
}
}
+ memory_region_transaction_commit();
+
s->starting = false;
vblk->dataplane_started = true;
trace_virtio_blk_data_plane_start(s);
@@ -246,8 +257,15 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
return 0;
fail_aio_context:
+ memory_region_transaction_begin();
+
for (i = 0; i < nvqs; i++) {
virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+ }
+
+ memory_region_transaction_commit();
+
+ for (i = 0; i < nvqs; i++) {
virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
}
fail_host_notifiers:
@@ -312,8 +330,15 @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
aio_context_release(s->ctx);
+ memory_region_transaction_begin();
+
for (i = 0; i < nvqs; i++) {
virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+ }
+
+ memory_region_transaction_commit();
+
+ for (i = 0; i < nvqs; i++) {
virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
}
--
2.26.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [for-6.1 2/4] virtio-blk: Configure all host notifiers in a single MR transaction
2021-04-07 14:34 ` [for-6.1 2/4] virtio-blk: Configure all host notifiers in a single MR transaction Greg Kurz
@ 2021-05-05 10:14 ` Stefan Hajnoczi
2021-05-05 11:15 ` Greg Kurz
0 siblings, 1 reply; 12+ messages in thread
From: Stefan Hajnoczi @ 2021-05-05 10:14 UTC (permalink / raw)
To: Greg Kurz
Cc: Kevin Wolf, Fam Zheng, qemu-block, Michael S. Tsirkin,
qemu-devel, Max Reitz, Paolo Bonzini, David Gibson
[-- Attachment #1: Type: text/plain, Size: 2356 bytes --]
On Wed, Apr 07, 2021 at 04:34:59PM +0200, Greg Kurz wrote:
> This allows the virtio-blk-pci device to batch the setup of all its
> host notifiers. This significantly improves boot time of VMs with a
> high number of vCPUs, e.g. from 3m26.186s down to 0m58.023s for a
> pseries machine with 384 vCPUs.
>
> Note that memory_region_transaction_commit() must be called before
> virtio_bus_cleanup_host_notifier() because the latter might close
> ioeventfds that the transaction still assumes to be around when it
> commits.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> hw/block/dataplane/virtio-blk.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index d7b5c95d26d9..cd81893d1d01 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -198,19 +198,30 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
> goto fail_guest_notifiers;
> }
>
> + memory_region_transaction_begin();
This call is optional and an optimization. It would be nice to have a
comment here explaining that - otherwise people may wonder why this is
necessary.
> +
> /* Set up virtqueue notify */
> for (i = 0; i < nvqs; i++) {
> r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, true);
> if (r != 0) {
> + int j = i;
> +
> fprintf(stderr, "virtio-blk failed to set host notifier (%d)\n", r);
> while (i--) {
> virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
> + }
> +
> + memory_region_transaction_commit();
> +
> + while (j--) {
> virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
> }
> goto fail_host_notifiers;
> }
> }
>
> + memory_region_transaction_commit();
> +
> s->starting = false;
> vblk->dataplane_started = true;
> trace_virtio_blk_data_plane_start(s);
> @@ -246,8 +257,15 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
> return 0;
>
> fail_aio_context:
> + memory_region_transaction_begin();
Probably unnecessary since this is an error code path. We don't need to
optimize it.
Doesn't hurt though.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [for-6.1 2/4] virtio-blk: Configure all host notifiers in a single MR transaction
2021-05-05 10:14 ` Stefan Hajnoczi
@ 2021-05-05 11:15 ` Greg Kurz
2021-05-06 8:15 ` Michael S. Tsirkin
0 siblings, 1 reply; 12+ messages in thread
From: Greg Kurz @ 2021-05-05 11:15 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Fam Zheng, qemu-block, Michael S. Tsirkin,
qemu-devel, Max Reitz, Paolo Bonzini, David Gibson
[-- Attachment #1: Type: text/plain, Size: 2766 bytes --]
On Wed, 5 May 2021 11:14:23 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Wed, Apr 07, 2021 at 04:34:59PM +0200, Greg Kurz wrote:
> > This allows the virtio-blk-pci device to batch the setup of all its
> > host notifiers. This significantly improves boot time of VMs with a
> > high number of vCPUs, e.g. from 3m26.186s down to 0m58.023s for a
> > pseries machine with 384 vCPUs.
> >
> > Note that memory_region_transaction_commit() must be called before
> > virtio_bus_cleanup_host_notifier() because the latter might close
> > ioeventfds that the transaction still assumes to be around when it
> > commits.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > hw/block/dataplane/virtio-blk.c | 25 +++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> > index d7b5c95d26d9..cd81893d1d01 100644
> > --- a/hw/block/dataplane/virtio-blk.c
> > +++ b/hw/block/dataplane/virtio-blk.c
> > @@ -198,19 +198,30 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
> > goto fail_guest_notifiers;
> > }
> >
> > + memory_region_transaction_begin();
>
> This call is optional and an optimization. It would be nice to have a
> comment here explaining that - otherwise people may wonder why this is
> necessary.
>
Indeed. Same goes for patch 4 actually.
Michael,
Should I re-post an updated version of this series or can the
comments be added in a followup ?
> > +
> > /* Set up virtqueue notify */
> > for (i = 0; i < nvqs; i++) {
> > r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, true);
> > if (r != 0) {
> > + int j = i;
> > +
> > fprintf(stderr, "virtio-blk failed to set host notifier (%d)\n", r);
> > while (i--) {
> > virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
> > + }
> > +
> > + memory_region_transaction_commit();
> > +
> > + while (j--) {
> > virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
> > }
> > goto fail_host_notifiers;
> > }
> > }
> >
> > + memory_region_transaction_commit();
> > +
> > s->starting = false;
> > vblk->dataplane_started = true;
> > trace_virtio_blk_data_plane_start(s);
> > @@ -246,8 +257,15 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
> > return 0;
> >
> > fail_aio_context:
> > + memory_region_transaction_begin();
>
> Probably unnecessary since this is an error code path. We don't need to
> optimize it.
>
> Doesn't hurt though.
True. I can drop this if I repost.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [for-6.1 2/4] virtio-blk: Configure all host notifiers in a single MR transaction
2021-05-05 11:15 ` Greg Kurz
@ 2021-05-06 8:15 ` Michael S. Tsirkin
0 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2021-05-06 8:15 UTC (permalink / raw)
To: Greg Kurz
Cc: Kevin Wolf, Fam Zheng, qemu-block, qemu-devel, Max Reitz,
Stefan Hajnoczi, Paolo Bonzini, David Gibson
On Wed, May 05, 2021 at 01:15:51PM +0200, Greg Kurz wrote:
> On Wed, 5 May 2021 11:14:23 +0100
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> > On Wed, Apr 07, 2021 at 04:34:59PM +0200, Greg Kurz wrote:
> > > This allows the virtio-blk-pci device to batch the setup of all its
> > > host notifiers. This significantly improves boot time of VMs with a
> > > high number of vCPUs, e.g. from 3m26.186s down to 0m58.023s for a
> > > pseries machine with 384 vCPUs.
> > >
> > > Note that memory_region_transaction_commit() must be called before
> > > virtio_bus_cleanup_host_notifier() because the latter might close
> > > ioeventfds that the transaction still assumes to be around when it
> > > commits.
> > >
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > hw/block/dataplane/virtio-blk.c | 25 +++++++++++++++++++++++++
> > > 1 file changed, 25 insertions(+)
> > >
> > > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> > > index d7b5c95d26d9..cd81893d1d01 100644
> > > --- a/hw/block/dataplane/virtio-blk.c
> > > +++ b/hw/block/dataplane/virtio-blk.c
> > > @@ -198,19 +198,30 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
> > > goto fail_guest_notifiers;
> > > }
> > >
> > > + memory_region_transaction_begin();
> >
> > This call is optional and an optimization. It would be nice to have a
> > comment here explaining that - otherwise people may wonder why this is
> > necessary.
> >
>
> Indeed. Same goes for patch 4 actually.
>
> Michael,
>
> Should I re-post an updated version of this series or can the
> comments be added in a followup ?
An updated version is better. Stefan did not ack anyway.
> > > +
> > > /* Set up virtqueue notify */
> > > for (i = 0; i < nvqs; i++) {
> > > r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, true);
> > > if (r != 0) {
> > > + int j = i;
> > > +
> > > fprintf(stderr, "virtio-blk failed to set host notifier (%d)\n", r);
> > > while (i--) {
> > > virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
> > > + }
> > > +
> > > + memory_region_transaction_commit();
> > > +
> > > + while (j--) {
> > > virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
> > > }
> > > goto fail_host_notifiers;
> > > }
> > > }
> > >
> > > + memory_region_transaction_commit();
> > > +
> > > s->starting = false;
> > > vblk->dataplane_started = true;
> > > trace_virtio_blk_data_plane_start(s);
> > > @@ -246,8 +257,15 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
> > > return 0;
> > >
> > > fail_aio_context:
> > > + memory_region_transaction_begin();
> >
> > Probably unnecessary since this is an error code path. We don't need to
> > optimize it.
> >
> > Doesn't hurt though.
>
> True. I can drop this if I repost.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [for-6.1 3/4] virtio-scsi: Set host notifiers and callbacks separately
2021-04-07 14:34 [for-6.1 0/4] virtio: Improve boot time of virtio-scsi-pci and virtio-blk-pci Greg Kurz
2021-04-07 14:34 ` [for-6.1 1/4] virtio-blk: Fix rollback path in virtio_blk_data_plane_start() Greg Kurz
2021-04-07 14:34 ` [for-6.1 2/4] virtio-blk: Configure all host notifiers in a single MR transaction Greg Kurz
@ 2021-04-07 14:35 ` Greg Kurz
2021-05-05 10:16 ` Stefan Hajnoczi
2021-04-07 14:35 ` [for-6.1 4/4] virtio-scsi: Configure all host notifiers in a single MR transaction Greg Kurz
` (2 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Greg Kurz @ 2021-04-07 14:35 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, qemu-block, Michael S. Tsirkin, Greg Kurz,
Max Reitz, Stefan Hajnoczi, Paolo Bonzini, David Gibson
Host notifiers are guaranteed to be idle until the callbacks are
hooked up with virtio_queue_aio_set_host_notifier_handler(). They
thus don't need to be set or unset with the AioContext lock held.
Do this outside the critical section, like virtio-blk already
does : basically downgrading virtio_scsi_vring_init() to only
setup the host notifier and set the callback in the caller.
This will allow to batch addition/deletion of ioeventds in
a single memory transaction, which is expected to greatly
improve initialization time.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/scsi/virtio-scsi-dataplane.c | 40 ++++++++++++++++++---------------
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 4ad879340645..b2cb3d9dcc64 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -94,8 +94,7 @@ static bool virtio_scsi_data_plane_handle_event(VirtIODevice *vdev,
return progress;
}
-static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n,
- VirtIOHandleAIOOutput fn)
+static int virtio_scsi_set_host_notifier(VirtIOSCSI *s, VirtQueue *vq, int n)
{
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
int rc;
@@ -109,7 +108,6 @@ static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n,
return rc;
}
- virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, fn);
return 0;
}
@@ -154,38 +152,44 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
goto fail_guest_notifiers;
}
- aio_context_acquire(s->ctx);
- rc = virtio_scsi_vring_init(s, vs->ctrl_vq, 0,
- virtio_scsi_data_plane_handle_ctrl);
- if (rc) {
- goto fail_vrings;
+ rc = virtio_scsi_set_host_notifier(s, vs->ctrl_vq, 0);
+ if (rc != 0) {
+ goto fail_host_notifiers;
}
vq_init_count++;
- rc = virtio_scsi_vring_init(s, vs->event_vq, 1,
- virtio_scsi_data_plane_handle_event);
- if (rc) {
- goto fail_vrings;
+ rc = virtio_scsi_set_host_notifier(s, vs->event_vq, 1);
+ if (rc != 0) {
+ goto fail_host_notifiers;
}
vq_init_count++;
+
for (i = 0; i < vs->conf.num_queues; i++) {
- rc = virtio_scsi_vring_init(s, vs->cmd_vqs[i], i + 2,
- virtio_scsi_data_plane_handle_cmd);
+ rc = virtio_scsi_set_host_notifier(s, vs->cmd_vqs[i], i + 2);
if (rc) {
- goto fail_vrings;
+ goto fail_host_notifiers;
}
vq_init_count++;
}
+ aio_context_acquire(s->ctx);
+ virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx,
+ virtio_scsi_data_plane_handle_ctrl);
+ virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx,
+ virtio_scsi_data_plane_handle_event);
+
+ for (i = 0; i < vs->conf.num_queues; i++) {
+ virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx,
+ virtio_scsi_data_plane_handle_cmd);
+ }
+
s->dataplane_starting = false;
s->dataplane_started = true;
aio_context_release(s->ctx);
return 0;
-fail_vrings:
- aio_wait_bh_oneshot(s->ctx, virtio_scsi_dataplane_stop_bh, s);
- aio_context_release(s->ctx);
+fail_host_notifiers:
for (i = 0; i < vq_init_count; i++) {
virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
--
2.26.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [for-6.1 3/4] virtio-scsi: Set host notifiers and callbacks separately
2021-04-07 14:35 ` [for-6.1 3/4] virtio-scsi: Set host notifiers and callbacks separately Greg Kurz
@ 2021-05-05 10:16 ` Stefan Hajnoczi
0 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2021-05-05 10:16 UTC (permalink / raw)
To: Greg Kurz
Cc: Kevin Wolf, Fam Zheng, qemu-block, Michael S. Tsirkin,
qemu-devel, Max Reitz, Paolo Bonzini, David Gibson
[-- Attachment #1: Type: text/plain, Size: 861 bytes --]
On Wed, Apr 07, 2021 at 04:35:00PM +0200, Greg Kurz wrote:
> Host notifiers are guaranteed to be idle until the callbacks are
> hooked up with virtio_queue_aio_set_host_notifier_handler(). They
> thus don't need to be set or unset with the AioContext lock held.
>
> Do this outside the critical section, like virtio-blk already
> does : basically downgrading virtio_scsi_vring_init() to only
> setup the host notifier and set the callback in the caller.
>
> This will allow to batch addition/deletion of ioeventds in
> a single memory transaction, which is expected to greatly
> improve initialization time.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> hw/scsi/virtio-scsi-dataplane.c | 40 ++++++++++++++++++---------------
> 1 file changed, 22 insertions(+), 18 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* [for-6.1 4/4] virtio-scsi: Configure all host notifiers in a single MR transaction
2021-04-07 14:34 [for-6.1 0/4] virtio: Improve boot time of virtio-scsi-pci and virtio-blk-pci Greg Kurz
` (2 preceding siblings ...)
2021-04-07 14:35 ` [for-6.1 3/4] virtio-scsi: Set host notifiers and callbacks separately Greg Kurz
@ 2021-04-07 14:35 ` Greg Kurz
2021-05-05 10:17 ` Stefan Hajnoczi
2021-04-19 15:31 ` [for-6.1 0/4] virtio: Improve boot time of virtio-scsi-pci and virtio-blk-pci Greg Kurz
2021-04-19 21:51 ` Michael S. Tsirkin
5 siblings, 1 reply; 12+ messages in thread
From: Greg Kurz @ 2021-04-07 14:35 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, qemu-block, Michael S. Tsirkin, Greg Kurz,
Max Reitz, Stefan Hajnoczi, Paolo Bonzini, David Gibson
This allows the virtio-scsi-pci device to batch the setup of all its
host notifiers. This significantly improves boot time of VMs with a
high number of vCPUs, e.g. from 6m5.563s down to 1m2.884s for a
pseries machine with 384 vCPUs.
Note that memory_region_transaction_commit() must be called before
virtio_bus_cleanup_host_notifier() because the latter might close
ioeventfds that the transaction still assumes to be around when it
commits.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/scsi/virtio-scsi-dataplane.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index b2cb3d9dcc64..28e003250a11 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -152,6 +152,8 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
goto fail_guest_notifiers;
}
+ memory_region_transaction_begin();
+
rc = virtio_scsi_set_host_notifier(s, vs->ctrl_vq, 0);
if (rc != 0) {
goto fail_host_notifiers;
@@ -173,6 +175,8 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
vq_init_count++;
}
+ memory_region_transaction_commit();
+
aio_context_acquire(s->ctx);
virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx,
virtio_scsi_data_plane_handle_ctrl);
@@ -192,6 +196,11 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
fail_host_notifiers:
for (i = 0; i < vq_init_count; i++) {
virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+ }
+
+ memory_region_transaction_commit();
+
+ for (i = 0; i < vq_init_count; i++) {
virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
}
k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
@@ -229,8 +238,15 @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
blk_drain_all(); /* ensure there are no in-flight requests */
+ memory_region_transaction_begin();
+
for (i = 0; i < vs->conf.num_queues + 2; i++) {
virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
+ }
+
+ memory_region_transaction_commit();
+
+ for (i = 0; i < vs->conf.num_queues + 2; i++) {
virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), i);
}
--
2.26.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [for-6.1 4/4] virtio-scsi: Configure all host notifiers in a single MR transaction
2021-04-07 14:35 ` [for-6.1 4/4] virtio-scsi: Configure all host notifiers in a single MR transaction Greg Kurz
@ 2021-05-05 10:17 ` Stefan Hajnoczi
0 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2021-05-05 10:17 UTC (permalink / raw)
To: Greg Kurz
Cc: Kevin Wolf, Fam Zheng, qemu-block, Michael S. Tsirkin,
qemu-devel, Max Reitz, Paolo Bonzini, David Gibson
[-- Attachment #1: Type: text/plain, Size: 735 bytes --]
On Wed, Apr 07, 2021 at 04:35:01PM +0200, Greg Kurz wrote:
> This allows the virtio-scsi-pci device to batch the setup of all its
> host notifiers. This significantly improves boot time of VMs with a
> high number of vCPUs, e.g. from 6m5.563s down to 1m2.884s for a
> pseries machine with 384 vCPUs.
>
> Note that memory_region_transaction_commit() must be called before
> virtio_bus_cleanup_host_notifier() because the latter might close
> ioeventfds that the transaction still assumes to be around when it
> commits.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> hw/scsi/virtio-scsi-dataplane.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [for-6.1 0/4] virtio: Improve boot time of virtio-scsi-pci and virtio-blk-pci
2021-04-07 14:34 [for-6.1 0/4] virtio: Improve boot time of virtio-scsi-pci and virtio-blk-pci Greg Kurz
` (3 preceding siblings ...)
2021-04-07 14:35 ` [for-6.1 4/4] virtio-scsi: Configure all host notifiers in a single MR transaction Greg Kurz
@ 2021-04-19 15:31 ` Greg Kurz
2021-04-19 21:51 ` Michael S. Tsirkin
5 siblings, 0 replies; 12+ messages in thread
From: Greg Kurz @ 2021-04-19 15:31 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Fam Zheng, qemu-block, Michael S. Tsirkin, Max Reitz,
Stefan Hajnoczi, Paolo Bonzini, David Gibson
Ping ?
On Wed, 7 Apr 2021 16:34:57 +0200
Greg Kurz <groug@kaod.org> wrote:
> Now that virtio-scsi-pci and virtio-blk-pci map 1 virtqueue per vCPU,
> a serious slow down may be observed on setups with a big enough number
> of vCPUs.
>
> Exemple with a pseries guest on a bi-POWER9 socket system (128 HW threads):
>
> virtio-scsi virtio-blk
>
> 1 0m20.922s 0m21.346s
> 2 0m21.230s 0m20.350s
> 4 0m21.761s 0m20.997s
> 8 0m22.770s 0m20.051s
> 16 0m22.038s 0m19.994s
> 32 0m22.928s 0m20.803s
> 64 0m26.583s 0m22.953s
> 128 0m41.273s 0m32.333s
> 256 2m4.727s 1m16.924s
> 384 6m5.563s 3m26.186s
>
> Both perf and gprof indicate that QEMU is hogging CPUs when setting up
> the ioeventfds:
>
> 67.88% swapper [kernel.kallsyms] [k] power_pmu_enable
> 9.47% qemu-kvm [kernel.kallsyms] [k] smp_call_function_single
> 8.64% qemu-kvm [kernel.kallsyms] [k] power_pmu_enable
> =>2.79% qemu-kvm qemu-kvm [.] memory_region_ioeventfd_before
> =>2.12% qemu-kvm qemu-kvm [.] address_space_update_ioeventfds
> 0.56% kworker/8:0-mm [kernel.kallsyms] [k] smp_call_function_single
>
> address_space_update_ioeventfds() is called when committing an MR
> transaction, i.e. for each ioeventfd with the current code base,
> and it internally loops on all ioventfds:
>
> static void address_space_update_ioeventfds(AddressSpace *as)
> {
> [...]
> FOR_EACH_FLAT_RANGE(fr, view) {
> for (i = 0; i < fr->mr->ioeventfd_nb; ++i) {
>
> This means that the setup of ioeventfds for these devices has
> quadratic time complexity.
>
> This series simply changes the device models to extend the transaction
> to all virtqueueues, like already done in the past in the generic
> code with 710fccf80d78 ("virtio: improve virtio devices initialization
> time").
>
> Only virtio-scsi and virtio-blk are covered here, but a similar change
> might also be beneficial to other device types such as host-scsi-pci,
> vhost-user-scsi-pci and vhost-user-blk-pci.
>
> virtio-scsi virtio-blk
>
> 1 0m21.271s 0m22.076s
> 2 0m20.912s 0m19.716s
> 4 0m20.508s 0m19.310s
> 8 0m21.374s 0m20.273s
> 16 0m21.559s 0m21.374s
> 32 0m22.532s 0m21.271s
> 64 0m26.550s 0m22.007s
> 128 0m29.115s 0m27.446s
> 256 0m44.752s 0m41.004s
> 384 1m2.884s 0m58.023s
>
> This should fix https://bugzilla.redhat.com/show_bug.cgi?id=1927108
> which reported the issue for virtio-scsi-pci.
>
> Changes since RFC:
>
> As suggested by Stefan, splimplify the code by directly beginning and
> committing the memory transaction from the device model, without all
> the virtio specific proxying code and no changes needed in the memory
> subsystem.
>
> Greg Kurz (4):
> virtio-blk: Fix rollback path in virtio_blk_data_plane_start()
> virtio-blk: Configure all host notifiers in a single MR transaction
> virtio-scsi: Set host notifiers and callbacks separately
> virtio-scsi: Configure all host notifiers in a single MR transaction
>
> hw/block/dataplane/virtio-blk.c | 36 +++++++++++++++++++--
> hw/scsi/virtio-scsi-dataplane.c | 56 ++++++++++++++++++++++-----------
> 2 files changed, 72 insertions(+), 20 deletions(-)
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [for-6.1 0/4] virtio: Improve boot time of virtio-scsi-pci and virtio-blk-pci
2021-04-07 14:34 [for-6.1 0/4] virtio: Improve boot time of virtio-scsi-pci and virtio-blk-pci Greg Kurz
` (4 preceding siblings ...)
2021-04-19 15:31 ` [for-6.1 0/4] virtio: Improve boot time of virtio-scsi-pci and virtio-blk-pci Greg Kurz
@ 2021-04-19 21:51 ` Michael S. Tsirkin
5 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2021-04-19 21:51 UTC (permalink / raw)
To: Greg Kurz
Cc: Kevin Wolf, Fam Zheng, qemu-block, qemu-devel, Max Reitz,
Stefan Hajnoczi, Paolo Bonzini, David Gibson
On Wed, Apr 07, 2021 at 04:34:57PM +0200, Greg Kurz wrote:
> Now that virtio-scsi-pci and virtio-blk-pci map 1 virtqueue per vCPU,
> a serious slow down may be observed on setups with a big enough number
> of vCPUs.
>
> Exemple with a pseries guest on a bi-POWER9 socket system (128 HW threads):
>
> virtio-scsi virtio-blk
>
> 1 0m20.922s 0m21.346s
> 2 0m21.230s 0m20.350s
> 4 0m21.761s 0m20.997s
> 8 0m22.770s 0m20.051s
> 16 0m22.038s 0m19.994s
> 32 0m22.928s 0m20.803s
> 64 0m26.583s 0m22.953s
> 128 0m41.273s 0m32.333s
> 256 2m4.727s 1m16.924s
> 384 6m5.563s 3m26.186s
>
> Both perf and gprof indicate that QEMU is hogging CPUs when setting up
> the ioeventfds:
>
> 67.88% swapper [kernel.kallsyms] [k] power_pmu_enable
> 9.47% qemu-kvm [kernel.kallsyms] [k] smp_call_function_single
> 8.64% qemu-kvm [kernel.kallsyms] [k] power_pmu_enable
> =>2.79% qemu-kvm qemu-kvm [.] memory_region_ioeventfd_before
> =>2.12% qemu-kvm qemu-kvm [.] address_space_update_ioeventfds
> 0.56% kworker/8:0-mm [kernel.kallsyms] [k] smp_call_function_single
>
> address_space_update_ioeventfds() is called when committing an MR
> transaction, i.e. for each ioeventfd with the current code base,
> and it internally loops on all ioventfds:
>
> static void address_space_update_ioeventfds(AddressSpace *as)
> {
> [...]
> FOR_EACH_FLAT_RANGE(fr, view) {
> for (i = 0; i < fr->mr->ioeventfd_nb; ++i) {
>
> This means that the setup of ioeventfds for these devices has
> quadratic time complexity.
>
> This series simply changes the device models to extend the transaction
> to all virtqueueues, like already done in the past in the generic
> code with 710fccf80d78 ("virtio: improve virtio devices initialization
> time").
>
> Only virtio-scsi and virtio-blk are covered here, but a similar change
> might also be beneficial to other device types such as host-scsi-pci,
> vhost-user-scsi-pci and vhost-user-blk-pci.
>
> virtio-scsi virtio-blk
>
> 1 0m21.271s 0m22.076s
> 2 0m20.912s 0m19.716s
> 4 0m20.508s 0m19.310s
> 8 0m21.374s 0m20.273s
> 16 0m21.559s 0m21.374s
> 32 0m22.532s 0m21.271s
> 64 0m26.550s 0m22.007s
> 128 0m29.115s 0m27.446s
> 256 0m44.752s 0m41.004s
> 384 1m2.884s 0m58.023s
>
> This should fix https://bugzilla.redhat.com/show_bug.cgi?id=1927108
> which reported the issue for virtio-scsi-pci.
>
> Changes since RFC:
>
> As suggested by Stefan, splimplify the code by directly beginning and
> committing the memory transaction from the device model, without all
> the virtio specific proxying code and no changes needed in the memory
> subsystem.
>
> Greg Kurz (4):
> virtio-blk: Fix rollback path in virtio_blk_data_plane_start()
> virtio-blk: Configure all host notifiers in a single MR transaction
> virtio-scsi: Set host notifiers and callbacks separately
> virtio-scsi: Configure all host notifiers in a single MR transaction
>
> hw/block/dataplane/virtio-blk.c | 36 +++++++++++++++++++--
> hw/scsi/virtio-scsi-dataplane.c | 56 ++++++++++++++++++++++-----------
> 2 files changed, 72 insertions(+), 20 deletions(-)
Tagged for 6.1, thanks!
> --
> 2.26.3
>
^ permalink raw reply [flat|nested] 12+ messages in thread