All of lore.kernel.org
 help / color / mirror / Atom feed
From: Eric Blake <eblake@redhat.com>
To: qemu-devel@nongnu.org
Cc: Fam Zheng <fam@euphon.net>, Kevin Wolf <kwolf@redhat.com>,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	"open list:Block I/O path" <qemu-block@nongnu.org>,
	Max Reitz <mreitz@redhat.com>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: [PULL 01/34] async: the main AioContext is only "current" if under the BQL
Date: Tue, 15 Jun 2021 15:47:23 -0500	[thread overview]
Message-ID: <20210615204756.281505-2-eblake@redhat.com> (raw)
In-Reply-To: <20210615204756.281505-1-eblake@redhat.com>

From: Paolo Bonzini <pbonzini@redhat.com>

If we want to wake up a coroutine from a worker thread, aio_co_wake()
currently does not work.  In that scenario, aio_co_wake() calls
aio_co_enter(), but there is no current AioContext and therefore
qemu_get_current_aio_context() returns the main thread.  aio_co_wake()
then attempts to call aio_context_acquire() instead of going through
aio_co_schedule().

The default case of qemu_get_current_aio_context() was added to cover
synchronous I/O started from the vCPU thread, but the main and vCPU
threads are quite different.  The main thread is an I/O thread itself,
only running a more complicated event loop; the vCPU thread instead
is essentially a worker thread that occasionally calls
qemu_mutex_lock_iothread().  It is only in those critical sections
that it acts as if it were the home thread of the main AioContext.

Therefore, this patch detaches qemu_get_current_aio_context() from
iothreads, which is a useless complication.  The AioContext pointer
is stored directly in the thread-local variable, including for the
main loop.  Worker threads (including vCPU threads) optionally behave
as temporary home threads if they have taken the big QEMU lock,
but if that is not the case they will always schedule coroutines
on remote threads via aio_co_schedule().

