[Qemu-devel] [RFC PATCH 0/4] aio: Don't poll ioeventfd in nested aio_poll()

[Qemu-devel] [RFC PATCH 0/4] aio: Don't poll ioeventfd in nested aio_poll()

[Fam Zheng]

This series looks at the other side of the broken "qmp transaction" problem
with dataplane [1] - the event loop.

Before, an ioeventfd of a non-dataplane device is registered via
qemu_set_fd_handler, which is only polled directly by main_loop_wait(), not in
aio_poll(); an ioeventfd of a dataplane device (virtio-blk/virtio-scsi) is
registered specially via aio_set_event_notifier, which is a wrapper of
aio_set_fd_handler, thus it WILL be polled by all aio_poll().

As explained in [1], and in patch 3, this is wrong. The handlers mustn't run.

[1] Fixes it by unregistering them temporarily around such nested poll, while
this series fixes it by skipping those file descriptors in all nested
aio_poll(), just like how iohandler behaves in the main loop.

On the one hand, it is simpler than [1]; on the other hand, this approach is
also interesting because once we remove qemu_set_fd_handler2 [2], iohandler.c
can be removed by converting all qemu_set_fd_handler to the new
aio_set_io_event_notifier introduced in this series.

Note that patch 2 is not tested.

[1]: https://lists.gnu.org/archive/html/qemu-block/2015-05/msg00800.html
[2]: https://lists.gnu.org/archive/html/qemu-block/2015-05/msg00647.html


Fam Zheng (4):
  aio-posix: Introduce aio_set_io_event_notifier
  aio-win32: Implement aio_set_io_event_notifier
  virtio-blk: Use aio_set_io_event_notifier in dataplane
  virtio-scsi-dataplane: User aio_set_io_event_notifier

 aio-posix.c                     | 33 +++++++++++++++++++-----
 aio-win32.c                     | 56 ++++++++++++++++++++++++++++++++++-------
 hw/block/dataplane/virtio-blk.c |  4 +--
 hw/scsi/virtio-scsi-dataplane.c | 18 +++++++------
 include/block/aio.h             | 23 ++++++++++++++---
 5 files changed, 106 insertions(+), 28 deletions(-)

-- 
2.4.1

[Qemu-devel] [RFC PATCH 1/4] aio-posix: Introduce aio_set_io_event_notifier

[Fam Zheng]

This function will register the fd handler similar to
aio_set_event_notifier, but the difference is the fd will only be polled
in the outmost aio_poll.

This is useful in some cases like device ioeventfd, where the handler
typically processes a guest request, therefore nested aio_poll shouldn't
include this.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 aio-posix.c         | 33 +++++++++++++++++++++++++++------
 include/block/aio.h | 23 ++++++++++++++++++++---
 2 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/aio-posix.c b/aio-posix.c
index 4abec38..05a0502 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -24,6 +24,7 @@ struct AioHandler
     IOHandler *io_read;
     IOHandler *io_write;
     int deleted;
+    bool outmost;
     void *opaque;
     QLIST_ENTRY(AioHandler) node;
 };
@@ -41,11 +42,12 @@ static AioHandler *find_aio_handler(AioContext *ctx, int fd)
     return NULL;
 }
 
