[for-6.1 2/4] virtio-blk: Configure all host notifiers in a single MR transaction
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
---
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
Re: [for-6.1 2/4] virtio-blk: Configure all host notifiers in a single MR transaction
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
> ---
> 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.
signature.asc
Description: PGP signature
Re: [for-6.1 2/4] virtio-blk: Configure all host notifiers in a single MR transaction
On Wed, 5 May 2021 11:14:23 +0100
Stefan Hajnoczi 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
> > ---
> > 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.
pgpHxP92LGZgm.pgp
Description: OpenPGP digital signature
Re: [for-6.1 2/4] virtio-blk: Configure all host notifiers in a single MR transaction
On Wed, May 05, 2021 at 01:15:51PM +0200, Greg Kurz wrote:
> On Wed, 5 May 2021 11:14:23 +0100
> Stefan Hajnoczi 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
> > > ---
> > > 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.
