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/qapi') diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 5a9cf82472..0c2f467028 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 1.4.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 --- docs/devel/qapi-code-gen.txt | 29 +++++++++++++++++++++++++++++ docs/sphinx/qapidoc.py | 2 +- include/qapi/qmp/dispatch.h | 1 + scripts/qapi/commands.py | 10 +++++++--- scripts/qapi/expr.py | 11 +++++++++-- scripts/qapi/introspect.py | 2 +- scripts/qapi/schema.py | 13 +++++++++---- tests/qapi-schema/meson.build | 1 + tests/qapi-schema/oob-coroutine.err | 2 ++ tests/qapi-schema/oob-coroutine.json | 2 ++ tests/qapi-schema/oob-coroutine.out | 0 tests/qapi-schema/qapi-schema-test.json | 1 + tests/qapi-schema/qapi-schema-test.out | 2 ++ tests/qapi-schema/test-qapi.py | 7 ++++--- tests/test-qmp-cmds.c | 4 ++++ 15 files changed, 73 insertions(+), 14 deletions(-) create mode 100644 tests/qapi-schema/oob-coroutine.err create mode 100644 tests/qapi-schema/oob-coroutine.json create mode 100644 tests/qapi-schema/oob-coroutine.out (limited to 'include/qapi') 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. diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py index 6944ffa6aa..e03abcbb95 100644 --- a/docs/sphinx/qapidoc.py +++ b/docs/sphinx/qapidoc.py @@ -330,7 +330,7 @@ class QAPISchemaGenRSTVisitor(QAPISchemaVisitor): def visit_command(self, name, info, ifcond, features, arg_type, ret_type, gen, success_response, boxed, allow_oob, - allow_preconfig): + allow_preconfig, coroutine): doc = self._cur_doc self._add_doc('Command', self._nodes_for_arguments(doc, diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 0c2f467028..9fd2b720a7 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 diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py index 3cf9e1110b..6e6fc94a14 100644 --- a/scripts/qapi/commands.py +++ b/scripts/qapi/commands.py @@ -176,7 +176,8 @@ out: return ret -def gen_register_command(name, success_response, allow_oob, allow_preconfig): +def gen_register_command(name, success_response, allow_oob, allow_preconfig, + coroutine): options = [] if not success_response: @@ -185,6 +186,8 @@ def gen_register_command(name, success_response, allow_oob, allow_preconfig): options += ['QCO_ALLOW_OOB'] if allow_preconfig: options += ['QCO_ALLOW_PRECONFIG'] + if coroutine: + options += ['QCO_COROUTINE'] if not options: options = ['QCO_NO_OPTIONS'] @@ -267,7 +270,7 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds); def visit_command(self, name, info, ifcond, features, arg_type, ret_type, gen, success_response, boxed, - allow_oob, allow_preconfig): + allow_oob, allow_preconfig, coroutine): if not gen: return # FIXME: If T is a user-defined type, the user is responsible @@ -285,7 +288,8 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds); self._genh.add(gen_marshal_decl(name)) self._genc.add(gen_marshal(name, arg_type, boxed, ret_type)) self._regy.add(gen_register_command(name, success_response, - allow_oob, allow_preconfig)) + allow_oob, allow_preconfig, + coroutine)) def gen_commands(schema, output_dir, prefix): diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 2942520399..a15c1fb474 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -88,10 +88,17 @@ def check_flags(expr, info): if key in expr and expr[key] is not False: raise QAPISemError( info, "flag '%s' may only use false value" % key) - for key in ['boxed', 'allow-oob', 'allow-preconfig']: + for key in ['boxed', 'allow-oob', 'allow-preconfig', 'coroutine']: if key in expr and expr[key] is not True: raise QAPISemError( info, "flag '%s' may only use true value" % key) + if 'allow-oob' in expr and 'coroutine' in expr: + # This is not necessarily a fundamental incompatibility, but + # we don't have a use case and the desired semantics isn't + # obvious. The simplest solution is to forbid it until we get + # a use case for it. + raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' " + "are incompatible") def check_if(expr, info, source): @@ -342,7 +349,7 @@ def check_exprs(exprs): ['command'], ['data', 'returns', 'boxed', 'if', 'features', 'gen', 'success-response', 'allow-oob', - 'allow-preconfig']) + 'allow-preconfig', 'coroutine']) normalize_members(expr.get('data')) check_command(expr, info) elif meta == 'event': diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index 23652be810..5907b09cd5 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -216,7 +216,7 @@ const QLitObject %(c_name)s = %(c_string)s; def visit_command(self, name, info, ifcond, features, arg_type, ret_type, gen, success_response, boxed, - allow_oob, allow_preconfig): + allow_oob, allow_preconfig, coroutine): arg_type = arg_type or self._schema.the_empty_object_type ret_type = ret_type or self._schema.the_empty_object_type obj = {'arg-type': self._use_type(arg_type), diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 78309a00f0..d1307ec661 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -128,7 +128,7 @@ class QAPISchemaVisitor: def visit_command(self, name, info, ifcond, features, arg_type, ret_type, gen, success_response, boxed, - allow_oob, allow_preconfig): + allow_oob, allow_preconfig, coroutine): pass def visit_event(self, name, info, ifcond, features, arg_type, boxed): @@ -713,7 +713,8 @@ class QAPISchemaCommand(QAPISchemaEntity): def __init__(self, name, info, doc, ifcond, features, arg_type, ret_type, - gen, success_response, boxed, allow_oob, allow_preconfig): + gen, success_response, boxed, allow_oob, allow_preconfig, + coroutine): super().__init__(name, info, doc, ifcond, features) assert not arg_type or isinstance(arg_type, str) assert not ret_type or isinstance(ret_type, str) @@ -726,6 +727,7 @@ class QAPISchemaCommand(QAPISchemaEntity): self.boxed = boxed self.allow_oob = allow_oob self.allow_preconfig = allow_preconfig + self.coroutine = coroutine def check(self, schema): super().check(schema) @@ -768,7 +770,8 @@ class QAPISchemaCommand(QAPISchemaEntity): visitor.visit_command( self.name, self.info, self.ifcond, self.features, self.arg_type, self.ret_type, self.gen, self.success_response, - self.boxed, self.allow_oob, self.allow_preconfig) + self.boxed, self.allow_oob, self.allow_preconfig, + self.coroutine) class QAPISchemaEvent(QAPISchemaEntity): @@ -1074,6 +1077,7 @@ class QAPISchema: boxed = expr.get('boxed', False) allow_oob = expr.get('allow-oob', False) allow_preconfig = expr.get('allow-preconfig', False) + coroutine = expr.get('coroutine', False) ifcond = expr.get('if') features = self._make_features(expr.get('features'), info) if isinstance(data, OrderedDict): @@ -1086,7 +1090,8 @@ class QAPISchema: self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, features, data, rets, gen, success_response, - boxed, allow_oob, allow_preconfig)) + boxed, allow_oob, allow_preconfig, + coroutine)) def _def_event(self, expr, info, doc): name = expr['event'] diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build index f08c902911..1f222a7a13 100644 --- a/tests/qapi-schema/meson.build +++ b/tests/qapi-schema/meson.build @@ -142,6 +142,7 @@ schemas = [ 'nested-struct-data.json', 'nested-struct-data-invalid-dict.json', 'non-objects.json', + 'oob-coroutine.json', 'oob-test.json', 'allow-preconfig-test.json', 'pragma-doc-required-crap.json', diff --git a/tests/qapi-schema/oob-coroutine.err b/tests/qapi-schema/oob-coroutine.err new file mode 100644 index 0000000000..c01a4992bd --- /dev/null +++ b/tests/qapi-schema/oob-coroutine.err @@ -0,0 +1,2 @@ +oob-coroutine.json: In command 'oob-command-1': +oob-coroutine.json:2: flags 'allow-oob' and 'coroutine' are incompatible diff --git a/tests/qapi-schema/oob-coroutine.json b/tests/qapi-schema/oob-coroutine.json new file mode 100644 index 0000000000..0f67663bcd --- /dev/null +++ b/tests/qapi-schema/oob-coroutine.json @@ -0,0 +1,2 @@ +# Check that incompatible flags allow-oob and coroutine are rejected +{ 'command': 'oob-command-1', 'allow-oob': true, 'coroutine': true } diff --git a/tests/qapi-schema/oob-coroutine.out b/tests/qapi-schema/oob-coroutine.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 3a9f2cbb33..63f92adf68 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -148,6 +148,7 @@ 'returns': 'UserDefTwo' } { 'command': 'cmd-success-response', 'data': {}, 'success-response': false } +{ 'command': 'coroutine-cmd', 'data': {}, 'coroutine': true } # Returning a non-dictionary requires a name from the whitelist { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' }, diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 891b4101e0..8868ca0dca 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -203,6 +203,8 @@ command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo gen=True success_response=True boxed=False oob=False preconfig=False command cmd-success-response None -> None gen=True success_response=False boxed=False oob=False preconfig=False +command coroutine-cmd None -> None + gen=True success_response=True boxed=False oob=False preconfig=False coroutine=True object q_obj_guest-get-time-arg member a: int optional=False member b: int optional=True diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py index f396b471eb..e8db9d09d9 100755 --- a/tests/qapi-schema/test-qapi.py +++ b/tests/qapi-schema/test-qapi.py @@ -68,12 +68,13 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): def visit_command(self, name, info, ifcond, features, arg_type, ret_type, gen, success_response, boxed, - allow_oob, allow_preconfig): + allow_oob, allow_preconfig, coroutine): print('command %s %s -> %s' % (name, arg_type and arg_type.name, ret_type and ret_type.name)) - print(' gen=%s success_response=%s boxed=%s oob=%s preconfig=%s' - % (gen, success_response, boxed, allow_oob, allow_preconfig)) + print(' gen=%s success_response=%s boxed=%s oob=%s preconfig=%s%s' + % (gen, success_response, boxed, allow_oob, allow_preconfig, + " coroutine=True" if coroutine else "")) self._print_if(ifcond) self._print_features(features) diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c index 5f1b245e19..d3413bfef0 100644 --- a/tests/test-qmp-cmds.c +++ b/tests/test-qmp-cmds.c @@ -36,6 +36,10 @@ void qmp_cmd_success_response(Error **errp) { } +void qmp_coroutine_cmd(Error **errp) +{ +} + Empty2 *qmp_user_def_cmd0(Error **errp) { return g_new0(Empty2, 1); -- cgit 1.4.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 + monitor/monitor-internal.h | 6 ++- monitor/monitor.c | 55 +++++++++++++++++--- monitor/qmp.c | 122 +++++++++++++++++++++++++++++++++----------- qapi/qmp-dispatch.c | 65 +++++++++++++++++++++-- qapi/qmp-registry.c | 3 ++ util/aio-posix.c | 8 ++- 7 files changed, 214 insertions(+), 46 deletions(-) (limited to 'include/qapi') diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 9fd2b720a7..af8d96c570 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; diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index b39e03b744..b55d6df07f 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -155,7 +155,9 @@ static inline bool monitor_is_qmp(const Monitor *mon) typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList; extern IOThread *mon_iothread; -extern QEMUBH *qmp_dispatcher_bh; +extern Coroutine *qmp_dispatcher_co; +extern bool qmp_dispatcher_co_shutdown; +extern bool qmp_dispatcher_co_busy; extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands; extern QemuMutex monitor_lock; extern MonitorList mon_list; @@ -173,7 +175,7 @@ void monitor_fdsets_cleanup(void); void qmp_send_response(MonitorQMP *mon, const QDict *rsp); void monitor_data_destroy_qmp(MonitorQMP *mon); -void monitor_qmp_bh_dispatcher(void *data); +void coroutine_fn monitor_qmp_dispatcher_co(void *data); int get_monitor_def(int64_t *pval, const char *name); void help_cmd(Monitor *mon, const char *name); diff --git a/monitor/monitor.c b/monitor/monitor.c index ef68ca9d21..ceffe1a83b 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -55,8 +55,32 @@ typedef struct { /* Shared monitor I/O thread */ IOThread *mon_iothread; -/* Bottom half to dispatch the requests received from I/O thread */ -QEMUBH *qmp_dispatcher_bh; +/* Coroutine to dispatch the requests received from I/O thread */ +Coroutine *qmp_dispatcher_co; + +/* Set to true when the dispatcher coroutine should terminate */ +bool qmp_dispatcher_co_shutdown; + +/* + * qmp_dispatcher_co_busy is used for synchronisation between the + * monitor thread and the main thread to ensure that the dispatcher + * coroutine never gets scheduled a second time when it's already + * scheduled (scheduling the same coroutine twice is forbidden). + * + * It is true if the coroutine is active and processing requests. + * Additional requests may then be pushed onto mon->qmp_requests, + * and @qmp_dispatcher_co_shutdown may be set without further ado. + * @qmp_dispatcher_co_busy must not be woken up in this case. + * + * If false, you also have to set @qmp_dispatcher_co_busy to true and + * wake up @qmp_dispatcher_co after pushing the new requests. + * + * The coroutine will automatically change this variable back to false + * before it yields. Nobody else may set the variable to false. + * + * Access must be atomic for thread safety. + */ +bool qmp_dispatcher_co_busy; /* * Protects mon_list, monitor_qapi_event_state, coroutine_mon, @@ -623,9 +647,24 @@ void monitor_cleanup(void) } qemu_mutex_unlock(&monitor_lock); - /* QEMUBHs needs to be deleted before destroying the I/O thread */ - qemu_bh_delete(qmp_dispatcher_bh); - qmp_dispatcher_bh = NULL; + /* + * The dispatcher needs to stop before destroying the I/O thread. + * + * We need to poll both qemu_aio_context and iohandler_ctx to make + * sure that the dispatcher coroutine keeps making progress and + * eventually terminates. qemu_aio_context is automatically + * polled by calling AIO_WAIT_WHILE on it, but we must poll + * iohandler_ctx manually. + */ + qmp_dispatcher_co_shutdown = true; + if (!qatomic_xchg(&qmp_dispatcher_co_busy, true)) { + aio_co_wake(qmp_dispatcher_co); + } + + AIO_WAIT_WHILE(qemu_get_aio_context(), + (aio_poll(iohandler_get_aio_context(), false), + qatomic_mb_read(&qmp_dispatcher_co_busy))); + if (mon_iothread) { iothread_destroy(mon_iothread); mon_iothread = NULL; @@ -649,9 +688,9 @@ void monitor_init_globals_core(void) * have commands assuming that context. It would be nice to get * rid of those assumptions. */ - qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(), - monitor_qmp_bh_dispatcher, - NULL); + qmp_dispatcher_co = qemu_coroutine_create(monitor_qmp_dispatcher_co, NULL); + qatomic_mb_set(&qmp_dispatcher_co_busy, true); + aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co); } int monitor_init(MonitorOptions *opts, bool allow_hmp, Error **errp) diff --git a/monitor/qmp.c b/monitor/qmp.c index e746b3575d..b42f8c6af3 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -133,6 +133,10 @@ static void monitor_qmp_respond(MonitorQMP *mon, QDict *rsp) } } +/* + * Runs outside of coroutine context for OOB commands, but in + * coroutine context for everything else. + */ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req) { QDict *rsp; @@ -206,43 +210,99 @@ static QMPRequest *monitor_qmp_requests_pop_any_with_lock(void) return req_obj; } -void monitor_qmp_bh_dispatcher(void *data) +void coroutine_fn monitor_qmp_dispatcher_co(void *data) { - QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock(); + QMPRequest *req_obj = NULL; QDict *rsp; bool need_resume; MonitorQMP *mon; - if (!req_obj) { - return; - } + while (true) { + assert(qatomic_mb_read(&qmp_dispatcher_co_busy) == true); - mon = req_obj->mon; - /* qmp_oob_enabled() might change after "qmp_capabilities" */ - need_resume = !qmp_oob_enabled(mon) || - mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1; - qemu_mutex_unlock(&mon->qmp_queue_lock); - if (req_obj->req) { - QDict *qdict = qobject_to(QDict, req_obj->req); - QObject *id = qdict ? qdict_get(qdict, "id") : NULL; - trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: ""); - monitor_qmp_dispatch(mon, req_obj->req); - } else { - assert(req_obj->err); - rsp = qmp_error_response(req_obj->err); - req_obj->err = NULL; - monitor_qmp_respond(mon, rsp); - qobject_unref(rsp); - } + /* + * Mark the dispatcher as not busy already here so that we + * don't miss any new requests coming in the middle of our + * processing. + */ + qatomic_mb_set(&qmp_dispatcher_co_busy, false); + + while (!(req_obj = monitor_qmp_requests_pop_any_with_lock())) { + /* + * No more requests to process. Wait to be reentered from + * handle_qmp_command() when it pushes more requests, or + * from monitor_cleanup() when it requests shutdown. + */ + if (!qmp_dispatcher_co_shutdown) { + qemu_coroutine_yield(); + + /* + * busy must be set to true again by whoever + * rescheduled us to avoid double scheduling + */ + assert(qatomic_xchg(&qmp_dispatcher_co_busy, false) == true); + } + + /* + * qmp_dispatcher_co_shutdown may have changed if we + * yielded and were reentered from monitor_cleanup() + */ + if (qmp_dispatcher_co_shutdown) { + return; + } + } - if (need_resume) { - /* Pairs with the monitor_suspend() in handle_qmp_command() */ - monitor_resume(&mon->common); - } - qmp_request_free(req_obj); + if (qatomic_xchg(&qmp_dispatcher_co_busy, true) == true) { + /* + * Someone rescheduled us (probably because a new requests + * came in), but we didn't actually yield. Do that now, + * only to be immediately reentered and removed from the + * list of scheduled coroutines. + */ + qemu_coroutine_yield(); + } - /* Reschedule instead of looping so the main loop stays responsive */ - qemu_bh_schedule(qmp_dispatcher_bh); + /* + * Move the coroutine from iohandler_ctx to qemu_aio_context for + * executing the command handler so that it can make progress if it + * involves an AIO_WAIT_WHILE(). + */ + aio_co_schedule(qemu_get_aio_context(), qmp_dispatcher_co); + qemu_coroutine_yield(); + + mon = req_obj->mon; + /* qmp_oob_enabled() might change after "qmp_capabilities" */ + need_resume = !qmp_oob_enabled(mon) || + mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1; + qemu_mutex_unlock(&mon->qmp_queue_lock); + if (req_obj->req) { + QDict *qdict = qobject_to(QDict, req_obj->req); + QObject *id = qdict ? qdict_get(qdict, "id") : NULL; + trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: ""); + monitor_qmp_dispatch(mon, req_obj->req); + } else { + assert(req_obj->err); + rsp = qmp_error_response(req_obj->err); + req_obj->err = NULL; + monitor_qmp_respond(mon, rsp); + qobject_unref(rsp); + } + + if (need_resume) { + /* Pairs with the monitor_suspend() in handle_qmp_command() */ + monitor_resume(&mon->common); + } + qmp_request_free(req_obj); + + /* + * Yield and reschedule so the main loop stays responsive. + * + * Move back to iohandler_ctx so that nested event loops for + * qemu_aio_context don't start new monitor commands. + */ + aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co); + qemu_coroutine_yield(); + } } static void handle_qmp_command(void *opaque, QObject *req, Error *err) @@ -303,7 +363,9 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err) qemu_mutex_unlock(&mon->qmp_queue_lock); /* Kick the dispatcher routine */ - qemu_bh_schedule(qmp_dispatcher_bh); + if (!qatomic_xchg(&qmp_dispatcher_co_busy, true)) { + aio_co_wake(qmp_dispatcher_co); + } } static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size) diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 5677ba92ca..9a2d7dd29a 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -12,12 +12,16 @@ */ #include "qemu/osdep.h" + +#include "block/aio.h" #include "qapi/error.h" #include "qapi/qmp/dispatch.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qjson.h" #include "sysemu/runstate.h" #include "qapi/qmp/qbool.h" +#include "qemu/coroutine.h" +#include "qemu/main-loop.h" static QDict *qmp_dispatch_check_obj(QDict *dict, bool allow_oob, Error **errp) @@ -88,6 +92,30 @@ bool qmp_is_oob(const QDict *dict) && !qdict_haskey(dict, "execute"); } +typedef struct QmpDispatchBH { + const QmpCommand *cmd; + Monitor *cur_mon; + QDict *args; + QObject **ret; + Error **errp; + Coroutine *co; +} QmpDispatchBH; + +static void do_qmp_dispatch_bh(void *opaque) +{ + QmpDispatchBH *data = opaque; + + assert(monitor_cur() == NULL); + monitor_set_cur(qemu_coroutine_self(), data->cur_mon); + data->cmd->fn(data->args, data->ret, data->errp); + monitor_set_cur(qemu_coroutine_self(), NULL); + aio_co_wake(data->co); +} + +/* + * Runs outside of coroutine context for OOB commands, but in coroutine + * context for everything else. + */ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request, bool allow_oob, Monitor *cur_mon) { @@ -153,12 +181,39 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request, qobject_ref(args); } + assert(!(oob && qemu_in_coroutine())); assert(monitor_cur() == NULL); - monitor_set_cur(qemu_coroutine_self(), cur_mon); - - cmd->fn(args, &ret, &err); - - monitor_set_cur(qemu_coroutine_self(), NULL); + if (!!(cmd->options & QCO_COROUTINE) == qemu_in_coroutine()) { + monitor_set_cur(qemu_coroutine_self(), cur_mon); + cmd->fn(args, &ret, &err); + monitor_set_cur(qemu_coroutine_self(), NULL); + } else { + /* + * Actual context doesn't match the one the command needs. + * + * Case 1: we are in coroutine context, but command does not + * have QCO_COROUTINE. We need to drop out of coroutine + * context for executing it. + * + * Case 2: we are outside coroutine context, but command has + * QCO_COROUTINE. Can't actually happen, because we get here + * outside coroutine context only when executing a command + * out of band, and OOB commands never have QCO_COROUTINE. + */ + assert(!oob && qemu_in_coroutine() && !(cmd->options & QCO_COROUTINE)); + + QmpDispatchBH data = { + .cur_mon = cur_mon, + .cmd = cmd, + .args = args, + .ret = &ret, + .errp = &err, + .co = qemu_coroutine_self(), + }; + aio_bh_schedule_oneshot(qemu_get_aio_context(), do_qmp_dispatch_bh, + &data); + qemu_coroutine_yield(); + } qobject_unref(args); if (err) { /* or assert(!ret) after reviewing all handlers: */ diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c index d0f9a1d3e3..58c65b5052 100644 --- a/qapi/qmp-registry.c +++ b/qapi/qmp-registry.c @@ -20,6 +20,9 @@ void qmp_register_command(QmpCommandList *cmds, const char *name, { QmpCommand *cmd = g_malloc0(sizeof(*cmd)); + /* QCO_COROUTINE and QCO_ALLOW_OOB are incompatible for now */ + assert(!((options & QCO_COROUTINE) && (options & QCO_ALLOW_OOB))); + cmd->name = name; cmd->fn = fn; cmd->enabled = true; diff --git a/util/aio-posix.c b/util/aio-posix.c index 280f27bb99..30f5354b1e 100644 --- a/util/aio-posix.c +++ b/util/aio-posix.c @@ -15,6 +15,7 @@ #include "qemu/osdep.h" #include "block/block.h" +#include "qemu/main-loop.h" #include "qemu/rcu.h" #include "qemu/rcu_queue.h" #include "qemu/sockets.h" @@ -558,8 +559,13 @@ bool aio_poll(AioContext *ctx, bool blocking) * There cannot be two concurrent aio_poll calls for the same AioContext (or * an aio_poll concurrent with a GSource prepare/check/dispatch callback). * We rely on this below to avoid slow locked accesses to ctx->notify_me. + * + * aio_poll() may only be called in the AioContext's thread. iohandler_ctx + * is special in that it runs in the main thread, but that thread's context + * is qemu_aio_context. */ - assert(in_aio_context_home_thread(ctx)); + assert(in_aio_context_home_thread(ctx == iohandler_get_aio_context() ? + qemu_get_aio_context() : ctx)); qemu_lockcnt_inc(&ctx->list_lock); -- cgit 1.4.1