-void aio_set_fd_handler(AioContext *ctx,
-                        int fd,
-                        IOHandler *io_read,
-                        IOHandler *io_write,
-                        void *opaque)
+static void aio_set_fd_handler_do(AioContext *ctx,
+                                  int fd,
+                                  IOHandler *io_read,
+                                  IOHandler *io_write,
+                                  void *opaque,
+                                  bool outmost)
 {
     AioHandler *node;
 
@@ -82,6 +84,7 @@ void aio_set_fd_handler(AioContext *ctx,
         node->io_read = io_read;
         node->io_write = io_write;
         node->opaque = opaque;
+        node->outmost = outmost;
 
         node->pfd.events = (io_read ? G_IO_IN | G_IO_HUP | G_IO_ERR : 0);
         node->pfd.events |= (io_write ? G_IO_OUT | G_IO_ERR : 0);
@@ -90,6 +93,15 @@ void aio_set_fd_handler(AioContext *ctx,
     aio_notify(ctx);
 }
 
+void aio_set_fd_handler(AioContext *ctx,
+                        int fd,
+                        IOHandler *io_read,
+                        IOHandler *io_write,
+                        void *opaque)
+{
+    aio_set_fd_handler_do(ctx, fd, io_read, io_write, opaque, false);
+}
+
 void aio_set_event_notifier(AioContext *ctx,
                             EventNotifier *notifier,
                             EventNotifierHandler *io_read)
@@ -98,6 +110,14 @@ void aio_set_event_notifier(AioContext *ctx,
                        (IOHandler *)io_read, NULL, notifier);
 }
 
+void aio_set_io_event_notifier(AioContext *ctx,
+                               EventNotifier *notifier,
+                               EventNotifierHandler *io_read)
+{
+    aio_set_fd_handler_do(ctx, event_notifier_get_fd(notifier),
+                          (IOHandler *)io_read, NULL, notifier, true);
+}
+
 bool aio_prepare(AioContext *ctx)
 {
     return false;
@@ -260,7 +280,8 @@ bool aio_poll(AioContext *ctx, bool blocking)
 
     /* fill pollfds */
     QLIST_FOREACH(node, &ctx->aio_handlers, node) {
-        if (!node->deleted && node->pfd.events) {
+        if (!node->deleted && node->pfd.events
+                && !(was_dispatching && node->outmost)) {
             add_pollfd(node);
         }
     }
diff --git a/include/block/aio.h b/include/block/aio.h
index d2bb423..20bfb2b 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -240,9 +240,7 @@ bool aio_dispatch(AioContext *ctx);
  */
 bool aio_poll(AioContext *ctx, bool blocking);
 
-/* Register a file descriptor and associated callbacks.  Behaves very similarly
- * to qemu_set_fd_handler2.  Unlike qemu_set_fd_handler2, these callbacks will
- * be invoked when using aio_poll().
+/* Register a file descriptor and associated callbacks.
  *
  * Code that invokes AIO completion functions should rely on this function
  * instead of qemu_set_fd_handler[2].
@@ -253,6 +251,15 @@ void aio_set_fd_handler(AioContext *ctx,
                         IOHandler *io_write,
                         void *opaque);
 
+/* Register a file descriptor and associated callbacks.  Behaves very similarly
+ * to qemu_set_fd_handler: these callbacks will be invoked only by outmost
+ * aio_poll() directly, but not in nested polls (such as bdrv_drain() or
+ * bdrv_aio_cancel()).
+ */
+void aio_set_iohandler(AioContext *ctx, int fd,
+                       IOHandler *io_read,
+                       IOHandler *io_write,
+                       void *opaque);
 /* Register an event notifier and associated callbacks.  Behaves very similarly
  * to event_notifier_set_handler.  Unlike event_notifier_set_handler, these callbacks
  * will be invoked when using aio_poll().
@@ -264,6 +271,16 @@ void aio_set_event_notifier(AioContext *ctx,
                             EventNotifier *notifier,
                             EventNotifierHandler *io_read);
 
+/* Similar to aio_set_event_notifier except the notifier will only be checked
+ * in top level poll.
+ *
+ * Notifiers for external requests like guest I/O or monitor command should use
+ * this function.
+ */
+void aio_set_io_event_notifier(AioContext *ctx,
+                               EventNotifier *notifier,
+                               EventNotifierHandler *io_read);
+
 /* Return a GSource that lets the main loop poll the file descriptors attached
  * to this AioContext.
  */
-- 
2.4.1

Re: [Qemu-devel] [RFC PATCH 1/4] aio-posix: Introduce aio_set_io_event_notifier

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]

On 05/27/2015 01:19 AM, Fam Zheng wrote:
> This function will register the fd handler similar to
> aio_set_event_notifier, but the difference is the fd will only be polled
> in the outmost aio_poll.

s/outmost/outermost/
that will have a ripple effect throughout the patch series...

> 
> This is useful in some cases like device ioeventfd, where the handler
> typically processes a guest request, therefore nested aio_poll shouldn't
> include this.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  aio-posix.c         | 33 +++++++++++++++++++++++++++------
>  include/block/aio.h | 23 ++++++++++++++++++++---
>  2 files changed, 47 insertions(+), 9 deletions(-)
> 
> diff --git a/aio-posix.c b/aio-posix.c
> index 4abec38..05a0502 100644
> --- a/aio-posix.c
> +++ b/aio-posix.c
> @@ -24,6 +24,7 @@ struct AioHandler
>      IOHandler *io_read;
>      IOHandler *io_write;
>      int deleted;
> +    bool outmost;

...like here

But I'll probably let others do the technical review.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [RFC PATCH 2/4] aio-win32: Implement aio_set_io_event_notifier

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 aio-win32.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 9 deletions(-)

diff --git a/aio-win32.c b/aio-win32.c
index 233d8f5..52867ab 100644
--- a/aio-win32.c
+++ b/aio-win32.c
@@ -27,15 +27,17 @@ struct AioHandler {
     EventNotifierHandler *io_notify;
     GPollFD pfd;
     int deleted;
+    bool outmost;
     void *opaque;
     QLIST_ENTRY(AioHandler) node;
 };
 
-void aio_set_fd_handler(AioContext *ctx,
-                        int fd,
-                        IOHandler *io_read,
-                        IOHandler *io_write,
-                        void *opaque)
+void aio_set_fd_handler_do(AioContext *ctx,
+                           int fd,
+                           IOHandler *io_read,
+                           IOHandler *io_write,
+                           void *opaque,
+                           bool outmost)
 {
     /* fd is a SOCKET in our case */
     AioHandler *node;
@@ -86,6 +88,7 @@ void aio_set_fd_handler(AioContext *ctx,
         node->opaque = opaque;
         node->io_read = io_read;
         node->io_write = io_write;
+        node->outmost = outmost;
 
         event = event_notifier_get_handle(&ctx->notifier);
         WSAEventSelect(node->pfd.fd, event,
@@ -96,9 +99,29 @@ void aio_set_fd_handler(AioContext *ctx,
     aio_notify(ctx);
 }
 
-void aio_set_event_notifier(AioContext *ctx,
-                            EventNotifier *e,
-                            EventNotifierHandler *io_notify)
+void aio_set_fd_handler(AioContext *ctx,
+                        int fd,
+                        IOHandler *io_read,
+                        IOHandler *io_write,
+                        void *opaque)
+{
+    aio_set_fd_handler_do(ctx, fd, io_read, io_write, opaque, false);
+}
+
+void aio_set_iohandler(AioContext *ctx,
+                       int fd,
+                       IOHandler *io_read,
+                       IOHandler *io_write,
+                       void *opaque)
+{
+    aio_set_fd_handler_do(ctx, fd, io_read, io_write, opaque, true);
+}
+
+
+static void aio_set_event_notifier_do(AioContext *ctx,
+                                      EventNotifier *e,
+                                      EventNotifierHandler *io_notify,
+                                      bool outmost)
 {
     AioHandler *node;
 
@@ -144,6 +167,20 @@ void aio_set_event_notifier(AioContext *ctx,
     aio_notify(ctx);
 }
 
+void aio_set_event_notifier(AioContext *ctx,
+                            EventNotifier *e,
+                            EventNotifierHandler *io_notify)
+{
+    return aio_set_event_notifier_do(ctx, e, io_notify, false);
+}
+
+void aio_set_io_event_notifier(AioContext *ctx,
+                               EventNotifier *e,
+                               EventNotifierHandler *io_notify)
+{
+    return aio_set_event_notifier_do(ctx, e, io_notify, true);
+}
+
 bool aio_prepare(AioContext *ctx)
 {
     static struct timeval tv0;
@@ -309,7 +346,8 @@ bool aio_poll(AioContext *ctx, bool blocking)
     /* fill fd sets */
     count = 0;
     QLIST_FOREACH(node, &ctx->aio_handlers, node) {
-        if (!node->deleted && node->io_notify) {
+        if (!node->deleted && node->io_notify &&
+                !(was_dispatching && node->outmost)) {
             events[count++] = event_notifier_get_handle(node->e);
         }
     }
-- 
2.4.1

[Qemu-devel] [RFC PATCH 3/4] virtio-blk: Use aio_set_io_event_notifier in dataplane

[Fam Zheng]

Currently the host notifier is checked by all aio_poll, which is not
safe. For example, in a qmp transaction that takes snapshots or starts
drive-backup on multiple dataplane disks, the atomicity could be broken:
There could be one or more bdrv_drain_all() calls in each transaction
opeartion, which will unconditinally calls one or more aio_poll on the
AioContext:

    qmp transaction
        backup A prepare
            ...
            bdrv_drain_all()
                aio_poll(A)
                aio_poll(B)
                aio_poll(C)
                ...
            ...
        backup B prepare
            ...
            bdrv_drain_all()
                aio_poll(A)
                aio_poll(B)
->              aio_poll(C)
            ...
        snapshot C prepare
            ...
            bdrv_drain_all()
                aio_poll(A)
                aio_poll(B)
                aio_poll(C)
            ...

If the aio_poll(C) in the middle receives a new virtio-blk request from
ioeventfd, a new request will be submitted to C, then snapshot C is
inconsistent.

To avoid that, use aio_set_io_event_notifier so the behavior is the same
as in main loop.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 3db139b..027c5c5 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -283,7 +283,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 
     /* Get this show started by hooking up our callbacks */
     aio_context_acquire(s->ctx);
-    aio_set_event_notifier(s->ctx, &s->host_notifier, handle_notify);
+    aio_set_io_event_notifier(s->ctx, &s->host_notifier, handle_notify);
     aio_context_release(s->ctx);
     return;
 
@@ -319,7 +319,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     aio_context_acquire(s->ctx);
 
     /* Stop notifications for new requests from guest */
-    aio_set_event_notifier(s->ctx, &s->host_notifier, NULL);
+    aio_set_io_event_notifier(s->ctx, &s->host_notifier, NULL);
 
     /* Drain and switch bs back to the QEMU main loop */
     blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context());
-- 
2.4.1

Re: [Qemu-devel] [RFC PATCH 3/4] virtio-blk: Use aio_set_io_event_notifier in dataplane

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1718 bytes --]

On 05/27/2015 01:19 AM, Fam Zheng wrote:
> Currently the host notifier is checked by all aio_poll, which is not
> safe. For example, in a qmp transaction that takes snapshots or starts
> drive-backup on multiple dataplane disks, the atomicity could be broken:
> There could be one or more bdrv_drain_all() calls in each transaction
> opeartion, which will unconditinally calls one or more aio_poll on the

s/opeartion/operation/
s/unconditinally calls/unconditionally call/

> AioContext:
> 
>     qmp transaction
>         backup A prepare
>             ...
>             bdrv_drain_all()
>                 aio_poll(A)
>                 aio_poll(B)
>                 aio_poll(C)
>                 ...
>             ...
>         backup B prepare
>             ...
>             bdrv_drain_all()
>                 aio_poll(A)
>                 aio_poll(B)
> ->              aio_poll(C)
>             ...
>         snapshot C prepare
>             ...
>             bdrv_drain_all()
>                 aio_poll(A)
>                 aio_poll(B)
>                 aio_poll(C)
>             ...
> 
> If the aio_poll(C) in the middle receives a new virtio-blk request from
> ioeventfd, a new request will be submitted to C, then snapshot C is
> inconsistent.
> 
> To avoid that, use aio_set_io_event_notifier so the behavior is the same
> as in main loop.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/block/dataplane/virtio-blk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

again, letting others comment on the technical merits

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [RFC PATCH 4/4] virtio-scsi-dataplane: User aio_set_io_event_notifier

[Fam Zheng]

For the same reason as previous commit.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/virtio-scsi-dataplane.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 5575648..e602da7 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -60,7 +60,7 @@ static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI *s,
     r = g_slice_new(VirtIOSCSIVring);
     r->host_notifier = *virtio_queue_get_host_notifier(vq);
     r->guest_notifier = *virtio_queue_get_guest_notifier(vq);
-    aio_set_event_notifier(s->ctx, &r->host_notifier, handler);
+    aio_set_io_event_notifier(s->ctx, &r->host_notifier, handler);
 
     r->parent = s;
 
@@ -71,7 +71,7 @@ static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI *s,
     return r;
 
 fail_vring:
-    aio_set_event_notifier(s->ctx, &r->host_notifier, NULL);
+    aio_set_io_event_notifier(s->ctx, &r->host_notifier, NULL);
     k->set_host_notifier(qbus->parent, n, false);
     g_slice_free(VirtIOSCSIVring, r);
     return NULL;
@@ -162,14 +162,15 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s)
     int i;
 
     if (s->ctrl_vring) {
-        aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier, NULL);
+        aio_set_io_event_notifier(s->ctx, &s->ctrl_vring->host_notifier, NULL);
     }
     if (s->event_vring) {
-        aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier, NULL);
+        aio_set_io_event_notifier(s->ctx, &s->event_vring->host_notifier, NULL);
     }
     if (s->cmd_vrings) {
         for (i = 0; i < vs->conf.num_queues && s->cmd_vrings[i]; i++) {
-            aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier, NULL);
+            aio_set_io_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier,
+                                      NULL);
         }
     }
 }
