Re: [RFC 3/8] virtio: Add API to batch set host notifiers

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 5517 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]