With this change, the stub qemu_mutex_iothread_locked() must be changed
from true to false.  The previous value of true was needed because the
main thread did not have an AioContext in the thread-local variable,
but now it does have one.

Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20210609122234.544153-1-pbonzini@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[eblake: tweak commit message per Vladimir's review]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/aio.h   |  5 ++++-
 iothread.c            |  9 +--------
 stubs/iothread-lock.c |  2 +-
 stubs/iothread.c      |  8 --------
 tests/unit/iothread.c |  9 +--------
 util/async.c          | 20 ++++++++++++++++++++
 util/main-loop.c      |  1 +
 stubs/meson.build     |  1 -
 8 files changed, 28 insertions(+), 27 deletions(-)
 delete mode 100644 stubs/iothread.c

diff --git a/include/block/aio.h b/include/block/aio.h
index 5f342267d5ce..10fcae151540 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -691,10 +691,13 @@ void aio_co_enter(AioContext *ctx, struct Coroutine *co);
  * Return the AioContext whose event loop runs in the current thread.
  *
  * If called from an IOThread this will be the IOThread's AioContext.  If
- * called from another thread it will be the main loop AioContext.
+ * called from the main thread or with the "big QEMU lock" taken it
+ * will be the main loop AioContext.
  */
 AioContext *qemu_get_current_aio_context(void);

+void qemu_set_current_aio_context(AioContext *ctx);
+
 /**
  * aio_context_setup:
  * @ctx: the aio context
diff --git a/iothread.c b/iothread.c
index 7f086387be9a..2c5ccd736733 100644
--- a/iothread.c
+++ b/iothread.c
@@ -39,13 +39,6 @@ DECLARE_CLASS_CHECKERS(IOThreadClass, IOTHREAD,
 #define IOTHREAD_POLL_MAX_NS_DEFAULT 0ULL
 #endif

-static __thread IOThread *my_iothread;
-
-AioContext *qemu_get_current_aio_context(void)
-{
-    return my_iothread ? my_iothread->ctx : qemu_get_aio_context();
-}
-
 static void *iothread_run(void *opaque)
 {
     IOThread *iothread = opaque;
@@ -56,7 +49,7 @@ static void *iothread_run(void *opaque)
      * in this new thread uses glib.
      */
     g_main_context_push_thread_default(iothread->worker_context);
-    my_iothread = iothread;
+    qemu_set_current_aio_context(iothread->ctx);
     iothread->thread_id = qemu_get_thread_id();
     qemu_sem_post(&iothread->init_done_sem);

diff --git a/stubs/iothread-lock.c b/stubs/iothread-lock.c
index 2a6efad64a16..5b45b7fc8b90 100644
--- a/stubs/iothread-lock.c
+++ b/stubs/iothread-lock.c
@@ -3,7 +3,7 @@

 bool qemu_mutex_iothread_locked(void)
 {
-    return true;
+    return false;
 }

 void qemu_mutex_lock_iothread_impl(const char *file, int line)
diff --git a/stubs/iothread.c b/stubs/iothread.c
deleted file mode 100644
index 8cc9e28c5555..000000000000
--- a/stubs/iothread.c
+++ /dev/null
@@ -1,8 +0,0 @@
-#include "qemu/osdep.h"
-#include "block/aio.h"
-#include "qemu/main-loop.h"
-
-AioContext *qemu_get_current_aio_context(void)
-{
-    return qemu_get_aio_context();
-}
diff --git a/tests/unit/iothread.c b/tests/unit/iothread.c
index afde12b4efb5..f9b0791084e7 100644
--- a/tests/unit/iothread.c
+++ b/tests/unit/iothread.c
@@ -30,13 +30,6 @@ struct IOThread {
     bool stopping;
 };

-static __thread IOThread *my_iothread;
-
-AioContext *qemu_get_current_aio_context(void)
-{
-    return my_iothread ? my_iothread->ctx : qemu_get_aio_context();
-}
-
 static void iothread_init_gcontext(IOThread *iothread)
 {
     GSource *source;
@@ -54,9 +47,9 @@ static void *iothread_run(void *opaque)

     rcu_register_thread();

-    my_iothread = iothread;
     qemu_mutex_lock(&iothread->init_done_lock);
     iothread->ctx = aio_context_new(&error_abort);
+    qemu_set_current_aio_context(iothread->ctx);

     /*
      * We must connect the ctx to a GMainContext, because in older versions
diff --git a/util/async.c b/util/async.c
index 674dbefb7c24..5d9b7cc1eba2 100644
--- a/util/async.c
+++ b/util/async.c
@@ -649,3 +649,23 @@ void aio_context_release(AioContext *ctx)
 {
     qemu_rec_mutex_unlock(&ctx->lock);
 }
+
+static __thread AioContext *my_aiocontext;
+
+AioContext *qemu_get_current_aio_context(void)
+{
+    if (my_aiocontext) {
+        return my_aiocontext;
+    }
+    if (qemu_mutex_iothread_locked()) {
+        /* Possibly in a vCPU thread.  */
+        return qemu_get_aio_context();
+    }
+    return NULL;
+}
+
+void qemu_set_current_aio_context(AioContext *ctx)
+{
+    assert(!my_aiocontext);
+    my_aiocontext = ctx;
+}
diff --git a/util/main-loop.c b/util/main-loop.c
index d9c55df6f5e7..4ae5b23e991e 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -170,6 +170,7 @@ int qemu_init_main_loop(Error **errp)
     if (!qemu_aio_context) {
         return -EMFILE;
     }
+    qemu_set_current_aio_context(qemu_aio_context);
     qemu_notify_bh = qemu_bh_new(notify_event_cb, NULL);
     gpollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD));
     src = aio_get_g_source(qemu_aio_context);
diff --git a/stubs/meson.build b/stubs/meson.build
index 65c22c0568ce..4993797f0550 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -16,7 +16,6 @@ stub_ss.add(files('fw_cfg.c'))
 stub_ss.add(files('gdbstub.c'))
 stub_ss.add(files('get-vm-name.c'))
 stub_ss.add(when: 'CONFIG_LINUX_IO_URING', if_true: files('io_uring.c'))
-stub_ss.add(files('iothread.c'))
 stub_ss.add(files('iothread-lock.c'))
 stub_ss.add(files('isa-bus.c'))
 stub_ss.add(files('is-daemonized.c'))
