[PATCH v2 0/2] fdmon-poll: reset npfd when upgrading to fdmon-epoll

[PATCH v2 0/2] fdmon-poll: reset npfd when upgrading to fdmon-epoll

[Stefan Hajnoczi]

v2:
 * Rebased onto qemu.git/master for meson

Fix an assertion failure when aio_poll() is called in a aio_disable_external()
region. The failure happens when fdmon-poll is first upgraded to fdmon-epoll
and then downgraded again due to aio_disable_external().

Stefan Hajnoczi (2):
  fdmon-poll: reset npfd when upgrading to fdmon-epoll
  tests: add test-fdmon-epoll

 MAINTAINERS              |  1 +
 tests/test-fdmon-epoll.c | 73 ++++++++++++++++++++++++++++++++++++++++
 util/fdmon-poll.c        |  1 +
 tests/meson.build        |  3 ++
 4 files changed, 78 insertions(+)
 create mode 100644 tests/test-fdmon-epoll.c

-- 
2.26.2

[PATCH v2 1/2] fdmon-poll: reset npfd when upgrading to fdmon-epoll

[Stefan Hajnoczi]

npfd keeps track of how many pollfds are currently being monitored. It
must be reset to 0 when fdmon_poll_wait() returns.

When npfd reaches a treshold we switch to fdmon-epoll because it scales
better.

This patch resets npfd in the case where we switch to fdmon-epoll.
Forgetting to do so results in the following assertion failure:

  util/fdmon-poll.c:65: fdmon_poll_wait: Assertion `npfd == 0' failed.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1869952
Fixes: 1f050a4690f62a1e7dabc4f44141e9f762c3769f ("aio-posix: extract ppoll(2) and epoll(7) fd monitoring")
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/fdmon-poll.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/util/fdmon-poll.c b/util/fdmon-poll.c
index 488067b679..5fe3b47865 100644
--- a/util/fdmon-poll.c
+++ b/util/fdmon-poll.c
@@ -73,6 +73,7 @@ static int fdmon_poll_wait(AioContext *ctx, AioHandlerList *ready_list,
 
     /* epoll(7) is faster above a certain number of fds */
     if (fdmon_epoll_try_upgrade(ctx, npfd)) {
+        npfd = 0; /* we won't need pollfds[], reset npfd */
         return ctx->fdmon_ops->wait(ctx, ready_list, timeout);
     }
 
-- 
2.26.2

[PATCH v2 2/2] tests: add test-fdmon-epoll

[Stefan Hajnoczi]

Test aio_disable_external(), which switches from fdmon-epoll back to
fdmon-poll. This resulted in an assertion failure that was fixed in the
previous patch.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 MAINTAINERS              |  1 +
 tests/test-fdmon-epoll.c | 73 ++++++++++++++++++++++++++++++++++++++++
 tests/meson.build        |  3 ++
 3 files changed, 77 insertions(+)
 create mode 100644 tests/test-fdmon-epoll.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3d17cad19a..99e4010623 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2132,6 +2132,7 @@ F: migration/block*
 F: include/block/aio.h
 F: include/block/aio-wait.h
 F: scripts/qemugdb/aio.py
+F: tests/test-fdmon-epoll.c
 T: git https://github.com/stefanha/qemu.git block
 
 Block SCSI subsystem
diff --git a/tests/test-fdmon-epoll.c b/tests/test-fdmon-epoll.c
new file mode 100644
index 0000000000..11fd8a2fa9
--- /dev/null
+++ b/tests/test-fdmon-epoll.c
@@ -0,0 +1,73 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * fdmon-epoll tests
+ *
+ * Copyright (c) 2020 Red Hat, Inc.
+ */
+
+#include "qemu/osdep.h"
+#include "block/aio.h"
+#include "qapi/error.h"
+#include "qemu/main-loop.h"
+
+static AioContext *ctx;
+
+static void dummy_fd_handler(EventNotifier *notifier)
+{
+    event_notifier_test_and_clear(notifier);
+}
+
+static void add_event_notifiers(EventNotifier *notifiers, size_t n)
+{
+    for (size_t i = 0; i < n; i++) {
+        event_notifier_init(&notifiers[i], false);
+        aio_set_event_notifier(ctx, &notifiers[i], false,
+                               dummy_fd_handler, NULL);
+    }
+}
+
+static void remove_event_notifiers(EventNotifier *notifiers, size_t n)
+{
+    for (size_t i = 0; i < n; i++) {
+        aio_set_event_notifier(ctx, &notifiers[i], false, NULL, NULL);
+        event_notifier_cleanup(&notifiers[i]);
+    }
+}
+
+/* Check that fd handlers work when external clients are disabled */
+static void test_external_disabled(void)
+{
+    EventNotifier notifiers[100];
+
+    /* fdmon-epoll is only enabled when many fd handlers are registered */
+    add_event_notifiers(notifiers, G_N_ELEMENTS(notifiers));
+
+    event_notifier_set(&notifiers[0]);
+    assert(aio_poll(ctx, true));
+
+    aio_disable_external(ctx);
+    event_notifier_set(&notifiers[0]);
+    assert(aio_poll(ctx, true));
+    aio_enable_external(ctx);
+
+    remove_event_notifiers(notifiers, G_N_ELEMENTS(notifiers));
+}
+
+int main(int argc, char **argv)
+{
+    /*
+     * This code relies on the fact that fdmon-io_uring disables itself when
+     * the glib main loop is in use. The main loop uses fdmon-poll and upgrades
+     * to fdmon-epoll when the number of fds exceeds a threshold.
+     */
+    qemu_init_main_loop(&error_fatal);
+    ctx = qemu_get_aio_context();
+
+    while (g_main_context_iteration(NULL, false)) {
+        /* Do nothing */
+    }
+
+    g_test_init(&argc, &argv, NULL);
+    g_test_add_func("/fdmon-epoll/external-disabled", test_external_disabled);
+    return g_test_run();
+}
diff --git a/tests/meson.build b/tests/meson.build
index dae8a77df1..62277e6858 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -169,6 +169,9 @@ if have_block
   if 'CONFIG_NETTLE' in config_host or 'CONFIG_GCRYPT' in config_host
     tests += {'test-crypto-pbkdf': [io]}
   endif
+  if 'CONFIG_EPOLL_CREATE1' in config_host
+    tests += {'test-fdmon-epoll': [testblock]}
+  endif
   benchs += {
      'benchmark-crypto-hash': [crypto],
      'benchmark-crypto-hmac': [crypto],
-- 
2.26.2

Re: [PATCH v2 0/2] fdmon-poll: reset npfd when upgrading to fdmon-epoll

[Stefan Hajnoczi]

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

On Tue, Sep 15, 2020 at 01:03:37PM +0100, Stefan Hajnoczi wrote:
> v2:
>  * Rebased onto qemu.git/master for meson
> 
> Fix an assertion failure when aio_poll() is called in a aio_disable_external()
> region. The failure happens when fdmon-poll is first upgraded to fdmon-epoll
> and then downgraded again due to aio_disable_external().
> 
> Stefan Hajnoczi (2):
>   fdmon-poll: reset npfd when upgrading to fdmon-epoll
>   tests: add test-fdmon-epoll
> 
>  MAINTAINERS              |  1 +
>  tests/test-fdmon-epoll.c | 73 ++++++++++++++++++++++++++++++++++++++++
>  util/fdmon-poll.c        |  1 +
>  tests/meson.build        |  3 ++
>  4 files changed, 78 insertions(+)
>  create mode 100644 tests/test-fdmon-epoll.c
> 
> -- 
> 2.26.2
> 

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

Stefan

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