Re: [PATCH v3 1/4] qapi: Add a 'coroutine' flag for commands

[Kevin Wolf]

Am 15.01.2020 um 15:59 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> 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.
> >
> > Signed-off-by: Kevin Wolf <address@hidden>
> > Reviewed-by: Marc-André Lureau <address@hidden>
> > ---
> >  tests/qapi-schema/qapi-schema-test.json |  1 +
> >  docs/devel/qapi-code-gen.txt            |  4 ++++
> >  include/qapi/qmp/dispatch.h             |  1 +
> >  tests/test-qmp-cmds.c                   |  4 ++++
> >  scripts/qapi/commands.py                | 17 +++++++++++------
> >  scripts/qapi/doc.py                     |  2 +-
> >  scripts/qapi/expr.py                    |  4 ++--
> >  scripts/qapi/introspect.py              |  2 +-
> >  scripts/qapi/schema.py                  |  9 ++++++---
> >  tests/qapi-schema/qapi-schema-test.out  |  2 ++
> >  tests/qapi-schema/test-qapi.py          |  7 ++++---
> >  11 files changed, 37 insertions(+), 16 deletions(-)
> >
> > diff --git a/tests/qapi-schema/qapi-schema-test.json 
> > b/tests/qapi-schema/qapi-schema-test.json
> > index 9abf175fe0..1a850fe171 100644
> > --- a/tests/qapi-schema/qapi-schema-test.json
> > +++ b/tests/qapi-schema/qapi-schema-test.json
> > @@ -147,6 +147,7 @@
> >    'returns': 'UserDefTwo' }
> >  
> >  { 'command': 'cmd-success-response', 'data': {}, 'success-response': false 
> > }
> > +{ 'command': 'coroutine-cmd', 'data': {}, 'coroutine': true }
> >  
> >  # Returning a non-dictionary requires a name from the whitelist
> >  { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' },
> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> > index 45c93a43cc..753f6711d3 100644
> > --- a/docs/devel/qapi-code-gen.txt
> > +++ b/docs/devel/qapi-code-gen.txt
> > @@ -457,6 +457,7 @@ Syntax:
> >                  '*gen': false,
> >                  '*allow-oob': true,
> >                  '*allow-preconfig': true,
> > +                '*coroutine': true,
> >                  '*if': COND,
> >                  '*features': FEATURES }
> >  
> > @@ -581,6 +582,9 @@ 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. It defaults to false.
> 
> Two spaces after sentence-ending period, for consistency with the rest
> of the file.

Ok.

> As discussed in review of prior versions, coroutine-safety is an
> implementation detail that should not be exposed to management
> applications.  Therefore, we want a flag, not a feature.
> 
> As far as I can tell, the new flag has no effect until PATCH 3 puts it
> to use.  That's okay.
> 
> The doc update tells us when we may say 'coroutine': true, namely when
> the handler function is coroutine-safe.  It doesn't quite tell us what
> difference it makes, or rather will make after PATCH 3.  I think it
> should.

Fair requirement. Can I describe it as if patch 3 were already in? That
is, the documentation says that the handler _will_ be run in a coroutine
rather than _may_ run it in a coroutine?

> In review of a prior version, Marc-André wondered whether keeping
> allow-oob and coroutine separate makes sense.  Recall qapi-code-gen.txt:
> 
>     An OOB-capable command handler must satisfy the following conditions:
> 
>     - It terminates quickly.
>     - It does not invoke system calls that may block.
>     - It does not access guest RAM that may block when userfaultfd is
>       enabled for postcopy live migration.
>     - It takes only "fast" locks, i.e. all critical sections protected by
>       any lock it takes also satisfy the conditions for OOB command
>       handler code.
> 
>     The restrictions on locking limit access to shared state.  Such access
>     requires synchronization, but OOB commands can't take the BQL or any
>     other "slow" lock.
> 
> Kevin, does this rule out coroutine use?

Not strictly, though I also can't think of a case where you would want
to use a coroutine with these requirements.

