From dcba65f824817596e817a43f83ef83bac9099e76 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 5 Oct 2020 17:58:42 +0200 Subject: monitor: Add Monitor parameter to monitor_set_cpu() Most callers actually don't have to rely on cur_mon, but already know for which monitor they call monitor_set_cpu(). Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Message-Id: <20201005155855.256490-2-kwolf@redhat.com> Reviewed-by: Markus Armbruster Reviewed-by: Stefan Hajnoczi Signed-off-by: Markus Armbruster --- include/monitor/monitor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'include') diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index c017077..04f472a 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -33,7 +33,7 @@ int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0); int monitor_printf(Monitor *mon, const char *fmt, ...) GCC_FMT_ATTR(2, 3); void monitor_flush(Monitor *mon); -int monitor_set_cpu(int cpu_index); +int monitor_set_cpu(Monitor *mon, int cpu_index); int monitor_get_cpu_index(void); void monitor_read_command(MonitorHMP *mon, int show_prompt); -- cgit v1.1 From 87e6f4a4d6885006931b371771e2933c40700427 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 5 Oct 2020 17:58:43 +0200 Subject: monitor: Add Monitor parameter to monitor_get_cpu_index() Most callers actually don't have to rely on cur_mon, but already know for which monitor they call monitor_get_cpu_index(). Signed-off-by: Kevin Wolf Message-Id: <20201005155855.256490-3-kwolf@redhat.com> Reviewed-by: Markus Armbruster Reviewed-by: Stefan Hajnoczi Signed-off-by: Markus Armbruster --- include/monitor/monitor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'include') diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 04f472a..93bedf0 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -34,7 +34,7 @@ int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) int monitor_printf(Monitor *mon, const char *fmt, ...) GCC_FMT_ATTR(2, 3); void monitor_flush(Monitor *mon); int monitor_set_cpu(Monitor *mon, int cpu_index); -int monitor_get_cpu_index(void); +int monitor_get_cpu_index(Monitor *mon); void monitor_read_command(MonitorHMP *mon, int show_prompt); int monitor_read_password(MonitorHMP *mon, ReadLineFunc *readline_func, -- cgit v1.1 From 947e47448dcc4e4d7a8b7c42b43acb3435b3ad35 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 5 Oct 2020 17:58:44 +0200 Subject: monitor: Use getter/setter functions for cur_mon cur_mon really needs to be coroutine-local as soon as we move monitor command handlers to coroutines and let them yield. As a first step, just remove all direct accesses to cur_mon so that we can implement this in the getter function later. Signed-off-by: Kevin Wolf Message-Id: <20201005155855.256490-4-kwolf@redhat.com> Reviewed-by: Markus Armbruster Reviewed-by: Stefan Hajnoczi Signed-off-by: Markus Armbruster --- include/monitor/monitor.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'include') diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 93bedf0..543eafc 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -5,7 +5,6 @@ #include "qapi/qapi-types-misc.h" #include "qemu/readline.h" -extern __thread Monitor *cur_mon; typedef struct MonitorHMP MonitorHMP; typedef struct MonitorOptions MonitorOptions; @@ -13,6 +12,8 @@ typedef struct MonitorOptions MonitorOptions; extern QemuOptsList qemu_mon_opts; +Monitor *monitor_cur(void); +Monitor *monitor_set_cur(Monitor *mon); bool monitor_cur_is_qmp(void); void monitor_init_globals(void); -- cgit v1.1 From 41725fa7eda1d97576fc8c79b58d04a61629f40e Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 5 Oct 2020 17:58:47 +0200 Subject: qmp: Call monitor_set_cur() only in qmp_dispatch() The correct way to set the current monitor for a coroutine handler will be different than for a blocking handler, so monitor_set_cur() needs to be called in qmp_dispatch(). Signed-off-by: Kevin Wolf Message-Id: <20201005155855.256490-7-kwolf@redhat.com> Reviewed-by: Markus Armbruster Reviewed-by: Stefan Hajnoczi Signed-off-by: Markus Armbruster --- include/qapi/qmp/dispatch.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'include') diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 5a9cf82..0c2f467 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -14,6 +14,7 @@ #ifndef QAPI_QMP_DISPATCH_H #define QAPI_QMP_DISPATCH_H +#include "monitor/monitor.h" #include "qemu/queue.h" typedef void (QmpCommandFunc)(QDict *, QObject **, Error **); @@ -49,7 +50,7 @@ const char *qmp_command_name(const QmpCommand *cmd); bool qmp_has_success_response(const QmpCommand *cmd); QDict *qmp_error_response(Error *err); QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request, - bool allow_oob); + bool allow_oob, Monitor *cur_mon); bool qmp_is_oob(const QDict *dict); typedef void (*qmp_cmd_callback_fn)(const QmpCommand *cmd, void *opaque); -- cgit v1.1 From e69ee454b5f9dff3af48bcfc3d9691b3edb02fe2 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 5 Oct 2020 17:58:48 +0200 Subject: monitor: Make current monitor a per-coroutine property This way, a monitor command handler will still be able to access the current monitor, but when it yields, all other code code will correctly get NULL from monitor_cur(). This uses a hash table to map the coroutine pointer to the current monitor of that coroutine. Outside of coroutine context, we associate the current monitor with the leader coroutine of the current thread. Approaches to implement some form of coroutine local storage directly in the coroutine core code have been considered and discarded because they didn't end up being much more generic than the hash table and their performance impact on coroutines not using coroutine local storage was unclear. As the block layer uses a coroutine per I/O request, this is a fast path and we have to be careful. It's safest to just stay out of this path with code only used by the monitor. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Message-Id: <20201005155855.256490-8-kwolf@redhat.com> Reviewed-by: Markus Armbruster Reviewed-by: Stefan Hajnoczi Signed-off-by: Markus Armbruster --- include/monitor/monitor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'include') diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 543eafc..348bfad 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -13,7 +13,7 @@ typedef struct MonitorOptions MonitorOptions; extern QemuOptsList qemu_mon_opts; Monitor *monitor_cur(void); -Monitor *monitor_set_cur(Monitor *mon); +Monitor *monitor_set_cur(Coroutine *co, Monitor *mon); bool monitor_cur_is_qmp(void); void monitor_init_globals(void); -- cgit v1.1 From 04f22362f14b028c2632ce01e74e6a78c2b45e89 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 5 Oct 2020 17:58:49 +0200 Subject: qapi: Add a 'coroutine' flag for commands This patch adds a new 'coroutine' flag to QMP command definitions that tells the QMP dispatcher that the command handler is safe to be run in a coroutine. The documentation of the new flag pretends that this flag is already used as intended, which it isn't yet after this patch. We'll implement this in another patch in this series. Signed-off-by: Kevin Wolf Message-Id: <20201005155855.256490-9-kwolf@redhat.com> Reviewed-by: Markus Armbruster Reviewed-by: Stefan Hajnoczi Signed-off-by: Markus Armbruster --- include/qapi/qmp/dispatch.h | 1 + 1 file changed, 1 insertion(+) (limited to 'include') diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 0c2f467..9fd2b72 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -25,6 +25,7 @@ typedef enum QmpCommandOptions QCO_NO_SUCCESS_RESP = (1U << 0), QCO_ALLOW_OOB = (1U << 1), QCO_ALLOW_PRECONFIG = (1U << 2), + QCO_COROUTINE = (1U << 3), } QmpCommandOptions; typedef struct QmpCommand -- cgit v1.1 From 9ce44e2ce267caf5559904a201aa1986b0a8326b Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 5 Oct 2020 17:58:50 +0200 Subject: qmp: Move dispatcher to a coroutine This moves the QMP dispatcher to a coroutine and runs all QMP command handlers that declare 'coroutine': true in coroutine context so they can avoid blocking the main loop while doing I/O or waiting for other events. For commands that are not declared safe to run in a coroutine, the dispatcher drops out of coroutine context by calling the QMP command handler from a bottom half. Signed-off-by: Kevin Wolf Reviewed-by: Markus Armbruster Message-Id: <20201005155855.256490-10-kwolf@redhat.com> Reviewed-by: Markus Armbruster Reviewed-by: Stefan Hajnoczi Signed-off-by: Markus Armbruster --- include/qapi/qmp/dispatch.h | 1 + 1 file changed, 1 insertion(+) (limited to 'include') diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 9fd2b72..af8d96c 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -31,6 +31,7 @@ typedef enum QmpCommandOptions typedef struct QmpCommand { const char *name; + /* Runs in coroutine context if QCO_COROUTINE is set */ QmpCommandFunc *fn; QmpCommandOptions options; QTAILQ_ENTRY(QmpCommand) node; -- cgit v1.1 From 26b0b698c00bd9176f86c539aeb680481fa19473 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 5 Oct 2020 17:58:52 +0200 Subject: util/async: Add aio_co_reschedule_self() Add a function that can be used to move the currently running coroutine to a different AioContext (and therefore potentially a different thread). Signed-off-by: Kevin Wolf Reviewed-by: Stefan Hajnoczi Message-Id: <20201005155855.256490-12-kwolf@redhat.com> Reviewed-by: Markus Armbruster Reviewed-by: Stefan Hajnoczi Signed-off-by: Markus Armbruster --- include/block/aio.h | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'include') diff --git a/include/block/aio.h b/include/block/aio.h index ec8c5af..5f34226 100644 --- a/include/block/aio.h +++ b/include/block/aio.h @@ -17,6 +17,7 @@ #ifdef CONFIG_LINUX_IO_URING #include #endif +#include "qemu/coroutine.h" #include "qemu/queue.h" #include "qemu/event_notifier.h" #include "qemu/thread.h" @@ -655,6 +656,15 @@ static inline bool aio_node_check(AioContext *ctx, bool is_external) void aio_co_schedule(AioContext *ctx, struct Coroutine *co); /** + * aio_co_reschedule_self: + * @new_ctx: the new context + * + * Move the currently running coroutine to new_ctx. If the coroutine is already + * running in new_ctx, do nothing. + */ +void coroutine_fn aio_co_reschedule_self(AioContext *new_ctx); + +/** * aio_co_wake: * @co: the coroutine * -- cgit v1.1 From e336fd4c4b2fa04e5d6c7f8ee524bfd2d9e9e8f1 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 5 Oct 2020 17:58:53 +0200 Subject: block: Add bdrv_co_enter()/leave() Add a pair of functions to temporarily move the current coroutine to the AioContext of a given BlockDriverState. Signed-off-by: Kevin Wolf Message-Id: <20201005155855.256490-13-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Markus Armbruster --- include/block/block.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) (limited to 'include') diff --git a/include/block/block.h b/include/block/block.h index ce2ac39..1027c58 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -641,6 +641,23 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag); AioContext *bdrv_get_aio_context(BlockDriverState *bs); /** + * Move the current coroutine to the AioContext of @bs and return the old + * AioContext of the coroutine. Increase bs->in_flight so that draining @bs + * will wait for the operation to proceed until the corresponding + * bdrv_co_leave(). + * + * Consequently, you can't call drain inside a bdrv_co_enter/leave() section as + * this will deadlock. + */ +AioContext *coroutine_fn bdrv_co_enter(BlockDriverState *bs); + +/** + * Ends a section started by bdrv_co_enter(). Move the current coroutine back + * to old_ctx and decrease bs->in_flight again. + */ +void coroutine_fn bdrv_co_leave(BlockDriverState *bs, AioContext *old_ctx); + +/** * Transfer control to @co in the aio context of @bs */ void bdrv_coroutine_enter(BlockDriverState *bs, Coroutine *co); -- cgit v1.1 From 18c6ac1c6eb7cc541249585836659d0d3ed3a539 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 5 Oct 2020 17:58:54 +0200 Subject: block: Add bdrv_lock()/unlock() Inside of coroutine context, we can't directly use aio_context_acquire() for the AioContext of a block node because we already own the lock of the current AioContext and we need to avoid double locking to prevent deadlocks. This provides helper functions to lock the AioContext of a node only if it's not the same as the current AioContext. Signed-off-by: Kevin Wolf Message-Id: <20201005155855.256490-14-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Markus Armbruster --- include/block/block.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'include') diff --git a/include/block/block.h b/include/block/block.h index 1027c58..d16c401 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -658,6 +658,20 @@ AioContext *coroutine_fn bdrv_co_enter(BlockDriverState *bs); void coroutine_fn bdrv_co_leave(BlockDriverState *bs, AioContext *old_ctx); /** + * Locks the AioContext of @bs if it's not the current AioContext. This avoids + * double locking which could lead to deadlocks: This is a coroutine_fn, so we + * know we already own the lock of the current AioContext. + * + * May only be called in the main thread. + */ +void coroutine_fn bdrv_co_lock(BlockDriverState *bs); + +/** + * Unlocks the AioContext of @bs if it's not the current AioContext. + */ +void coroutine_fn bdrv_co_unlock(BlockDriverState *bs); + +/** * Transfer control to @co in the aio context of @bs */ void bdrv_coroutine_enter(BlockDriverState *bs, Coroutine *co); -- cgit v1.1