diff mbox

[3/9] dataplane: remove EventPoll in favor of AioContext

Message ID 5139A359.9030407@de.ibm.com
State New
Headers show

Commit Message

Christian Borntraeger March 8, 2013, 8:37 a.m. UTC 
On 04/03/13 10:15, Stefan Hajnoczi wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> During the review of the dataplane code, the EventPoll API morphed itself
> (not concidentially) into something very very similar to an AioContext.
> Thus, it is trivial to convert virtio-blk-dataplane to use AioContext,
> and a first baby step towards letting dataplane talk directly to the
> QEMU block layer.
> 
> The only interesting note is the value-copy of EventNotifiers.  At least
> in my opinion this is part of the EventNotifier API and is even portable
> to Windows.  Of course, in this case you should not close the notifier's
> underlying file descriptors or handle with event_notifier_cleanup.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Hmm, this broke data plane on our internal notifier prototype code on virtio-ccw
(attached for reference)
[...]


> +    /* Note that these EventNotifiers are assigned by value.  This is
> +     * fine as long as you do not call event_notifier_cleanup on them
> +     * (because you don't own the file descriptor or handle; you just
> +     * use it).
> +     */

And this might be the reason. Currently we only have eventfd to wire up 
guest_to_host notifiers. The host_to_guest notification is not done
via vectors/irqfd, instead we let our qemu transport listen to the irq
eventfd. Worked fine so far with vhost and dataplane without this patch.

Any ideas how to properly enable a transport that has full host notifiers
but only poor mans guest notifiers?

Christian

Comments

Paolo Bonzini March 8, 2013, 9:22 a.m. UTC | #1 
Il 08/03/2013 09:37, Christian Borntraeger ha scritto:
> +    if (assign) {
> +        int r = event_notifier_init(notifier, 0);
> +
> +        if (r < 0) {
> +            return r;
> +        }
> +        virtio_queue_set_guest_notifier_fd_handler(vq, true, with_irqfd);

Perhaps you can simply hard-code with_irqfd to false in this call to
virtio_queue_set_guest_notifier_fd_handler and the one below?  Then the
guest notifier will be emulated in userspace and processed via
vdev->binding->notify.

You will not need to overwrite the EventNotifier which is IMO a pretty
ufly violation of encapsulation. :)

Paolo

