diff mbox

Regression: virtio-pci: convert to ioeventfd callbacks

Message ID 57722B75.9090406@kamp.de (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Lieven June 28, 2016, 7:47 a.m. UTC
Am 28.06.2016 um 09:42 schrieb Cornelia Huck:
> On Tue, 28 Jun 2016 10:03:21 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> I notice cleanup is a bit weird:
>>
>>          virtio_queue_set_host_notifier_fd_handler(vq, false, false);
>>          k->ioeventfd_assign(proxy, notifier, n, assign);
>>          event_notifier_cleanup(notifier);
>>
>> I think virtio_queue_set_host_notifier_fd_handler should happen
>> after ioeventfd_assign for symmetry with init?
> Looking at the pre-rework code, ccw used the order now in common code,
> while pci and mmio used the order you suggest.
>
> "Switch the handler back, then unassign the transport's ioeventfd
> backing" made more sense to me (regardless of symmetry) - but we might
> lose a notification?
>
> Peter: Can you check whether your problem goes away if you switch the
> two lines around?
>

The problem goes away, but its horribly slow. Maybe the lost notifications
you were thinking off.


Peter

Comments

Cornelia Huck June 28, 2016, 7:59 a.m. UTC | #1
On Tue, 28 Jun 2016 09:47:01 +0200
Peter Lieven <pl@kamp.de> wrote:

> Am 28.06.2016 um 09:42 schrieb Cornelia Huck:
> > On Tue, 28 Jun 2016 10:03:21 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> >> I notice cleanup is a bit weird:
> >>
> >>          virtio_queue_set_host_notifier_fd_handler(vq, false, false);
> >>          k->ioeventfd_assign(proxy, notifier, n, assign);
> >>          event_notifier_cleanup(notifier);
> >>
> >> I think virtio_queue_set_host_notifier_fd_handler should happen
> >> after ioeventfd_assign for symmetry with init?
> > Looking at the pre-rework code, ccw used the order now in common code,
> > while pci and mmio used the order you suggest.
> >
> > "Switch the handler back, then unassign the transport's ioeventfd
> > backing" made more sense to me (regardless of symmetry) - but we might
> > lose a notification?
> >
> > Peter: Can you check whether your problem goes away if you switch the
> > two lines around?
> >
> 
> The problem goes away, but its horribly slow. Maybe the lost notifications
> you were thinking off.

It points into that direction.

> 
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 1313760..7924a59 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -176,9 +176,9 @@ 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);

The virtio_queue_...() should go here.

>           event_notifier_cleanup(notifier);
> +        virtio_queue_set_host_notifier_fd_handler(vq, false, false);
>       }
>       return r;
>   }
> 
> Peter
>
diff mbox

Patch

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 1313760..7924a59 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -176,9 +176,9 @@  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);
          event_notifier_cleanup(notifier);
+        virtio_queue_set_host_notifier_fd_handler(vq, false, false);
      }
      return r;
  }