[Qemu-devel] [PATCH] Revert "virtio: postpone the execution of event_notifier_cleanup function"

[Qemu-devel] [PATCH] Revert "virtio: postpone the execution of event_notifier_cleanup function"

[Jose Ricardo Ziviani]

This reverts commit 4fe6d78b2e241f41208dfb07605aace4becfc747.

As reported http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg05457.html
The referred commit is causing regression issues in virtio.

Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
Reported-by: Anton Blanchard <anton@samba.org>
---
 accel/kvm/kvm-all.c    |  4 ----
 hw/virtio/virtio-bus.c | 19 ++++++++-----------
 2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 071f4f57c0..f290f487a5 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -812,10 +812,6 @@ static void kvm_mem_ioeventfd_del(MemoryListener *listener,
     if (r < 0) {
         abort();
     }
-
-    if (e->cleanup) {
-        e->cleanup(e);
-    }
 }
 
 static void kvm_io_ioeventfd_add(MemoryListener *listener,
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 8106346927..3042232daf 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -256,15 +256,6 @@ bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus)
     return k->ioeventfd_assign && k->ioeventfd_enabled(proxy);
 }
 
-static void virtio_bus_cleanup_event_notifier(EventNotifier *notifier)
-{
-    /* Test and clear notifier after disabling event,
-     * in case poll callback didn't have time to run.
-     */
-    virtio_queue_host_notifier_read(notifier);
-    event_notifier_cleanup(notifier);
-}
-
 /*
  * This function switches ioeventfd on/off in the device.
  * The caller must set or clear the handlers for the EventNotifier.
@@ -292,13 +283,19 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
         r = k->ioeventfd_assign(proxy, notifier, n, true);
         if (r < 0) {
             error_report("%s: unable to assign ioeventfd: %d", __func__, r);
-            virtio_bus_cleanup_event_notifier(notifier);
+            goto cleanup_event_notifier;
         }
+        return 0;
     } else {
-        notifier->cleanup = virtio_bus_cleanup_event_notifier;
         k->ioeventfd_assign(proxy, notifier, n, false);
     }
 
+cleanup_event_notifier:
+    /* Test and clear notifier after disabling event,
+     * in case poll callback didn't have time to run.
+     */
+    virtio_queue_host_notifier_read(notifier);
+    event_notifier_cleanup(notifier);
     return r;
 }
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH] Revert "virtio: postpone the execution of event_notifier_cleanup function"

[Laurent Vivier]

On 23/01/2018 14:20, Jose Ricardo Ziviani wrote:
> This reverts commit 4fe6d78b2e241f41208dfb07605aace4becfc747.
> 
> As reported http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg05457.html
> The referred commit is causing regression issues in virtio.
> 
> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
> Reported-by: Anton Blanchard <anton@samba.org>
> ---
>  accel/kvm/kvm-all.c    |  4 ----
>  hw/virtio/virtio-bus.c | 19 ++++++++-----------
>  2 files changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 071f4f57c0..f290f487a5 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -812,10 +812,6 @@ static void kvm_mem_ioeventfd_del(MemoryListener *listener,
>      if (r < 0) {
>          abort();
>      }
> -
> -    if (e->cleanup) {
> -        e->cleanup(e);
> -    }
>  }
>  
>  static void kvm_io_ioeventfd_add(MemoryListener *listener,
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 8106346927..3042232daf 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -256,15 +256,6 @@ bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus)
>      return k->ioeventfd_assign && k->ioeventfd_enabled(proxy);
>  }
>  
> -static void virtio_bus_cleanup_event_notifier(EventNotifier *notifier)
> -{
> -    /* Test and clear notifier after disabling event,
> -     * in case poll callback didn't have time to run.
> -     */
> -    virtio_queue_host_notifier_read(notifier);
> -    event_notifier_cleanup(notifier);
> -}
> -
>  /*
>   * This function switches ioeventfd on/off in the device.
>   * The caller must set or clear the handlers for the EventNotifier.
> @@ -292,13 +283,19 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
>          r = k->ioeventfd_assign(proxy, notifier, n, true);
>          if (r < 0) {
>              error_report("%s: unable to assign ioeventfd: %d", __func__, r);
> -            virtio_bus_cleanup_event_notifier(notifier);
> +            goto cleanup_event_notifier;
>          }
> +        return 0;
>      } else {
> -        notifier->cleanup = virtio_bus_cleanup_event_notifier;
>          k->ioeventfd_assign(proxy, notifier, n, false);
>      }
>  
> +cleanup_event_notifier:
> +    /* Test and clear notifier after disabling event,
> +     * in case poll callback didn't have time to run.
> +     */
> +    virtio_queue_host_notifier_read(notifier);
> +    event_notifier_cleanup(notifier);
>      return r;
>  }
>  
> 

