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 --- docs/devel/qapi-code-gen.txt | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) (limited to 'docs/devel') diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt index 5fc67c99cd..4a41e36a75 100644 --- a/docs/devel/qapi-code-gen.txt +++ b/docs/devel/qapi-code-gen.txt @@ -472,6 +472,7 @@ Syntax: '*gen': false, '*allow-oob': true, '*allow-preconfig': true, + '*coroutine': true, '*if': COND, '*features': FEATURES } @@ -596,6 +597,34 @@ before the machine is built. It defaults to false. For example: QMP is available before the machine is built only when QEMU was started with --preconfig. +Member 'coroutine' tells the QMP dispatcher whether the command handler +is safe to be run in a coroutine. It defaults to false. If it is true, +the command handler is called from coroutine context and may yield while +waiting for an external event (such as I/O completion) in order to avoid +blocking the guest and other background operations. + +Coroutine safety can be hard to prove, similar to thread safety. Common +pitfalls are: + +- The global mutex isn't held across qemu_coroutine_yield(), so + operations that used to assume that they execute atomically may have + to be more careful to protect against changes in the global state. + +- Nested event loops (AIO_WAIT_WHILE() etc.) are problematic in + coroutine context and can easily lead to deadlocks. They should be + replaced by yielding and reentering the coroutine when the condition + becomes false. + +Since the command handler may assume coroutine context, any callers +other than the QMP dispatcher must also call it in coroutine context. +In particular, HMP commands calling such a QMP command handler must +enter coroutine context before calling the handler. + +It is an error to specify both 'coroutine': true and 'allow-oob': true +for a command. We don't currently have a use case for both together and +without a use case, it's not entirely clear what the semantics should +be. + The optional 'if' member specifies a conditional. See "Configuring the schema" below for more on this. -- cgit v1.2.3-55-g7522 From bb4b9ead95c3aaca84823e28dd9f11ccaa875c14 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 5 Oct 2020 17:58:51 +0200 Subject: hmp: Add support for coroutine command handlers Often, QMP command handlers are not only called to handle QMP commands, but also from a corresponding HMP command handler. In order to give them a consistent environment, optionally run HMP command handlers in a coroutine, too. The implementation is a lot simpler than in QMP because for HMP, we still block the VM while the coroutine is running. Signed-off-by: Kevin Wolf Reviewed-by: Dr. David Alan Gilbert Message-Id: <20201005155855.256490-11-kwolf@redhat.com> Reviewed-by: Markus Armbruster Reviewed-by: Stefan Hajnoczi Signed-off-by: Markus Armbruster --- docs/devel/qapi-code-gen.txt | 4 ++-- monitor/hmp.c | 37 ++++++++++++++++++++++++++++++++----- monitor/monitor-internal.h | 1 + 3 files changed, 35 insertions(+), 7 deletions(-) (limited to 'docs/devel') diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt index 4a41e36a75..c6438c6aa9 100644 --- a/docs/devel/qapi-code-gen.txt +++ b/docs/devel/qapi-code-gen.txt @@ -617,8 +617,8 @@ pitfalls are: Since the command handler may assume coroutine context, any callers other than the QMP dispatcher must also call it in coroutine context. -In particular, HMP commands calling such a QMP command handler must -enter coroutine context before calling the handler. +In particular, HMP commands calling such a QMP command handler must be +marked .coroutine = true in hmp-commands.hx. It is an error to specify both 'coroutine': true and 'allow-oob': true for a command. We don't currently have a use case for both together and diff --git a/monitor/hmp.c b/monitor/hmp.c index abaf939b2d..c5cd9d372b 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -1056,12 +1056,26 @@ fail: return NULL; } +typedef struct HandleHmpCommandCo { + Monitor *mon; + const HMPCommand *cmd; + QDict *qdict; + bool done; +} HandleHmpCommandCo; + +static void handle_hmp_command_co(void *opaque) +{ + HandleHmpCommandCo *data = opaque; + data->cmd->cmd(data->mon, data->qdict); + monitor_set_cur(qemu_coroutine_self(), NULL); + data->done = true; +} + void handle_hmp_command(MonitorHMP *mon, const char *cmdline) { QDict *qdict; const HMPCommand *cmd; const char *cmd_start = cmdline; - Monitor *old_mon; trace_handle_hmp_command(mon, cmdline); @@ -1080,10 +1094,23 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline) return; } - /* old_mon is non-NULL when called from qmp_human_monitor_command() */ - old_mon = monitor_set_cur(qemu_coroutine_self(), &mon->common); - cmd->cmd(&mon->common, qdict); - monitor_set_cur(qemu_coroutine_self(), old_mon); + if (!cmd->coroutine) { + /* old_mon is non-NULL when called from qmp_human_monitor_command() */ + Monitor *old_mon = monitor_set_cur(qemu_coroutine_self(), &mon->common); + cmd->cmd(&mon->common, qdict); + monitor_set_cur(qemu_coroutine_self(), old_mon); + } else { + HandleHmpCommandCo data = { + .mon = &mon->common, + .cmd = cmd, + .qdict = qdict, + .done = false, + }; + Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data); + monitor_set_cur(co, &mon->common); + aio_co_enter(qemu_get_aio_context(), co); + AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done); + } qobject_unref(qdict); } diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index b55d6df07f..ad2e64be13 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -74,6 +74,7 @@ typedef struct HMPCommand { const char *help; const char *flags; /* p=preconfig */ void (*cmd)(Monitor *mon, const QDict *qdict); + bool coroutine; /* * @sub_table is a list of 2nd level of commands. If it does not exist, * cmd should be used. If it exists, sub_table[?].cmd should be -- cgit v1.2.3-55-g7522