> +        /* We do not support irqfd for classic I/O interrupts, because the
> +         * classic interrupts are intermixed with the subchannel status, that
> +         * is queried with test subchannel. We want to use vhost, though.
> +         * Lets make sure to have vhost running and wire up the irq fd to 
> +         * land in qemu (and only the irq fd) in this code.
> +         */
> +        if (dev->vdev->guest_notifier_mask) {
> +            dev->vdev->guest_notifier_mask(dev->vdev, n, false);
> +        }
> +        /* get lost events and re-inject */
> +        if (dev->vdev->guest_notifier_pending &&
> +            dev->vdev->guest_notifier_pending(dev->vdev, n)) {
> +            event_notifier_set(notifier);
> +        }
> +    } else {
> +        if (dev->vdev->guest_notifier_mask) {
> +            dev->vdev->guest_notifier_mask(dev->vdev, n, true);
> +        }
> +        virtio_queue_set_guest_notifier_fd_handler(vq, false, with_irqfd);
> +        event_notifier_cleanup(notifier);
Cornelia Huck March 8, 2013, 12:44 p.m. UTC | #2 
On Fri, 08 Mar 2013 10:22:36 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 08/03/2013 09:37, Christian Borntraeger ha scritto:
> > +    if (assign) {
> > +        int r = event_notifier_init(notifier, 0);
> > +
> > +        if (r < 0) {
> > +            return r;
> > +        }
> > +        virtio_queue_set_guest_notifier_fd_handler(vq, true, with_irqfd);
> 
> Perhaps you can simply hard-code with_irqfd to false in this call to
> virtio_queue_set_guest_notifier_fd_handler and the one below?  Then the
> guest notifier will be emulated in userspace and processed via
> vdev->binding->notify.

Well, effectively with_irqfd is already hardcoded to false (as
virtio_ccw_set_guest_notifiers() always calls this function with
with_irqfd=false), so that doesn't seem to be the problem here.

> 
> You will not need to overwrite the EventNotifier which is IMO a pretty
> ufly violation of encapsulation. :)
> 
> Paolo
> 
> > +        /* We do not support irqfd for classic I/O interrupts, because the
> > +         * classic interrupts are intermixed with the subchannel status, that
> > +         * is queried with test subchannel. We want to use vhost, though.
> > +         * Lets make sure to have vhost running and wire up the irq fd to 
> > +         * land in qemu (and only the irq fd) in this code.
> > +         */
> > +        if (dev->vdev->guest_notifier_mask) {
> > +            dev->vdev->guest_notifier_mask(dev->vdev, n, false);
> > +        }
> > +        /* get lost events and re-inject */
> > +        if (dev->vdev->guest_notifier_pending &&
> > +            dev->vdev->guest_notifier_pending(dev->vdev, n)) {
> > +            event_notifier_set(notifier);
> > +        }
> > +    } else {
> > +        if (dev->vdev->guest_notifier_mask) {
> > +            dev->vdev->guest_notifier_mask(dev->vdev, n, true);
> > +        }
> > +        virtio_queue_set_guest_notifier_fd_handler(vq, false, with_irqfd);
> > +        event_notifier_cleanup(notifier);
>
Paolo Bonzini March 8, 2013, 1:51 p.m. UTC | #3 
Il 08/03/2013 13:44, Cornelia Huck ha scritto:
>> > Perhaps you can simply hard-code with_irqfd to false in this call to
>> > virtio_queue_set_guest_notifier_fd_handler and the one below?  Then the
>> > guest notifier will be emulated in userspace and processed via
>> > vdev->binding->notify.
> 
> Well, effectively with_irqfd is already hardcoded to false (as
> virtio_ccw_set_guest_notifiers() always calls this function with
> with_irqfd=false), so that doesn't seem to be the problem here.

Actually, the guest->host notifier is not touched by either the old or
the new code.  The code I modified only worries about the host->guest
notifier.

How did you track the problem to the assignment by value of EventNotifiers?

BTW:

>> You will not need to overwrite the EventNotifier which is IMO a pretty
>> ufly violation of encapsulation. 

This was nonsense. :)

Paolo
diff mbox

Patch

From 76ceaec73c44f71b2e703accb157c09fef94ccd1 Mon Sep 17 00:00:00 2001
From: Cornelia Huck <cornelia.huck@de.ibm.com>
Date: Tue, 19 Feb 2013 13:48:17 +0100
Subject: [PATCH 23/28] Re-add guest/host notifiers.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390x/virtio-ccw.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/s390x/virtio-ccw.h |  1 +
 2 files changed, 91 insertions(+)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 63c851f..d4fa42a 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -95,6 +95,7 @@  static void virtio_ccw_start_ioeventfd(VirtioCcwDevice *dev)
     int n, r;
 
     if (!(dev->flags & VIRTIO_CCW_FLAG_USE_IOEVENTFD) ||
+        dev->ioeventfd_disabled ||
         dev->ioeventfd_started) {
         return;
     }
@@ -793,6 +794,89 @@  static void virtio_ccw_vmstate_change(DeviceState *d, bool running)
     }
 }
 