@@ -290,10 +291,11 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
 
     aio_context_acquire(s->ctx);
 
-    aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier, NULL);
-    aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier, NULL);
+    aio_set_io_event_notifier(s->ctx, &s->ctrl_vring->host_notifier, NULL);
+    aio_set_io_event_notifier(s->ctx, &s->event_vring->host_notifier, NULL);
     for (i = 0; i < vs->conf.num_queues; i++) {
-        aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier, NULL);
+        aio_set_io_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier,
+                                  NULL);
     }
 
     blk_drain_all(); /* ensure there are no in-flight requests */
-- 
2.4.1

Re: [Qemu-devel] [RFC PATCH 0/4] aio: Don't poll ioeventfd in nested aio_poll()

[Paolo Bonzini]

On 27/05/2015 09:19, Fam Zheng wrote:
> This series looks at the other side of the broken "qmp transaction" problem
> with dataplane [1] - the event loop.
> 
> Before, an ioeventfd of a non-dataplane device is registered via
> qemu_set_fd_handler, which is only polled directly by main_loop_wait(), not in
> aio_poll(); an ioeventfd of a dataplane device (virtio-blk/virtio-scsi) is
> registered specially via aio_set_event_notifier, which is a wrapper of
> aio_set_fd_handler, thus it WILL be polled by all aio_poll().
> 
> As explained in [1], and in patch 3, this is wrong. The handlers mustn't run.
> 
> [1] Fixes it by unregistering them temporarily around such nested poll, while
> this series fixes it by skipping those file descriptors in all nested
> aio_poll(), just like how iohandler behaves in the main loop.
> 
> On the one hand, it is simpler than [1]; on the other hand, this approach is
> also interesting because once we remove qemu_set_fd_handler2 [2], iohandler.c
> can be removed by converting all qemu_set_fd_handler to the new
> aio_set_io_event_notifier introduced in this series.

