[Qemu-devel] [PATCH v4 0/3] aio: Use epoll in aio_poll()

[Qemu-devel] [PATCH v4 0/3] aio: Use epoll in aio_poll()

[Fam Zheng]

v4: Rebase onto master (with aio_disable_external):
    Don't use epoll if aio_external_disabled(ctx);
    Change assert on epoll_ctl return code to disable epoll;
    Rerun benchmark;

v3: Remove the redundant check in aio_epoll_try_enable. [Stefan]

v2: Merge aio-epoll.c into aio-posix.c. [Paolo]
    Capture some benchmark data in commit log.

This series adds the ability to use epoll in aio_poll() on Linux. It's switched
on in a dynamic way rather than static for two reasons: 1) when the number of
fds is not high enough, using epoll has little advantage; 2) when an epoll
incompatible fd needs to be handled, we need to fall back.  The epoll is
enabled when a fd number threshold is met.


Fam Zheng (3):
  aio: Introduce aio_external_disabled
  aio: Introduce aio_context_setup
  aio: Introduce aio-epoll.c

 aio-posix.c         | 188 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 aio-win32.c         |   4 ++
 async.c             |  13 +++-
 include/block/aio.h |  24 +++++++
 4 files changed, 226 insertions(+), 3 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v4 1/3] aio: Introduce aio_external_disabled

[Fam Zheng]

This allows AioContext users to check the enable/disable state of
external clients.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/block/aio.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/block/aio.h b/include/block/aio.h
index bcc7d43..dcf74be 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -401,6 +401,17 @@ static inline void aio_enable_external(AioContext *ctx)
 }
 
 /**
+ * aio_external_disabled:
+ * @ctx: the aio context
+ *
+ * Return true if the external clients are disabled.
+ */
+static inline bool aio_external_disabled(AioContext *ctx)
+{
+    return atomic_read(&ctx->external_disable_cnt);
+}
+
+/**
  * aio_node_check:
  * @ctx: the aio context
  * @is_external: Whether or not the checked node is an external event source.
-- 
2.4.3

[Qemu-devel] [PATCH v4 2/3] aio: Introduce aio_context_setup

[Fam Zheng]

This is the place to initialize platform specific bits of AioContext.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 aio-posix.c         |  4 ++++
 aio-win32.c         |  4 ++++
 async.c             | 13 +++++++++++--
 include/block/aio.h |  8 ++++++++
 4 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/aio-posix.c b/aio-posix.c
index 0467f23..5bff3cd 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -302,3 +302,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
 
     return progress;
 }
+
+void aio_context_setup(AioContext *ctx, Error **errp)
+{
+}
diff --git a/aio-win32.c b/aio-win32.c
index 43c4c79..cdc4456 100644
--- a/aio-win32.c
+++ b/aio-win32.c
@@ -369,3 +369,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
     aio_context_release(ctx);
     return progress;
 }
+
+void aio_context_setup(AioContext *ctx, Error **errp)
+{
+}
diff --git a/async.c b/async.c
index bdc64a3..51ff2eb 100644
--- a/async.c
+++ b/async.c
@@ -320,12 +320,18 @@ AioContext *aio_context_new(Error **errp)
 {
     int ret;
     AioContext *ctx;
+    Error *local_err = NULL;
+
     ctx = (AioContext *) g_source_new(&aio_source_funcs, sizeof(AioContext));
+    aio_context_setup(ctx, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto fail;
+    }
     ret = event_notifier_init(&ctx->notifier, false);
     if (ret < 0) {
-        g_source_destroy(&ctx->source);
         error_setg_errno(errp, -ret, "Failed to initialize event notifier");
-        return NULL;
+        goto fail;
     }
     g_source_set_can_recurse(&ctx->source, true);
     aio_set_event_notifier(ctx, &ctx->notifier,
@@ -340,6 +346,9 @@ AioContext *aio_context_new(Error **errp)
     ctx->notify_dummy_bh = aio_bh_new(ctx, notify_dummy_bh, NULL);
 
     return ctx;
+fail:
+    g_source_destroy(&ctx->source);
+    return NULL;
 }
 
 void aio_context_ref(AioContext *ctx)
diff --git a/include/block/aio.h b/include/block/aio.h
index dcf74be..2b1ff4f 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -424,4 +424,12 @@ static inline bool aio_node_check(AioContext *ctx, bool is_external)
     return !is_external || !atomic_read(&ctx->external_disable_cnt);
 }
 
+/**
+ * aio_context_setup:
+ * @ctx: the aio context
+ *
+ * Initialize the aio context.
+ */
+void aio_context_setup(AioContext *ctx, Error **errp);
+
 #endif