Tested-by: Laurent Vivier <lvivier@redhat.com>

Re: [Qemu-devel] [PATCH] Revert "virtio: postpone the execution of event_notifier_cleanup function"

[Michael S. Tsirkin]

On Tue, Jan 23, 2018 at 11:20:21AM -0200, Jose Ricardo Ziviani wrote:
> This reverts commit 4fe6d78b2e241f41208dfb07605aace4becfc747.
> 
> As reported http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg05457.html
> The referred commit is causing regression issues in virtio.
> 
> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
> Reported-by: Anton Blanchard <anton@samba.org>

Can you limit the patch to virtio bus please?
Maybe we want to drop the callback from the structure
but we don't want it to be there and just be ignored
I think.

> ---
>  accel/kvm/kvm-all.c    |  4 ----
>  hw/virtio/virtio-bus.c | 19 ++++++++-----------
>  2 files changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 071f4f57c0..f290f487a5 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -812,10 +812,6 @@ static void kvm_mem_ioeventfd_del(MemoryListener *listener,
>      if (r < 0) {
>          abort();
>      }
> -
> -    if (e->cleanup) {
> -        e->cleanup(e);
> -    }
>  }
>  
>  static void kvm_io_ioeventfd_add(MemoryListener *listener,
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 8106346927..3042232daf 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -256,15 +256,6 @@ bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus)
>      return k->ioeventfd_assign && k->ioeventfd_enabled(proxy);
>  }
>  
> -static void virtio_bus_cleanup_event_notifier(EventNotifier *notifier)
> -{
> -    /* Test and clear notifier after disabling event,
> -     * in case poll callback didn't have time to run.
> -     */
> -    virtio_queue_host_notifier_read(notifier);
> -    event_notifier_cleanup(notifier);
> -}
> -
>  /*
>   * This function switches ioeventfd on/off in the device.
>   * The caller must set or clear the handlers for the EventNotifier.
> @@ -292,13 +283,19 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
>          r = k->ioeventfd_assign(proxy, notifier, n, true);
>          if (r < 0) {
>              error_report("%s: unable to assign ioeventfd: %d", __func__, r);
> -            virtio_bus_cleanup_event_notifier(notifier);
> +            goto cleanup_event_notifier;
>          }
> +        return 0;
>      } else {
> -        notifier->cleanup = virtio_bus_cleanup_event_notifier;
>          k->ioeventfd_assign(proxy, notifier, n, false);
>      }
>  
> +cleanup_event_notifier:
> +    /* Test and clear notifier after disabling event,
> +     * in case poll callback didn't have time to run.
> +     */
> +    virtio_queue_host_notifier_read(notifier);
> +    event_notifier_cleanup(notifier);
>      return r;
>  }
>  
> -- 
> 2.14.3

Re: [Qemu-devel] [PATCH] Revert "virtio: postpone the execution of event_notifier_cleanup function"

[joserz]

On Tue, Jan 23, 2018 at 05:22:12PM +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 23, 2018 at 11:20:21AM -0200, Jose Ricardo Ziviani wrote:
> > This reverts commit 4fe6d78b2e241f41208dfb07605aace4becfc747.
> > 
> > As reported http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg05457.html
> > The referred commit is causing regression issues in virtio.
> > 
> > Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
> > Reported-by: Anton Blanchard <anton@samba.org>
> 
> Can you limit the patch to virtio bus please?
> Maybe we want to drop the callback from the structure
> but we don't want it to be there and just be ignored
> I think.
> 

Absolutely, I'm working on it

Thank you!

