* [PATCH 0/2] aio-posix: fix fdmon-io_uring memory leak
@ 2020-05-11 18:36 Stefan Hajnoczi
2020-05-11 18:36 ` [PATCH 1/2] aio-posix: don't duplicate fd handler deletion in fdmon_io_uring_destroy() Stefan Hajnoczi
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2020-05-11 18:36 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Kevin Wolf, qemu-block, Stefan Weil, qemu-stable,
Max Reitz, Stefan Hajnoczi, Paolo Bonzini
This bug was introduced in QEMU 5.0 and causes guests to slow down because
AioHandlers are not freed when the fdmon-io_uring file descriptor monitoring
implementation is used by the main loop thread's glib event loop. This issue
does not apply to IOThread usage of fdmon-io_uring.
In practice few distros build with io_uring support enabled at the moment, so
the number of affected users is likely to be small. The fix is still suitable
for a stable release though.
https://bugs.launchpad.net/qemu/+bug/1877716
https://bugs.launchpad.net/qemu/+bug/1873032
Stefan Hajnoczi (2):
aio-posix: don't duplicate fd handler deletion in
fdmon_io_uring_destroy()
aio-posix: disable fdmon-io_uring when GSource is used
include/block/aio.h | 3 +++
util/aio-posix.c | 13 +++++++++++++
util/aio-win32.c | 4 ++++
util/async.c | 1 +
util/fdmon-io_uring.c | 13 ++++++++++---
5 files changed, 31 insertions(+), 3 deletions(-)
--
2.25.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] aio-posix: don't duplicate fd handler deletion in fdmon_io_uring_destroy()
2020-05-11 18:36 [PATCH 0/2] aio-posix: fix fdmon-io_uring memory leak Stefan Hajnoczi
@ 2020-05-11 18:36 ` Stefan Hajnoczi
2020-05-14 7:48 ` Oleksandr Natalenko
2020-05-11 18:36 ` [PATCH 2/2] aio-posix: disable fdmon-io_uring when GSource is used Stefan Hajnoczi
2020-05-21 13:49 ` [PATCH 0/2] aio-posix: fix fdmon-io_uring memory leak Stefan Hajnoczi
2 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2020-05-11 18:36 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Kevin Wolf, qemu-block, Stefan Weil, qemu-stable,
Max Reitz, Stefan Hajnoczi, Paolo Bonzini
The io_uring file descriptor monitoring implementation has an internal
list of fd handlers that are pending submission to io_uring.
fdmon_io_uring_destroy() deletes all fd handlers on the list.
Don't delete fd handlers directly in fdmon_io_uring_destroy() for two
reasons:
1. This duplicates the aio-posix.c AioHandler deletion code and could
become outdated if the struct changes.
2. Only handlers with the FDMON_IO_URING_REMOVE flag set are safe to
remove. If the flag is not set then something still has a pointer to
the fd handler. Let aio-posix.c and its user worry about that. In
practice this isn't an issue because fdmon_io_uring_destroy() is only
called when shutting down so all users have removed their fd
handlers, but the next patch will need this!
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
util/aio-posix.c | 1 +
util/fdmon-io_uring.c | 13 ++++++++++---
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/util/aio-posix.c b/util/aio-posix.c
index c3613d299e..8af334ab19 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -679,6 +679,7 @@ void aio_context_destroy(AioContext *ctx)
{
fdmon_io_uring_destroy(ctx);
fdmon_epoll_disable(ctx);
+ aio_free_deleted_handlers(ctx);
}
void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
diff --git a/util/fdmon-io_uring.c b/util/fdmon-io_uring.c
index d5a80ed6fb..1d14177df0 100644
--- a/util/fdmon-io_uring.c
+++ b/util/fdmon-io_uring.c
@@ -342,11 +342,18 @@ void fdmon_io_uring_destroy(AioContext *ctx)
io_uring_queue_exit(&ctx->fdmon_io_uring);
- /* No need to submit these anymore, just free them. */
+ /* Move handlers due to be removed onto the deleted list */
while ((node = QSLIST_FIRST_RCU(&ctx->submit_list))) {
+ unsigned flags = atomic_fetch_and(&node->flags,
+ ~(FDMON_IO_URING_PENDING |
+ FDMON_IO_URING_ADD |
+ FDMON_IO_URING_REMOVE));
+
+ if (flags & FDMON_IO_URING_REMOVE) {
+ QLIST_INSERT_HEAD_RCU(&ctx->deleted_aio_handlers, node, node_deleted);
+ }
+
QSLIST_REMOVE_HEAD_RCU(&ctx->submit_list, node_submitted);
- QLIST_REMOVE(node, node);
- g_free(node);
}
ctx->fdmon_ops = &fdmon_poll_ops;
--
2.25.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] aio-posix: disable fdmon-io_uring when GSource is used
2020-05-11 18:36 [PATCH 0/2] aio-posix: fix fdmon-io_uring memory leak Stefan Hajnoczi
2020-05-11 18:36 ` [PATCH 1/2] aio-posix: don't duplicate fd handler deletion in fdmon_io_uring_destroy() Stefan Hajnoczi
@ 2020-05-11 18:36 ` Stefan Hajnoczi
2020-05-14 7:49 ` Oleksandr Natalenko
2020-05-21 13:49 ` [PATCH 0/2] aio-posix: fix fdmon-io_uring memory leak Stefan Hajnoczi
2 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2020-05-11 18:36 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Kevin Wolf, qemu-block, Stefan Weil, qemu-stable,
Max Reitz, Stefan Hajnoczi, Paolo Bonzini
The glib event loop does not call fdmon_io_uring_wait() so fd handlers
waiting to be submitted build up in the list. There is no benefit is
using io_uring when the glib GSource is being used, so disable it
instead of implementing a more complex fix.
This fixes a memory leak where AioHandlers would build up and increasing
amounts of CPU time were spent iterating them in aio_pending(). The
symptom is that guests become slow when QEMU is built with io_uring
support.
Buglink: https://bugs.launchpad.net/qemu/+bug/1877716
Fixes: 73fd282e7b6dd4e4ea1c3bbb3d302c8db51e4ccf ("aio-posix: add io_uring fd monitoring implementation")
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/block/aio.h | 3 +++
util/aio-posix.c | 12 ++++++++++++
util/aio-win32.c | 4 ++++
util/async.c | 1 +
4 files changed, 20 insertions(+)
diff --git a/include/block/aio.h b/include/block/aio.h
index 62ed954344..b2f703fa3f 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -701,6 +701,9 @@ void aio_context_setup(AioContext *ctx);
*/
void aio_context_destroy(AioContext *ctx);
+/* Used internally, do not call outside AioContext code */
+void aio_context_use_g_source(AioContext *ctx);
+
/**
* aio_context_set_poll_params:
* @ctx: the aio context
diff --git a/util/aio-posix.c b/util/aio-posix.c
index 8af334ab19..1b2a3af65b 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -682,6 +682,18 @@ void aio_context_destroy(AioContext *ctx)
aio_free_deleted_handlers(ctx);
}
+void aio_context_use_g_source(AioContext *ctx)
+{
+ /*
+ * Disable io_uring when the glib main loop is used because it doesn't
+ * support mixed glib/aio_poll() usage. It relies on aio_poll() being
+ * called regularly so that changes to the monitored file descriptors are
+ * submitted, otherwise a list of pending fd handlers builds up.
+ */
+ fdmon_io_uring_destroy(ctx);
+ aio_free_deleted_handlers(ctx);
+}
+
void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
int64_t grow, int64_t shrink, Error **errp)
{
diff --git a/util/aio-win32.c b/util/aio-win32.c
index 729d533faf..953c56ab48 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -414,6 +414,10 @@ void aio_context_destroy(AioContext *ctx)
{
}
+void aio_context_use_g_source(AioContext *ctx)
+{
+}
+
void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
int64_t grow, int64_t shrink, Error **errp)
{
diff --git a/util/async.c b/util/async.c
index 3165a28f2f..1319eee3bc 100644
--- a/util/async.c
+++ b/util/async.c
@@ -362,6 +362,7 @@ static GSourceFuncs aio_source_funcs = {
GSource *aio_get_g_source(AioContext *ctx)
{
+ aio_context_use_g_source(ctx);
g_source_ref(&ctx->source);
return &ctx->source;
}
--
2.25.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] aio-posix: don't duplicate fd handler deletion in fdmon_io_uring_destroy()
2020-05-11 18:36 ` [PATCH 1/2] aio-posix: don't duplicate fd handler deletion in fdmon_io_uring_destroy() Stefan Hajnoczi
@ 2020-05-14 7:48 ` Oleksandr Natalenko
0 siblings, 0 replies; 6+ messages in thread
From: Oleksandr Natalenko @ 2020-05-14 7:48 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Fam Zheng, Kevin Wolf, qemu-block, Stefan Weil, qemu-stable,
qemu-devel, Paolo Bonzini, Max Reitz
On Mon, May 11, 2020 at 07:36:29PM +0100, Stefan Hajnoczi wrote:
> The io_uring file descriptor monitoring implementation has an internal
> list of fd handlers that are pending submission to io_uring.
> fdmon_io_uring_destroy() deletes all fd handlers on the list.
>
> Don't delete fd handlers directly in fdmon_io_uring_destroy() for two
> reasons:
> 1. This duplicates the aio-posix.c AioHandler deletion code and could
> become outdated if the struct changes.
> 2. Only handlers with the FDMON_IO_URING_REMOVE flag set are safe to
> remove. If the flag is not set then something still has a pointer to
> the fd handler. Let aio-posix.c and its user worry about that. In
> practice this isn't an issue because fdmon_io_uring_destroy() is only
> called when shutting down so all users have removed their fd
> handlers, but the next patch will need this!
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> util/aio-posix.c | 1 +
> util/fdmon-io_uring.c | 13 ++++++++++---
> 2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/util/aio-posix.c b/util/aio-posix.c
> index c3613d299e..8af334ab19 100644
> --- a/util/aio-posix.c
> +++ b/util/aio-posix.c
> @@ -679,6 +679,7 @@ void aio_context_destroy(AioContext *ctx)
> {
> fdmon_io_uring_destroy(ctx);
> fdmon_epoll_disable(ctx);
> + aio_free_deleted_handlers(ctx);
> }
>
> void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
> diff --git a/util/fdmon-io_uring.c b/util/fdmon-io_uring.c
> index d5a80ed6fb..1d14177df0 100644
> --- a/util/fdmon-io_uring.c
> +++ b/util/fdmon-io_uring.c
> @@ -342,11 +342,18 @@ void fdmon_io_uring_destroy(AioContext *ctx)
>
> io_uring_queue_exit(&ctx->fdmon_io_uring);
>
> - /* No need to submit these anymore, just free them. */
> + /* Move handlers due to be removed onto the deleted list */
> while ((node = QSLIST_FIRST_RCU(&ctx->submit_list))) {
> + unsigned flags = atomic_fetch_and(&node->flags,
> + ~(FDMON_IO_URING_PENDING |
> + FDMON_IO_URING_ADD |
> + FDMON_IO_URING_REMOVE));
> +
> + if (flags & FDMON_IO_URING_REMOVE) {
> + QLIST_INSERT_HEAD_RCU(&ctx->deleted_aio_handlers, node, node_deleted);
> + }
> +
> QSLIST_REMOVE_HEAD_RCU(&ctx->submit_list, node_submitted);
> - QLIST_REMOVE(node, node);
> - g_free(node);
> }
>
> ctx->fdmon_ops = &fdmon_poll_ops;
Tested-by: Oleksandr Natalenko <oleksandr@redhat.com>
(run Windows 10 VM with storage accessible via io_uring on qemu v5.0.0
with these 2 patches)
Thank you.
--
Best regards,
Oleksandr Natalenko (post-factum)
Principal Software Maintenance Engineer
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] aio-posix: disable fdmon-io_uring when GSource is used
2020-05-11 18:36 ` [PATCH 2/2] aio-posix: disable fdmon-io_uring when GSource is used Stefan Hajnoczi
@ 2020-05-14 7:49 ` Oleksandr Natalenko
0 siblings, 0 replies; 6+ messages in thread
From: Oleksandr Natalenko @ 2020-05-14 7:49 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Fam Zheng, Kevin Wolf, qemu-block, Stefan Weil, qemu-stable,
qemu-devel, Paolo Bonzini, Max Reitz
On Mon, May 11, 2020 at 07:36:30PM +0100, Stefan Hajnoczi wrote:
> The glib event loop does not call fdmon_io_uring_wait() so fd handlers
> waiting to be submitted build up in the list. There is no benefit is
> using io_uring when the glib GSource is being used, so disable it
> instead of implementing a more complex fix.
>
> This fixes a memory leak where AioHandlers would build up and increasing
> amounts of CPU time were spent iterating them in aio_pending(). The
> symptom is that guests become slow when QEMU is built with io_uring
> support.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1877716
> Fixes: 73fd282e7b6dd4e4ea1c3bbb3d302c8db51e4ccf ("aio-posix: add io_uring fd monitoring implementation")
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> include/block/aio.h | 3 +++
> util/aio-posix.c | 12 ++++++++++++
> util/aio-win32.c | 4 ++++
> util/async.c | 1 +
> 4 files changed, 20 insertions(+)
>
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 62ed954344..b2f703fa3f 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -701,6 +701,9 @@ void aio_context_setup(AioContext *ctx);
> */
> void aio_context_destroy(AioContext *ctx);
>
> +/* Used internally, do not call outside AioContext code */
> +void aio_context_use_g_source(AioContext *ctx);
> +
> /**
> * aio_context_set_poll_params:
> * @ctx: the aio context
> diff --git a/util/aio-posix.c b/util/aio-posix.c
> index 8af334ab19..1b2a3af65b 100644
> --- a/util/aio-posix.c
> +++ b/util/aio-posix.c
> @@ -682,6 +682,18 @@ void aio_context_destroy(AioContext *ctx)
> aio_free_deleted_handlers(ctx);
> }
>
> +void aio_context_use_g_source(AioContext *ctx)
> +{
> + /*
> + * Disable io_uring when the glib main loop is used because it doesn't
> + * support mixed glib/aio_poll() usage. It relies on aio_poll() being
> + * called regularly so that changes to the monitored file descriptors are
> + * submitted, otherwise a list of pending fd handlers builds up.
> + */
> + fdmon_io_uring_destroy(ctx);
> + aio_free_deleted_handlers(ctx);
> +}
> +
> void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
> int64_t grow, int64_t shrink, Error **errp)
> {
> diff --git a/util/aio-win32.c b/util/aio-win32.c
> index 729d533faf..953c56ab48 100644
> --- a/util/aio-win32.c
> +++ b/util/aio-win32.c
> @@ -414,6 +414,10 @@ void aio_context_destroy(AioContext *ctx)
> {
> }
>
> +void aio_context_use_g_source(AioContext *ctx)
> +{
> +}
> +
> void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
> int64_t grow, int64_t shrink, Error **errp)
> {
> diff --git a/util/async.c b/util/async.c
> index 3165a28f2f..1319eee3bc 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -362,6 +362,7 @@ static GSourceFuncs aio_source_funcs = {
>
> GSource *aio_get_g_source(AioContext *ctx)
> {
> + aio_context_use_g_source(ctx);
> g_source_ref(&ctx->source);
> return &ctx->source;
> }
Tested-by: Oleksandr Natalenko <oleksandr@redhat.com>
(run Windows 10 VM with storage accessible via io_uring on qemu v5.0.0
with these 2 patches)
Thank you.
--
Best regards,
Oleksandr Natalenko (post-factum)
Principal Software Maintenance Engineer
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] aio-posix: fix fdmon-io_uring memory leak
2020-05-11 18:36 [PATCH 0/2] aio-posix: fix fdmon-io_uring memory leak Stefan Hajnoczi
2020-05-11 18:36 ` [PATCH 1/2] aio-posix: don't duplicate fd handler deletion in fdmon_io_uring_destroy() Stefan Hajnoczi
2020-05-11 18:36 ` [PATCH 2/2] aio-posix: disable fdmon-io_uring when GSource is used Stefan Hajnoczi
@ 2020-05-21 13:49 ` Stefan Hajnoczi
2 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2020-05-21 13:49 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Fam Zheng, Kevin Wolf, qemu-block, Stefan Weil, qemu-stable,
qemu-devel, Paolo Bonzini, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 1140 bytes --]
On Mon, May 11, 2020 at 07:36:28PM +0100, Stefan Hajnoczi wrote:
> This bug was introduced in QEMU 5.0 and causes guests to slow down because
> AioHandlers are not freed when the fdmon-io_uring file descriptor monitoring
> implementation is used by the main loop thread's glib event loop. This issue
> does not apply to IOThread usage of fdmon-io_uring.
>
> In practice few distros build with io_uring support enabled at the moment, so
> the number of affected users is likely to be small. The fix is still suitable
> for a stable release though.
>
> https://bugs.launchpad.net/qemu/+bug/1877716
> https://bugs.launchpad.net/qemu/+bug/1873032
>
> Stefan Hajnoczi (2):
> aio-posix: don't duplicate fd handler deletion in
> fdmon_io_uring_destroy()
> aio-posix: disable fdmon-io_uring when GSource is used
>
> include/block/aio.h | 3 +++
> util/aio-posix.c | 13 +++++++++++++
> util/aio-win32.c | 4 ++++
> util/async.c | 1 +
> util/fdmon-io_uring.c | 13 ++++++++++---
> 5 files changed, 31 insertions(+), 3 deletions(-)
This has been merged into qemu.git/master.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-21 13:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-11 18:36 [PATCH 0/2] aio-posix: fix fdmon-io_uring memory leak Stefan Hajnoczi
2020-05-11 18:36 ` [PATCH 1/2] aio-posix: don't duplicate fd handler deletion in fdmon_io_uring_destroy() Stefan Hajnoczi
2020-05-14 7:48 ` Oleksandr Natalenko
2020-05-11 18:36 ` [PATCH 2/2] aio-posix: disable fdmon-io_uring when GSource is used Stefan Hajnoczi
2020-05-14 7:49 ` Oleksandr Natalenko
2020-05-21 13:49 ` [PATCH 0/2] aio-posix: fix fdmon-io_uring memory leak Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).