I don't think this can work.

Whenever the main context is doing synchronous work for dataplane, it is
in the outermost aio_poll.

Paolo

Re: [Qemu-devel] [RFC PATCH 0/4] aio: Don't poll ioeventfd in nested aio_poll()

[Fam Zheng]

On Wed, 05/27 11:38, Paolo Bonzini wrote:
> 
> 
> On 27/05/2015 09:19, Fam Zheng wrote:
> > This series looks at the other side of the broken "qmp transaction" problem
> > with dataplane [1] - the event loop.
> > 
> > Before, an ioeventfd of a non-dataplane device is registered via
> > qemu_set_fd_handler, which is only polled directly by main_loop_wait(), not in
> > aio_poll(); an ioeventfd of a dataplane device (virtio-blk/virtio-scsi) is
> > registered specially via aio_set_event_notifier, which is a wrapper of
> > aio_set_fd_handler, thus it WILL be polled by all aio_poll().
> > 
> > As explained in [1], and in patch 3, this is wrong. The handlers mustn't run.
> > 
> > [1] Fixes it by unregistering them temporarily around such nested poll, while
> > this series fixes it by skipping those file descriptors in all nested
> > aio_poll(), just like how iohandler behaves in the main loop.
> > 
> > On the one hand, it is simpler than [1]; on the other hand, this approach is
> > also interesting because once we remove qemu_set_fd_handler2 [2], iohandler.c
> > can be removed by converting all qemu_set_fd_handler to the new
> > aio_set_io_event_notifier introduced in this series.
> 
> I don't think this can work.
> 
> Whenever the main context is doing synchronous work for dataplane, it is
> in the outermost aio_poll.
> 