If I understand correctly, OOB-capable commands can be run either OOB
with 'exec-oob' or like normal commands with 'execute'. If an OOB
handler is marked as coroutine-safe, 'execute' will run it in a
coroutine (and the restriction above don't apply) and 'exec-oob' will
run it outside of coroutine context.

I have no idea if we will eventually get a case where the command wants
to behave different between the two modes and actually has use for a
coroutine. I hope not.

But using two bools rather than a single enum keeps the code simple and
leaves us all options open if it turns out that we do have a use case.

> > +
> >  The optional 'if' member specifies a conditional.  See "Configuring
> >  the schema" below for more on this.
> >  
> > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> > index 9aa426a398..d6ce9efc8e 100644
> > --- a/include/qapi/qmp/dispatch.h
> > +++ b/include/qapi/qmp/dispatch.h
> > @@ -24,6 +24,7 @@ typedef enum QmpCommandOptions
> >      QCO_NO_SUCCESS_RESP       =  (1U << 0),
> >      QCO_ALLOW_OOB             =  (1U << 1),
> >      QCO_ALLOW_PRECONFIG       =  (1U << 2),
> > +    QCO_COROUTINE             =  (1U << 3),
> >  } QmpCommandOptions;
> >  
> >  typedef struct QmpCommand
> > diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
> > index 27b0afe55a..e2f71e42a1 100644
> > --- a/tests/test-qmp-cmds.c
> > +++ b/tests/test-qmp-cmds.c
> > @@ -34,6 +34,10 @@ void qmp_cmd_success_response(Error **errp)
> >  {
> >  }
> >  
> > +void qmp_coroutine_cmd(Error **errp)
> > +{
> > +}
> > +
> >  Empty2 *qmp_user_def_cmd0(Error **errp)
> >  {
> >      return g_new0(Empty2, 1);
> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > index ab98e504f3..97e51401f1 100644
> > --- a/scripts/qapi/commands.py
> > +++ b/scripts/qapi/commands.py
> > @@ -15,6 +15,7 @@ See the COPYING file in the top-level directory.
> >  
> >  from qapi.common import *
> >  from qapi.gen import QAPIGenCCode, QAPISchemaModularCVisitor, ifcontext
> > +from typing import List
> >  
> >  
> >  def gen_command_decl(name, arg_type, boxed, ret_type):
> > @@ -194,8 +195,9 @@ out:
> >      return ret
> >  
> >  
> > -def gen_register_command(name, success_response, allow_oob, 
> > allow_preconfig):
> > -    options = []
> > +def gen_register_command(name: str, success_response: bool, allow_oob: 
> > bool,
> > +                         allow_preconfig: bool, coroutine: bool) -> str:
> > +    options = [] # type: List[str]
> 
> Not sure such isolated type hints make sense.  I'd welcome patches to
> explore systematic use of type hints.  Suggest to start with just one
> module, so we can gauge effort and benefit before we jump in whole hog.

Any documentation is better than no documentation, imho.

If you insist, I'll remove the type hints, but finding the origin of
values passed as parameters to find out what type they are is a very
common activity for me when touching the QAPI code. Doing that every
time from scratch is not a good use of my time.

> >  
> >      if not success_response:
> >          options += ['QCO_NO_SUCCESS_RESP']
> > @@ -203,18 +205,20 @@ def gen_register_command(name, success_response, 
> > allow_oob, allow_preconfig):
> >          options += ['QCO_ALLOW_OOB']
> >      if allow_preconfig:
> >          options += ['QCO_ALLOW_PRECONFIG']
> > +    if coroutine:
> > +        options += ['QCO_COROUTINE']
> >  
> >      if not options:
> >          options = ['QCO_NO_OPTIONS']
> >  
> > -    options = " | ".join(options)
> > +    options_str = " | ".join(options)
> >  
> >      ret = mcgen('''
> >      qmp_register_command(cmds, "%(name)s",
> >                           qmp_marshal_%(c_name)s, %(opts)s);
> >  ''',
> >                  name=name, c_name=c_name(name),
> > -                opts=options)
> > +                opts=options_str)
> >      return ret
> >  
> >  
> 
> Some extra churn due to type hints here.  Distracting.  Suggest not to
> mix adding type hints to existing code with feature work.

If you would be open for a compromise, I could leave options
unannotated, but keep the typed parameter list.

Kevin