-- 
2.4.3

[Qemu-devel] [PATCH v4 3/3] aio: Introduce aio-epoll.c

[Fam Zheng]

To minimize code duplication, epoll is hooked into aio-posix's
aio_poll() instead of rolling its own. This approach also has both
compile-time and run-time switchability.

1) When QEMU starts with a small number of fds in the event loop, ppoll
is used.

2) When QEMU starts with a big number of fds, or when more devices are
hot plugged, epoll kicks in when the number of fds hits the threshold.

3) Some fds may not support epoll, such as tty based stdio. In this
case, it falls back to ppoll.

A rough benchmark with scsi-disk on virtio-scsi dataplane (epoll gets
enabled from 64 onward). Numbers are in MB/s.

===============================================
             |     master     |     epoll
             |                |
scsi disks # | read    randrw | read    randrw
-------------|----------------|----------------
1            | 86      36     | 92      45
8            | 87      43     | 86      41
64           | 71      32     | 70      38
128          | 48      24     | 58      31
256          | 37      19     | 57      28
===============================================

To comply with aio_{disable,enable}_external, we always use ppoll when
aio_external_disabled() is true.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 aio-posix.c         | 184 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 include/block/aio.h |   5 ++
 2 files changed, 188 insertions(+), 1 deletion(-)

diff --git a/aio-posix.c b/aio-posix.c
index 5bff3cd..06148a9 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -17,6 +17,9 @@
 #include "block/block.h"
 #include "qemu/queue.h"
 #include "qemu/sockets.h"
+#ifdef CONFIG_EPOLL
+#include <sys/epoll.h>
+#endif
 
 struct AioHandler
 {
@@ -29,6 +32,162 @@ struct AioHandler
     QLIST_ENTRY(AioHandler) node;
 };
 