> > ---
> >  accel/kvm/kvm-all.c    |  4 ----
> >  hw/virtio/virtio-bus.c | 19 ++++++++-----------
> >  2 files changed, 8 insertions(+), 15 deletions(-)
> > 
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index 071f4f57c0..f290f487a5 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -812,10 +812,6 @@ static void kvm_mem_ioeventfd_del(MemoryListener *listener,
> >      if (r < 0) {
> >          abort();
> >      }
> > -
> > -    if (e->cleanup) {
> > -        e->cleanup(e);
> > -    }
> >  }
> >  
> >  static void kvm_io_ioeventfd_add(MemoryListener *listener,
> > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> > index 8106346927..3042232daf 100644
> > --- a/hw/virtio/virtio-bus.c
> > +++ b/hw/virtio/virtio-bus.c
> > @@ -256,15 +256,6 @@ bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus)
> >      return k->ioeventfd_assign && k->ioeventfd_enabled(proxy);
> >  }
> >  
> > -static void virtio_bus_cleanup_event_notifier(EventNotifier *notifier)
> > -{
> > -    /* Test and clear notifier after disabling event,
> > -     * in case poll callback didn't have time to run.
> > -     */
> > -    virtio_queue_host_notifier_read(notifier);
> > -    event_notifier_cleanup(notifier);
> > -}
> > -
> >  /*
> >   * This function switches ioeventfd on/off in the device.
> >   * The caller must set or clear the handlers for the EventNotifier.
> > @@ -292,13 +283,19 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
> >          r = k->ioeventfd_assign(proxy, notifier, n, true);
> >          if (r < 0) {
> >              error_report("%s: unable to assign ioeventfd: %d", __func__, r);
> > -            virtio_bus_cleanup_event_notifier(notifier);
> > +            goto cleanup_event_notifier;
> >          }
> > +        return 0;
> >      } else {
> > -        notifier->cleanup = virtio_bus_cleanup_event_notifier;
> >          k->ioeventfd_assign(proxy, notifier, n, false);
> >      }
> >  
> > +cleanup_event_notifier:
> > +    /* Test and clear notifier after disabling event,
> > +     * in case poll callback didn't have time to run.
> > +     */
> > +    virtio_queue_host_notifier_read(notifier);
> > +    event_notifier_cleanup(notifier);
> >      return r;
> >  }
> >  
> > -- 
> > 2.14.3
> 

Re: [Qemu-devel] [PATCH] Revert "virtio: postpone the execution of event_notifier_cleanup function"

[joserz]

On Tue, Jan 23, 2018 at 05:22:12PM +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 23, 2018 at 11:20:21AM -0200, Jose Ricardo Ziviani wrote:
> > This reverts commit 4fe6d78b2e241f41208dfb07605aace4becfc747.
> > 
> > As reported http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg05457.html
> > The referred commit is causing regression issues in virtio.
> > 
> > Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com>
> > Reported-by: Anton Blanchard <anton@samba.org>

Hello Michael,

> Can you limit the patch to virtio bus please?

Unfortunately it doesn't work. I made the change on virtio bus only but
the error persists. Actually the only way I found to fix it is by
reverting only the kvm_mem_ioeventfd_del() @ kvm-all.c, removing that
if (e->cleanup) code. I'm sending the V2.

thank you!!


> Maybe we want to drop the callback from the structure
> but we don't want it to be there and just be ignored
> I think.
> 
> > ---
> >  accel/kvm/kvm-all.c    |  4 ----
> >  hw/virtio/virtio-bus.c | 19 ++++++++-----------
> >  2 files changed, 8 insertions(+), 15 deletions(-)
> > 
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index 071f4f57c0..f290f487a5 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -812,10 +812,6 @@ static void kvm_mem_ioeventfd_del(MemoryListener *listener,
> >      if (r < 0) {
> >          abort();
> >      }
> > -
> > -    if (e->cleanup) {
> > -        e->cleanup(e);
> > -    }
> >  }
> >  
> >  static void kvm_io_ioeventfd_add(MemoryListener *listener,
> > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> > index 8106346927..3042232daf 100644
> > --- a/hw/virtio/virtio-bus.c
> > +++ b/hw/virtio/virtio-bus.c
> > @@ -256,15 +256,6 @@ bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus)
> >      return k->ioeventfd_assign && k->ioeventfd_enabled(proxy);
> >  }
> >  
> > -static void virtio_bus_cleanup_event_notifier(EventNotifier *notifier)
> > -{
> > -    /* Test and clear notifier after disabling event,
> > -     * in case poll callback didn't have time to run.
> > -     */
> > -    virtio_queue_host_notifier_read(notifier);
> > -    event_notifier_cleanup(notifier);
> > -}
> > -
> >  /*
> >   * This function switches ioeventfd on/off in the device.
> >   * The caller must set or clear the handlers for the EventNotifier.
> > @@ -292,13 +283,19 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
> >          r = k->ioeventfd_assign(proxy, notifier, n, true);
> >          if (r < 0) {
> >              error_report("%s: unable to assign ioeventfd: %d", __func__, r);
> > -            virtio_bus_cleanup_event_notifier(notifier);
> > +            goto cleanup_event_notifier;
> >          }
> > +        return 0;
> >      } else {
> > -        notifier->cleanup = virtio_bus_cleanup_event_notifier;
> >          k->ioeventfd_assign(proxy, notifier, n, false);
> >      }
> >  
> > +cleanup_event_notifier:
> > +    /* Test and clear notifier after disabling event,
> > +     * in case poll callback didn't have time to run.
> > +     */
> > +    virtio_queue_host_notifier_read(notifier);
> > +    event_notifier_cleanup(notifier);
> >      return r;
> >  }
> >  
> > -- 
> > 2.14.3
>