Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands

[Markus Armbruster <armbru@redhat.com>]

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 :)

[...]