Kevin Wolf <kw...@redhat.com> writes: > Am 14.09.2020 um 17:15 hat Markus Armbruster geschrieben: >> Kevin Wolf <kw...@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 <kw...@redhat.com> >> > Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com> >> > Reviewed-by: Markus Armbruster <arm...@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....@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.