[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: |
Markus Armbruster |
Subject: |
Re: [PATCH v7 08/13] qapi: Add a 'coroutine' flag for commands |
Date: |
Mon, 28 Sep 2020 10:23:33 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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.
[PATCH v7 09/13] qmp: Move dispatcher to a coroutine, Kevin Wolf, 2020/09/09
[PATCH v7 11/13] util/async: Add aio_co_reschedule_self(), Kevin Wolf, 2020/09/09
[PATCH v7 10/13] hmp: Add support for coroutine command handlers, Kevin Wolf, 2020/09/09
[PATCH v7 12/13] block: Add bdrv_co_move_to_aio_context(), Kevin Wolf, 2020/09/09