* [PATCH v3 0/4] qmp: Optionally run handlers in coroutines
@ 2020-01-15 12:23 Kevin Wolf
2020-01-15 12:23 ` [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands Kevin Wolf
` (3 more replies)
0 siblings, 4 replies; 26+ messages in thread
From: Kevin Wolf @ 2020-01-15 12:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, qemu-block, armbru, marcandre.lureau, stefanha
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.
v3:
- Fix race between monitor thread and dispatcher that could schedule the
dispatcher coroutine twice if a second requests comes in before the
dispatcher can wake up [Patchew]
v2:
- Fix typo in a commit message [Eric]
- Use hyphen instead of underscore for the test command [Eric]
- Mark qmp_block_resize() as coroutine_fn [Stefan]
Kevin Wolf (4):
qapi: Add a 'coroutine' flag for commands
vl: Initialise main loop earlier
qmp: Move dispatcher to a coroutine
block: Mark 'block_resize' as 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 +-
blockdev.c | 6 +-
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 ++-
18 files changed, 158 insertions(+), 66 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands
2020-01-15 12:23 [PATCH v3 0/4] qmp: Optionally run handlers in coroutines Kevin Wolf
@ 2020-01-15 12:23 ` Kevin Wolf
2020-01-15 14:59 ` Markus Armbruster
2020-01-15 12:23 ` [PATCH v3 2/4] vl: Initialise main loop earlier Kevin Wolf
` (2 subsequent siblings)
3 siblings, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2020-01-15 12:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, qemu-block, armbru, marcandre.lureau, stefanha
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.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@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..1a850fe171 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..51bfe8bfc7 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] 26+ messages in thread
* [PATCH v3 2/4] vl: Initialise main loop earlier
2020-01-15 12:23 [PATCH v3 0/4] qmp: Optionally run handlers in coroutines Kevin Wolf
2020-01-15 12:23 ` [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands Kevin Wolf
@ 2020-01-15 12:23 ` Kevin Wolf
2020-01-15 16:26 ` Markus Armbruster
2020-01-15 12:23 ` [PATCH v3 3/4] qmp: Move dispatcher to a coroutine Kevin Wolf
2020-01-15 12:23 ` [PATCH v3 4/4] block: Mark 'block_resize' as coroutine Kevin Wolf
3 siblings, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2020-01-15 12:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, qemu-block, armbru, marcandre.lureau, stefanha
We want to be able to use qemu_aio_context in the monitor
initialisation.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@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] 26+ messages in thread
* [PATCH v3 3/4] qmp: Move dispatcher to a coroutine
2020-01-15 12:23 [PATCH v3 0/4] qmp: Optionally run handlers in coroutines Kevin Wolf
2020-01-15 12:23 ` [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands Kevin Wolf
2020-01-15 12:23 ` [PATCH v3 2/4] vl: Initialise main loop earlier Kevin Wolf
@ 2020-01-15 12:23 ` Kevin Wolf
2020-01-17 12:20 ` Markus Armbruster
2020-01-15 12:23 ` [PATCH v3 4/4] block: Mark 'block_resize' as coroutine Kevin Wolf
3 siblings, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2020-01-15 12:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, qemu-block, armbru, marcandre.lureau, stefanha
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>
Reviewed-by: Marc-André Lureau <marcandre.lureau@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..f29a8fe497 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;
+ }
+ assert(atomic_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);
- }
+ 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_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 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] 26+ messages in thread
* [PATCH v3 4/4] block: Mark 'block_resize' as coroutine
2020-01-15 12:23 [PATCH v3 0/4] qmp: Optionally run handlers in coroutines Kevin Wolf
` (2 preceding siblings ...)
2020-01-15 12:23 ` [PATCH v3 3/4] qmp: Move dispatcher to a coroutine Kevin Wolf
@ 2020-01-15 12:23 ` Kevin Wolf
2020-01-16 9:45 ` Markus Armbruster
3 siblings, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2020-01-15 12:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, qemu-block, armbru, marcandre.lureau, stefanha
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>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
qapi/block-core.json | 3 ++-
blockdev.c | 6 +++---
2 files changed, 5 insertions(+), 4 deletions(-)
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:
diff --git a/blockdev.c b/blockdev.c
index 8e029e9c01..b5e5d1e072 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
aio_context_release(aio_context);
}
-void qmp_block_resize(bool has_device, const char *device,
- bool has_node_name, const char *node_name,
- int64_t size, Error **errp)
+void coroutine_fn qmp_block_resize(bool has_device, const char *device,
+ bool has_node_name, const char *node_name,
+ int64_t size, Error **errp)
{
Error *local_err = NULL;
BlockBackend *blk = NULL;
--
2.20.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands
2020-01-15 12:23 ` [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands Kevin Wolf
@ 2020-01-15 14:59 ` Markus Armbruster
2020-01-15 15:58 ` Kevin Wolf
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2020-01-15 14:59 UTC (permalink / raw)
To: Kevin Wolf; +Cc: stefanha, marcandre.lureau, qemu-devel, qemu-block
Kevin Wolf <kwolf@redhat.com> writes:
> 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.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@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..1a850fe171 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.
Two spaces after sentence-ending period, for consistency with the rest
of the file.
As discussed in review of prior versions, coroutine-safety is an
implementation detail that should not be exposed to management
applications. Therefore, we want a flag, not a feature.
As far as I can tell, the new flag has no effect until PATCH 3 puts it
to use. That's okay.
The doc update tells us when we may say 'coroutine': true, namely when
the handler function is coroutine-safe. It doesn't quite tell us what
difference it makes, or rather will make after PATCH 3. I think it
should.
In review of a prior version, Marc-André wondered whether keeping
allow-oob and coroutine separate makes sense. Recall qapi-code-gen.txt:
An OOB-capable command handler must satisfy the following conditions:
- It terminates quickly.
- It does not invoke system calls that may block.
- It does not access guest RAM that may block when userfaultfd is
enabled for postcopy live migration.
- It takes only "fast" locks, i.e. all critical sections protected by
any lock it takes also satisfy the conditions for OOB command
handler code.
The restrictions on locking limit access to shared state. Such access
requires synchronization, but OOB commands can't take the BQL or any
other "slow" lock.
Kevin, does this rule out coroutine use?
> +
> 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]
Not sure such isolated type hints make sense. I'd welcome patches to
explore systematic use of type hints. Suggest to start with just one
module, so we can gauge effort and benefit before we jump in whole hog.
>
> 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
>
>
Some extra churn due to type hints here. Distracting. Suggest not to
mix adding type hints to existing code with feature work.
> @@ -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..51bfe8bfc7 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)
Looks sane to me.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands
2020-01-15 14:59 ` Markus Armbruster
@ 2020-01-15 15:58 ` Kevin Wolf
2020-01-16 13:00 ` Markus Armbruster
0 siblings, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2020-01-15 15:58 UTC (permalink / raw)
To: Markus Armbruster; +Cc: stefanha, marcandre.lureau, qemu-devel, qemu-block
Am 15.01.2020 um 15:59 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > 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.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@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..1a850fe171 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.
>
> Two spaces after sentence-ending period, for consistency with the rest
> of the file.
Ok.
> As discussed in review of prior versions, coroutine-safety is an
> implementation detail that should not be exposed to management
> applications. Therefore, we want a flag, not a feature.
>
> As far as I can tell, the new flag has no effect until PATCH 3 puts it
> to use. That's okay.
>
> The doc update tells us when we may say 'coroutine': true, namely when
> the handler function is coroutine-safe. It doesn't quite tell us what
> difference it makes, or rather will make after PATCH 3. I think it
> should.
Fair requirement. Can I describe it as if patch 3 were already in? That
is, the documentation says that the handler _will_ be run in a coroutine
rather than _may_ run it in a coroutine?
> In review of a prior version, Marc-André wondered whether keeping
> allow-oob and coroutine separate makes sense. Recall qapi-code-gen.txt:
>
> An OOB-capable command handler must satisfy the following conditions:
>
> - It terminates quickly.
> - It does not invoke system calls that may block.
> - It does not access guest RAM that may block when userfaultfd is
> enabled for postcopy live migration.
> - It takes only "fast" locks, i.e. all critical sections protected by
> any lock it takes also satisfy the conditions for OOB command
> handler code.
>
> The restrictions on locking limit access to shared state. Such access
> requires synchronization, but OOB commands can't take the BQL or any
> other "slow" lock.
>
> Kevin, does this rule out coroutine use?
Not strictly, though I also can't think of a case where you would want
to use a coroutine with these requirements.
If I understand correctly, OOB-capable commands can be run either OOB
with 'exec-oob' or like normal commands with 'execute'. If an OOB
handler is marked as coroutine-safe, 'execute' will run it in a
coroutine (and the restriction above don't apply) and 'exec-oob' will
run it outside of coroutine context.
I have no idea if we will eventually get a case where the command wants
to behave different between the two modes and actually has use for a
coroutine. I hope not.
But using two bools rather than a single enum keeps the code simple and
leaves us all options open if it turns out that we do have a use case.
> > +
> > 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]
>
> Not sure such isolated type hints make sense. I'd welcome patches to
> explore systematic use of type hints. Suggest to start with just one
> module, so we can gauge effort and benefit before we jump in whole hog.
Any documentation is better than no documentation, imho.
If you insist, I'll remove the type hints, but finding the origin of
values passed as parameters to find out what type they are is a very
common activity for me when touching the QAPI code. Doing that every
time from scratch is not a good use of my time.
> >
> > 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
> >
> >
>
> Some extra churn due to type hints here. Distracting. Suggest not to
> mix adding type hints to existing code with feature work.
If you would be open for a compromise, I could leave options
unannotated, but keep the typed parameter list.
Kevin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/4] vl: Initialise main loop earlier
2020-01-15 12:23 ` [PATCH v3 2/4] vl: Initialise main loop earlier Kevin Wolf
@ 2020-01-15 16:26 ` Markus Armbruster
0 siblings, 0 replies; 26+ messages in thread
From: Markus Armbruster @ 2020-01-15 16:26 UTC (permalink / raw)
To: Kevin Wolf; +Cc: stefanha, marcandre.lureau, qemu-devel, qemu-block
Kevin Wolf <kwolf@redhat.com> writes:
> We want to be able to use qemu_aio_context in the monitor
> initialisation.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@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) {
Can't see anything wrong with this. Doesn't mean much; every time I do
something like it, it comes back to bite me.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine
2020-01-15 12:23 ` [PATCH v3 4/4] block: Mark 'block_resize' as coroutine Kevin Wolf
@ 2020-01-16 9:45 ` Markus Armbruster
2020-01-16 10:13 ` Kevin Wolf
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2020-01-16 9:45 UTC (permalink / raw)
To: Kevin Wolf; +Cc: stefanha, marcandre.lureau, qemu-devel, qemu-block
Kevin Wolf <kwolf@redhat.com> writes:
> 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>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qapi/block-core.json | 3 ++-
> blockdev.c | 6 +++---
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> 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:
> diff --git a/blockdev.c b/blockdev.c
> index 8e029e9c01..b5e5d1e072 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
> aio_context_release(aio_context);
> }
>
> -void qmp_block_resize(bool has_device, const char *device,
> - bool has_node_name, const char *node_name,
> - int64_t size, Error **errp)
> +void coroutine_fn qmp_block_resize(bool has_device, const char *device,
> + bool has_node_name, const char *node_name,
> + int64_t size, Error **errp)
> {
> Error *local_err = NULL;
> BlockBackend *blk = NULL;
Pardon my ignorant question: what exactly makes a function a
coroutine_fn?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine
2020-01-16 9:45 ` Markus Armbruster
@ 2020-01-16 10:13 ` Kevin Wolf
2020-01-16 15:13 ` Markus Armbruster
2020-01-17 8:13 ` Markus Armbruster
0 siblings, 2 replies; 26+ messages in thread
From: Kevin Wolf @ 2020-01-16 10:13 UTC (permalink / raw)
To: Markus Armbruster; +Cc: stefanha, marcandre.lureau, qemu-devel, qemu-block
Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> > 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>
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > diff --git a/blockdev.c b/blockdev.c
> > index 8e029e9c01..b5e5d1e072 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
> > aio_context_release(aio_context);
> > }
> >
> > -void qmp_block_resize(bool has_device, const char *device,
> > - bool has_node_name, const char *node_name,
> > - int64_t size, Error **errp)
> > +void coroutine_fn qmp_block_resize(bool has_device, const char *device,
> > + bool has_node_name, const char *node_name,
> > + int64_t size, Error **errp)
> > {
> > Error *local_err = NULL;
> > BlockBackend *blk = NULL;
>
> Pardon my ignorant question: what exactly makes a function a
> coroutine_fn?
When Stefan requested adding the coroutine_fn marker, it seemed to make
sense to me because the QMP dispatcher will always call it from
coroutine context now, and being always run in coroutine context makes a
function a coroutine_fn.
However, it's also called from hmp_block_resize(), so at least for now
coroutine_fn is actually wrong.
Kevin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands
2020-01-15 15:58 ` Kevin Wolf
@ 2020-01-16 13:00 ` Markus Armbruster
2020-01-16 15:02 ` Kevin Wolf
2020-01-17 7:50 ` Markus Armbruster
0 siblings, 2 replies; 26+ messages in thread
From: Markus Armbruster @ 2020-01-16 13:00 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, marcandre.lureau, qemu-devel, stefanha
Kevin Wolf <kwolf@redhat.com> writes:
> Am 15.01.2020 um 15:59 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > 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.
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > Reviewed-by: Marc-André Lureau <marcandre.lureau@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..1a850fe171 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.
>>
>> Two spaces after sentence-ending period, for consistency with the rest
>> of the file.
>
> Ok.
>
>> As discussed in review of prior versions, coroutine-safety is an
>> implementation detail that should not be exposed to management
>> applications. Therefore, we want a flag, not a feature.
>>
>> As far as I can tell, the new flag has no effect until PATCH 3 puts it
>> to use. That's okay.
>>
>> The doc update tells us when we may say 'coroutine': true, namely when
>> the handler function is coroutine-safe. It doesn't quite tell us what
>> difference it makes, or rather will make after PATCH 3. I think it
>> should.
>
> Fair requirement. Can I describe it as if patch 3 were already in? That
> is, the documentation says that the handler _will_ be run in a coroutine
> rather than _may_ run it in a coroutine?
Your choice. If you choose to pretend PATCH 3 was in, have your commit
message point that out.
>> In review of a prior version, Marc-André wondered whether keeping
>> allow-oob and coroutine separate makes sense. Recall qapi-code-gen.txt:
>>
>> An OOB-capable command handler must satisfy the following conditions:
>>
>> - It terminates quickly.
>> - It does not invoke system calls that may block.
>> - It does not access guest RAM that may block when userfaultfd is
>> enabled for postcopy live migration.
>> - It takes only "fast" locks, i.e. all critical sections protected by
>> any lock it takes also satisfy the conditions for OOB command
>> handler code.
>>
>> The restrictions on locking limit access to shared state. Such access
>> requires synchronization, but OOB commands can't take the BQL or any
>> other "slow" lock.
>>
>> Kevin, does this rule out coroutine use?
>
> Not strictly, though I also can't think of a case where you would want
> to use a coroutine with these requirements.
>
> If I understand correctly, OOB-capable commands can be run either OOB
> with 'exec-oob' or like normal commands with 'execute'.
Correct.
> If an OOB
> handler is marked as coroutine-safe, 'execute' will run it in a
> coroutine (and the restriction above don't apply) and 'exec-oob' will
> run it outside of coroutine context.
Let me convince myself you're right.
Cases before this series:
(exec) execute, allow-oob does not matter
Run in main loop bottom half monitor_qmp_bh_dispatcher(), outside
coroutine context, scheduled by handle_qmp_command()
(err1) exec-oob, QMP_CAPABILITY_OOB off, allow-oob does not matter
Error
(err2) exec-oob, QMP_CAPABILITY_OOB on, allow-oob: false
Error
(exec-oob) exec-oob, QMP_CAPABILITY_OOB on, allow-oob: true
Run in iothread / handle_qmp_command(), outside coroutine context
Peeking ahead to PATCH 3... it split cases (exec):
(exec-co): execute, allow-oob does not matter, coroutine: true
Run in main loop coroutine qmp_dispatcher_co(), in coroutine context,
woken up by handle_qmp_command()
(exec): execute, allow-oob does not matter, coroutine: false
Run in main loop bottom half do_qmp_dispatch_bh(), outside coroutine
context, scheduled by qmp_dispatcher_co()
It appears not to touch case exec-oob. Thus, coroutine: true has no
effect when the command is executed with exec-oob.
Looks like you're right :)
> I have no idea if we will eventually get a case where the command wants
> to behave different between the two modes and actually has use for a
> coroutine. I hope not.
>
> But using two bools rather than a single enum keeps the code simple and
> leaves us all options open if it turns out that we do have a use case.
I can buy the argument "the two are conceptually orthogonal, although we
don't haven't found a use for one of the four cases".
Let's review the four combinations of the two flags once more:
* allow-oob: false, coroutine: false
Handler runs in main loop, outside coroutine context. Okay.
* allow-oob: false, coroutine: true
Handler runs in main loop, in coroutine context. Okay.
* allow-oob: true, coroutine: false
Handler may run in main loop or in iothread, outside coroutine
context. Okay.
* allow-oob: true, coroutine: true
Handler may run (in main loop, in coroutine context) or (in iothread,
outside coroutine context). This "in coroutine context only with
execute, not with exec-oob" behavior is a bit surprising.
We could document it, noting that it may change to always run in
coroutine context. Or we simply reject this case as "not
implemented". Since we have no uses, I'm leaning towards reject. One
fewer case to test then.
>> > +
>> > 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]
One more: this is a PEP 484 type hint. With Python 3, we can use PEP
526 instead:
options: List[str] = []
I think we should.
>>
>> Not sure such isolated type hints make sense. I'd welcome patches to
>> explore systematic use of type hints. Suggest to start with just one
>> module, so we can gauge effort and benefit before we jump in whole hog.
>
> Any documentation is better than no documentation, imho.
Inconsistent or grossly incomplete documentation can be worse than no
documentation.
> If you insist, I'll remove the type hints, but finding the origin of
> values passed as parameters to find out what type they are is a very
> common activity for me when touching the QAPI code. Doing that every
> time from scratch is not a good use of my time.
Understand, and I therefore support the idea of exploring use of type
hints. I'd rather not mix it up with other work, though.
>> >
>> > 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
>> >
>> >
>>
>> Some extra churn due to type hints here. Distracting. Suggest not to
>> mix adding type hints to existing code with feature work.
>
> If you would be open for a compromise, I could leave options
> unannotated, but keep the typed parameter list.
Keeping just the function annotation is much less distracting. I can't
reject that with a "separate patches for separate things" argument.
I'd still prefer not to, because:
* If we do add systematic type hints in the near future, then delaying
this one until then shouldn't hurt your productivity.
* If we don't, this lone one won't help your productivity much, but
it'll look out of place.
I really don't want us to add type hints as we go, because such
open-ended "while we touch it anyway" conversions take forever and a
day. Maximizes the confusion integral over time.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands
2020-01-16 13:00 ` Markus Armbruster
@ 2020-01-16 15:02 ` Kevin Wolf
2020-01-17 7:57 ` Markus Armbruster
2020-01-17 7:50 ` Markus Armbruster
1 sibling, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2020-01-16 15:02 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-block, marcandre.lureau, qemu-devel, stefanha
Am 16.01.2020 um 14:00 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> > I have no idea if we will eventually get a case where the command wants
> > to behave different between the two modes and actually has use for a
> > coroutine. I hope not.
> >
> > But using two bools rather than a single enum keeps the code simple and
> > leaves us all options open if it turns out that we do have a use case.
>
> I can buy the argument "the two are conceptually orthogonal, although we
> don't haven't found a use for one of the four cases".
>
> Let's review the four combinations of the two flags once more:
>
> * allow-oob: false, coroutine: false
>
> Handler runs in main loop, outside coroutine context. Okay.
>
> * allow-oob: false, coroutine: true
>
> Handler runs in main loop, in coroutine context. Okay.
>
> * allow-oob: true, coroutine: false
>
> Handler may run in main loop or in iothread, outside coroutine
> context. Okay.
>
> * allow-oob: true, coroutine: true
>
> Handler may run (in main loop, in coroutine context) or (in iothread,
> outside coroutine context). This "in coroutine context only with
> execute, not with exec-oob" behavior is a bit surprising.
>
> We could document it, noting that it may change to always run in
> coroutine context. Or we simply reject this case as "not
> implemented". Since we have no uses, I'm leaning towards reject. One
> fewer case to test then.
What would be the right mode of rejecting it?
I assume we should catch it somewhere in the QAPI generator (where?) and
then just assert in the C code that both flags aren't set at the same
time?
> >> > @@ -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]
>
> One more: this is a PEP 484 type hint. With Python 3, we can use PEP
> 526 instead:
>
> options: List[str] = []
>
> I think we should.
This requires Python 3.6, unfortunately. The minimum requirement for
building QEMU is 3.5.
> >> Some extra churn due to type hints here. Distracting. Suggest not to
> >> mix adding type hints to existing code with feature work.
> >
> > If you would be open for a compromise, I could leave options
> > unannotated, but keep the typed parameter list.
>
> Keeping just the function annotation is much less distracting. I can't
> reject that with a "separate patches for separate things" argument.
>
> I'd still prefer not to, because:
>
> * If we do add systematic type hints in the near future, then delaying
> this one until then shouldn't hurt your productivity.
>
> * If we don't, this lone one won't help your productivity much, but
> it'll look out of place.
>
> I really don't want us to add type hints as we go, because such
> open-ended "while we touch it anyway" conversions take forever and a
> day. Maximizes the confusion integral over time.
I think it's a first time that I'm asked not to document things, but
I'll remove them.
Kevin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine
2020-01-16 10:13 ` Kevin Wolf
@ 2020-01-16 15:13 ` Markus Armbruster
2020-01-16 15:23 ` Kevin Wolf
2020-01-17 8:13 ` Markus Armbruster
1 sibling, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2020-01-16 15:13 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, marcandre.lureau, qemu-devel, stefanha
Kevin Wolf <kwolf@redhat.com> writes:
> Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> > 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>
>> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>> > diff --git a/blockdev.c b/blockdev.c
>> > index 8e029e9c01..b5e5d1e072 100644
>> > --- a/blockdev.c
>> > +++ b/blockdev.c
>> > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
>> > aio_context_release(aio_context);
>> > }
>> >
>> > -void qmp_block_resize(bool has_device, const char *device,
>> > - bool has_node_name, const char *node_name,
>> > - int64_t size, Error **errp)
>> > +void coroutine_fn qmp_block_resize(bool has_device, const char *device,
>> > + bool has_node_name, const char *node_name,
>> > + int64_t size, Error **errp)
>> > {
>> > Error *local_err = NULL;
>> > BlockBackend *blk = NULL;
>>
>> Pardon my ignorant question: what exactly makes a function a
>> coroutine_fn?
>
> When Stefan requested adding the coroutine_fn marker, it seemed to make
> sense to me because the QMP dispatcher will always call it from
> coroutine context now, and being always run in coroutine context makes a
> function a coroutine_fn.
>
> However, it's also called from hmp_block_resize(), so at least for now
> coroutine_fn is actually wrong.
This answers the question when we mark a function a coroutine_fn. I
meant to ask what conditions the function itself must satisfy to be
eligible for this mark.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine
2020-01-16 15:13 ` Markus Armbruster
@ 2020-01-16 15:23 ` Kevin Wolf
2020-01-17 5:44 ` Markus Armbruster
0 siblings, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2020-01-16 15:23 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-block, marcandre.lureau, qemu-devel, stefanha
Am 16.01.2020 um 16:13 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> > 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>
> >> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> >> > diff --git a/blockdev.c b/blockdev.c
> >> > index 8e029e9c01..b5e5d1e072 100644
> >> > --- a/blockdev.c
> >> > +++ b/blockdev.c
> >> > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
> >> > aio_context_release(aio_context);
> >> > }
> >> >
> >> > -void qmp_block_resize(bool has_device, const char *device,
> >> > - bool has_node_name, const char *node_name,
> >> > - int64_t size, Error **errp)
> >> > +void coroutine_fn qmp_block_resize(bool has_device, const char *device,
> >> > + bool has_node_name, const char *node_name,
> >> > + int64_t size, Error **errp)
> >> > {
> >> > Error *local_err = NULL;
> >> > BlockBackend *blk = NULL;
> >>
> >> Pardon my ignorant question: what exactly makes a function a
> >> coroutine_fn?
> >
> > When Stefan requested adding the coroutine_fn marker, it seemed to make
> > sense to me because the QMP dispatcher will always call it from
> > coroutine context now, and being always run in coroutine context makes a
> > function a coroutine_fn.
> >
> > However, it's also called from hmp_block_resize(), so at least for now
> > coroutine_fn is actually wrong.
>
> This answers the question when we mark a function a coroutine_fn. I
> meant to ask what conditions the function itself must satisfy to be
> eligible for this mark.
The requirement is actually not about the function itself, it's about
the callers, as stated above.
But being a coroutine_fn allows the function to call other functions
that only work in coroutine context (other coroutine_fns). In the end
the reason why a function only works in coroutine context is usually
that it (or any other coroutine_fns called by it) could yield, which
obviously doesn't work outside of coroutine contest.
Kevin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine
2020-01-16 15:23 ` Kevin Wolf
@ 2020-01-17 5:44 ` Markus Armbruster
2020-01-17 9:24 ` Kevin Wolf
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2020-01-17 5:44 UTC (permalink / raw)
To: Kevin Wolf; +Cc: stefanha, marcandre.lureau, qemu-devel, qemu-block
Kevin Wolf <kwolf@redhat.com> writes:
> Am 16.01.2020 um 16:13 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf <kwolf@redhat.com> writes:
>> >> > 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>
>> >> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> >> > diff --git a/blockdev.c b/blockdev.c
>> >> > index 8e029e9c01..b5e5d1e072 100644
>> >> > --- a/blockdev.c
>> >> > +++ b/blockdev.c
>> >> > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
>> >> > aio_context_release(aio_context);
>> >> > }
>> >> >
>> >> > -void qmp_block_resize(bool has_device, const char *device,
>> >> > - bool has_node_name, const char *node_name,
>> >> > - int64_t size, Error **errp)
>> >> > +void coroutine_fn qmp_block_resize(bool has_device, const char *device,
>> >> > + bool has_node_name, const char *node_name,
>> >> > + int64_t size, Error **errp)
>> >> > {
>> >> > Error *local_err = NULL;
>> >> > BlockBackend *blk = NULL;
>> >>
>> >> Pardon my ignorant question: what exactly makes a function a
>> >> coroutine_fn?
>> >
>> > When Stefan requested adding the coroutine_fn marker, it seemed to make
>> > sense to me because the QMP dispatcher will always call it from
>> > coroutine context now, and being always run in coroutine context makes a
>> > function a coroutine_fn.
>> >
>> > However, it's also called from hmp_block_resize(), so at least for now
>> > coroutine_fn is actually wrong.
>>
>> This answers the question when we mark a function a coroutine_fn. I
>> meant to ask what conditions the function itself must satisfy to be
>> eligible for this mark.
>
> The requirement is actually not about the function itself, it's about
> the callers, as stated above.
>
> But being a coroutine_fn allows the function to call other functions
> that only work in coroutine context (other coroutine_fns). In the end
> the reason why a function only works in coroutine context is usually
> that it (or any other coroutine_fns called by it) could yield, which
> obviously doesn't work outside of coroutine contest.
Thanks.
I think "being always run in coroutine context makes a function a
coroutine_fn" is inaccurate. It's "calling a coroutine_fn without
switching to coroutine context first when not already in coroutine
context". The induction terminates at basic coroutine_fn like
qemu_coroutine_yield().
Pertinent:
/**
* Return whether or not currently inside a coroutine
*
* This can be used to write functions that work both when in coroutine context
* and when not in coroutine context. Note that such functions cannot use the
* coroutine_fn annotation since they work outside coroutine context.
*/
bool qemu_in_coroutine(void);
For qmp_block_resize(), it's used like this, in bdrv_truncate():
if (qemu_in_coroutine()) {
/* Fast-path if already in coroutine context */
bdrv_truncate_co_entry(&tco);
} else {
co = qemu_coroutine_create(bdrv_truncate_co_entry, &tco);
bdrv_coroutine_enter(child->bs, co);
BDRV_POLL_WHILE(child->bs, tco.ret == NOT_DONE);
}
where bdrv_truncate_co_entry() is a coroutine_fn.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands
2020-01-16 13:00 ` Markus Armbruster
2020-01-16 15:02 ` Kevin Wolf
@ 2020-01-17 7:50 ` Markus Armbruster
1 sibling, 0 replies; 26+ messages in thread
From: Markus Armbruster @ 2020-01-17 7:50 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, marcandre.lureau, qemu-devel, stefanha
Markus Armbruster <armbru@redhat.com> writes:
> Kevin Wolf <kwolf@redhat.com> writes:
>
>> Am 15.01.2020 um 15:59 hat Markus Armbruster geschrieben:
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>
>>> > 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.
>>> >
>>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> > Reviewed-by: Marc-André Lureau <marcandre.lureau@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..1a850fe171 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.
>>>
>>> Two spaces after sentence-ending period, for consistency with the rest
>>> of the file.
>>
>> Ok.
>>
>>> As discussed in review of prior versions, coroutine-safety is an
>>> implementation detail that should not be exposed to management
>>> applications. Therefore, we want a flag, not a feature.
>>>
>>> As far as I can tell, the new flag has no effect until PATCH 3 puts it
>>> to use. That's okay.
>>>
>>> The doc update tells us when we may say 'coroutine': true, namely when
>>> the handler function is coroutine-safe. It doesn't quite tell us what
>>> difference it makes, or rather will make after PATCH 3. I think it
>>> should.
>>
>> Fair requirement. Can I describe it as if patch 3 were already in? That
>> is, the documentation says that the handler _will_ be run in a coroutine
>> rather than _may_ run it in a coroutine?
>
> Your choice. If you choose to pretend PATCH 3 was in, have your commit
> message point that out.
>
>>> In review of a prior version, Marc-André wondered whether keeping
>>> allow-oob and coroutine separate makes sense. Recall qapi-code-gen.txt:
>>>
>>> An OOB-capable command handler must satisfy the following conditions:
>>>
>>> - It terminates quickly.
>>> - It does not invoke system calls that may block.
>>> - It does not access guest RAM that may block when userfaultfd is
>>> enabled for postcopy live migration.
>>> - It takes only "fast" locks, i.e. all critical sections protected by
>>> any lock it takes also satisfy the conditions for OOB command
>>> handler code.
>>>
>>> The restrictions on locking limit access to shared state. Such access
>>> requires synchronization, but OOB commands can't take the BQL or any
>>> other "slow" lock.
>>>
>>> Kevin, does this rule out coroutine use?
>>
>> Not strictly, though I also can't think of a case where you would want
>> to use a coroutine with these requirements.
>>
>> If I understand correctly, OOB-capable commands can be run either OOB
>> with 'exec-oob' or like normal commands with 'execute'.
>
> Correct.
>
>> If an OOB
>> handler is marked as coroutine-safe, 'execute' will run it in a
>> coroutine (and the restriction above don't apply) and 'exec-oob' will
>> run it outside of coroutine context.
>
> Let me convince myself you're right.
>
> Cases before this series:
>
> (exec) execute, allow-oob does not matter
>
> Run in main loop bottom half monitor_qmp_bh_dispatcher(), outside
> coroutine context, scheduled by handle_qmp_command()
>
> (err1) exec-oob, QMP_CAPABILITY_OOB off, allow-oob does not matter
>
> Error
>
> (err2) exec-oob, QMP_CAPABILITY_OOB on, allow-oob: false
>
> Error
>
> (exec-oob) exec-oob, QMP_CAPABILITY_OOB on, allow-oob: true
>
> Run in iothread / handle_qmp_command(), outside coroutine context
>
> Peeking ahead to PATCH 3... it split cases (exec):
>
> (exec-co): execute, allow-oob does not matter, coroutine: true
>
> Run in main loop coroutine qmp_dispatcher_co(), in coroutine context,
> woken up by handle_qmp_command()
>
> (exec): execute, allow-oob does not matter, coroutine: false
>
> Run in main loop bottom half do_qmp_dispatch_bh(), outside coroutine
> context, scheduled by qmp_dispatcher_co()
>
> It appears not to touch case exec-oob. Thus, coroutine: true has no
> effect when the command is executed with exec-oob.
Looking at PATCH 3 again, I got temporarily confused again. Let me
spell things out even more, to improve my chances at staying not
confused...
To effect the split of (exec), you rewrite bottom half
monitor_qmp_bh_dispatcher() as coroutine monitor_qmp_dispatcher_co(),
then have do_qmp_dispatch() either call the the handler directly, or
schedule it to run in a bottom half. Cases:
(exec-co): handle_qmp_command() sends the command to coroutine
monitor_qmp_dispatcher_co(), which calls monitor_qmp_dispatch(), which
runs the handler, in coroutine context, in the main loop.
(exec): Likewise, except monitor_qmp_dispatch() schedules the handler to
run in a bottom half, outside coroutine context, in the main loop.
(exec-oob): handle_qmp_command() runs monitor_qmp_dispatch(), which runs
the handler, outside coroutine context, in the iothread.
> Looks like you're right :)
[...]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands
2020-01-16 15:02 ` Kevin Wolf
@ 2020-01-17 7:57 ` Markus Armbruster
2020-01-17 9:40 ` Kevin Wolf
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2020-01-17 7:57 UTC (permalink / raw)
To: Kevin Wolf; +Cc: stefanha, marcandre.lureau, qemu-devel, qemu-block
Kevin Wolf <kwolf@redhat.com> writes:
> Am 16.01.2020 um 14:00 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> > I have no idea if we will eventually get a case where the command wants
>> > to behave different between the two modes and actually has use for a
>> > coroutine. I hope not.
>> >
>> > But using two bools rather than a single enum keeps the code simple and
>> > leaves us all options open if it turns out that we do have a use case.
>>
>> I can buy the argument "the two are conceptually orthogonal, although we
>> don't haven't found a use for one of the four cases".
>>
>> Let's review the four combinations of the two flags once more:
>>
>> * allow-oob: false, coroutine: false
>>
>> Handler runs in main loop, outside coroutine context. Okay.
>>
>> * allow-oob: false, coroutine: true
>>
>> Handler runs in main loop, in coroutine context. Okay.
>>
>> * allow-oob: true, coroutine: false
>>
>> Handler may run in main loop or in iothread, outside coroutine
>> context. Okay.
>>
>> * allow-oob: true, coroutine: true
>>
>> Handler may run (in main loop, in coroutine context) or (in iothread,
>> outside coroutine context). This "in coroutine context only with
>> execute, not with exec-oob" behavior is a bit surprising.
>>
>> We could document it, noting that it may change to always run in
>> coroutine context. Or we simply reject this case as "not
>> implemented". Since we have no uses, I'm leaning towards reject. One
>> fewer case to test then.
>
> What would be the right mode of rejecting it?
>
> I assume we should catch it somewhere in the QAPI generator (where?) and
check_flags() in expr.py?
> then just assert in the C code that both flags aren't set at the same
> time?
I think you already do, in do_qmp_dispatch():
assert(!(oob && qemu_in_coroutine()));
Not sure that's the best spot. Let's see when I review PATCH 3.
>> >> > @@ -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]
>>
>> One more: this is a PEP 484 type hint. With Python 3, we can use PEP
>> 526 instead:
>>
>> options: List[str] = []
>>
>> I think we should.
>
> This requires Python 3.6, unfortunately. The minimum requirement for
> building QEMU is 3.5.
*Sigh*
>> >> Some extra churn due to type hints here. Distracting. Suggest not to
>> >> mix adding type hints to existing code with feature work.
>> >
>> > If you would be open for a compromise, I could leave options
>> > unannotated, but keep the typed parameter list.
>>
>> Keeping just the function annotation is much less distracting. I can't
>> reject that with a "separate patches for separate things" argument.
>>
>> I'd still prefer not to, because:
>>
>> * If we do add systematic type hints in the near future, then delaying
>> this one until then shouldn't hurt your productivity.
>>
>> * If we don't, this lone one won't help your productivity much, but
>> it'll look out of place.
>>
>> I really don't want us to add type hints as we go, because such
>> open-ended "while we touch it anyway" conversions take forever and a
>> day. Maximizes the confusion integral over time.
>
> I think it's a first time that I'm asked not to document things, but
> I'll remove them.
I'm asking you not to mix documenting existing code with adding a
new feature to it in the same patch.
Hopefully, that won't lead to the documentation getting dropped for
good. That would be sad.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine
2020-01-16 10:13 ` Kevin Wolf
2020-01-16 15:13 ` Markus Armbruster
@ 2020-01-17 8:13 ` Markus Armbruster
2020-01-17 9:13 ` Kevin Wolf
1 sibling, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2020-01-17 8:13 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, marcandre.lureau, qemu-devel, stefanha
Kevin Wolf <kwolf@redhat.com> writes:
> Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> > 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>
>> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>> > diff --git a/blockdev.c b/blockdev.c
>> > index 8e029e9c01..b5e5d1e072 100644
>> > --- a/blockdev.c
>> > +++ b/blockdev.c
>> > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
>> > aio_context_release(aio_context);
>> > }
>> >
>> > -void qmp_block_resize(bool has_device, const char *device,
>> > - bool has_node_name, const char *node_name,
>> > - int64_t size, Error **errp)
>> > +void coroutine_fn qmp_block_resize(bool has_device, const char *device,
>> > + bool has_node_name, const char *node_name,
>> > + int64_t size, Error **errp)
>> > {
>> > Error *local_err = NULL;
>> > BlockBackend *blk = NULL;
>>
>> Pardon my ignorant question: what exactly makes a function a
>> coroutine_fn?
>
> When Stefan requested adding the coroutine_fn marker, it seemed to make
> sense to me because the QMP dispatcher will always call it from
> coroutine context now, and being always run in coroutine context makes a
> function a coroutine_fn.
>
> However, it's also called from hmp_block_resize(), so at least for now
> coroutine_fn is actually wrong.
With the coroutine_fn dropped:
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Any plans to make more QMP commands 'coroutine': true?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine
2020-01-17 8:13 ` Markus Armbruster
@ 2020-01-17 9:13 ` Kevin Wolf
0 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2020-01-17 9:13 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-block, marcandre.lureau, qemu-devel, stefanha
Am 17.01.2020 um 09:13 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> > 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>
> >> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> >> > diff --git a/blockdev.c b/blockdev.c
> >> > index 8e029e9c01..b5e5d1e072 100644
> >> > --- a/blockdev.c
> >> > +++ b/blockdev.c
> >> > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
> >> > aio_context_release(aio_context);
> >> > }
> >> >
> >> > -void qmp_block_resize(bool has_device, const char *device,
> >> > - bool has_node_name, const char *node_name,
> >> > - int64_t size, Error **errp)
> >> > +void coroutine_fn qmp_block_resize(bool has_device, const char *device,
> >> > + bool has_node_name, const char *node_name,
> >> > + int64_t size, Error **errp)
> >> > {
> >> > Error *local_err = NULL;
> >> > BlockBackend *blk = NULL;
> >>
> >> Pardon my ignorant question: what exactly makes a function a
> >> coroutine_fn?
> >
> > When Stefan requested adding the coroutine_fn marker, it seemed to make
> > sense to me because the QMP dispatcher will always call it from
> > coroutine context now, and being always run in coroutine context makes a
> > function a coroutine_fn.
> >
> > However, it's also called from hmp_block_resize(), so at least for now
> > coroutine_fn is actually wrong.
>
> With the coroutine_fn dropped:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Any plans to make more QMP commands 'coroutine': true?
No immediate plans from me. Myself, I was interested in block_resize
because without the conversion, Max wanted me to jump through some hoops
with a bdrv_truncate() fix to make sure that it wouldn't block the guest
during block_resize.
Of course, apart from that, there is Marc-André's long-standing
screendump bug that will finally have all of the required
infrastructure (and I think I saw a patch on the list already).
Kevin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine
2020-01-17 5:44 ` Markus Armbruster
@ 2020-01-17 9:24 ` Kevin Wolf
2020-01-17 10:46 ` Markus Armbruster
0 siblings, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2020-01-17 9:24 UTC (permalink / raw)
To: Markus Armbruster; +Cc: stefanha, marcandre.lureau, qemu-devel, qemu-block
Am 17.01.2020 um 06:44 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 16.01.2020 um 16:13 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >>
> >> > Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben:
> >> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> >> > 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>
> >> >> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >
> >> >> > diff --git a/blockdev.c b/blockdev.c
> >> >> > index 8e029e9c01..b5e5d1e072 100644
> >> >> > --- a/blockdev.c
> >> >> > +++ b/blockdev.c
> >> >> > @@ -3161,9 +3161,9 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
> >> >> > aio_context_release(aio_context);
> >> >> > }
> >> >> >
> >> >> > -void qmp_block_resize(bool has_device, const char *device,
> >> >> > - bool has_node_name, const char *node_name,
> >> >> > - int64_t size, Error **errp)
> >> >> > +void coroutine_fn qmp_block_resize(bool has_device, const char *device,
> >> >> > + bool has_node_name, const char *node_name,
> >> >> > + int64_t size, Error **errp)
> >> >> > {
> >> >> > Error *local_err = NULL;
> >> >> > BlockBackend *blk = NULL;
> >> >>
> >> >> Pardon my ignorant question: what exactly makes a function a
> >> >> coroutine_fn?
> >> >
> >> > When Stefan requested adding the coroutine_fn marker, it seemed to make
> >> > sense to me because the QMP dispatcher will always call it from
> >> > coroutine context now, and being always run in coroutine context makes a
> >> > function a coroutine_fn.
> >> >
> >> > However, it's also called from hmp_block_resize(), so at least for now
> >> > coroutine_fn is actually wrong.
> >>
> >> This answers the question when we mark a function a coroutine_fn. I
> >> meant to ask what conditions the function itself must satisfy to be
> >> eligible for this mark.
> >
> > The requirement is actually not about the function itself, it's about
> > the callers, as stated above.
> >
> > But being a coroutine_fn allows the function to call other functions
> > that only work in coroutine context (other coroutine_fns). In the end
> > the reason why a function only works in coroutine context is usually
> > that it (or any other coroutine_fns called by it) could yield, which
> > obviously doesn't work outside of coroutine contest.
>
> Thanks.
>
> I think "being always run in coroutine context makes a function a
> coroutine_fn" is inaccurate. It's "calling a coroutine_fn without
> switching to coroutine context first when not already in coroutine
> context". The induction terminates at basic coroutine_fn like
> qemu_coroutine_yield().
I think we would tend to mark things as coroutine_fn even if they don't
call other coroutine_fns (yet), but would be allowed to. But this is now
really splitting hairs.
Maybe I should just have referred to the documentation in the source:
/**
* Mark a function that executes in coroutine context
*
* Functions that execute in coroutine context cannot be called directly from
* normal functions. In the future it would be nice to enable compiler or
* static checker support for catching such errors. This annotation might make
* it possible and in the meantime it serves as documentation.
*
* For example:
*
* static void coroutine_fn foo(void) {
* ....
* }
*/
#define coroutine_fn
Kevin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands
2020-01-17 7:57 ` Markus Armbruster
@ 2020-01-17 9:40 ` Kevin Wolf
2020-01-17 10:43 ` Markus Armbruster
0 siblings, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2020-01-17 9:40 UTC (permalink / raw)
To: Markus Armbruster; +Cc: stefanha, marcandre.lureau, qemu-devel, qemu-block
Am 17.01.2020 um 08:57 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 16.01.2020 um 14:00 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> > I have no idea if we will eventually get a case where the command wants
> >> > to behave different between the two modes and actually has use for a
> >> > coroutine. I hope not.
> >> >
> >> > But using two bools rather than a single enum keeps the code simple and
> >> > leaves us all options open if it turns out that we do have a use case.
> >>
> >> I can buy the argument "the two are conceptually orthogonal, although we
> >> don't haven't found a use for one of the four cases".
> >>
> >> Let's review the four combinations of the two flags once more:
> >>
> >> * allow-oob: false, coroutine: false
> >>
> >> Handler runs in main loop, outside coroutine context. Okay.
> >>
> >> * allow-oob: false, coroutine: true
> >>
> >> Handler runs in main loop, in coroutine context. Okay.
> >>
> >> * allow-oob: true, coroutine: false
> >>
> >> Handler may run in main loop or in iothread, outside coroutine
> >> context. Okay.
> >>
> >> * allow-oob: true, coroutine: true
> >>
> >> Handler may run (in main loop, in coroutine context) or (in iothread,
> >> outside coroutine context). This "in coroutine context only with
> >> execute, not with exec-oob" behavior is a bit surprising.
> >>
> >> We could document it, noting that it may change to always run in
> >> coroutine context. Or we simply reject this case as "not
> >> implemented". Since we have no uses, I'm leaning towards reject. One
> >> fewer case to test then.
> >
> > What would be the right mode of rejecting it?
> >
> > I assume we should catch it somewhere in the QAPI generator (where?) and
>
> check_flags() in expr.py?
Looks like the right place, thanks.
> > then just assert in the C code that both flags aren't set at the same
> > time?
>
> I think you already do, in do_qmp_dispatch():
>
> assert(!(oob && qemu_in_coroutine()));
>
> Not sure that's the best spot. Let's see when I review PATCH 3.
This asserts that exec-oob handlers aren't executed in coroutine
context. It doesn't assert that the handler doesn't have QCO_COROUTINE
and QCO_ALLOW_OOB set at the same time.
> >> >> > @@ -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]
> >>
> >> One more: this is a PEP 484 type hint. With Python 3, we can use PEP
> >> 526 instead:
> >>
> >> options: List[str] = []
> >>
> >> I think we should.
> >
> > This requires Python 3.6, unfortunately. The minimum requirement for
> > building QEMU is 3.5.
>
> *Sigh*
One of the reasons why I would have preferred 3.6 as the minimum, but
our policy says that Debian oldstabe is still relevant for another two
years. *shrug*
Kevin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands
2020-01-17 9:40 ` Kevin Wolf
@ 2020-01-17 10:43 ` Markus Armbruster
2020-01-17 11:08 ` Kevin Wolf
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2020-01-17 10:43 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, marcandre.lureau, qemu-devel, stefanha
Kevin Wolf <kwolf@redhat.com> writes:
> Am 17.01.2020 um 08:57 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > Am 16.01.2020 um 14:00 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf <kwolf@redhat.com> writes:
>> >> > I have no idea if we will eventually get a case where the command wants
>> >> > to behave different between the two modes and actually has use for a
>> >> > coroutine. I hope not.
>> >> >
>> >> > But using two bools rather than a single enum keeps the code simple and
>> >> > leaves us all options open if it turns out that we do have a use case.
>> >>
>> >> I can buy the argument "the two are conceptually orthogonal, although we
>> >> don't haven't found a use for one of the four cases".
>> >>
>> >> Let's review the four combinations of the two flags once more:
>> >>
>> >> * allow-oob: false, coroutine: false
>> >>
>> >> Handler runs in main loop, outside coroutine context. Okay.
>> >>
>> >> * allow-oob: false, coroutine: true
>> >>
>> >> Handler runs in main loop, in coroutine context. Okay.
>> >>
>> >> * allow-oob: true, coroutine: false
>> >>
>> >> Handler may run in main loop or in iothread, outside coroutine
>> >> context. Okay.
>> >>
>> >> * allow-oob: true, coroutine: true
>> >>
>> >> Handler may run (in main loop, in coroutine context) or (in iothread,
>> >> outside coroutine context). This "in coroutine context only with
>> >> execute, not with exec-oob" behavior is a bit surprising.
>> >>
>> >> We could document it, noting that it may change to always run in
>> >> coroutine context. Or we simply reject this case as "not
>> >> implemented". Since we have no uses, I'm leaning towards reject. One
>> >> fewer case to test then.
>> >
>> > What would be the right mode of rejecting it?
>> >
>> > I assume we should catch it somewhere in the QAPI generator (where?) and
>>
>> check_flags() in expr.py?
>
> Looks like the right place, thanks.
>
>> > then just assert in the C code that both flags aren't set at the same
>> > time?
>>
>> I think you already do, in do_qmp_dispatch():
>>
>> assert(!(oob && qemu_in_coroutine()));
>>
>> Not sure that's the best spot. Let's see when I review PATCH 3.
>
> This asserts that exec-oob handlers aren't executed in coroutine
> context. It doesn't assert that the handler doesn't have QCO_COROUTINE
> and QCO_ALLOW_OOB set at the same time.
Asserting this explicitly can't hurt. qmp_register_command()?
>> >> >> > @@ -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]
>> >>
>> >> One more: this is a PEP 484 type hint. With Python 3, we can use PEP
>> >> 526 instead:
>> >>
>> >> options: List[str] = []
>> >>
>> >> I think we should.
>> >
>> > This requires Python 3.6, unfortunately. The minimum requirement for
>> > building QEMU is 3.5.
>>
>> *Sigh*
>
> One of the reasons why I would have preferred 3.6 as the minimum, but
> our policy says that Debian oldstabe is still relevant for another two
> years. *shrug*
3.5 EOL is scheduled for 2020-09-13.
https://devguide.python.org/#status-of-python-branches
Whether Debian can support it beyond that date seems doubtful.
For another reason to want 3.6, see
[PATCH] qapi: Fix code generation with Python 3.5
Message-Id: <20200116202558.31473-1-armbru@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine
2020-01-17 9:24 ` Kevin Wolf
@ 2020-01-17 10:46 ` Markus Armbruster
0 siblings, 0 replies; 26+ messages in thread
From: Markus Armbruster @ 2020-01-17 10:46 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, marcandre.lureau, qemu-devel, stefanha
Kevin Wolf <kwolf@redhat.com> writes:
> Am 17.01.2020 um 06:44 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > Am 16.01.2020 um 16:13 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf <kwolf@redhat.com> writes:
>> >>
>> >> > Am 16.01.2020 um 10:45 hat Markus Armbruster geschrieben:
[...]
>> >> >> Pardon my ignorant question: what exactly makes a function a
>> >> >> coroutine_fn?
>> >> >
>> >> > When Stefan requested adding the coroutine_fn marker, it seemed to make
>> >> > sense to me because the QMP dispatcher will always call it from
>> >> > coroutine context now, and being always run in coroutine context makes a
>> >> > function a coroutine_fn.
>> >> >
>> >> > However, it's also called from hmp_block_resize(), so at least for now
>> >> > coroutine_fn is actually wrong.
>> >>
>> >> This answers the question when we mark a function a coroutine_fn. I
>> >> meant to ask what conditions the function itself must satisfy to be
>> >> eligible for this mark.
>> >
>> > The requirement is actually not about the function itself, it's about
>> > the callers, as stated above.
>> >
>> > But being a coroutine_fn allows the function to call other functions
>> > that only work in coroutine context (other coroutine_fns). In the end
>> > the reason why a function only works in coroutine context is usually
>> > that it (or any other coroutine_fns called by it) could yield, which
>> > obviously doesn't work outside of coroutine contest.
>>
>> Thanks.
>>
>> I think "being always run in coroutine context makes a function a
>> coroutine_fn" is inaccurate. It's "calling a coroutine_fn without
>> switching to coroutine context first when not already in coroutine
>> context". The induction terminates at basic coroutine_fn like
>> qemu_coroutine_yield().
>
> I think we would tend to mark things as coroutine_fn even if they don't
> call other coroutine_fns (yet), but would be allowed to. But this is now
> really splitting hairs.
Your hair-splitting is my education :)
> Maybe I should just have referred to the documentation in the source:
>
> /**
> * Mark a function that executes in coroutine context
> *
> * Functions that execute in coroutine context cannot be called directly from
> * normal functions. In the future it would be nice to enable compiler or
> * static checker support for catching such errors. This annotation might make
> * it possible and in the meantime it serves as documentation.
> *
> * For example:
> *
> * static void coroutine_fn foo(void) {
> * ....
> * }
> */
> #define coroutine_fn
I had read that, of course, but it didn't quite enlighten me, so I
asked.
Perhaps it would have if it said "Mark a function that expects to run in
coroutine context".
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands
2020-01-17 10:43 ` Markus Armbruster
@ 2020-01-17 11:08 ` Kevin Wolf
0 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2020-01-17 11:08 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-block, marcandre.lureau, qemu-devel, stefanha
Am 17.01.2020 um 11:43 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 17.01.2020 um 08:57 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >>
> >> > Am 16.01.2020 um 14:00 hat Markus Armbruster geschrieben:
> >> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> >> > I have no idea if we will eventually get a case where the command wants
> >> >> > to behave different between the two modes and actually has use for a
> >> >> > coroutine. I hope not.
> >> >> >
> >> >> > But using two bools rather than a single enum keeps the code simple and
> >> >> > leaves us all options open if it turns out that we do have a use case.
> >> >>
> >> >> I can buy the argument "the two are conceptually orthogonal, although we
> >> >> don't haven't found a use for one of the four cases".
> >> >>
> >> >> Let's review the four combinations of the two flags once more:
> >> >>
> >> >> * allow-oob: false, coroutine: false
> >> >>
> >> >> Handler runs in main loop, outside coroutine context. Okay.
> >> >>
> >> >> * allow-oob: false, coroutine: true
> >> >>
> >> >> Handler runs in main loop, in coroutine context. Okay.
> >> >>
> >> >> * allow-oob: true, coroutine: false
> >> >>
> >> >> Handler may run in main loop or in iothread, outside coroutine
> >> >> context. Okay.
> >> >>
> >> >> * allow-oob: true, coroutine: true
> >> >>
> >> >> Handler may run (in main loop, in coroutine context) or (in iothread,
> >> >> outside coroutine context). This "in coroutine context only with
> >> >> execute, not with exec-oob" behavior is a bit surprising.
> >> >>
> >> >> We could document it, noting that it may change to always run in
> >> >> coroutine context. Or we simply reject this case as "not
> >> >> implemented". Since we have no uses, I'm leaning towards reject. One
> >> >> fewer case to test then.
> >> >
> >> > What would be the right mode of rejecting it?
> >> >
> >> > I assume we should catch it somewhere in the QAPI generator (where?) and
> >>
> >> check_flags() in expr.py?
> >
> > Looks like the right place, thanks.
> >
> >> > then just assert in the C code that both flags aren't set at the same
> >> > time?
> >>
> >> I think you already do, in do_qmp_dispatch():
> >>
> >> assert(!(oob && qemu_in_coroutine()));
> >>
> >> Not sure that's the best spot. Let's see when I review PATCH 3.
> >
> > This asserts that exec-oob handlers aren't executed in coroutine
> > context. It doesn't assert that the handler doesn't have QCO_COROUTINE
> > and QCO_ALLOW_OOB set at the same time.
>
> Asserting this explicitly can't hurt. qmp_register_command()?
Yes, that's where I put it.
> >> >> >> > @@ -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]
> >> >>
> >> >> One more: this is a PEP 484 type hint. With Python 3, we can use PEP
> >> >> 526 instead:
> >> >>
> >> >> options: List[str] = []
> >> >>
> >> >> I think we should.
> >> >
> >> > This requires Python 3.6, unfortunately. The minimum requirement for
> >> > building QEMU is 3.5.
> >>
> >> *Sigh*
> >
> > One of the reasons why I would have preferred 3.6 as the minimum, but
> > our policy says that Debian oldstabe is still relevant for another two
> > years. *shrug*
>
> 3.5 EOL is scheduled for 2020-09-13.
> https://devguide.python.org/#status-of-python-branches
>
> Whether Debian can support it beyond that date seems doubtful.
You may doubt the quality of their support, but I think it's even more
doubtful that they'll do a version upgrade in oldstable.
> For another reason to want 3.6, see
> [PATCH] qapi: Fix code generation with Python 3.5
> Message-Id: <20200116202558.31473-1-armbru@redhat.com>
The release notes for 3.6 call this an implementation detail that you
shouldn't rely on. However, 3.7 guarantees the order, so I guess we can
effectively rely on it starting from 3.6.
Kevin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 3/4] qmp: Move dispatcher to a coroutine
2020-01-15 12:23 ` [PATCH v3 3/4] qmp: Move dispatcher to a coroutine Kevin Wolf
@ 2020-01-17 12:20 ` Markus Armbruster
2020-01-17 14:03 ` Kevin Wolf
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2020-01-17 12:20 UTC (permalink / raw)
To: Kevin Wolf; +Cc: stefanha, marcandre.lureau, qemu-devel, qemu-block
Kevin Wolf <kwolf@redhat.com> writes:
> 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>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@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 */
Needs an update if we outlaw 'allow-oob': true, 'coroutine': true.
> 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;
Purpose of @qmp_dispatcher_co_busy is not obvious. Could it use a
comment?
>
> /* 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);
Looks rather low-level to my (untrained!) eyes.
> + qmp_dispatcher_co = NULL;
As we'll see below, there's a subtle reason for zapping
qmp_dispatcher_co here.
> + }
> +
> + AIO_WAIT_WHILE(qemu_get_aio_context(),
> + (aio_bh_poll(iohandler_get_aio_context()),
> + atomic_mb_read(&qmp_dispatcher_co_busy)));
> +
Looks like it waits for @qmp_dispatcher_co_busy to become false. The
details are greek to me.
aio_bh_poll()'s function comment says "These are internal functions used
by the QEMU main loop." This seems to be the first call outside
util/aio-{posix,win32}.c. Hmm.
How exactly this implies the coroutine terminated I can't quite tell.
> 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);
Ignorant question: why not qemu_coroutine_enter()?
> }
>
> QemuOptsList qemu_mon_opts = {
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index b67a8e7d1f..f29a8fe497 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. */
Wing this comment, please.
Note: the precondition is asserted in do_qmp_dispatch() below.
Asserting here is impractical, because we don't know whether this is an
OOB command.
> 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);
Note for later: qmp_dispatcher_co_busy is false now.
> + 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;
> + }
!qmp_dispatcher_co asks the coroutine to terminate.
monitor_init_globals_core() sets it before scheduling the newly created
coroutine. monitor_cleanup() clears it after scheduling the non-busy
coroutine.
When asked to terminate, we don't want to yield (first conditional), and
we don't want to loop (second conditional).
> + assert(atomic_mb_read(&qmp_dispatcher_co_busy) == true);
How does qmp_dispatcher_co_busy become true again? Is it the
atomic_xchg() in handle_qmp_command() below?
> + }
Looks like the purpose of the loop above is "get request if we have
requests, else terminate if asked to, else yield".
The initial kick (in monitor_init_globals_core()) hits "else yield".
Subsequent kicks in handle_qmp_command() hit "get request".
The final kick in monitor_cleanup() hits "terminate".
Correct?
>
> - 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 we get here, we have a @req_obj.
> + aio_co_schedule(qemu_get_aio_context(), qmp_dispatcher_co);
> + qemu_coroutine_yield();
I'm confused... why do we yield here?
> +
> + 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);
Unchanged apart from indentation.
>
> - /* 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();
Does the comment need tweaking? We actually loop now, just not
immediately...
> + }
> }
>
> 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_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 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"
Ugh.
> #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. */
Wing this comment, please.
Note: the precondition is asserted in do_qmp_dispatch() above. We don't
want to assert here, because we don't want to duplicate
do_qmp_dispatch()'s computation of "execute OOB".
> QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
> bool allow_oob)
> {
Puh, I made it!
My problem with this patch isn't that I don't trust it to work. It's
that I don't trust my ability to maintain such subtle code going
forward.
Me learning more about low-level coroutine stuff should help.
Us making the code less subtle will certainly help.
Here's one idea. The way we make the coroutine terminate was faitly
hard to grasp for me. Can we use the existing communication pipe,
namely mon->qmp_requests? It's a queue of QMPRequest. A QMPRequest is
either a request object (req && !err), or an Error to be reported (!req
&& err). We could use !req && !err to mean "terminate".
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 3/4] qmp: Move dispatcher to a coroutine
2020-01-17 12:20 ` Markus Armbruster
@ 2020-01-17 14:03 ` Kevin Wolf
0 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2020-01-17 14:03 UTC (permalink / raw)
To: Markus Armbruster; +Cc: stefanha, marcandre.lureau, qemu-devel, qemu-block
Am 17.01.2020 um 13:20 hat Markus Armbruster geschrieben:
> > @@ -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);
>
> Looks rather low-level to my (untrained!) eyes.
>
> > + qmp_dispatcher_co = NULL;
>
> As we'll see below, there's a subtle reason for zapping
> qmp_dispatcher_co here.
As you complain below that the method for signalling a termination
request to the dispatcher coroutine, I think I could introduce an
additional boolean. The aio_co_schedule() can probably be simplified,
too, so that we would get something like:
if (!atomic_mb_read(&qmp_dispatcher_co_busy)) {
qmp_dispatcher_shutdown = true;
aio_co_wake(qmp_dispatcher_co);
}
Does this look better?
> > + }
> > +
> > + AIO_WAIT_WHILE(qemu_get_aio_context(),
> > + (aio_bh_poll(iohandler_get_aio_context()),
> > + atomic_mb_read(&qmp_dispatcher_co_busy)));
> > +
>
> Looks like it waits for @qmp_dispatcher_co_busy to become false. The
> details are greek to me.
>
> aio_bh_poll()'s function comment says "These are internal functions used
> by the QEMU main loop." This seems to be the first call outside
> util/aio-{posix,win32}.c. Hmm.
Much of this complication comes from the fact that the monitor runs in
iohandler_ctx, which is not the main AioContext of the main loop thread
(or any thread). This makes waiting for something in this AioContext
rather complicated because nothing wil poll that AioContext if I don't
do it in the loop condition.
I would have called aio_poll(), but it's forbidden for iohandler_ctx:
util/aio-posix.c:619: aio_poll: Assertion `in_aio_context_home_thread(ctx)' failed.
Maybe if we want to use aio_poll() instead of aio_bh_poll(), we could
special case iohandler_ctx in in_aio_context_home_thread().
> How exactly this implies the coroutine terminated I can't quite tell.
There is only one place which sets qmp_dispatcher_co_busy to false,
which is immediately before terminating for qmp_dispatcher_co == NULL.
What's probably missing is setting it to true above before waking up the
dispatcher coroutine. The atomic_mb_read() needs to be atomic_xchg()
like in handle_qmp_command().
> > 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);
>
> Ignorant question: why not qemu_coroutine_enter()?
Because the old code didn't run the BH right away. Should it? We're
pretty early in the initialisation of QEMU, but it should work as long
as we're allowed to call monitor_qmp_requests_pop_any_with_lock()
already.
> > }
> >
> > QemuOptsList qemu_mon_opts = {
> > diff --git a/monitor/qmp.c b/monitor/qmp.c
> > index b67a8e7d1f..f29a8fe497 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. */
>
> Wing this comment, please.
>
> Note: the precondition is asserted in do_qmp_dispatch() below.
> Asserting here is impractical, because we don't know whether this is an
> OOB command.
>
> > 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);
>
> Note for later: qmp_dispatcher_co_busy is false now.
>
> > + 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;
> > + }
>
> !qmp_dispatcher_co asks the coroutine to terminate.
>
> monitor_init_globals_core() sets it before scheduling the newly created
> coroutine. monitor_cleanup() clears it after scheduling the non-busy
> coroutine.
>
> When asked to terminate, we don't want to yield (first conditional), and
> we don't want to loop (second conditional).
>
> > + assert(atomic_mb_read(&qmp_dispatcher_co_busy) == true);
>
> How does qmp_dispatcher_co_busy become true again? Is it the
> atomic_xchg() in handle_qmp_command() below?
>
> > + }
>
> Looks like the purpose of the loop above is "get request if we have
> requests, else terminate if asked to, else yield".
>
> The initial kick (in monitor_init_globals_core()) hits "else yield".
>
> Subsequent kicks in handle_qmp_command() hit "get request".
>
> The final kick in monitor_cleanup() hits "terminate".
>
> Correct?
Yes.
> >
> > - 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 we get here, we have a @req_obj.
>
> > + aio_co_schedule(qemu_get_aio_context(), qmp_dispatcher_co);
> > + qemu_coroutine_yield();
>
> I'm confused... why do we yield here?
We're waiting for new requests in iohandler_ctx because that's where the
monitor has always been running and I don't want to make changes that I
don't fully understand myself (I think it means that AIO_WAIT_WHILE()
doesn't allow new monitor requests to run).
But the QMP command handler coroutine needs to execute in
qemu_aio_context so that the handlers can make progress during
AIO_WAIT_WHILE(), otherwise you get hangs.
So the QMP dispatcher moves back and forth between both contexts. One
for getting new requests, the other for executing handlers.
This clearly needs a comment.
> > +
> > + 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);
>
> Unchanged apart from indentation.
>
> >
> > - /* 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();
>
> Does the comment need tweaking? We actually loop now, just not
> immediately...
Hm, it's a loop now, but we're still rescheduling instead of directly
looping for the same reason. Suggestions how to phrase it?
> > + }
> > }
> >
> > 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_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 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"
>
> Ugh.
Actually unused. I think I removed the offender because I thought the
same, but forgot to remove the #include.
> > #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. */
>
> Wing this comment, please.
>
> Note: the precondition is asserted in do_qmp_dispatch() above. We don't
> want to assert here, because we don't want to duplicate
> do_qmp_dispatch()'s computation of "execute OOB".
>
> > QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request,
> > bool allow_oob)
> > {
>
> Puh, I made it!
>
> My problem with this patch isn't that I don't trust it to work. It's
> that I don't trust my ability to maintain such subtle code going
> forward.
>
> Me learning more about low-level coroutine stuff should help.
>
> Us making the code less subtle will certainly help.
>
> Here's one idea. The way we make the coroutine terminate was faitly
> hard to grasp for me. Can we use the existing communication pipe,
> namely mon->qmp_requests? It's a queue of QMPRequest. A QMPRequest is
> either a request object (req && !err), or an Error to be reported (!req
> && err). We could use !req && !err to mean "terminate".
I don't think this would make things any easier. All of the
synchronisation between the monitor thread and dispatcher stays, and all
of the iohandler_ctx vs. qemu_aio_context stays, too.
The only way I see to make termination a bit more obvious would be the
additional bool to request termination that I mentioned above.
Kevin
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2020-01-17 14:05 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-15 12:23 [PATCH v3 0/4] qmp: Optionally run handlers in coroutines Kevin Wolf
2020-01-15 12:23 ` [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands Kevin Wolf
2020-01-15 14:59 ` Markus Armbruster
2020-01-15 15:58 ` Kevin Wolf
2020-01-16 13:00 ` Markus Armbruster
2020-01-16 15:02 ` Kevin Wolf
2020-01-17 7:57 ` Markus Armbruster
2020-01-17 9:40 ` Kevin Wolf
2020-01-17 10:43 ` Markus Armbruster
2020-01-17 11:08 ` Kevin Wolf
2020-01-17 7:50 ` Markus Armbruster
2020-01-15 12:23 ` [PATCH v3 2/4] vl: Initialise main loop earlier Kevin Wolf
2020-01-15 16:26 ` Markus Armbruster
2020-01-15 12:23 ` [PATCH v3 3/4] qmp: Move dispatcher to a coroutine Kevin Wolf
2020-01-17 12:20 ` Markus Armbruster
2020-01-17 14:03 ` Kevin Wolf
2020-01-15 12:23 ` [PATCH v3 4/4] block: Mark 'block_resize' as coroutine Kevin Wolf
2020-01-16 9:45 ` Markus Armbruster
2020-01-16 10:13 ` Kevin Wolf
2020-01-16 15:13 ` Markus Armbruster
2020-01-16 15:23 ` Kevin Wolf
2020-01-17 5:44 ` Markus Armbruster
2020-01-17 9:24 ` Kevin Wolf
2020-01-17 10:46 ` Markus Armbruster
2020-01-17 8:13 ` Markus Armbruster
2020-01-17 9:13 ` Kevin Wolf
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.