* [Qemu-devel] [PATCH] Revert "virtio: postpone the execution of event_notifier_cleanup function"
@ 2018-01-23 13:20 Jose Ricardo Ziviani
2018-01-23 14:27 ` Laurent Vivier
2018-01-23 15:22 ` Michael S. Tsirkin
0 siblings, 2 replies; 5+ messages in thread
From: Jose Ricardo Ziviani @ 2018-01-23 13:20 UTC (permalink / raw)
To: qemu-devel, mst, ghammer; +Cc: anton, danielhb, pbonzini
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] Revert "virtio: postpone the execution of event_notifier_cleanup function"
2018-01-23 13:20 [Qemu-devel] [PATCH] Revert "virtio: postpone the execution of event_notifier_cleanup function" Jose Ricardo Ziviani
@ 2018-01-23 14:27 ` Laurent Vivier
2018-01-23 15:22 ` Michael S. Tsirkin
1 sibling, 0 replies; 5+ messages in thread
From: Laurent Vivier @ 2018-01-23 14:27 UTC (permalink / raw)
To: Jose Ricardo Ziviani, qemu-devel, mst, ghammer; +Cc: pbonzini, danielhb, anton
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] Revert "virtio: postpone the execution of event_notifier_cleanup function"
2018-01-23 13:20 [Qemu-devel] [PATCH] Revert "virtio: postpone the execution of event_notifier_cleanup function" Jose Ricardo Ziviani
2018-01-23 14:27 ` Laurent Vivier
@ 2018-01-23 15:22 ` Michael S. Tsirkin
2018-01-23 15:46 ` joserz
2018-01-23 17:48 ` joserz
1 sibling, 2 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2018-01-23 15:22 UTC (permalink / raw)
To: Jose Ricardo Ziviani; +Cc: qemu-devel, ghammer, anton, danielhb, pbonzini
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] Revert "virtio: postpone the execution of event_notifier_cleanup function"
2018-01-23 15:22 ` Michael S. Tsirkin
@ 2018-01-23 15:46 ` joserz
2018-01-23 17:48 ` joserz
1 sibling, 0 replies; 5+ messages in thread
From: joserz @ 2018-01-23 15:46 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: ghammer, pbonzini, danielhb, qemu-devel, anton
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] Revert "virtio: postpone the execution of event_notifier_cleanup function"
2018-01-23 15:22 ` Michael S. Tsirkin
2018-01-23 15:46 ` joserz
@ 2018-01-23 17:48 ` joserz
1 sibling, 0 replies; 5+ messages in thread
From: joserz @ 2018-01-23 17:48 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: ghammer, pbonzini, danielhb, qemu-devel, anton
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-01-23 17:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-23 13:20 [Qemu-devel] [PATCH] Revert "virtio: postpone the execution of event_notifier_cleanup function" Jose Ricardo Ziviani
2018-01-23 14:27 ` Laurent Vivier
2018-01-23 15:22 ` Michael S. Tsirkin
2018-01-23 15:46 ` joserz
2018-01-23 17:48 ` joserz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.