diff mbox

[v2] virtio: Fix setting up host notifiers for vhost

Message ID 1467287567-2632-1-git-send-email-cornelia.huck@de.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Cornelia Huck June 30, 2016, 11:52 a.m. UTC
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(-)

Comments

Peter Lieven June 30, 2016, 12:04 p.m. UTC | #1
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.
Marc-André Lureau June 30, 2016, 2:12 p.m. UTC | #2
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
>
Cornelia Huck June 30, 2016, 3:07 p.m. UTC | #3
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 mbox

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