[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands |
Date: |
Fri, 17 Jan 2020 08:50:12 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Markus Armbruster <address@hidden> writes:
> Kevin Wolf <address@hidden> writes:
>
>> Am 15.01.2020 um 15:59 hat Markus Armbruster geschrieben:
>>> Kevin Wolf <address@hidden> 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 <address@hidden>
>>> > Reviewed-by: Marc-André Lureau <address@hidden>
>>> > ---
>>> > 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 :)
[...]
- [PATCH v3 0/4] qmp: Optionally run handlers in coroutines, Kevin Wolf, 2020/01/15
- [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Kevin Wolf, 2020/01/15
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Markus Armbruster, 2020/01/15
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Kevin Wolf, 2020/01/15
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Markus Armbruster, 2020/01/16
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Kevin Wolf, 2020/01/16
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Markus Armbruster, 2020/01/17
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Kevin Wolf, 2020/01/17
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Markus Armbruster, 2020/01/17
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands, Kevin Wolf, 2020/01/17
- Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands,
Markus Armbruster <=
[PATCH v3 2/4] vl: Initialise main loop earlier, Kevin Wolf, 2020/01/15
[PATCH v3 4/4] block: Mark 'block_resize' as coroutine, Kevin Wolf, 2020/01/15
- Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine, Markus Armbruster, 2020/01/16
- Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine, Kevin Wolf, 2020/01/16
- Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine, Markus Armbruster, 2020/01/16
- Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine, Kevin Wolf, 2020/01/16
- Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine, Markus Armbruster, 2020/01/17
- Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine, Kevin Wolf, 2020/01/17
- Re: [PATCH v3 4/4] block: Mark 'block_resize' as coroutine, Markus Armbruster, 2020/01/17