+static bool virtio_ccw_query_guest_notifiers(DeviceState *d)
+{
+    VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+
+    return !!(dev->sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_ENA);
+}
+
+static int virtio_ccw_set_host_notifier(DeviceState *d, int n, bool assign)
+{
+    VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+
+    /* Stop using the generic ioeventfd, we are doing eventfd handling
+     * ourselves below */
+    dev->ioeventfd_disabled = assign;
+    if (assign) {
+        virtio_ccw_stop_ioeventfd(dev);
+    }
+    return virtio_ccw_set_guest2host_notifier(dev, n, assign, false);
+}
+
+static int virtio_ccw_set_guest_notifier(VirtioCcwDevice *dev, int n,
+                                         bool assign, bool with_irqfd)
+{
+    VirtQueue *vq = virtio_get_queue(dev->vdev, n);
+    EventNotifier *notifier = virtio_queue_get_guest_notifier(vq);
+
+    if (assign) {
+        int r = event_notifier_init(notifier, 0);
+
+        if (r < 0) {
+            return r;
+        }
+        virtio_queue_set_guest_notifier_fd_handler(vq, true, with_irqfd);
+        /* We do not support irqfd for classic I/O interrupts, because the
+         * classic interrupts are intermixed with the subchannel status, that
+         * is queried with test subchannel. We want to use vhost, though.
+         * Lets make sure to have vhost running and wire up the irq fd to 
+         * land in qemu (and only the irq fd) in this code.
+         */
+        if (dev->vdev->guest_notifier_mask) {
+            dev->vdev->guest_notifier_mask(dev->vdev, n, false);
+        }
+        /* get lost events and re-inject */
+        if (dev->vdev->guest_notifier_pending &&
+            dev->vdev->guest_notifier_pending(dev->vdev, n)) {
+            event_notifier_set(notifier);
+        }
+    } else {
+        if (dev->vdev->guest_notifier_mask) {
+            dev->vdev->guest_notifier_mask(dev->vdev, n, true);
+        }
+        virtio_queue_set_guest_notifier_fd_handler(vq, false, with_irqfd);
+        event_notifier_cleanup(notifier);
+    }
+    return 0;
+}
+
+static int virtio_ccw_set_guest_notifiers(DeviceState *d, int nvqs,
+                                          bool assigned)
+{
+    VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+    VirtIODevice *vdev = dev->vdev;
+    int r, n;
+
+    for (n = 0; n < nvqs; n++) {
+        if (!virtio_queue_get_num(vdev, n)) {
+            break;
+        }
+        /* false -> true, as soon as irqfd works */
+        r = virtio_ccw_set_guest_notifier(dev, n, assigned, false);
+        if (r < 0) {
+            goto assign_error;
+        }
+    }
+    return 0;
+
+assign_error:
+    while (--n >= 0) {
+        virtio_ccw_set_guest_notifier(dev, n, !assigned, false);
+    }
+    return r;
+}
+
 static void virtio_ccw_save_queue(DeviceState *d, int n, QEMUFile *f)
 {
     VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
@@ -840,6 +924,9 @@  static const VirtIOBindings virtio_ccw_bindings = {
     .notify = virtio_ccw_notify,
     .get_features = virtio_ccw_get_features,
     .vmstate_change = virtio_ccw_vmstate_change,
+    .query_guest_notifiers = virtio_ccw_query_guest_notifiers,
+    .set_host_notifier = virtio_ccw_set_host_notifier,
+    .set_guest_notifiers = virtio_ccw_set_guest_notifiers,
     .save_queue = virtio_ccw_save_queue,
     .load_queue = virtio_ccw_load_queue,
     .save_config = virtio_ccw_save_config,
@@ -1093,6 +1180,9 @@  static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data)
     k->notify = virtio_ccw_notify;
     k->get_features = virtio_ccw_get_features;
     k->vmstate_change = virtio_ccw_vmstate_change;
+    k->query_guest_notifiers = virtio_ccw_query_guest_notifiers;
+    k->set_host_notifier = virtio_ccw_set_host_notifier;
+    k->set_guest_notifiers = virtio_ccw_set_guest_notifiers;
     k->save_queue = virtio_ccw_save_queue;
     k->load_queue = virtio_ccw_load_queue;
     k->save_config = virtio_ccw_save_config;
diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index db83e0f..a2e066f 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -84,6 +84,7 @@  struct VirtioCcwDevice {
     VirtIOSCSIConf scsi;
     VirtioBusState bus;
     bool ioeventfd_started;
+    bool ioeventfd_disabled;
     uint32_t flags;
     /* Guest provided values: */
     hwaddr indicators;
-- 
1.8.0.1