-- 
2.31.1



  reply	other threads:[~2021-06-15 20:49 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-15 20:47 [PULL 00/34] NBD patches for 2021-06-15 Eric Blake
2021-06-15 20:47 ` Eric Blake [this message]
2021-06-15 20:47 ` [PULL 02/34] tests: cover aio_co_enter from a worker thread without BQL taken Eric Blake
2021-06-15 20:47 ` [PULL 03/34] co-queue: drop extra coroutine_fn marks Eric Blake
2021-06-15 20:47 ` [PULL 04/34] block/nbd: fix channel object leak Eric Blake
2021-06-15 20:47 ` [PULL 05/34] block/nbd: fix how state is cleared on nbd_open() failure paths Eric Blake
2021-06-15 20:47 ` [PULL 06/34] block/nbd: connect_thread_func(): do qio_channel_set_delay(false) Eric Blake
2021-06-15 20:47 ` [PULL 07/34] qemu-sockets: introduce socket_address_parse_named_fd() Eric Blake
2021-06-15 20:47 ` [PULL 08/34] block/nbd: call socket_address_parse_named_fd() in advance Eric Blake
2021-06-15 20:47 ` [PULL 09/34] block/nbd: ensure ->connection_thread is always valid Eric Blake
2021-06-15 20:47 ` [PULL 10/34] block/nbd: nbd_client_handshake(): fix leak of s->ioc Eric Blake
2021-06-15 20:47 ` [PULL 11/34] block/nbd: BDRVNBDState: drop unused connect_err and connect_status Eric Blake
2021-06-15 20:47 ` [PULL 12/34] block/nbd: simplify waking of nbd_co_establish_connection() Eric Blake
2021-06-15 20:47 ` [PULL 13/34] block/nbd: drop thr->state Eric Blake
2021-06-15 20:47 ` [PULL 14/34] block/nbd: bs-independent interface for nbd_co_establish_connection() Eric Blake
2021-06-15 20:47 ` [PULL 15/34] block/nbd: make nbd_co_establish_connection_cancel() bs-independent Eric Blake
2021-06-15 20:47 ` [PULL 16/34] block/nbd: rename NBDConnectThread to NBDClientConnection Eric Blake
2021-06-15 20:47 ` [PULL 17/34] block/nbd: introduce nbd_client_connection_new() Eric Blake
2021-06-15 20:47 ` [PULL 18/34] block/nbd: introduce nbd_client_connection_release() Eric Blake
2021-06-15 20:47 ` [PULL 19/34] nbd: move connection code from block/nbd to nbd/client-connection Eric Blake
2021-06-15 20:47 ` [PULL 20/34] nbd/client-connection: use QEMU_LOCK_GUARD Eric Blake
2021-06-15 20:47 ` [PULL 21/34] nbd/client-connection: add possibility of negotiation Eric Blake
2021-06-15 20:47 ` [PULL 22/34] nbd/client-connection: implement connection retry Eric Blake
2021-06-15 20:47 ` [PULL 23/34] nbd/client-connection: shutdown connection on release Eric Blake
2021-06-15 20:47 ` [PULL 24/34] block/nbd: split nbd_handle_updated_info out of nbd_client_handshake() Eric Blake
2021-06-15 20:47 ` [PULL 25/34] block/nbd: use negotiation of NBDClientConnection Eric Blake
2021-06-15 20:47 ` [PULL 26/34] block/nbd: don't touch s->sioc in nbd_teardown_connection() Eric Blake
2021-06-15 20:47 ` [PULL 27/34] block/nbd: drop BDRVNBDState::sioc Eric Blake
2021-06-15 20:47 ` [PULL 28/34] nbd/client-connection: return only one io channel Eric Blake
2021-06-17 18:32   ` Vladimir Sementsov-Ogievskiy
2021-06-18 15:55     ` Eric Blake
2021-06-15 20:47 ` [PULL 29/34] block-coroutine-wrapper: allow non bdrv_ prefix Eric Blake
2021-06-15 20:47 ` [PULL 30/34] block/nbd: split nbd_co_do_establish_connection out of nbd_reconnect_attempt Eric Blake
2021-06-15 20:47 ` [PULL 31/34] nbd/client-connection: add option for non-blocking connection attempt Eric Blake
2021-06-15 20:47 ` [PULL 32/34] block/nbd: reuse nbd_co_do_establish_connection() in nbd_open() Eric Blake
2021-06-15 20:47 ` [PULL 33/34] block/nbd: add nbd_client_connected() helper Eric Blake
2021-06-15 20:47 ` [PULL 34/34] block/nbd: safer transition to receiving request Eric Blake
2021-06-17  9:42 ` [PULL 00/34] NBD patches for 2021-06-15 Peter Maydell
2021-06-17 18:35   ` Vladimir Sementsov-Ogievskiy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210615204756.281505-2-eblake@redhat.com \
    --to=eblake@redhat.com \
    --cc=fam@euphon.net \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    --cc=vsementsov@virtuozzo.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.