+#ifdef CONFIG_EPOLL
+
+/* The fd number threashold to switch to epoll */
+#define EPOLL_ENABLE_THRESHOLD 64
+
+static void aio_epoll_disable(AioContext *ctx)
+{
+    ctx->epoll_available = false;
+    if (!ctx->epoll_enabled) {
+        return;
+    }
+    ctx->epoll_enabled = false;
+    close(ctx->epollfd);
+}
+
+static inline int epoll_events_from_pfd(int pfd_events)
+{
+    return (pfd_events & G_IO_IN ? EPOLLIN : 0) |
+           (pfd_events & G_IO_OUT ? EPOLLOUT : 0) |
+           (pfd_events & G_IO_HUP ? EPOLLHUP : 0) |
+           (pfd_events & G_IO_ERR ? EPOLLERR : 0);
+}
+
+static bool aio_epoll_try_enable(AioContext *ctx)
+{
+    AioHandler *node;
+    struct epoll_event event;
+
+    QLIST_FOREACH(node, &ctx->aio_handlers, node) {
+        int r;
+        if (node->deleted || !node->pfd.events) {
+            continue;
+        }
+        event.events = epoll_events_from_pfd(node->pfd.events);
+        event.data.ptr = node;
+        r = epoll_ctl(ctx->epollfd, EPOLL_CTL_ADD, node->pfd.fd, &event);
+        if (r) {
+            return false;
+        }
+    }
+    ctx->epoll_enabled = true;
+    return true;
+}
+
+static void aio_epoll_update(AioContext *ctx, AioHandler *node, bool is_new)
+{
+    struct epoll_event event;
+    int r;
+
+    if (!ctx->epoll_enabled) {
+        return;
+    }
+    if (!node->pfd.events) {
+        r = epoll_ctl(ctx->epollfd, EPOLL_CTL_DEL, node->pfd.fd, &event);
+        if (r) {
+            aio_epoll_disable(ctx);
+        }
+    } else {
+        event.data.ptr = node;
+        event.events = epoll_events_from_pfd(node->pfd.events);
+        if (is_new) {
+            r = epoll_ctl(ctx->epollfd, EPOLL_CTL_ADD, node->pfd.fd, &event);
+            if (r) {
+                aio_epoll_disable(ctx);
+            }
+        } else {
+            r = epoll_ctl(ctx->epollfd, EPOLL_CTL_MOD, node->pfd.fd, &event);
+            if (r) {
+                aio_epoll_disable(ctx);
+            }
+        }
+    }
+}
+
+static int aio_epoll(AioContext *ctx, GPollFD *pfds,
+                     unsigned npfd, int64_t timeout)
+{
+    AioHandler *node;
+    int i, ret = 0;
+    struct epoll_event events[128];
+
+    assert(npfd == 1);
+    assert(pfds[0].fd == ctx->epollfd);
+    if (timeout > 0) {
+        ret = qemu_poll_ns(pfds, npfd, timeout);
+    }
+    if (timeout <= 0 || ret > 0) {
+        ret = epoll_wait(ctx->epollfd, events,
+                         sizeof(events) / sizeof(events[0]),
+                         timeout);
+        if (ret <= 0) {
+            goto out;
+        }
+        for (i = 0; i < ret; i++) {
+            int ev = events[i].events;
+            node = events[i].data.ptr;
+            node->pfd.revents = (ev & EPOLLIN ? G_IO_IN : 0) |
+                (ev & EPOLLOUT ? G_IO_OUT : 0) |
+                (ev & EPOLLHUP ? G_IO_HUP : 0) |
+                (ev & EPOLLERR ? G_IO_ERR : 0);
+        }
+    }
+out:
+    return ret;
+}
+
+static bool aio_epoll_enabled(AioContext *ctx)
+{
+    /* Fall back to ppoll when external clients are disabled. */
+    return !aio_external_disabled(ctx) && ctx->epoll_enabled;
+}
+
+static bool aio_epoll_check_poll(AioContext *ctx, GPollFD *pfds,
+                                 unsigned npfd, int64_t timeout)
+{
+    if (!ctx->epoll_available) {
+        return false;
+    }
+    if (aio_epoll_enabled(ctx)) {
+        return true;
+    }
+    if (npfd >= EPOLL_ENABLE_THRESHOLD) {
+        if (aio_epoll_try_enable(ctx)) {
+            return true;
+        } else {
+            aio_epoll_disable(ctx);
+        }
+    }
+    return false;
+}
+
+#else
+
+static void aio_epoll_update(AioContext *ctx, AioHandler *node, bool is_new)
+{
+}
+
+static int aio_epoll(AioContext *ctx, GPollFD *pfds,
+                     unsigned npfd, int64_t timeout)
+{
+    assert(false);
+}
+
+static bool aio_epoll_enabled(AioContext *ctx)
+{
+    return false;
+}
+
+static bool aio_epoll_check_poll(AioContext *ctx, GPollFD *pfds,
+                          unsigned npfd, int64_t timeout)
+{
+    return false;
+}
+
+#endif
+
 static AioHandler *find_aio_handler(AioContext *ctx, int fd)
 {
     AioHandler *node;
@@ -50,6 +209,7 @@ void aio_set_fd_handler(AioContext *ctx,
                         void *opaque)
 {
     AioHandler *node;
+    bool is_new = false;
 
     node = find_aio_handler(ctx, fd);
 
@@ -79,6 +239,7 @@ void aio_set_fd_handler(AioContext *ctx,
             QLIST_INSERT_HEAD(&ctx->aio_handlers, node, node);
 
             g_source_add_poll(&ctx->source, &node->pfd);
+            is_new = true;
         }
         /* Update handler with latest information */
         node->io_read = io_read;
@@ -90,6 +251,7 @@ void aio_set_fd_handler(AioContext *ctx,
         node->pfd.events |= (io_write ? G_IO_OUT | G_IO_ERR : 0);
     }
 
+    aio_epoll_update(ctx, node, is_new);
     aio_notify(ctx);
 }
 
@@ -262,6 +424,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
     /* fill pollfds */
     QLIST_FOREACH(node, &ctx->aio_handlers, node) {
         if (!node->deleted && node->pfd.events
+            && !aio_epoll_enabled(ctx)
             && aio_node_check(ctx, node->is_external)) {
             add_pollfd(node);
         }
@@ -273,7 +436,17 @@ bool aio_poll(AioContext *ctx, bool blocking)
     if (timeout) {
         aio_context_release(ctx);
     }
-    ret = qemu_poll_ns((GPollFD *)pollfds, npfd, timeout);
+    if (aio_epoll_check_poll(ctx, pollfds, npfd, timeout)) {
+        AioHandler epoll_handler;
+
+        epoll_handler.pfd.fd = ctx->epollfd;
+        epoll_handler.pfd.events = G_IO_IN | G_IO_OUT | G_IO_HUP | G_IO_ERR;
+        npfd = 0;
+        add_pollfd(&epoll_handler);
+        ret = aio_epoll(ctx, pollfds, npfd, timeout);
+    } else  {
+        ret = qemu_poll_ns(pollfds, npfd, timeout);
+    }
     if (blocking) {
         atomic_sub(&ctx->notify_me, 2);
     }
@@ -305,4 +478,13 @@ bool aio_poll(AioContext *ctx, bool blocking)
 
 void aio_context_setup(AioContext *ctx, Error **errp)
 {
+#ifdef CONFIG_EPOLL
+    assert(!ctx->epollfd);
+    ctx->epollfd = epoll_create1(EPOLL_CLOEXEC);
+    if (ctx->epollfd == -1) {
+        ctx->epoll_available = false;
+    } else {
+        ctx->epoll_available = true;
+    }
+#endif
 }
diff --git a/include/block/aio.h b/include/block/aio.h
index 2b1ff4f..91737d5 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -124,6 +124,11 @@ struct AioContext {
     QEMUTimerListGroup tlg;
 
     int external_disable_cnt;
+#ifdef CONFIG_EPOLL
+    int epollfd;
+    bool epoll_enabled;
+    bool epoll_available;
+#endif
 };
 
 /**
-- 
2.4.3

Re: [Qemu-devel] [PATCH v4 3/3] aio: Introduce aio-epoll.c

[Stefan Hajnoczi]

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

On Fri, Oct 30, 2015 at 12:06:29PM +0800, Fam Zheng wrote:
> To comply with aio_{disable,enable}_external, we always use ppoll when
> aio_external_disabled() is true.

All file descriptors are added to the epoll fd.  Does that mean epoll
will report the same fds again after we come out of
ppoll()/aio_external_disabled()?

The two constraints to think about:
1. Ideally there should be no duplicated events.
2. There absolutely cannot be any missed events.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v4 3/3] aio: Introduce aio-epoll.c

[Fam Zheng]

On Fri, 10/30 10:07, Stefan Hajnoczi wrote:
> On Fri, Oct 30, 2015 at 12:06:29PM +0800, Fam Zheng wrote:
> > To comply with aio_{disable,enable}_external, we always use ppoll when
> > aio_external_disabled() is true.
> 
> All file descriptors are added to the epoll fd.  Does that mean epoll
> will report the same fds again after we come out of
> ppoll()/aio_external_disabled()?
> 
> The two constraints to think about:
> 1. Ideally there should be no duplicated events.
> 2. There absolutely cannot be any missed events.
> 

I'm not sure I understood your question. The file descriptors added to epollfd
are always in sync with ppoll, so there is no difference between calling
epoll_wait and ppoll. When we come out of aio_external_disabled(), the same set
of fds will be polled, but the events got by ppoll should already be handled.

What am I missing?

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v4 3/3] aio: Introduce aio-epoll.c

[Stefan Hajnoczi]

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

On Mon, Nov 02, 2015 at 10:32:54AM +0800, Fam Zheng wrote:
> On Fri, 10/30 10:07, Stefan Hajnoczi wrote:
> > On Fri, Oct 30, 2015 at 12:06:29PM +0800, Fam Zheng wrote:
> > > To comply with aio_{disable,enable}_external, we always use ppoll when
> > > aio_external_disabled() is true.
> > 
> > All file descriptors are added to the epoll fd.  Does that mean epoll
> > will report the same fds again after we come out of
> > ppoll()/aio_external_disabled()?
> > 
> > The two constraints to think about:
> > 1. Ideally there should be no duplicated events.
> > 2. There absolutely cannot be any missed events.
> > 
> 
> I'm not sure I understood your question. The file descriptors added to epollfd
> are always in sync with ppoll, so there is no difference between calling
> epoll_wait and ppoll. When we come out of aio_external_disabled(), the same set
> of fds will be polled, but the events got by ppoll should already be handled.
> 
> What am I missing?

I'm asking about duplicated events.  The epoll fd monitors the same set
of fds as ppoll().  When we come out of aio_external_disabled() will
epoll fd see the old events that have already been handled by ppoll()?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v4 3/3] aio: Introduce aio-epoll.c

[Fam Zheng]

On Mon, 11/02 13:19, Stefan Hajnoczi wrote:
> On Mon, Nov 02, 2015 at 10:32:54AM +0800, Fam Zheng wrote:
> > On Fri, 10/30 10:07, Stefan Hajnoczi wrote:
> > > On Fri, Oct 30, 2015 at 12:06:29PM +0800, Fam Zheng wrote:
> > > > To comply with aio_{disable,enable}_external, we always use ppoll when
> > > > aio_external_disabled() is true.
> > > 
> > > All file descriptors are added to the epoll fd.  Does that mean epoll
> > > will report the same fds again after we come out of
> > > ppoll()/aio_external_disabled()?
> > > 
> > > The two constraints to think about:
> > > 1. Ideally there should be no duplicated events.
> > > 2. There absolutely cannot be any missed events.
> > > 
> > 
> > I'm not sure I understood your question. The file descriptors added to epollfd
> > are always in sync with ppoll, so there is no difference between calling
> > epoll_wait and ppoll. When we come out of aio_external_disabled(), the same set
> > of fds will be polled, but the events got by ppoll should already be handled.
> > 
> > What am I missing?
> 
> I'm asking about duplicated events.  The epoll fd monitors the same set
> of fds as ppoll().  When we come out of aio_external_disabled() will
> epoll fd see the old events that have already been handled by ppoll()?

No, we don't get duplicated events. epoll fd will only see unhandled (new)
events.

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v4 3/3] aio: Introduce aio-epoll.c

[Stefan Hajnoczi]

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

On Mon, Nov 02, 2015 at 09:33:19PM +0800, Fam Zheng wrote:
> On Mon, 11/02 13:19, Stefan Hajnoczi wrote:
> > On Mon, Nov 02, 2015 at 10:32:54AM +0800, Fam Zheng wrote:
> > > On Fri, 10/30 10:07, Stefan Hajnoczi wrote:
> > > > On Fri, Oct 30, 2015 at 12:06:29PM +0800, Fam Zheng wrote:
> > > > > To comply with aio_{disable,enable}_external, we always use ppoll when
> > > > > aio_external_disabled() is true.
> > > > 
> > > > All file descriptors are added to the epoll fd.  Does that mean epoll
> > > > will report the same fds again after we come out of
> > > > ppoll()/aio_external_disabled()?
> > > > 
> > > > The two constraints to think about:
> > > > 1. Ideally there should be no duplicated events.
> > > > 2. There absolutely cannot be any missed events.
> > > > 
> > > 
> > > I'm not sure I understood your question. The file descriptors added to epollfd
> > > are always in sync with ppoll, so there is no difference between calling
> > > epoll_wait and ppoll. When we come out of aio_external_disabled(), the same set
> > > of fds will be polled, but the events got by ppoll should already be handled.
> > > 
> > > What am I missing?
> > 
> > I'm asking about duplicated events.  The epoll fd monitors the same set
> > of fds as ppoll().  When we come out of aio_external_disabled() will
> > epoll fd see the old events that have already been handled by ppoll()?
> 
> No, we don't get duplicated events. epoll fd will only see unhandled (new)
> events.

Excellent.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v4 0/3] aio: Use epoll in aio_poll()

[Stefan Hajnoczi]

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

On Fri, Oct 30, 2015 at 12:06:26PM +0800, Fam Zheng wrote:
> v4: Rebase onto master (with aio_disable_external):
>     Don't use epoll if aio_external_disabled(ctx);
>     Change assert on epoll_ctl return code to disable epoll;
>     Rerun benchmark;
> 
> v3: Remove the redundant check in aio_epoll_try_enable. [Stefan]
> 
> v2: Merge aio-epoll.c into aio-posix.c. [Paolo]
>     Capture some benchmark data in commit log.
> 
> This series adds the ability to use epoll in aio_poll() on Linux. It's switched
> on in a dynamic way rather than static for two reasons: 1) when the number of
> fds is not high enough, using epoll has little advantage; 2) when an epoll
> incompatible fd needs to be handled, we need to fall back.  The epoll is
> enabled when a fd number threshold is met.
> 
> 
> Fam Zheng (3):
>   aio: Introduce aio_external_disabled
>   aio: Introduce aio_context_setup
>   aio: Introduce aio-epoll.c
> 
>  aio-posix.c         | 188 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  aio-win32.c         |   4 ++
>  async.c             |  13 +++-
>  include/block/aio.h |  24 +++++++
>  4 files changed, 226 insertions(+), 3 deletions(-)
> 
> -- 
> 2.4.3
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]