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

[Kevin Wolf <kwolf@redhat.com>]

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.

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

> >                                     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.

Kevin