| Message ID | 1467287567-2632-1-git-send-email-cornelia.huck@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 30.06.2016 um 13:52 schrieb Cornelia Huck: > 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, and will completely break things if the > ioeventfd is disabled for the device. > > Fix this by starting the ioeventfd on assign if that has not > happened before, and only switch the handler if the ioeventfd > has really been started. > > While we're at it, also fixup the unsetting path of > set_host_notifier_internal(). This fixes also iSCSI + dataplane. Peter.
Hi On Thu, Jun 30, 2016 at 1:52 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, and will completely break things if the > ioeventfd is disabled for the device. > > Fix this by starting the ioeventfd on assign if that has not > happened before, and only switch the handler if the ioeventfd > has really been started. > > 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> That's indeed enough to fix vhost-user-test, however, vhost-user is still broken Start tests/vhost-user-bridge and then qemu -enable-kvm -m 1024 -object memory-backend-file,id=mem,size=1024M,mem-path=/tmp,share=on -numa node,memdev=mem -mem-prealloc -chardev socket,id=char0,path=/tmp/vubr.sock -netdev type=vhost-user,id=mynet1,chardev=char0,vhostforce -device virtio-net-pci,netdev=mynet1 Before your series, you can see data coming after init completed, now it stops at: vhost-user-bridge: tests/vhost-user-bridge.c:1014: vubr_set_vring_kick_exec: Assertion `(u64_arg & VHOST_USER_VRING_NOFD_MASK) == 0' failed. > --- > Changes v1->v2: > - don't switch the handler if the eventfd has not been setup - this > fixes the failure of vhost-user-test for me > - add more comments > --- > hw/virtio/virtio-bus.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > index 1313760..dfe24fc 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; > @@ -246,6 +246,9 @@ void virtio_bus_stop_ioeventfd(VirtioBusState *bus) > /* > * This function switches from/to the generic ioeventfd handler. > * assign==false means 'use generic ioeventfd handler'. > + * It is only considered an error if the binding does not support > + * host notifiers at all, not when they are not available for the > + * individual device. > */ > int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) > { > @@ -259,6 +262,13 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) > } > if (assign) { > /* > + * Not yet started? assign==true implies we want to use an > + * eventfd, so let's do the requisite setup. > + */ > + if (!k->ioeventfd_started(proxy)) { > + virtio_bus_start_ioeventfd(bus); > + } > + /* > * Stop using the generic ioeventfd, we are doing eventfd handling > * ourselves below > */ > @@ -269,8 +279,13 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) > * Otherwise, there's a window where we don't have an > * ioeventfd and we may end up with a notification where > * we don't expect one. > + * > + * Also note that we should only do something with the eventfd > + * handler if the eventfd has actually been setup. > */ > - virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign); > + if (k->ioeventfd_started(proxy)) { > + virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign); > + } > if (!assign) { > /* Use generic ioeventfd handler again. */ > k->ioeventfd_set_disabled(proxy, false); > -- > 2.6.6 >
On Thu, 30 Jun 2016 16:12:31 +0200 Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi > > On Thu, Jun 30, 2016 at 1:52 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, and will completely break things if the > > ioeventfd is disabled for the device. > > > > Fix this by starting the ioeventfd on assign if that has not > > happened before, and only switch the handler if the ioeventfd > > has really been started. > > > > 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> > > That's indeed enough to fix vhost-user-test, however, vhost-user is still broken > > Start tests/vhost-user-bridge and then qemu -enable-kvm -m 1024 > -object memory-backend-file,id=mem,size=1024M,mem-path=/tmp,share=on > -numa node,memdev=mem -mem-prealloc -chardev > socket,id=char0,path=/tmp/vubr.sock -netdev > type=vhost-user,id=mynet1,chardev=char0,vhostforce -device > virtio-net-pci,netdev=mynet1 > > Before your series, you can see data coming after init completed, now > it stops at: > > vhost-user-bridge: tests/vhost-user-bridge.c:1014: > vubr_set_vring_kick_exec: Assertion `(u64_arg & > VHOST_USER_VRING_NOFD_MASK) == 0' failed. Grgh, the whole semantics of host notifiers are a mess. Before, the host notifier callbacks would (on assign) deregister the old eventfd and then register a new notifier - regardless whether the device had disabled ioeventfd. Now, we try to keep a preexisting eventfd registered, but don't try to force eventfds on a device that disabled ioeventfd - and that is what breaks vhost-user, apparently. I think the best way to deal with this is to have the now common host notifier setter revert to the old semantics for now. This re-introduces the 'no ioeventfd for a while' hole (which does not seem to show up in the wild) but fixes vhost in its various incarnations. We have time to come up with a better solution then (while still keeping the benefits of the unified host notifier handling). I'll cook up a patch.
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c index 1313760..dfe24fc 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; @@ -246,6 +246,9 @@ void virtio_bus_stop_ioeventfd(VirtioBusState *bus) /* * This function switches from/to the generic ioeventfd handler. * assign==false means 'use generic ioeventfd handler'. + * It is only considered an error if the binding does not support + * host notifiers at all, not when they are not available for the + * individual device. */ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) { @@ -259,6 +262,13 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) } if (assign) { /* + * Not yet started? assign==true implies we want to use an + * eventfd, so let's do the requisite setup. + */ + if (!k->ioeventfd_started(proxy)) { + virtio_bus_start_ioeventfd(bus); + } + /* * Stop using the generic ioeventfd, we are doing eventfd handling * ourselves below */ @@ -269,8 +279,13 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) * Otherwise, there's a window where we don't have an * ioeventfd and we may end up with a notification where * we don't expect one. + * + * Also note that we should only do something with the eventfd + * handler if the eventfd has actually been setup. */ - virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign); + if (k->ioeventfd_started(proxy)) { + virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign); + } if (!assign) { /* Use generic ioeventfd handler again. */ k->ioeventfd_set_disabled(proxy, false);
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, and will completely break things if the ioeventfd is disabled for the device. Fix this by starting the ioeventfd on assign if that has not happened before, and only switch the handler if the ioeventfd has really been started. 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> --- Changes v1->v2: - don't switch the handler if the eventfd has not been setup - this fixes the failure of vhost-user-test for me - add more comments --- hw/virtio/virtio-bus.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)