You're right. :(

The main context uses iohandler and aio_dispatch, neither calls
aio_set_dispatching(). However, if we have [2], they can be changed to
aio_poll(), then would this idea work?

Fam

Re: [Qemu-devel] [RFC PATCH 0/4] aio: Don't poll ioeventfd in nested aio_poll()

[Paolo Bonzini]

On 28/05/2015 03:46, Fam Zheng wrote:
> The main context uses iohandler and aio_dispatch, neither calls
> aio_set_dispatching(). However, if we have [2], they can be changed to
> aio_poll(), then would this idea work?

I think it's a bad idea to handle aio_poll for context B in a different
way, just because you have an outer aio_poll for context A...

Paolo

Re: [Qemu-devel] [RFC PATCH 0/4] aio: Don't poll ioeventfd in nested aio_poll()

[Fam Zheng]

On Thu, 05/28 10:21, Paolo Bonzini wrote:
> 
> 
> On 28/05/2015 03:46, Fam Zheng wrote:
> > The main context uses iohandler and aio_dispatch, neither calls
> > aio_set_dispatching(). However, if we have [2], they can be changed to
> > aio_poll(), then would this idea work?
> 
> I think it's a bad idea to handle aio_poll for context B in a different
> way, just because you have an outer aio_poll for context A...

