[Qemu-devel] [PATCH] virtio: Fix setting up host notifiers for vhost

[Qemu-devel] [PATCH] virtio: Fix setting up host notifiers for vhost

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

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

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

Re: [Qemu-devel] [PATCH] virtio: Fix setting up host notifiers for vhost

[Marc-André Lureau]

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
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] virtio: Fix setting up host notifiers for vhost

[Cornelia Huck]

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

Re: [Qemu-devel] [PATCH] virtio: Fix setting up host notifiers for vhost

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] virtio: Fix setting up host notifiers for vhost

[Cornelia Huck]

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.

Re: [Qemu-devel] [PATCH] virtio: Fix setting up host notifiers for vhost

[Cornelia Huck]

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.