[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 08/13] qapi: Add a 'coroutine' flag for commands
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v7 08/13] qapi: Add a 'coroutine' flag for commands |
Date: |
Fri, 25 Sep 2020 17:37:07 +0200 |
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
[PATCH v7 09/13] qmp: Move dispatcher to a coroutine, Kevin Wolf, 2020/09/09
[PATCH v7 10/13] hmp: Add support for coroutine command handlers, Kevin Wolf, 2020/09/09
[PATCH v7 11/13] util/async: Add aio_co_reschedule_self(), Kevin Wolf, 2020/09/09
[PATCH v7 12/13] block: Add bdrv_co_move_to_aio_context(), Kevin Wolf, 2020/09/09