But we already do something similar: ignoring slirp, main_loop_wait() is like
an iothread aio_poll() without the "outermost differentiation", while the
current aio_poll() in bdrv_drain() is roughly "main_loop_wait() minus
iohandlers".

Fam

Re: [Qemu-devel] [RFC PATCH 0/4] aio: Don't poll ioeventfd in nested aio_poll()

[Paolo Bonzini]

On 28/05/2015 13:16, Fam Zheng wrote:
> > On 28/05/2015 03:46, Fam Zheng wrote:
> > > The main context uses iohandler and aio_dispatch, neither calls
> > > aio_set_dispatching(). However, if we have [2], they can be changed to
> > > aio_poll(), then would this idea work?
> > 
> > I think it's a bad idea to handle aio_poll for context B in a different
> > way, just because you have an outer aio_poll for context A...
> 
> But we already do something similar: ignoring slirp, main_loop_wait() is like
> an iothread aio_poll() without the "outermost differentiation", while the
> current aio_poll() in bdrv_drain() is roughly "main_loop_wait() minus
> iohandlers".

Right, but the two sets of iohandlers are stored in different places, so
it's obvious that you don't execute all of them.  On the other hand,
examining global state in aio_poll is really bad.

Paolo

Re: [Qemu-devel] [RFC PATCH 0/4] aio: Don't poll ioeventfd in nested aio_poll()

[Fam Zheng]

On Thu, 05/28 13:19, Paolo Bonzini wrote:
> 
> 
> On 28/05/2015 13:16, Fam Zheng wrote:
> > > On 28/05/2015 03:46, Fam Zheng wrote:
> > > > The main context uses iohandler and aio_dispatch, neither calls
> > > > aio_set_dispatching(). However, if we have [2], they can be changed to
> > > > aio_poll(), then would this idea work?
> > > 
> > > I think it's a bad idea to handle aio_poll for context B in a different
> > > way, just because you have an outer aio_poll for context A...
> > 
> > But we already do something similar: ignoring slirp, main_loop_wait() is like
> > an iothread aio_poll() without the "outermost differentiation", while the
> > current aio_poll() in bdrv_drain() is roughly "main_loop_wait() minus
> > iohandlers".
> 
> Right, but the two sets of iohandlers are stored in different places, so
> it's obvious that you don't execute all of them.  On the other hand,
> examining global state in aio_poll is really bad.
> 

OK.

Would moving the ioeventfds to a new top level aio_loop_wait() be any better?
That way no global state is needed.

Fam

Re: [Qemu-devel] [RFC PATCH 0/4] aio: Don't poll ioeventfd in nested aio_poll()

[Paolo Bonzini]

