* [PATCH 0/4] qmp: Optionally run handlers in coroutines
@ 2020-01-09 18:35 Kevin Wolf
2020-01-09 18:35 ` [PATCH 1/4] qapi: Add a 'coroutine' flag for commands Kevin Wolf
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Kevin Wolf @ 2020-01-09 18:35 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, marcandre.lureau, armbru, qemu-block
Some QMP command handlers can block the main loop for a relatively long
time, for example because they perform some I/O. This is quite nasty.
Allowing such handlers to run in a coroutine where they can yield (and
therefore release the BQL) while waiting for an event such as I/O
completion solves the problem.
This series adds the infrastructure to allow this and switches
block_resize to run in a coroutine as a first example.
This is an alternative solution to Marc-André's "monitor: add
asynchronous command type" series.
Kevin Wolf (4):
qapi: Add a 'coroutine' flag for commands
block: Mark 'block_resize' as coroutine
vl: Initialise main loop earlier
qmp: Move dispatcher to a coroutine
qapi/block-core.json | 3 +-
tests/qapi-schema/qapi-schema-test.json | 1 +
docs/devel/qapi-code-gen.txt | 4 ++
include/qapi/qmp/dispatch.h | 3 +
monitor/monitor-internal.h | 5 +-
monitor/monitor.c | 24 ++++---
monitor/qmp.c | 83 ++++++++++++++++---------
qapi/qmp-dispatch.c | 38 ++++++++++-
tests/test-qmp-cmds.c | 4 ++
vl.c | 10 +--
scripts/qapi/commands.py | 17 +++--
scripts/qapi/doc.py | 2 +-
scripts/qapi/expr.py | 4 +-
scripts/qapi/introspect.py | 2 +-
scripts/qapi/schema.py | 9 ++-
tests/qapi-schema/qapi-schema-test.out | 2 +
tests/qapi-schema/test-qapi.py | 7 ++-
17 files changed, 155 insertions(+), 63 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/4] qapi: Add a 'coroutine' flag for commands 2020-01-09 18:35 [PATCH 0/4] qmp: Optionally run handlers in coroutines Kevin Wolf @ 2020-01-09 18:35 ` Kevin Wolf 2020-01-10 19:00 ` Eric Blake 2020-01-09 18:35 ` [PATCH 2/4] block: Mark 'block_resize' as coroutine Kevin Wolf ` (3 subsequent siblings) 4 siblings, 1 reply; 11+ messages in thread From: Kevin Wolf @ 2020-01-09 18:35 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, marcandre.lureau, armbru, qemu-block This patch adds a new 'coroutine' flag to QMP command definitions that tells the QMP dispatcher than the command handler is safe to be run in a coroutine. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qapi-schema/qapi-schema-test.json | 1 + docs/devel/qapi-code-gen.txt | 4 ++++ include/qapi/qmp/dispatch.h | 1 + tests/test-qmp-cmds.c | 4 ++++ scripts/qapi/commands.py | 17 +++++++++++------ scripts/qapi/doc.py | 2 +- scripts/qapi/expr.py | 4 ++-- scripts/qapi/introspect.py | 2 +- scripts/qapi/schema.py | 9 ++++++--- tests/qapi-schema/qapi-schema-test.out | 2 ++ tests/qapi-schema/test-qapi.py | 7 ++++--- 11 files changed, 37 insertions(+), 16 deletions(-) diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 9abf175fe0..55f596dbaa 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -147,6 +147,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/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt index 45c93a43cc..753f6711d3 100644 --- a/docs/devel/qapi-code-gen.txt +++ b/docs/devel/qapi-code-gen.txt @@ -457,6 +457,7 @@ Syntax: '*gen': false, '*allow-oob': true, '*allow-preconfig': true, + '*coroutine': true, '*if': COND, '*features': FEATURES } @@ -581,6 +582,9 @@ 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. + The optional 'if' member specifies a conditional. See "Configuring the schema" below for more on this. diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 9aa426a398..d6ce9efc8e 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -24,6 +24,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/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c index 27b0afe55a..e2f71e42a1 100644 --- a/tests/test-qmp-cmds.c +++ b/tests/test-qmp-cmds.c @@ -34,6 +34,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); diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py index ab98e504f3..97e51401f1 100644 --- a/scripts/qapi/commands.py +++ b/scripts/qapi/commands.py @@ -15,6 +15,7 @@ See the COPYING file in the top-level directory. from qapi.common import * from qapi.gen import QAPIGenCCode, QAPISchemaModularCVisitor, ifcontext +from typing import List def gen_command_decl(name, arg_type, boxed, ret_type): @@ -194,8 +195,9 @@ out: return ret -def gen_register_command(name, success_response, allow_oob, allow_preconfig): - options = [] +def gen_register_command(name: str, success_response: bool, allow_oob: bool, + allow_preconfig: bool, coroutine: bool) -> str: + options = [] # type: List[str] if not success_response: options += ['QCO_NO_SUCCESS_RESP'] @@ -203,18 +205,20 @@ 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'] - options = " | ".join(options) + options_str = " | ".join(options) ret = mcgen(''' qmp_register_command(cmds, "%(name)s", qmp_marshal_%(c_name)s, %(opts)s); ''', name=name, c_name=c_name(name), - opts=options) + opts=options_str) return ret @@ -278,7 +282,7 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds); def visit_command(self, name, info, ifcond, arg_type, ret_type, gen, success_response, boxed, allow_oob, allow_preconfig, - features): + coroutine, features): if not gen: return # FIXME: If T is a user-defined type, the user is responsible @@ -296,7 +300,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/doc.py b/scripts/qapi/doc.py index 6f1c17f71f..8b6978c81e 100644 --- a/scripts/qapi/doc.py +++ b/scripts/qapi/doc.py @@ -265,7 +265,7 @@ class QAPISchemaGenDocVisitor(QAPISchemaVisitor): def visit_command(self, name, info, ifcond, arg_type, ret_type, gen, success_response, boxed, allow_oob, allow_preconfig, - features): + coroutine, features): doc = self.cur_doc self._gen.add(texi_msg('Command', doc, ifcond, texi_arguments(doc, diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index d7a289eded..9dbfa3edf0 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -89,7 +89,7 @@ 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) @@ -344,7 +344,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 b3a463dd8b..8a296a69d6 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -212,7 +212,7 @@ const QLitObject %(c_name)s = %(c_string)s; def visit_command(self, name, info, ifcond, arg_type, ret_type, gen, success_response, boxed, allow_oob, allow_preconfig, - features): + coroutine, features): 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 cf0045f34e..753bf233a6 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -128,7 +128,7 @@ class QAPISchemaVisitor(object): def visit_command(self, name, info, ifcond, arg_type, ret_type, gen, success_response, boxed, allow_oob, allow_preconfig, - features): + coroutine, features): pass def visit_event(self, name, info, ifcond, arg_type, boxed): @@ -678,7 +678,7 @@ class QAPISchemaCommand(QAPISchemaEntity): def __init__(self, name, info, doc, ifcond, arg_type, ret_type, gen, success_response, boxed, allow_oob, allow_preconfig, - features): + coroutine, features): QAPISchemaEntity.__init__(self, name, info, doc, ifcond, features) assert not arg_type or isinstance(arg_type, str) assert not ret_type or isinstance(ret_type, str) @@ -691,6 +691,7 @@ class QAPISchemaCommand(QAPISchemaEntity): self.boxed = boxed self.allow_oob = allow_oob self.allow_preconfig = allow_preconfig + self.coroutine = coroutine def check(self, schema): QAPISchemaEntity.check(self, schema) @@ -732,7 +733,7 @@ class QAPISchemaCommand(QAPISchemaEntity): self.arg_type, self.ret_type, self.gen, self.success_response, self.boxed, self.allow_oob, - self.allow_preconfig, + self.allow_preconfig, self.coroutine, self.features) @@ -1014,6 +1015,7 @@ class QAPISchema(object): 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 = expr.get('features', []) if isinstance(data, OrderedDict): @@ -1025,6 +1027,7 @@ class QAPISchema(object): self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, data, rets, gen, success_response, boxed, allow_oob, allow_preconfig, + coroutine, self._make_features(features, info))) def _def_event(self, expr, info, doc): diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 3660e75a48..771e736675 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -217,6 +217,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 bad14edb47..7a8e65188d 100755 --- a/tests/qapi-schema/test-qapi.py +++ b/tests/qapi-schema/test-qapi.py @@ -70,12 +70,13 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): def visit_command(self, name, info, ifcond, arg_type, ret_type, gen, success_response, boxed, allow_oob, allow_preconfig, - features): + coroutine, features): 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) -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] qapi: Add a 'coroutine' flag for commands 2020-01-09 18:35 ` [PATCH 1/4] qapi: Add a 'coroutine' flag for commands Kevin Wolf @ 2020-01-10 19:00 ` Eric Blake 2020-01-13 10:46 ` Kevin Wolf 0 siblings, 1 reply; 11+ messages in thread From: Eric Blake @ 2020-01-10 19:00 UTC (permalink / raw) To: Kevin Wolf, qemu-devel; +Cc: marcandre.lureau, armbru, qemu-block On 1/9/20 12:35 PM, Kevin Wolf wrote: > This patch adds a new 'coroutine' flag to QMP command definitions that > tells the QMP dispatcher than the command handler is safe to be run in a s/than/that/ > coroutine. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > tests/qapi-schema/qapi-schema-test.json | 1 + > docs/devel/qapi-code-gen.txt | 4 ++++ > include/qapi/qmp/dispatch.h | 1 + > tests/test-qmp-cmds.c | 4 ++++ > scripts/qapi/commands.py | 17 +++++++++++------ > scripts/qapi/doc.py | 2 +- > scripts/qapi/expr.py | 4 ++-- > scripts/qapi/introspect.py | 2 +- > scripts/qapi/schema.py | 9 ++++++--- > tests/qapi-schema/qapi-schema-test.out | 2 ++ > tests/qapi-schema/test-qapi.py | 7 ++++--- > 11 files changed, 37 insertions(+), 16 deletions(-) > > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json > index 9abf175fe0..55f596dbaa 100644 > --- a/tests/qapi-schema/qapi-schema-test.json > +++ b/tests/qapi-schema/qapi-schema-test.json > @@ -147,6 +147,7 @@ > 'returns': 'UserDefTwo' } > > { 'command': 'cmd-success-response', 'data': {}, 'success-response': false } > +{ 'command': 'coroutine_cmd', 'data': {}, 'coroutine': true } Not user-visible (it's the testsuite), but why not follow our naming convention of 'coroutine-cmd'? > +++ b/docs/devel/qapi-code-gen.txt > @@ -457,6 +457,7 @@ Syntax: > '*gen': false, > '*allow-oob': true, > '*allow-preconfig': true, > + '*coroutine': true, > '*if': COND, > '*features': FEATURES } > > @@ -581,6 +582,9 @@ 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. > + > The optional 'if' member specifies a conditional. See "Configuring Maybe "The optional 'coroutine' member tells..." for symmetry with the next paragraph. > +++ b/scripts/qapi/commands.py > @@ -15,6 +15,7 @@ See the COPYING file in the top-level directory. > > from qapi.common import * > from qapi.gen import QAPIGenCCode, QAPISchemaModularCVisitor, ifcontext > +from typing import List > > > def gen_command_decl(name, arg_type, boxed, ret_type): > @@ -194,8 +195,9 @@ out: > return ret > > > -def gen_register_command(name, success_response, allow_oob, allow_preconfig): > - options = [] > +def gen_register_command(name: str, success_response: bool, allow_oob: bool, > + allow_preconfig: bool, coroutine: bool) -> str: > + options = [] # type: List[str] Aha - now that we require python 3, you're going to exploit it ;) > +++ b/scripts/qapi/introspect.py > @@ -212,7 +212,7 @@ const QLitObject %(c_name)s = %(c_string)s; > > def visit_command(self, name, info, ifcond, arg_type, ret_type, gen, > success_response, boxed, allow_oob, allow_preconfig, > - features): > + coroutine, features): > 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), I'm assuming the new flag is internal only, and doesn't affect behavior seen by the user, and thus nothing to change in the introspection output. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] qapi: Add a 'coroutine' flag for commands 2020-01-10 19:00 ` Eric Blake @ 2020-01-13 10:46 ` Kevin Wolf 0 siblings, 0 replies; 11+ messages in thread From: Kevin Wolf @ 2020-01-13 10:46 UTC (permalink / raw) To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, qemu-block, armbru Am 10.01.2020 um 20:00 hat Eric Blake geschrieben: > On 1/9/20 12:35 PM, Kevin Wolf wrote: > > This patch adds a new 'coroutine' flag to QMP command definitions that > > tells the QMP dispatcher than the command handler is safe to be run in a > > s/than/that/ Thanks. If this remains the only change, I hope Markus can fix it while applying the patch. > > coroutine. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > tests/qapi-schema/qapi-schema-test.json | 1 + > > docs/devel/qapi-code-gen.txt | 4 ++++ > > include/qapi/qmp/dispatch.h | 1 + > > tests/test-qmp-cmds.c | 4 ++++ > > scripts/qapi/commands.py | 17 +++++++++++------ > > scripts/qapi/doc.py | 2 +- > > scripts/qapi/expr.py | 4 ++-- > > scripts/qapi/introspect.py | 2 +- > > scripts/qapi/schema.py | 9 ++++++--- > > tests/qapi-schema/qapi-schema-test.out | 2 ++ > > tests/qapi-schema/test-qapi.py | 7 ++++--- > > 11 files changed, 37 insertions(+), 16 deletions(-) > > > > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json > > index 9abf175fe0..55f596dbaa 100644 > > --- a/tests/qapi-schema/qapi-schema-test.json > > +++ b/tests/qapi-schema/qapi-schema-test.json > > @@ -147,6 +147,7 @@ > > 'returns': 'UserDefTwo' } > > { 'command': 'cmd-success-response', 'data': {}, 'success-response': false } > > +{ 'command': 'coroutine_cmd', 'data': {}, 'coroutine': true } > > Not user-visible (it's the testsuite), but why not follow our naming > convention of 'coroutine-cmd'? I just copied and modified the simplest example from a few lines above: { 'command': 'user_def_cmd', 'data': {} } The command names in the test schema seem to follow no particular style, there are even some CamelCaseCommands. But if I have to respin, I can change it. > > +++ b/docs/devel/qapi-code-gen.txt > > @@ -457,6 +457,7 @@ Syntax: > > '*gen': false, > > '*allow-oob': true, > > '*allow-preconfig': true, > > + '*coroutine': true, > > '*if': COND, > > '*features': FEATURES } > > @@ -581,6 +582,9 @@ 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. > > + > > The optional 'if' member specifies a conditional. See "Configuring > > Maybe "The optional 'coroutine' member tells..." for symmetry with the next > paragraph. Starting with 'Member ...' seems to be what is done for most other options. I phrased it this way specifically for consistency. > > +++ b/scripts/qapi/commands.py > > @@ -15,6 +15,7 @@ See the COPYING file in the top-level directory. > > from qapi.common import * > > from qapi.gen import QAPIGenCCode, QAPISchemaModularCVisitor, ifcontext > > +from typing import List > > def gen_command_decl(name, arg_type, boxed, ret_type): > > @@ -194,8 +195,9 @@ out: > > return ret > > -def gen_register_command(name, success_response, allow_oob, allow_preconfig): > > - options = [] > > +def gen_register_command(name: str, success_response: bool, allow_oob: bool, > > + allow_preconfig: bool, coroutine: bool) -> str: > > + options = [] # type: List[str] > > Aha - now that we require python 3, you're going to exploit it ;) Of course. :-) I was hoping that I could get the type checker to tell me if I forgot to change one of the callers, but that doesn't really work until we add type annotations to the callers as well. I'm going to send a separate series to do a little more about type checking. > > +++ b/scripts/qapi/introspect.py > > @@ -212,7 +212,7 @@ const QLitObject %(c_name)s = %(c_string)s; > > def visit_command(self, name, info, ifcond, arg_type, ret_type, gen, > > success_response, boxed, allow_oob, allow_preconfig, > > - features): > > + coroutine, features): > > 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), > > I'm assuming the new flag is internal only, and doesn't affect behavior seen > by the user, and thus nothing to change in the introspection output. Yes. The result would hopefully be visible to the user (the guest doesn't hang any more where it used to hang), but that's just a bug fix and nothing that would require a change in the way a client uses QMP. Kevin ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/4] block: Mark 'block_resize' as coroutine 2020-01-09 18:35 [PATCH 0/4] qmp: Optionally run handlers in coroutines Kevin Wolf 2020-01-09 18:35 ` [PATCH 1/4] qapi: Add a 'coroutine' flag for commands Kevin Wolf @ 2020-01-09 18:35 ` Kevin Wolf 2020-01-13 16:56 ` Stefan Hajnoczi 2020-01-09 18:35 ` [PATCH 3/4] vl: Initialise main loop earlier Kevin Wolf ` (2 subsequent siblings) 4 siblings, 1 reply; 11+ messages in thread From: Kevin Wolf @ 2020-01-09 18:35 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, marcandre.lureau, armbru, qemu-block block_resize is safe to run in a coroutine, so use it as an example for the new 'coroutine': true annotation in the QAPI schema. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index 7ff5e5edaf..1dbb2a9901 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1341,7 +1341,8 @@ { 'command': 'block_resize', 'data': { '*device': 'str', '*node-name': 'str', - 'size': 'int' } } + 'size': 'int' }, + 'coroutine': true } ## # @NewImageMode: -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] block: Mark 'block_resize' as coroutine 2020-01-09 18:35 ` [PATCH 2/4] block: Mark 'block_resize' as coroutine Kevin Wolf @ 2020-01-13 16:56 ` Stefan Hajnoczi 2020-01-13 17:10 ` Kevin Wolf 0 siblings, 1 reply; 11+ messages in thread From: Stefan Hajnoczi @ 2020-01-13 16:56 UTC (permalink / raw) To: Kevin Wolf; +Cc: marcandre.lureau, qemu-devel, qemu-block, armbru [-- Attachment #1: Type: text/plain, Size: 858 bytes --] On Thu, Jan 09, 2020 at 07:35:43PM +0100, Kevin Wolf wrote: > block_resize is safe to run in a coroutine, so use it as an example for > the new 'coroutine': true annotation in the QAPI schema. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > qapi/block-core.json | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 7ff5e5edaf..1dbb2a9901 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1341,7 +1341,8 @@ > { 'command': 'block_resize', > 'data': { '*device': 'str', > '*node-name': 'str', > - 'size': 'int' } } > + 'size': 'int' }, > + 'coroutine': true } > > ## > # @NewImageMode: coroutine_fn is missing on blockdev.c:void qmp_block_resize(bool has_device, const char *device, [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] block: Mark 'block_resize' as coroutine 2020-01-13 16:56 ` Stefan Hajnoczi @ 2020-01-13 17:10 ` Kevin Wolf 2020-01-14 16:30 ` Stefan Hajnoczi 0 siblings, 1 reply; 11+ messages in thread From: Kevin Wolf @ 2020-01-13 17:10 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: marcandre.lureau, qemu-devel, qemu-block, armbru [-- Attachment #1: Type: text/plain, Size: 1082 bytes --] Am 13.01.2020 um 17:56 hat Stefan Hajnoczi geschrieben: > On Thu, Jan 09, 2020 at 07:35:43PM +0100, Kevin Wolf wrote: > > block_resize is safe to run in a coroutine, so use it as an example for > > the new 'coroutine': true annotation in the QAPI schema. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > qapi/block-core.json | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > index 7ff5e5edaf..1dbb2a9901 100644 > > --- a/qapi/block-core.json > > +++ b/qapi/block-core.json > > @@ -1341,7 +1341,8 @@ > > { 'command': 'block_resize', > > 'data': { '*device': 'str', > > '*node-name': 'str', > > - 'size': 'int' } } > > + 'size': 'int' }, > > + 'coroutine': true } > > > > ## > > # @NewImageMode: > > coroutine_fn is missing on > blockdev.c:void qmp_block_resize(bool has_device, const char *device, It wouldn't even be true until after patch 4. Should I reorder the patches so I can add coroutine_fn? Kevin [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] block: Mark 'block_resize' as coroutine 2020-01-13 17:10 ` Kevin Wolf @ 2020-01-14 16:30 ` Stefan Hajnoczi 0 siblings, 0 replies; 11+ messages in thread From: Stefan Hajnoczi @ 2020-01-14 16:30 UTC (permalink / raw) To: Kevin Wolf; +Cc: marcandre.lureau, qemu-devel, qemu-block, armbru [-- Attachment #1: Type: text/plain, Size: 1220 bytes --] On Mon, Jan 13, 2020 at 06:10:09PM +0100, Kevin Wolf wrote: > Am 13.01.2020 um 17:56 hat Stefan Hajnoczi geschrieben: > > On Thu, Jan 09, 2020 at 07:35:43PM +0100, Kevin Wolf wrote: > > > block_resize is safe to run in a coroutine, so use it as an example for > > > the new 'coroutine': true annotation in the QAPI schema. > > > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > --- > > > qapi/block-core.json | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > > index 7ff5e5edaf..1dbb2a9901 100644 > > > --- a/qapi/block-core.json > > > +++ b/qapi/block-core.json > > > @@ -1341,7 +1341,8 @@ > > > { 'command': 'block_resize', > > > 'data': { '*device': 'str', > > > '*node-name': 'str', > > > - 'size': 'int' } } > > > + 'size': 'int' }, > > > + 'coroutine': true } > > > > > > ## > > > # @NewImageMode: > > > > coroutine_fn is missing on > > blockdev.c:void qmp_block_resize(bool has_device, const char *device, > > It wouldn't even be true until after patch 4. Should I reorder the > patches so I can add coroutine_fn? Yes, please. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/4] vl: Initialise main loop earlier 2020-01-09 18:35 [PATCH 0/4] qmp: Optionally run handlers in coroutines Kevin Wolf 2020-01-09 18:35 ` [PATCH 1/4] qapi: Add a 'coroutine' flag for commands Kevin Wolf 2020-01-09 18:35 ` [PATCH 2/4] block: Mark 'block_resize' as coroutine Kevin Wolf @ 2020-01-09 18:35 ` Kevin Wolf 2020-01-09 18:35 ` [PATCH 4/4] qmp: Move dispatcher to a coroutine Kevin Wolf 2020-01-13 7:00 ` [PATCH 0/4] qmp: Optionally run handlers in coroutines Marc-André Lureau 4 siblings, 0 replies; 11+ messages in thread From: Kevin Wolf @ 2020-01-09 18:35 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, marcandre.lureau, armbru, qemu-block We want to be able to use qemu_aio_context in the monitor initialisation. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- vl.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/vl.c b/vl.c index 86474a55c9..4c79a00857 100644 --- a/vl.c +++ b/vl.c @@ -2903,6 +2903,11 @@ int main(int argc, char **argv, char **envp) runstate_init(); precopy_infrastructure_init(); postcopy_infrastructure_init(); + + if (qemu_init_main_loop(&main_loop_err)) { + error_report_err(main_loop_err); + exit(1); + } monitor_init_globals(); if (qcrypto_init(&err) < 0) { @@ -3817,11 +3822,6 @@ int main(int argc, char **argv, char **envp) qemu_unlink_pidfile_notifier.notify = qemu_unlink_pidfile; qemu_add_exit_notifier(&qemu_unlink_pidfile_notifier); - if (qemu_init_main_loop(&main_loop_err)) { - error_report_err(main_loop_err); - exit(1); - } - #ifdef CONFIG_SECCOMP olist = qemu_find_opts_err("sandbox", NULL); if (olist) { -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/4] qmp: Move dispatcher to a coroutine 2020-01-09 18:35 [PATCH 0/4] qmp: Optionally run handlers in coroutines Kevin Wolf ` (2 preceding siblings ...) 2020-01-09 18:35 ` [PATCH 3/4] vl: Initialise main loop earlier Kevin Wolf @ 2020-01-09 18:35 ` Kevin Wolf 2020-01-13 7:00 ` [PATCH 0/4] qmp: Optionally run handlers in coroutines Marc-André Lureau 4 siblings, 0 replies; 11+ messages in thread From: Kevin Wolf @ 2020-01-09 18:35 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, marcandre.lureau, armbru, qemu-block 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 <kwolf@redhat.com> --- include/qapi/qmp/dispatch.h | 2 + monitor/monitor-internal.h | 5 ++- monitor/monitor.c | 24 +++++++---- monitor/qmp.c | 83 +++++++++++++++++++++++-------------- qapi/qmp-dispatch.c | 38 ++++++++++++++++- 5 files changed, 111 insertions(+), 41 deletions(-) diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index d6ce9efc8e..d6d5443391 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -30,6 +30,8 @@ typedef enum QmpCommandOptions typedef struct QmpCommand { const char *name; + /* Runs in coroutine context if QCO_COROUTINE is set, except for OOB + * commands */ QmpCommandFunc *fn; QmpCommandOptions options; QTAILQ_ENTRY(QmpCommand) node; diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index d78f5ca190..7389b6a56c 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -154,7 +154,8 @@ 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_busy; extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands; extern QemuMutex monitor_lock; extern MonitorList mon_list; @@ -172,7 +173,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 12898b6448..c72763fa4e 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -53,8 +53,9 @@ 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; +bool qmp_dispatcher_co_busy; /* Protects mon_list, monitor_qapi_event_state, monitor_destroyed. */ QemuMutex monitor_lock; @@ -579,9 +580,16 @@ 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 */ + if (!atomic_mb_read(&qmp_dispatcher_co_busy)) { + aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co); + qmp_dispatcher_co = NULL; + } + + AIO_WAIT_WHILE(qemu_get_aio_context(), + (aio_bh_poll(iohandler_get_aio_context()), + atomic_mb_read(&qmp_dispatcher_co_busy))); + if (mon_iothread) { iothread_destroy(mon_iothread); mon_iothread = NULL; @@ -604,9 +612,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); + atomic_mb_set(&qmp_dispatcher_co_busy, true); + aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co); } QemuOptsList qemu_mon_opts = { diff --git a/monitor/qmp.c b/monitor/qmp.c index b67a8e7d1f..9fd66c7b97 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -133,6 +133,8 @@ 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) { Monitor *old_mon; @@ -211,43 +213,62 @@ 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(atomic_mb_read(&qmp_dispatcher_co_busy) == true); + + while (!(req_obj = monitor_qmp_requests_pop_any_with_lock())) { + /* Wait to be reentered from handle_qmp_command, or terminate if + * qmp_dispatcher_co has been reset to NULL */ + atomic_mb_set(&qmp_dispatcher_co_busy, false); + if (qmp_dispatcher_co) { + qemu_coroutine_yield(); + } + /* qmp_dispatcher_co may have changed if we yielded and were + * reentered from monitor_cleanup() */ + if (!qmp_dispatcher_co) { + return; + } + atomic_mb_set(&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); - } + 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); + if (need_resume) { + /* Pairs with the monitor_suspend() in handle_qmp_command() */ + monitor_resume(&mon->common); + } + qmp_request_free(req_obj); - /* Reschedule instead of looping so the main loop stays responsive */ - qemu_bh_schedule(qmp_dispatcher_bh); + /* Reschedule instead of looping so the main loop stays responsive */ + aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co); + qemu_coroutine_yield(); + } } static void handle_qmp_command(void *opaque, QObject *req, Error *err) @@ -308,7 +329,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 (!atomic_mb_read(&qmp_dispatcher_co_busy)) { + 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 bc264b3c9b..6ccf19f2a2 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -12,6 +12,8 @@ */ #include "qemu/osdep.h" + +#include "monitor/monitor-internal.h" #include "qapi/error.h" #include "qapi/qmp/dispatch.h" #include "qapi/qmp/qdict.h" @@ -75,6 +77,23 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob, return dict; } +typedef struct QmpDispatchBH { + QmpCommand *cmd; + QDict *args; + QObject **ret; + Error **errp; + Coroutine *co; +} QmpDispatchBH; + +static void do_qmp_dispatch_bh(void *opaque) +{ + QmpDispatchBH *data = opaque; + data->cmd->fn(data->args, data->ret, data->errp); + aio_co_wake(data->co); +} + +/* Runs outside of coroutine context for OOB commands, but in coroutine context + * for everything else. */ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request, bool allow_oob, Error **errp) { @@ -129,7 +148,22 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request, qobject_ref(args); } - cmd->fn(args, &ret, &local_err); + assert(!(oob && qemu_in_coroutine())); + if ((cmd->options & QCO_COROUTINE) || !qemu_in_coroutine()) { + cmd->fn(args, &ret, &local_err); + } else { + /* Must drop out of coroutine context for this one */ + QmpDispatchBH data = { + .cmd = cmd, + .args = args, + .ret = &ret, + .errp = &local_err, + .co = qemu_coroutine_self(), + }; + aio_bh_schedule_oneshot(qemu_get_aio_context(), do_qmp_dispatch_bh, + &data); + qemu_coroutine_yield(); + } if (local_err) { error_propagate(errp, local_err); } else if (cmd->options & QCO_NO_SUCCESS_RESP) { @@ -164,6 +198,8 @@ bool qmp_is_oob(const QDict *dict) && !qdict_haskey(dict, "execute"); } +/* Runs outside of coroutine context for OOB commands, but in coroutine context + * for everything else. */ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request, bool allow_oob) { -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 0/4] qmp: Optionally run handlers in coroutines 2020-01-09 18:35 [PATCH 0/4] qmp: Optionally run handlers in coroutines Kevin Wolf ` (3 preceding siblings ...) 2020-01-09 18:35 ` [PATCH 4/4] qmp: Move dispatcher to a coroutine Kevin Wolf @ 2020-01-13 7:00 ` Marc-André Lureau 4 siblings, 0 replies; 11+ messages in thread From: Marc-André Lureau @ 2020-01-13 7:00 UTC (permalink / raw) To: Kevin Wolf; +Cc: QEMU, open list:Block layer core, Markus Armbruster Hi On Thu, Jan 9, 2020 at 10:36 PM Kevin Wolf <kwolf@redhat.com> wrote: > > Some QMP command handlers can block the main loop for a relatively long > time, for example because they perform some I/O. This is quite nasty. > Allowing such handlers to run in a coroutine where they can yield (and > therefore release the BQL) while waiting for an event such as I/O > completion solves the problem. > > This series adds the infrastructure to allow this and switches > block_resize to run in a coroutine as a first example. > > This is an alternative solution to Marc-André's "monitor: add > asynchronous command type" series. > > Kevin Wolf (4): > qapi: Add a 'coroutine' flag for commands > block: Mark 'block_resize' as coroutine > vl: Initialise main loop earlier > qmp: Move dispatcher to a coroutine > > qapi/block-core.json | 3 +- > tests/qapi-schema/qapi-schema-test.json | 1 + > docs/devel/qapi-code-gen.txt | 4 ++ > include/qapi/qmp/dispatch.h | 3 + > monitor/monitor-internal.h | 5 +- > monitor/monitor.c | 24 ++++--- > monitor/qmp.c | 83 ++++++++++++++++--------- > qapi/qmp-dispatch.c | 38 ++++++++++- > tests/test-qmp-cmds.c | 4 ++ > vl.c | 10 +-- > scripts/qapi/commands.py | 17 +++-- > scripts/qapi/doc.py | 2 +- > scripts/qapi/expr.py | 4 +- > scripts/qapi/introspect.py | 2 +- > scripts/qapi/schema.py | 9 ++- > tests/qapi-schema/qapi-schema-test.out | 2 + > tests/qapi-schema/test-qapi.py | 7 ++- > 17 files changed, 155 insertions(+), 63 deletions(-) Series: Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> -- Marc-André Lureau ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-01-14 16:32 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-09 18:35 [PATCH 0/4] qmp: Optionally run handlers in coroutines Kevin Wolf 2020-01-09 18:35 ` [PATCH 1/4] qapi: Add a 'coroutine' flag for commands Kevin Wolf 2020-01-10 19:00 ` Eric Blake 2020-01-13 10:46 ` Kevin Wolf 2020-01-09 18:35 ` [PATCH 2/4] block: Mark 'block_resize' as coroutine Kevin Wolf 2020-01-13 16:56 ` Stefan Hajnoczi 2020-01-13 17:10 ` Kevin Wolf 2020-01-14 16:30 ` Stefan Hajnoczi 2020-01-09 18:35 ` [PATCH 3/4] vl: Initialise main loop earlier Kevin Wolf 2020-01-09 18:35 ` [PATCH 4/4] qmp: Move dispatcher to a coroutine Kevin Wolf 2020-01-13 7:00 ` [PATCH 0/4] qmp: Optionally run handlers in coroutines Marc-André Lureau
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).