Re: [PATCH v7 08/13] qapi: Add a 'coroutine' flag for commands

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.09.2020 um 17:15 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.
>> >
>> > The documentation of the new flag pretends that this flag is already
>> > used as intended, which it isn't yet after this patch. We'll implement
>> > this in another patch in this series.
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> > ---
>> >  docs/devel/qapi-code-gen.txt            | 12 ++++++++++++
>> >  include/qapi/qmp/dispatch.h             |  1 +
>> >  tests/test-qmp-cmds.c                   |  4 ++++
>> >  scripts/qapi/commands.py                | 10 +++++++---
>> >  scripts/qapi/doc.py                     |  2 +-
>> >  scripts/qapi/expr.py                    | 10 ++++++++--
>> >  scripts/qapi/introspect.py              |  2 +-
>> >  scripts/qapi/schema.py                  | 12 ++++++++----
>> >  tests/qapi-schema/test-qapi.py          |  7 ++++---
>> >  tests/qapi-schema/meson.build           |  1 +
>> >  tests/qapi-schema/oob-coroutine.err     |  2 ++
>> >  tests/qapi-schema/oob-coroutine.json    |  2 ++
>> >  tests/qapi-schema/oob-coroutine.out     |  0
>> >  tests/qapi-schema/qapi-schema-test.json |  1 +
>> >  tests/qapi-schema/qapi-schema-test.out  |  2 ++
>> >  15 files changed, 54 insertions(+), 14 deletions(-)
>> >  create mode 100644 tests/qapi-schema/oob-coroutine.err
>> >  create mode 100644 tests/qapi-schema/oob-coroutine.json
>> >  create mode 100644 tests/qapi-schema/oob-coroutine.out
>> >
>> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
>> > index f3e7ced212..36daa9b5f8 100644
>> > --- a/docs/devel/qapi-code-gen.txt
>> > +++ b/docs/devel/qapi-code-gen.txt
>> > @@ -472,6 +472,7 @@ Syntax:
>> >                  '*gen': false,
>> >                  '*allow-oob': true,
>> >                  '*allow-preconfig': true,
>> > +                '*coroutine': true,
>> >                  '*if': COND,
>> >                  '*features': FEATURES }
>> >  
>> > @@ -596,6 +597,17 @@ 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.
>> 
>> We need to document what exactly makes a command handler coroutine safe
>> / unsafe.  We discussed this at some length in review of PATCH v4 1/4:
>> 
>>     Message-ID: <874kwnvgad.fsf@dusky.pond.sub.org>
>>     https://lists.nongnu.org/archive/html/qemu-devel/2020-01/msg05015.html
>> 
>> I'd be willing to accept a follow-up patch, if that's more convenient
>> for you.
>
> Did we ever arrive at a conclusion for a good definition?
>
> I mean I can give a definition like "it's coroutine safe if it results
> in the right behaviour even when called in coroutine context", but
> that's not really helpful.

If an actual definition is out of reach, we should still say
*something*.  Even a mere "it's complicated" would help, because it
warns the reader he's on thin ice.

Can we give examples for "unsafe"?  You mentioned nested event loops.

> FWIW, I would also have a hard time giving a much better definition than
> this for thread safety.

For what it's worth, here's how POSIX.1-2017 defined "thread-safe":

    A thread-safe function can be safely invoked concurrently with other
    calls to the same function, or with calls to any other thread-safe
    functions, by multiple threads.  Each function defined in the System
    Interfaces volume of POSIX.1-2017 is thread-safe unless explicitly
    stated otherwise.  Examples are any "pure" function, a function
    which holds a mutex locked while it is accessing static storage, or
    objects shared among threads.

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_407

>> >                                     It defaults to false.  If it is true,
>> > +the command handler is called from coroutine context and may yield while
>> 
>> Is it *always* called from coroutine context, or could it be called
>> outside coroutine context, too?  I guess the former, thanks to PATCH 10,
>> and as long as we diligently mark HMP commands that such call QMP
>> handlers, like you do in PATCH 13.
>
> Yes, it must *always* be called from coroutine context. This is the
> reason why I included HMP after all, even though I'm really mostly just
> interested in QMP.
>
>> What's the worst than can happen when we neglect to mark such an HMP
>> command?
>
> When the command handler tries to yield and it's not in coroutine
> context, it will abort(). If it never tries to yield, I think it would
> just work - but of course, the ability to yield is the only reason why
> you would want to run a command handler in a coroutine.

Let's spell this out.  After your paragraph

    Member 'coroutine' tells the QMP dispatcher whether the command handler
    is safe to be run in a coroutine.  It defaults to false.  If it is true,
    the command handler is called from coroutine context and may yield while
    waiting for an external event (such as I/O completion) in order to avoid
    blocking the guest and other background operations.

add something like

    Since the command handler may assume coroutine-context, any callers
    other than the QMP dispatcher must also call it in coroutine
    context.  In particular, HMP commands calling such a QMP command
    handler must be marked .coroutine = true in hmp-commands.hx.

Patch ordering issue: this becomes possible only in PATCH 10.
Possible solutions:

* Add the last sentence only in PATCH 10.

* Write "HMP commands cannot call such a QMP command handler" in PATCH
  8, replace it in PATCH 10.

* Add a "TODO make that possible" line here, drop it in PATCH 10.

* Reorder patches.

I don't like the first one much.  Anyway, up to you.