On 28/05/2015 13:49, Fam Zheng wrote:
> On Thu, 05/28 13:19, Paolo Bonzini wrote:
>>
>>
>> On 28/05/2015 13:16, Fam Zheng wrote:
>>>> On 28/05/2015 03:46, Fam Zheng wrote:
>>>>> The main context uses iohandler and aio_dispatch, neither calls
>>>>> aio_set_dispatching(). However, if we have [2], they can be changed to
>>>>> aio_poll(), then would this idea work?
>>>>
>>>> I think it's a bad idea to handle aio_poll for context B in a different
>>>> way, just because you have an outer aio_poll for context A...
>>>
>>> But we already do something similar: ignoring slirp, main_loop_wait() is like
>>> an iothread aio_poll() without the "outermost differentiation", while the
>>> current aio_poll() in bdrv_drain() is roughly "main_loop_wait() minus
>>> iohandlers".
>>
>> Right, but the two sets of iohandlers are stored in different places, so
>> it's obvious that you don't execute all of them.  On the other hand,
>> examining global state in aio_poll is really bad.
>>
> 
> OK.
> 
> Would moving the ioeventfds to a new top level aio_loop_wait() be any better?
> That way no global state is needed.

If we need pause/resume anyway due to block/mirror.c's use of
block_job_defer_to_main_loop, I think this is not a problem anymore?

Paolo

Re: [Qemu-devel] [RFC PATCH 0/4] aio: Don't poll ioeventfd in nested aio_poll()

[Fam Zheng]

On Thu, 05/28 14:01, Paolo Bonzini wrote:
> 
> 
> On 28/05/2015 13:49, Fam Zheng wrote:
> > On Thu, 05/28 13:19, Paolo Bonzini wrote:
> >>
> >>
> >> On 28/05/2015 13:16, Fam Zheng wrote:
> >>>> On 28/05/2015 03:46, Fam Zheng wrote:
> >>>>> The main context uses iohandler and aio_dispatch, neither calls
> >>>>> aio_set_dispatching(). However, if we have [2], they can be changed to
> >>>>> aio_poll(), then would this idea work?
> >>>>
> >>>> I think it's a bad idea to handle aio_poll for context B in a different
> >>>> way, just because you have an outer aio_poll for context A...
> >>>
> >>> But we already do something similar: ignoring slirp, main_loop_wait() is like
> >>> an iothread aio_poll() without the "outermost differentiation", while the
> >>> current aio_poll() in bdrv_drain() is roughly "main_loop_wait() minus
> >>> iohandlers".
> >>
> >> Right, but the two sets of iohandlers are stored in different places, so
> >> it's obvious that you don't execute all of them.  On the other hand,
> >> examining global state in aio_poll is really bad.
> >>
> > 
> > OK.
> > 
> > Would moving the ioeventfds to a new top level aio_loop_wait() be any better?
> > That way no global state is needed.
> 
> If we need pause/resume anyway due to block/mirror.c's use of
> block_job_defer_to_main_loop, I think this is not a problem anymore?
> 

I also hope to dedup the iohandler code with async.c, on top of [2]; and in the
longer term, convert slirp to use AioContext API too, so that all
*_pollfds_fill() will not be necessary - the whole event loop goes epoll style.

Fam

Re: [Qemu-devel] [RFC PATCH 0/4] aio: Don't poll ioeventfd in nested aio_poll()

[Paolo Bonzini]

On 28/05/2015 14:26, Fam Zheng wrote:
> > > > Right, but the two sets of iohandlers are stored in different places, so
> > > > it's obvious that you don't execute all of them.  On the other hand,
> > > > examining global state in aio_poll is really bad.
> > > 
> > > OK.
> > > 
> > > Would moving the ioeventfds to a new top level aio_loop_wait() be any better?
> > > That way no global state is needed.
> > 
> > If we need pause/resume anyway due to block/mirror.c's use of
> > block_job_defer_to_main_loop, I think this is not a problem anymore?
> 
> I also hope to dedup the iohandler code with async.c, on top of [2]; and in the
> longer term, convert slirp to use AioContext API too, so that all
> *_pollfds_fill() will not be necessary - the whole event loop goes epoll style.

You can use separate AioContexts for that: one for block devices, one
for (former) iohandlers.  The two AioContexts then can live together in
the main loop because the main loop just treats AioContexts as GSources.

However, this does not help with ioeventfds, for which we need pause/resume.

Paolo