| Message ID | 20210325150735.1098387-4-groug@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio: Improve boot time of virtio-scsi-pci and virtio-blk-pci | expand |
On Thu, Mar 25, 2021 at 04:07:30PM +0100, Greg Kurz wrote: > Introduce VirtioBusClass methods to begin and commit a transaction > of setting/unsetting host notifiers. These handlers will be implemented > by virtio-pci to batch addition and deletion of ioeventfds for multiqueue > devices like virtio-scsi-pci or virtio-blk-pci. > > Convert virtio_bus_set_host_notifiers() to use these handlers. Note that > virtio_bus_cleanup_host_notifier() closes eventfds, which could still be > passed to the KVM_IOEVENTFD ioctl() when the transaction ends and fail > with EBADF. The cleanup of the host notifiers is thus pushed to a > separate loop in virtio_bus_unset_and_cleanup_host_notifiers(), after > transaction commit. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > include/hw/virtio/virtio-bus.h | 4 ++++ > hw/virtio/virtio-bus.c | 34 ++++++++++++++++++++++++++++++++++ > 2 files changed, 38 insertions(+) > > diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h > index 6d1e4ee3e886..99704b2c090a 100644 > --- a/include/hw/virtio/virtio-bus.h > +++ b/include/hw/virtio/virtio-bus.h > @@ -82,6 +82,10 @@ struct VirtioBusClass { > */ > int (*ioeventfd_assign)(DeviceState *d, EventNotifier *notifier, > int n, bool assign); > + > + void (*ioeventfd_assign_begin)(DeviceState *d); > + void (*ioeventfd_assign_commit)(DeviceState *d); Please add doc comments for these new functions. > + > /* > * Whether queue number n is enabled. > */ > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > index c9e7cdb5c161..156484c4ca14 100644 > --- a/hw/virtio/virtio-bus.c > +++ b/hw/virtio/virtio-bus.c > @@ -295,6 +295,28 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) > return r; > } > > +static void virtio_bus_set_host_notifier_begin(VirtioBusState *bus) > +{ > + VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus); > + DeviceState *proxy = DEVICE(BUS(bus)->parent); > + > + if (k->ioeventfd_assign_begin) { > + assert(k->ioeventfd_assign_commit); > + k->ioeventfd_assign_begin(proxy); > + } > +} > + > +static void virtio_bus_set_host_notifier_commit(VirtioBusState *bus) > +{ > + VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus); > + DeviceState *proxy = DEVICE(BUS(bus)->parent); > + > + if (k->ioeventfd_assign_commit) { > + assert(k->ioeventfd_assign_begin); > + k->ioeventfd_assign_commit(proxy); > + } > +} > + > void virtio_bus_cleanup_host_notifier(VirtioBusState *bus, int n) > { > VirtIODevice *vdev = virtio_bus_get_device(bus); > @@ -308,6 +330,7 @@ void virtio_bus_cleanup_host_notifier(VirtioBusState *bus, int n) > event_notifier_cleanup(notifier); > } > > +/* virtio_bus_set_host_notifier_begin() must have been called */ > static void virtio_bus_unset_and_cleanup_host_notifiers(VirtioBusState *bus, > int nvqs, int n_offset) > { > @@ -315,6 +338,10 @@ static void virtio_bus_unset_and_cleanup_host_notifiers(VirtioBusState *bus, > > for (i = 0; i < nvqs; i++) { > virtio_bus_set_host_notifier(bus, i + n_offset, false); > + } > + /* Let address_space_update_ioeventfds() run before closing ioeventfds */ assert(memory_region_transaction_depth == 0)? > + virtio_bus_set_host_notifier_commit(bus); > + for (i = 0; i < nvqs; i++) { > virtio_bus_cleanup_host_notifier(bus, i + n_offset); > } > } > @@ -327,17 +354,24 @@ int virtio_bus_set_host_notifiers(VirtioBusState *bus, int nvqs, int n_offset, > int rc; > > if (assign) { > + virtio_bus_set_host_notifier_begin(bus); > + > for (i = 0; i < nvqs; i++) { > rc = virtio_bus_set_host_notifier(bus, i + n_offset, true); > if (rc != 0) { > warn_report_once("%s: Failed to set host notifier (%s).\n", > vdev->name, strerror(-rc)); > > + /* This also calls virtio_bus_set_host_notifier_commit() */ > virtio_bus_unset_and_cleanup_host_notifiers(bus, i, n_offset); > return rc; > } > } > + > + virtio_bus_set_host_notifier_commit(bus); > } else { > + virtio_bus_set_host_notifier_begin(bus); > + /* This also calls virtio_bus_set_host_notifier_commit() */ > virtio_bus_unset_and_cleanup_host_notifiers(bus, nvqs, n_offset); > } > > -- > 2.26.3 >
On Mon, 29 Mar 2021 18:10:57 +0100 Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Thu, Mar 25, 2021 at 04:07:30PM +0100, Greg Kurz wrote: > > Introduce VirtioBusClass methods to begin and commit a transaction > > of setting/unsetting host notifiers. These handlers will be implemented > > by virtio-pci to batch addition and deletion of ioeventfds for multiqueue > > devices like virtio-scsi-pci or virtio-blk-pci. > > > > Convert virtio_bus_set_host_notifiers() to use these handlers. Note that > > virtio_bus_cleanup_host_notifier() closes eventfds, which could still be > > passed to the KVM_IOEVENTFD ioctl() when the transaction ends and fail > > with EBADF. The cleanup of the host notifiers is thus pushed to a > > separate loop in virtio_bus_unset_and_cleanup_host_notifiers(), after > > transaction commit. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > include/hw/virtio/virtio-bus.h | 4 ++++ > > hw/virtio/virtio-bus.c | 34 ++++++++++++++++++++++++++++++++++ > > 2 files changed, 38 insertions(+) > > > > diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h > > index 6d1e4ee3e886..99704b2c090a 100644 > > --- a/include/hw/virtio/virtio-bus.h > > +++ b/include/hw/virtio/virtio-bus.h > > @@ -82,6 +82,10 @@ struct VirtioBusClass { > > */ > > int (*ioeventfd_assign)(DeviceState *d, EventNotifier *notifier, > > int n, bool assign); > > + > > + void (*ioeventfd_assign_begin)(DeviceState *d); > > + void (*ioeventfd_assign_commit)(DeviceState *d); > > Please add doc comments for these new functions. > Will do. > > + > > /* > > * Whether queue number n is enabled. > > */ > > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > > index c9e7cdb5c161..156484c4ca14 100644 > > --- a/hw/virtio/virtio-bus.c > > +++ b/hw/virtio/virtio-bus.c > > @@ -295,6 +295,28 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) > > return r; > > } > > > > +static void virtio_bus_set_host_notifier_begin(VirtioBusState *bus) > > +{ > > + VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus); > > + DeviceState *proxy = DEVICE(BUS(bus)->parent); > > + > > + if (k->ioeventfd_assign_begin) { > > + assert(k->ioeventfd_assign_commit); > > + k->ioeventfd_assign_begin(proxy); > > + } > > +} > > + > > +static void virtio_bus_set_host_notifier_commit(VirtioBusState *bus) > > +{ > > + VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus); > > + DeviceState *proxy = DEVICE(BUS(bus)->parent); > > + > > + if (k->ioeventfd_assign_commit) { > > + assert(k->ioeventfd_assign_begin); > > + k->ioeventfd_assign_commit(proxy); > > + } > > +} > > + > > void virtio_bus_cleanup_host_notifier(VirtioBusState *bus, int n) > > { > > VirtIODevice *vdev = virtio_bus_get_device(bus); > > @@ -308,6 +330,7 @@ void virtio_bus_cleanup_host_notifier(VirtioBusState *bus, int n) > > event_notifier_cleanup(notifier); > > } > > > > +/* virtio_bus_set_host_notifier_begin() must have been called */ > > static void virtio_bus_unset_and_cleanup_host_notifiers(VirtioBusState *bus, > > int nvqs, int n_offset) > > { > > @@ -315,6 +338,10 @@ static void virtio_bus_unset_and_cleanup_host_notifiers(VirtioBusState *bus, > > > > for (i = 0; i < nvqs; i++) { > > virtio_bus_set_host_notifier(bus, i + n_offset, false); > > + } > > + /* Let address_space_update_ioeventfds() run before closing ioeventfds */ > > assert(memory_region_transaction_depth == 0)? > Hmm... appart from the fact that memory_region_transaction_depth is a memory internal thing that shouldn't be exposed here, it seems to me that memory_region_transaction_depth can be != 0 when, e.g. when batching is used... or I'm missing something ? I was actually thinking of adding some asserts for that in the memory_region_*_eventfd_full() functions introduced by patch 1. if (!transaction) { memory_region_transaction_begin(); } assert(memory_region_transaction_depth != 0); > > + virtio_bus_set_host_notifier_commit(bus); > > + for (i = 0; i < nvqs; i++) { > > virtio_bus_cleanup_host_notifier(bus, i + n_offset); > > } > > } > > @@ -327,17 +354,24 @@ int virtio_bus_set_host_notifiers(VirtioBusState *bus, int nvqs, int n_offset, > > int rc; > > > > if (assign) { > > + virtio_bus_set_host_notifier_begin(bus); > > + > > for (i = 0; i < nvqs; i++) { > > rc = virtio_bus_set_host_notifier(bus, i + n_offset, true); > > if (rc != 0) { > > warn_report_once("%s: Failed to set host notifier (%s).\n", > > vdev->name, strerror(-rc)); > > > > + /* This also calls virtio_bus_set_host_notifier_commit() */ > > virtio_bus_unset_and_cleanup_host_notifiers(bus, i, n_offset); > > return rc; > > } > > } > > + > > + virtio_bus_set_host_notifier_commit(bus); > > } else { > > + virtio_bus_set_host_notifier_begin(bus); > > + /* This also calls virtio_bus_set_host_notifier_commit() */ > > virtio_bus_unset_and_cleanup_host_notifiers(bus, nvqs, n_offset); > > } > > > > -- > > 2.26.3 > >
On Tue, Mar 30, 2021 at 12:17:40PM +0200, Greg Kurz wrote: > On Mon, 29 Mar 2021 18:10:57 +0100 > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > On Thu, Mar 25, 2021 at 04:07:30PM +0100, Greg Kurz wrote: > > > @@ -315,6 +338,10 @@ static void virtio_bus_unset_and_cleanup_host_notifiers(VirtioBusState *bus, > > > > > > for (i = 0; i < nvqs; i++) { > > > virtio_bus_set_host_notifier(bus, i + n_offset, false); > > > + } > > > + /* Let address_space_update_ioeventfds() run before closing ioeventfds */ > > > > assert(memory_region_transaction_depth == 0)? > > > > Hmm... appart from the fact that memory_region_transaction_depth is > a memory internal thing that shouldn't be exposed here, it seems to > me that memory_region_transaction_depth can be != 0 when, e.g. when > batching is used... or I'm missing something ? > > I was actually thinking of adding some asserts for that in the > memory_region_*_eventfd_full() functions introduced by patch 1. > > if (!transaction) { > memory_region_transaction_begin(); > } > assert(memory_region_transaction_depth != 0); In that case is it safe to call virtio_bus_cleanup_host_notifier() below? I thought it depends on the transaction committing first. > > > > + virtio_bus_set_host_notifier_commit(bus); > > > + for (i = 0; i < nvqs; i++) { > > > virtio_bus_cleanup_host_notifier(bus, i + n_offset); > > > } > > > }
On Tue, 30 Mar 2021 14:55:42 +0100 Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Tue, Mar 30, 2021 at 12:17:40PM +0200, Greg Kurz wrote: > > On Mon, 29 Mar 2021 18:10:57 +0100 > > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > On Thu, Mar 25, 2021 at 04:07:30PM +0100, Greg Kurz wrote: > > > > @@ -315,6 +338,10 @@ static void virtio_bus_unset_and_cleanup_host_notifiers(VirtioBusState *bus, > > > > > > > > for (i = 0; i < nvqs; i++) { > > > > virtio_bus_set_host_notifier(bus, i + n_offset, false); > > > > + } > > > > + /* Let address_space_update_ioeventfds() run before closing ioeventfds */ > > > > > > assert(memory_region_transaction_depth == 0)? > > > > > > > Hmm... appart from the fact that memory_region_transaction_depth is > > a memory internal thing that shouldn't be exposed here, it seems to > > me that memory_region_transaction_depth can be != 0 when, e.g. when > > batching is used... or I'm missing something ? > > > > I was actually thinking of adding some asserts for that in the > > memory_region_*_eventfd_full() functions introduced by patch 1. > > > > if (!transaction) { > > memory_region_transaction_begin(); > > } > > assert(memory_region_transaction_depth != 0); > > In that case is it safe to call virtio_bus_cleanup_host_notifier() > below? I thought it depends on the transaction committing first. > Yes because the transaction ends... > > > > > > + virtio_bus_set_host_notifier_commit(bus); ... here ^^ > > > > + for (i = 0; i < nvqs; i++) { > > > > virtio_bus_cleanup_host_notifier(bus, i + n_offset); > > > > } > > > > }
On Tue, Mar 30, 2021 at 04:17:32PM +0200, Greg Kurz wrote: > On Tue, 30 Mar 2021 14:55:42 +0100 > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > On Tue, Mar 30, 2021 at 12:17:40PM +0200, Greg Kurz wrote: > > > On Mon, 29 Mar 2021 18:10:57 +0100 > > > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > On Thu, Mar 25, 2021 at 04:07:30PM +0100, Greg Kurz wrote: > > > > > @@ -315,6 +338,10 @@ static void virtio_bus_unset_and_cleanup_host_notifiers(VirtioBusState *bus, > > > > > > > > > > for (i = 0; i < nvqs; i++) { > > > > > virtio_bus_set_host_notifier(bus, i + n_offset, false); > > > > > + } > > > > > + /* Let address_space_update_ioeventfds() run before closing ioeventfds */ > > > > > > > > assert(memory_region_transaction_depth == 0)? > > > > > > > > > > Hmm... appart from the fact that memory_region_transaction_depth is > > > a memory internal thing that shouldn't be exposed here, it seems to > > > me that memory_region_transaction_depth can be != 0 when, e.g. when > > > batching is used... or I'm missing something ? > > > > > > I was actually thinking of adding some asserts for that in the > > > memory_region_*_eventfd_full() functions introduced by patch 1. > > > > > > if (!transaction) { > > > memory_region_transaction_begin(); > > > } > > > assert(memory_region_transaction_depth != 0); > > > > In that case is it safe to call virtio_bus_cleanup_host_notifier() > > below? I thought it depends on the transaction committing first. > > > > Yes because the transaction ends... > > > > > > > > > + virtio_bus_set_host_notifier_commit(bus); > ... here ^^ > > > > > > + for (i = 0; i < nvqs; i++) { > > > > > virtio_bus_cleanup_host_notifier(bus, i + n_offset); > > > > > } > > > > > } That contradicts what you said above: "it seems to me that memory_region_transaction_depth can be != 0 when, e.g. when batching is used". If memory_region_transaction_depth can be != 0 when this function is entered then memory_region_transaction_commit() will have no effect: void memory_region_transaction_commit(void) { AddressSpace *as; assert(memory_region_transaction_depth); assert(qemu_mutex_iothread_locked()); --memory_region_transaction_depth; if (!memory_region_transaction_depth) { ^--- we won't take this branch! So the code after memory_region_transaction_commit() cannot assume that anything was actually committed. That's why I asked about adding assert(memory_region_transaction_depth == 0) to guarantee that our commit takes effect immediately so that it's safe to call virtio_bus_cleanup_host_notifier(). Stefan
On Wed, 31 Mar 2021 15:47:45 +0100 Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Tue, Mar 30, 2021 at 04:17:32PM +0200, Greg Kurz wrote: > > On Tue, 30 Mar 2021 14:55:42 +0100 > > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > > On Tue, Mar 30, 2021 at 12:17:40PM +0200, Greg Kurz wrote: > > > > On Mon, 29 Mar 2021 18:10:57 +0100 > > > > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > > On Thu, Mar 25, 2021 at 04:07:30PM +0100, Greg Kurz wrote: > > > > > > @@ -315,6 +338,10 @@ static void virtio_bus_unset_and_cleanup_host_notifiers(VirtioBusState *bus, > > > > > > > > > > > > for (i = 0; i < nvqs; i++) { > > > > > > virtio_bus_set_host_notifier(bus, i + n_offset, false); > > > > > > + } > > > > > > + /* Let address_space_update_ioeventfds() run before closing ioeventfds */ > > > > > > > > > > assert(memory_region_transaction_depth == 0)? > > > > > > > > > > > > > Hmm... appart from the fact that memory_region_transaction_depth is > > > > a memory internal thing that shouldn't be exposed here, it seems to > > > > me that memory_region_transaction_depth can be != 0 when, e.g. when > > > > batching is used... or I'm missing something ? > > > > > > > > I was actually thinking of adding some asserts for that in the > > > > memory_region_*_eventfd_full() functions introduced by patch 1. > > > > > > > > if (!transaction) { > > > > memory_region_transaction_begin(); > > > > } > > > > assert(memory_region_transaction_depth != 0); > > > > > > In that case is it safe to call virtio_bus_cleanup_host_notifier() > > > below? I thought it depends on the transaction committing first. > > > > > > > Yes because the transaction ends... > > > > > > > > > > > > + virtio_bus_set_host_notifier_commit(bus); > > ... here ^^ > > > > > > > > + for (i = 0; i < nvqs; i++) { > > > > > > virtio_bus_cleanup_host_notifier(bus, i + n_offset); > > > > > > } > > > > > > } > > That contradicts what you said above: "it seems to me that > memory_region_transaction_depth can be != 0 when, e.g. when batching is > used". > > If memory_region_transaction_depth can be != 0 when this function is > entered then memory_region_transaction_commit() will have no effect: > > void memory_region_transaction_commit(void) > { > AddressSpace *as; > > assert(memory_region_transaction_depth); > assert(qemu_mutex_iothread_locked()); > > --memory_region_transaction_depth; > if (!memory_region_transaction_depth) { memory_region_transaction_depth should be equal to 1 when entering the function, not 0... which is the case when batching. > ^--- we won't take this branch! > > So the code after memory_region_transaction_commit() cannot assume that > anything was actually committed. > Right and nothing in the current code base seems to prevent memory_region_*_eventfd() to be called within an ongoing transaction actually. It looks that we might want to fix that first. > That's why I asked about adding assert(memory_region_transaction_depth > == 0) to guarantee that our commit takes effect immediately so that it's > safe to call virtio_bus_cleanup_host_notifier(). > Yes, it was just misplaced and I didn't get the intent at first :) > Stefan
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h index 6d1e4ee3e886..99704b2c090a 100644 --- a/include/hw/virtio/virtio-bus.h +++ b/include/hw/virtio/virtio-bus.h @@ -82,6 +82,10 @@ struct VirtioBusClass { */ int (*ioeventfd_assign)(DeviceState *d, EventNotifier *notifier, int n, bool assign); + + void (*ioeventfd_assign_begin)(DeviceState *d); + void (*ioeventfd_assign_commit)(DeviceState *d); + /* * Whether queue number n is enabled. */ diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c index c9e7cdb5c161..156484c4ca14 100644 --- a/hw/virtio/virtio-bus.c +++ b/hw/virtio/virtio-bus.c @@ -295,6 +295,28 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) return r; } +static void virtio_bus_set_host_notifier_begin(VirtioBusState *bus) +{ + VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus); + DeviceState *proxy = DEVICE(BUS(bus)->parent); + + if (k->ioeventfd_assign_begin) { + assert(k->ioeventfd_assign_commit); + k->ioeventfd_assign_begin(proxy); + } +} + +static void virtio_bus_set_host_notifier_commit(VirtioBusState *bus) +{ + VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus); + DeviceState *proxy = DEVICE(BUS(bus)->parent); + + if (k->ioeventfd_assign_commit) { + assert(k->ioeventfd_assign_begin); + k->ioeventfd_assign_commit(proxy); + } +} + void virtio_bus_cleanup_host_notifier(VirtioBusState *bus, int n) { VirtIODevice *vdev = virtio_bus_get_device(bus); @@ -308,6 +330,7 @@ void virtio_bus_cleanup_host_notifier(VirtioBusState *bus, int n) event_notifier_cleanup(notifier); } +/* virtio_bus_set_host_notifier_begin() must have been called */ static void virtio_bus_unset_and_cleanup_host_notifiers(VirtioBusState *bus, int nvqs, int n_offset) { @@ -315,6 +338,10 @@ static void virtio_bus_unset_and_cleanup_host_notifiers(VirtioBusState *bus, for (i = 0; i < nvqs; i++) { virtio_bus_set_host_notifier(bus, i + n_offset, false); + } + /* Let address_space_update_ioeventfds() run before closing ioeventfds */ + virtio_bus_set_host_notifier_commit(bus); + for (i = 0; i < nvqs; i++) { virtio_bus_cleanup_host_notifier(bus, i + n_offset); } } @@ -327,17 +354,24 @@ int virtio_bus_set_host_notifiers(VirtioBusState *bus, int nvqs, int n_offset, int rc; if (assign) { + virtio_bus_set_host_notifier_begin(bus); + for (i = 0; i < nvqs; i++) { rc = virtio_bus_set_host_notifier(bus, i + n_offset, true); if (rc != 0) { warn_report_once("%s: Failed to set host notifier (%s).\n", vdev->name, strerror(-rc)); + /* This also calls virtio_bus_set_host_notifier_commit() */ virtio_bus_unset_and_cleanup_host_notifiers(bus, i, n_offset); return rc; } } + + virtio_bus_set_host_notifier_commit(bus); } else { + virtio_bus_set_host_notifier_begin(bus); + /* This also calls virtio_bus_set_host_notifier_commit() */ virtio_bus_unset_and_cleanup_host_notifiers(bus, nvqs, n_offset); }
Introduce VirtioBusClass methods to begin and commit a transaction of setting/unsetting host notifiers. These handlers will be implemented by virtio-pci to batch addition and deletion of ioeventfds for multiqueue devices like virtio-scsi-pci or virtio-blk-pci. Convert virtio_bus_set_host_notifiers() to use these handlers. Note that virtio_bus_cleanup_host_notifier() closes eventfds, which could still be passed to the KVM_IOEVENTFD ioctl() when the transaction ends and fail with EBADF. The cleanup of the host notifiers is thus pushed to a separate loop in virtio_bus_unset_and_cleanup_host_notifiers(), after transaction commit. Signed-off-by: Greg Kurz <groug@kaod.org> --- include/hw/virtio/virtio-bus.h | 4 ++++ hw/virtio/virtio-bus.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+)