diff mbox

virtio: revert host notifiers to old semantics

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

Commit Message

Cornelia Huck June 30, 2016, 3:31 p.m. UTC
The host notifier rework tried both to unify host notifiers across
transports and plug a possible hole during host notifier
re-assignment. Unfortunately, this meant a change in semantics that
breaks vhost and iSCSI+dataplane.

As the minimal fix, keep the common host notifier code but revert
to the old semantics so that we have time to figure out the proper
fix.

Fixes: 6798e245a3 ("virtio-bus: common ioeventfd infrastructure")
Reported-by: Peter Lieven <pl@kamp.de>
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>
---
 hw/virtio/virtio-bus.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

Comments

Marc-André Lureau July 1, 2016, 8:42 a.m. UTC | #1
Hi

On Thu, Jun 30, 2016 at 5:31 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> The host notifier rework tried both to unify host notifiers across
> transports and plug a possible hole during host notifier
> re-assignment. Unfortunately, this meant a change in semantics that
> breaks vhost and iSCSI+dataplane.
>
> As the minimal fix, keep the common host notifier code but revert
> to the old semantics so that we have time to figure out the proper
> fix.
>
> Fixes: 6798e245a3 ("virtio-bus: common ioeventfd infrastructure")
> Reported-by: Peter Lieven <pl@kamp.de>
> 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>
> ---
>  hw/virtio/virtio-bus.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 1313760..a85b7c8 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;
> @@ -251,31 +251,25 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
>  {
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
>      DeviceState *proxy = DEVICE(BUS(bus)->parent);
> -    VirtIODevice *vdev = virtio_bus_get_device(bus);
> -    VirtQueue *vq = virtio_get_queue(vdev, n);
>
>      if (!k->ioeventfd_started) {
>          return -ENOSYS;
>      }
> +    k->ioeventfd_set_disabled(proxy, assign);
>      if (assign) {
>          /*
>           * Stop using the generic ioeventfd, we are doing eventfd handling
>           * ourselves below
> +         *
> +         * FIXME: We should just switch the handler and not deassign the
> +         * ioeventfd.
> +         * 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.
>           */
> -        k->ioeventfd_set_disabled(proxy, true);
> -    }
> -    /*
> -     * Just switch the handler, don't deassign the ioeventfd.
> -     * 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.
> -     */
> -    virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
> -    if (!assign) {
> -        /* Use generic ioeventfd handler again. */
> -        k->ioeventfd_set_disabled(proxy, false);
> +        virtio_bus_stop_ioeventfd(bus);
>      }
> -    return 0;
> +    return set_host_notifier_internal(proxy, bus, n, assign, false);
>  }
>
>  static char *virtio_bus_get_dev_path(DeviceState *dev)
> --
> 2.6.6
>

Tested-by: Marc-André Lureau <marcandre.lureau@redhat.com>

thanks
Peter Lieven July 4, 2016, 6:26 a.m. UTC | #2
Am 30.06.2016 um 17:31 schrieb Cornelia Huck:
> The host notifier rework tried both to unify host notifiers across
> transports and plug a possible hole during host notifier
> re-assignment. Unfortunately, this meant a change in semantics that
> breaks vhost and iSCSI+dataplane.
>
> As the minimal fix, keep the common host notifier code but revert
> to the old semantics so that we have time to figure out the proper
> fix.
>
> Fixes: 6798e245a3 ("virtio-bus: common ioeventfd infrastructure")
> Reported-by: Peter Lieven <pl@kamp.de>
> 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>

Works for iscsi + dateplane.

Peter
Jason Wang July 4, 2016, 6:59 a.m. UTC | #3
On 2016年06月30日 23:31, Cornelia Huck wrote:
> The host notifier rework tried both to unify host notifiers across
> transports and plug a possible hole during host notifier
> re-assignment. Unfortunately, this meant a change in semantics that
> breaks vhost and iSCSI+dataplane.
>
> As the minimal fix, keep the common host notifier code but revert
> to the old semantics so that we have time to figure out the proper
> fix.
>
> Fixes: 6798e245a3 ("virtio-bus: common ioeventfd infrastructure")
> Reported-by: Peter Lieven <pl@kamp.de>
> 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>
> ---
>   hw/virtio/virtio-bus.c | 26 ++++++++++----------------
>   1 file changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 1313760..a85b7c8 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;
> @@ -251,31 +251,25 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
>   {
>       VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
>       DeviceState *proxy = DEVICE(BUS(bus)->parent);
> -    VirtIODevice *vdev = virtio_bus_get_device(bus);
> -    VirtQueue *vq = virtio_get_queue(vdev, n);
>   
>       if (!k->ioeventfd_started) {
>           return -ENOSYS;
>       }
> +    k->ioeventfd_set_disabled(proxy, assign);
>       if (assign) {
>           /*
>            * Stop using the generic ioeventfd, we are doing eventfd handling
>            * ourselves below
> +         *
> +         * FIXME: We should just switch the handler and not deassign the
> +         * ioeventfd.
> +         * 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.
>            */
> -        k->ioeventfd_set_disabled(proxy, true);
> -    }
> -    /*
> -     * Just switch the handler, don't deassign the ioeventfd.
> -     * 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.
> -     */
> -    virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
> -    if (!assign) {
> -        /* Use generic ioeventfd handler again. */
> -        k->ioeventfd_set_disabled(proxy, false);
> +        virtio_bus_stop_ioeventfd(bus);
>       }
> -    return 0;
> +    return set_host_notifier_internal(proxy, bus, n, assign, false);
>   }
>   
>   static char *virtio_bus_get_dev_path(DeviceState *dev)

Reviewed-by: Jason Wang <jasowang@redhat.com>
Tested-by: Jason Wang <jasowang@redhat.com>
diff mbox

Patch

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 1313760..a85b7c8 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;
@@ -251,31 +251,25 @@  int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
 {
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus);
     DeviceState *proxy = DEVICE(BUS(bus)->parent);
-    VirtIODevice *vdev = virtio_bus_get_device(bus);
-    VirtQueue *vq = virtio_get_queue(vdev, n);
 
     if (!k->ioeventfd_started) {
         return -ENOSYS;
     }
+    k->ioeventfd_set_disabled(proxy, assign);
     if (assign) {
         /*
          * Stop using the generic ioeventfd, we are doing eventfd handling
          * ourselves below
+         *
+         * FIXME: We should just switch the handler and not deassign the
+         * ioeventfd.
+         * 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.
          */
-        k->ioeventfd_set_disabled(proxy, true);
-    }
-    /*
-     * Just switch the handler, don't deassign the ioeventfd.
-     * 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.
-     */
-    virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
-    if (!assign) {
-        /* Use generic ioeventfd handler again. */
-        k->ioeventfd_set_disabled(proxy, false);
+        virtio_bus_stop_ioeventfd(bus);
     }
-    return 0;
+    return set_host_notifier_internal(proxy, bus, n, assign, false);
 }
 
 static char *virtio_bus_get_dev_path(DeviceState *dev)