| Message ID | 1467202624-72941-1-git-send-email-cornelia.huck@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Wed, Jun 29, 2016 at 2:17 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > When setting up host notifiers, virtio_bus_set_host_notifier() > simply switches the handler. This will only work, however, if > the ioeventfd has already been setup; this is true for dataplane, > but not for vhost. > > Fix this by starting the ioeventfd if that has not happened > before. > > While we're at it, also fixup the unsetting path of > set_host_notifier_internal(). > > Fixes: 6798e245a3 ("virtio-bus: common ioeventfd infrastructure") > Reported-by: Jason Wang <jasowang@redhat.com> > Reported-by: Marc-André Lureau <marcandre.lureau@gmail.com> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > --- > > This fixes the vhost regression for me, while dataplane continues > to work. > That doesn't work here, QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 tests/vhost-user-test hangs in /x86_64/vhost-user/migrate > Peter, does this help with your iSCSI regression? > > --- > hw/virtio/virtio-bus.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > index 1313760..0136242 100644 > --- a/hw/virtio/virtio-bus.c > +++ b/hw/virtio/virtio-bus.c > @@ -176,8 +176,8 @@ static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus, > return r; > } > } else { > - virtio_queue_set_host_notifier_fd_handler(vq, false, false); > k->ioeventfd_assign(proxy, notifier, n, assign); > + virtio_queue_set_host_notifier_fd_handler(vq, false, false); > event_notifier_cleanup(notifier); > } > return r; > @@ -258,6 +258,9 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) > return -ENOSYS; > } > if (assign) { > + if (!k->ioeventfd_started(proxy)) { > + virtio_bus_start_ioeventfd(bus); > + } > /* > * Stop using the generic ioeventfd, we are doing eventfd handling > * ourselves below > -- > 2.6.6 >
On Wed, 29 Jun 2016 14:23:42 +0200 Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi > > On Wed, Jun 29, 2016 at 2:17 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > When setting up host notifiers, virtio_bus_set_host_notifier() > > simply switches the handler. This will only work, however, if > > the ioeventfd has already been setup; this is true for dataplane, > > but not for vhost. > > > > Fix this by starting the ioeventfd if that has not happened > > before. > > > > While we're at it, also fixup the unsetting path of > > set_host_notifier_internal(). > > > > Fixes: 6798e245a3 ("virtio-bus: common ioeventfd infrastructure") > > Reported-by: Jason Wang <jasowang@redhat.com> > > Reported-by: Marc-André Lureau <marcandre.lureau@gmail.com> > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > --- > > > > This fixes the vhost regression for me, while dataplane continues > > to work. > > > > That doesn't work here, > QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 tests/vhost-user-test > > hangs in /x86_64/vhost-user/migrate I can reproduce it, but I have zero ideas on how to proceed. I can see that one of the qemus sits on event_notifier_test_and_clear when vhost tries to shut down. (I am thoroughly confused by all of that qtest setup, so I have no idea which qemu instance this is...) Looking at the code path, we really should switch the handler around, but virtio_queue_set_host_notifier_fd_handler always unsets the handler unless both assign and set_handler are true. Is that really what we want? I fear I have stared at this for so long that I have now lost myself between all these flags, so I hope one of the folks on cc: has a good idea...
On 29/06/2016 16:15, Cornelia Huck wrote: > > I can see that one of the qemus sits on event_notifier_test_and_clear > when vhost tries to shut down. (I am thoroughly confused by all of > that qtest setup, so I have no idea which qemu instance this is...) Stupid question ahead---if you mean QEMU is sitting in a blocking read, isn't event_notifier_test_and_clear supposed to be non-blocking? Paolo
On Wed, 29 Jun 2016 16:52:40 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 29/06/2016 16:15, Cornelia Huck wrote: > > > > I can see that one of the qemus sits on event_notifier_test_and_clear > > when vhost tries to shut down. (I am thoroughly confused by all of > > that qtest setup, so I have no idea which qemu instance this is...) > > Stupid question ahead---if you mean QEMU is sitting in a blocking read, > isn't event_notifier_test_and_clear supposed to be non-blocking? Yes, and this is what completely threw me off. I may have been very confused at that point of time, though; I can recheck tomorrow.
On Wed, 29 Jun 2016 14:23:42 +0200 Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi > > On Wed, Jun 29, 2016 at 2:17 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > When setting up host notifiers, virtio_bus_set_host_notifier() > > simply switches the handler. This will only work, however, if > > the ioeventfd has already been setup; this is true for dataplane, > > but not for vhost. > > > > Fix this by starting the ioeventfd if that has not happened > > before. > > > > While we're at it, also fixup the unsetting path of > > set_host_notifier_internal(). > > > > Fixes: 6798e245a3 ("virtio-bus: common ioeventfd infrastructure") > > Reported-by: Jason Wang <jasowang@redhat.com> > > Reported-by: Marc-André Lureau <marcandre.lureau@gmail.com> > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > --- > > > > This fixes the vhost regression for me, while dataplane continues > > to work. > > > > That doesn't work here, > QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 tests/vhost-user-test > > hangs in /x86_64/vhost-user/migrate All of this is related to the event notifier not being ready when switching the handler. We'll try to read with EventNotifier not being setup (which results in the hang). Doing an exit on deassign if ioeventfd is not started makes the test pass. I'll give the requirements for starting the ioeventfd and switching the handler some more thought. But at least I now know where to poke.
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c index 1313760..0136242 100644 --- a/hw/virtio/virtio-bus.c +++ b/hw/virtio/virtio-bus.c @@ -176,8 +176,8 @@ static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus, return r; } } else { - virtio_queue_set_host_notifier_fd_handler(vq, false, false); k->ioeventfd_assign(proxy, notifier, n, assign); + virtio_queue_set_host_notifier_fd_handler(vq, false, false); event_notifier_cleanup(notifier); } return r; @@ -258,6 +258,9 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) return -ENOSYS; } if (assign) { + if (!k->ioeventfd_started(proxy)) { + virtio_bus_start_ioeventfd(bus); + } /* * Stop using the generic ioeventfd, we are doing eventfd handling * ourselves below
When setting up host notifiers, virtio_bus_set_host_notifier() simply switches the handler. This will only work, however, if the ioeventfd has already been setup; this is true for dataplane, but not for vhost. Fix this by starting the ioeventfd if that has not happened before. While we're at it, also fixup the unsetting path of set_host_notifier_internal(). Fixes: 6798e245a3 ("virtio-bus: common ioeventfd infrastructure") Reported-by: Jason Wang <jasowang@redhat.com> Reported-by: Marc-André Lureau <marcandre.lureau@gmail.com> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> --- This fixes the vhost regression for me, while dataplane continues to work. Peter, does this help with your iSCSI regression? --- hw/virtio/virtio-bus.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)