[RFC,14/15] qmp: support out-of-band (oob) execution

Message ID 1505375436-28439-15-git-send-email-peterx@redhat.com
State New
Headers show
Series
  • QMP: out-of-band (OOB) execution support
Related show

Commit Message

Peter Xu Sept. 14, 2017, 7:50 a.m.
Having "allow-oob" to true for a command does not mean that this command
will always be run in out-of-band mode.  The out-of-band quick path will
only be executed if we specify the extra "run-oob" flag when sending the
QMP request:

  { "execute": "command-that-allows-oob",
    "arguments": { ... },
    "control": { "run-oob": true } }

The "control" key is introduced to store this extra flag.  "control"
field is used to store arguments that are shared by all the commands,
rather than command specific arguments.  Let "run-oob" be the first.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 docs/devel/qapi-code-gen.txt | 10 ++++++++++
 include/qapi/qmp/dispatch.h  |  1 +
 monitor.c                    | 11 +++++++++++
 qapi/qmp-dispatch.c          | 34 ++++++++++++++++++++++++++++++++++
 trace-events                 |  2 ++
 5 files changed, 58 insertions(+)

Comments

Stefan Hajnoczi Sept. 14, 2017, 3:33 p.m. | #1
On Thu, Sep 14, 2017 at 03:50:35PM +0800, Peter Xu wrote:
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 61fa167..47d16bb 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -665,6 +665,16 @@ allowed to run out-of-band can also be introspected using
>  query-qmp-schema command.  Please see the section "Client JSON
>  Protocol introspection" for more information.
>  
> +To execute a command in out-of-band way, we need to specify the
> +"control" field in the request, with "run-oob" set to true. Example:
> +
> + => { "execute": "command-support-oob",
> +      "arguments": { ... },
> +      "control": { "run-oob": true } }
> + <= { "return": { } }
> +
> +Without it, even the commands that supports out-of-band execution will
> +still be run in-band.

Is there a more relevant place to document QMP run-oob behavior than the
"How to use the QAPI code generator document"?

> @@ -3963,6 +3964,16 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens,
>      req_obj->id = id;
>      req_obj->req = req;
>  
> +    if (qmp_is_oob(req)) {
> +        /*
> +         * Trigger fast-path to handle the out-of-band request, by
> +         * executing the command directly in parser.
> +         */
> +        trace_monitor_qmp_cmd_out_of_band(qobject_get_str(req_obj->id));
> +        monitor_qmp_dispatch_one(req_obj);
> +        return;
> +    }

A "fast-path" is a performance optimization.  OOB is not a performance
optimization, it changes the semantics of command execution.  Please
mention the semantics of OOB command execution instead.

Stefa
Peter Xu Sept. 15, 2017, 2:59 a.m. | #2
On Thu, Sep 14, 2017 at 04:33:34PM +0100, Stefan Hajnoczi wrote:
> On Thu, Sep 14, 2017 at 03:50:35PM +0800, Peter Xu wrote:
> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> > index 61fa167..47d16bb 100644
> > --- a/docs/devel/qapi-code-gen.txt
> > +++ b/docs/devel/qapi-code-gen.txt
> > @@ -665,6 +665,16 @@ allowed to run out-of-band can also be introspected using
> >  query-qmp-schema command.  Please see the section "Client JSON
> >  Protocol introspection" for more information.
> >  
> > +To execute a command in out-of-band way, we need to specify the
> > +"control" field in the request, with "run-oob" set to true. Example:
> > +
> > + => { "execute": "command-support-oob",
> > +      "arguments": { ... },
> > +      "control": { "run-oob": true } }
> > + <= { "return": { } }
> > +
> > +Without it, even the commands that supports out-of-band execution will
> > +still be run in-band.
> 
> Is there a more relevant place to document QMP run-oob behavior than the
> "How to use the QAPI code generator document"?

I agree, but I don't really know it. :(

Markus, could you provide a hint?

> 
> > @@ -3963,6 +3964,16 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens,
> >      req_obj->id = id;
> >      req_obj->req = req;
> >  
> > +    if (qmp_is_oob(req)) {
> > +        /*
> > +         * Trigger fast-path to handle the out-of-band request, by
> > +         * executing the command directly in parser.
> > +         */
> > +        trace_monitor_qmp_cmd_out_of_band(qobject_get_str(req_obj->id));
> > +        monitor_qmp_dispatch_one(req_obj);
> > +        return;
> > +    }
> 
> A "fast-path" is a performance optimization.  OOB is not a performance
> optimization, it changes the semantics of command execution.  Please
> mention the semantics of OOB command execution instead.

I'll remove the "fast-path" wording and try to think out something
better than this comment.  After I know a good place to document, I
can put it there as well.  Thanks,
Dr. David Alan Gilbert Sept. 15, 2017, 3:55 p.m. | #3
* Peter Xu (peterx@redhat.com) wrote:
> Having "allow-oob" to true for a command does not mean that this command
> will always be run in out-of-band mode.  The out-of-band quick path will
> only be executed if we specify the extra "run-oob" flag when sending the
> QMP request:
> 
>   { "execute": "command-that-allows-oob",
>     "arguments": { ... },
>     "control": { "run-oob": true } }
> 
> The "control" key is introduced to store this extra flag.  "control"
> field is used to store arguments that are shared by all the commands,
> rather than command specific arguments.  Let "run-oob" be the first.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

I don't understand how this enforces the allowoob, that is it stops
other commands being called with run-oob=true

> ---
>  docs/devel/qapi-code-gen.txt | 10 ++++++++++
>  include/qapi/qmp/dispatch.h  |  1 +
>  monitor.c                    | 11 +++++++++++
>  qapi/qmp-dispatch.c          | 34 ++++++++++++++++++++++++++++++++++
>  trace-events                 |  2 ++
>  5 files changed, 58 insertions(+)
> 
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 61fa167..47d16bb 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -665,6 +665,16 @@ allowed to run out-of-band can also be introspected using
>  query-qmp-schema command.  Please see the section "Client JSON
>  Protocol introspection" for more information.
>  
> +To execute a command in out-of-band way, we need to specify the
> +"control" field in the request, with "run-oob" set to true. Example:
> +
> + => { "execute": "command-support-oob",
> +      "arguments": { ... },
> +      "control": { "run-oob": true } }
> + <= { "return": { } }
> +
> +Without it, even the commands that supports out-of-band execution will
> +still be run in-band.
>  
>  === Events ===
>  
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index b767988..ee2b8ce 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -49,6 +49,7 @@ bool qmp_command_is_enabled(const QmpCommand *cmd);
>  const char *qmp_command_name(const QmpCommand *cmd);
>  bool qmp_has_success_response(const QmpCommand *cmd);
>  QObject *qmp_build_error_object(Error *err);
> +bool qmp_is_oob(const QObject *request);
>  
>  typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
>  
> diff --git a/monitor.c b/monitor.c
> index 599ea36..cb96204 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3928,6 +3928,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
>          if (!req_obj) {
>              break;
>          }
> +        trace_monitor_qmp_cmd_in_band(qobject_get_str(req_obj->id));
>          monitor_qmp_dispatch_one(req_obj);
>      }
>  }
> @@ -3963,6 +3964,16 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens,
>      req_obj->id = id;
>      req_obj->req = req;
>  
> +    if (qmp_is_oob(req)) {
> +        /*
> +         * Trigger fast-path to handle the out-of-band request, by
> +         * executing the command directly in parser.
> +         */
> +        trace_monitor_qmp_cmd_out_of_band(qobject_get_str(req_obj->id));
> +        monitor_qmp_dispatch_one(req_obj);
> +        return;
> +    }

I wonder if there is a way to run all allowoob commands in this way;
the only difference being if they're not started with run-oob
you wiat for completion of !oob commands.
That way the allowoob commands are always run from the same
thread/context which feels like it should simplify them.

Dave

>      /*
>       * Put the request to the end of queue so that requests will be
>       * handled in time order.  Ownership for req_obj, req, id,
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index b41fa17..9a05dfa 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -52,6 +52,12 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>                             "QMP input member 'arguments' must be an object");
>                  return NULL;
>              }
> +        } else if (!strcmp(arg_name, "control")) {
> +            if (qobject_type(arg_obj) != QTYPE_QDICT) {
> +                error_setg(errp,
> +                           "QMP input member 'control' must be an object");
> +                return NULL;
> +            }
>          } else {
>              error_setg(errp, "QMP input member '%s' is unexpected",
>                         arg_name);
> @@ -122,6 +128,34 @@ QObject *qmp_build_error_object(Error *err)
>                                error_get_pretty(err));
>  }
>  
> +/*
> + * Detect whether a request should be run out-of-band, by quickly
> + * peeking at whether we have: { "control": { "run-oob": True } }. By
> + * default commands are run in-band.
> + */
> +bool qmp_is_oob(const QObject *request)
> +{
> +    QDict *dict;
> +    QBool *bool_obj;
> +
> +    dict = qobject_to_qdict(request);
> +    if (!dict) {
> +        return false;
> +    }
> +
> +    dict = qdict_get_qdict(dict, "control");
> +    if (!dict) {
> +        return false;
> +    }
> +
> +    bool_obj = qobject_to_qbool(qdict_get(dict, "run-oob"));
> +    if (!bool_obj) {
> +        return false;
> +    }
> +
> +    return qbool_get_bool(bool_obj);
> +}
> +
>  QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request)
>  {
>      Error *err = NULL;
> diff --git a/trace-events b/trace-events
> index 1f50f56..f7900a6 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -47,6 +47,8 @@ monitor_protocol_event_emit(uint32_t event, void *data) "event=%d data=%p"
>  monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) "event=%d data=%p rate=%" PRId64
>  handle_hmp_command(void *mon, const char *cmdline) "mon %p cmdline: %s"
>  handle_qmp_command(void *mon, const char *req) "mon %p req: %s"
> +monitor_qmp_cmd_in_band(const char *id) "%s"
> +monitor_qmp_cmd_out_of_band(const char *id) "%s"
>  
>  # dma-helpers.c
>  dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Eric Blake Sept. 15, 2017, 6:34 p.m. | #4
On 09/14/2017 09:59 PM, Peter Xu wrote:
> On Thu, Sep 14, 2017 at 04:33:34PM +0100, Stefan Hajnoczi wrote:
>> On Thu, Sep 14, 2017 at 03:50:35PM +0800, Peter Xu wrote:
>>> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
>>> index 61fa167..47d16bb 100644
>>> --- a/docs/devel/qapi-code-gen.txt
>>> +++ b/docs/devel/qapi-code-gen.txt

>>
>> Is there a more relevant place to document QMP run-oob behavior than the
>> "How to use the QAPI code generator document"?
> 
> I agree, but I don't really know it. :(
> 
> Markus, could you provide a hint?

I'm not Markus, but I suggest docs/interop/qmp-spec.txt
Peter Xu Sept. 18, 2017, 7:36 a.m. | #5
On Fri, Sep 15, 2017 at 01:34:24PM -0500, Eric Blake wrote:
> On 09/14/2017 09:59 PM, Peter Xu wrote:
> > On Thu, Sep 14, 2017 at 04:33:34PM +0100, Stefan Hajnoczi wrote:
> >> On Thu, Sep 14, 2017 at 03:50:35PM +0800, Peter Xu wrote:
> >>> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> >>> index 61fa167..47d16bb 100644
> >>> --- a/docs/devel/qapi-code-gen.txt
> >>> +++ b/docs/devel/qapi-code-gen.txt
> 
> >>
> >> Is there a more relevant place to document QMP run-oob behavior than the
> >> "How to use the QAPI code generator document"?
> > 
> > I agree, but I don't really know it. :(
> > 
> > Markus, could you provide a hint?
> 
> I'm not Markus, but I suggest docs/interop/qmp-spec.txt

Sorry I should have noticed it before (I am always not sensitive on
the word "interop").  Thanks Eric!
Peter Xu Sept. 18, 2017, 7:53 a.m. | #6
On Fri, Sep 15, 2017 at 04:55:51PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > Having "allow-oob" to true for a command does not mean that this command
> > will always be run in out-of-band mode.  The out-of-band quick path will
> > only be executed if we specify the extra "run-oob" flag when sending the
> > QMP request:
> > 
> >   { "execute": "command-that-allows-oob",
> >     "arguments": { ... },
> >     "control": { "run-oob": true } }
> > 
> > The "control" key is introduced to store this extra flag.  "control"
> > field is used to store arguments that are shared by all the commands,
> > rather than command specific arguments.  Let "run-oob" be the first.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> I don't understand how this enforces the allowoob, that is it stops
> other commands being called with run-oob=true

Here's what I thought:

OOB commands are executed directly in the parser, and currently we
only have one single parser/IO thread, then we can't have two oob
commands in parallel, can we? Say, if we got one OOB command, we won't
handle anything else (no matter OOB or non-OOB) before we finished
processing that OOB command.

> 
> > ---
> >  docs/devel/qapi-code-gen.txt | 10 ++++++++++
> >  include/qapi/qmp/dispatch.h  |  1 +
> >  monitor.c                    | 11 +++++++++++
> >  qapi/qmp-dispatch.c          | 34 ++++++++++++++++++++++++++++++++++
> >  trace-events                 |  2 ++
> >  5 files changed, 58 insertions(+)
> > 
> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> > index 61fa167..47d16bb 100644
> > --- a/docs/devel/qapi-code-gen.txt
> > +++ b/docs/devel/qapi-code-gen.txt
> > @@ -665,6 +665,16 @@ allowed to run out-of-band can also be introspected using
> >  query-qmp-schema command.  Please see the section "Client JSON
> >  Protocol introspection" for more information.
> >  
> > +To execute a command in out-of-band way, we need to specify the
> > +"control" field in the request, with "run-oob" set to true. Example:
> > +
> > + => { "execute": "command-support-oob",
> > +      "arguments": { ... },
> > +      "control": { "run-oob": true } }
> > + <= { "return": { } }
> > +
> > +Without it, even the commands that supports out-of-band execution will
> > +still be run in-band.
> >  
> >  === Events ===
> >  
> > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> > index b767988..ee2b8ce 100644
> > --- a/include/qapi/qmp/dispatch.h
> > +++ b/include/qapi/qmp/dispatch.h
> > @@ -49,6 +49,7 @@ bool qmp_command_is_enabled(const QmpCommand *cmd);
> >  const char *qmp_command_name(const QmpCommand *cmd);
> >  bool qmp_has_success_response(const QmpCommand *cmd);
> >  QObject *qmp_build_error_object(Error *err);
> > +bool qmp_is_oob(const QObject *request);
> >  
> >  typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
> >  
> > diff --git a/monitor.c b/monitor.c
> > index 599ea36..cb96204 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -3928,6 +3928,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >          if (!req_obj) {
> >              break;
> >          }
> > +        trace_monitor_qmp_cmd_in_band(qobject_get_str(req_obj->id));
> >          monitor_qmp_dispatch_one(req_obj);
> >      }
> >  }
> > @@ -3963,6 +3964,16 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens,
> >      req_obj->id = id;
> >      req_obj->req = req;
> >  
> > +    if (qmp_is_oob(req)) {
> > +        /*
> > +         * Trigger fast-path to handle the out-of-band request, by
> > +         * executing the command directly in parser.
> > +         */
> > +        trace_monitor_qmp_cmd_out_of_band(qobject_get_str(req_obj->id));
> > +        monitor_qmp_dispatch_one(req_obj);
> > +        return;
> > +    }
> 
> I wonder if there is a way to run all allowoob commands in this way;
> the only difference being if they're not started with run-oob
> you wiat for completion of !oob commands.
> That way the allowoob commands are always run from the same
> thread/context which feels like it should simplify them.

Maybe I misread the comment... Even with current patch, all OOB
commands will be run from the same thread/context (which is the newly
created parser thread), right?  Thanks,

Patch

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 61fa167..47d16bb 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -665,6 +665,16 @@  allowed to run out-of-band can also be introspected using
 query-qmp-schema command.  Please see the section "Client JSON
 Protocol introspection" for more information.
 
+To execute a command in out-of-band way, we need to specify the
+"control" field in the request, with "run-oob" set to true. Example:
+
+ => { "execute": "command-support-oob",
+      "arguments": { ... },
+      "control": { "run-oob": true } }
+ <= { "return": { } }
+
+Without it, even the commands that supports out-of-band execution will
+still be run in-band.
 
 === Events ===
 
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index b767988..ee2b8ce 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -49,6 +49,7 @@  bool qmp_command_is_enabled(const QmpCommand *cmd);
 const char *qmp_command_name(const QmpCommand *cmd);
 bool qmp_has_success_response(const QmpCommand *cmd);
 QObject *qmp_build_error_object(Error *err);
+bool qmp_is_oob(const QObject *request);
 
 typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
 
diff --git a/monitor.c b/monitor.c
index 599ea36..cb96204 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3928,6 +3928,7 @@  static void monitor_qmp_bh_dispatcher(void *data)
         if (!req_obj) {
             break;
         }
+        trace_monitor_qmp_cmd_in_band(qobject_get_str(req_obj->id));
         monitor_qmp_dispatch_one(req_obj);
     }
 }
@@ -3963,6 +3964,16 @@  static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens,
     req_obj->id = id;
     req_obj->req = req;
 
+    if (qmp_is_oob(req)) {
+        /*
+         * Trigger fast-path to handle the out-of-band request, by
+         * executing the command directly in parser.
+         */
+        trace_monitor_qmp_cmd_out_of_band(qobject_get_str(req_obj->id));
+        monitor_qmp_dispatch_one(req_obj);
+        return;
+    }
+
     /*
      * Put the request to the end of queue so that requests will be
      * handled in time order.  Ownership for req_obj, req, id,
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index b41fa17..9a05dfa 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -52,6 +52,12 @@  static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
                            "QMP input member 'arguments' must be an object");
                 return NULL;
             }
+        } else if (!strcmp(arg_name, "control")) {
+            if (qobject_type(arg_obj) != QTYPE_QDICT) {
+                error_setg(errp,
+                           "QMP input member 'control' must be an object");
+                return NULL;
+            }
         } else {
             error_setg(errp, "QMP input member '%s' is unexpected",
                        arg_name);
@@ -122,6 +128,34 @@  QObject *qmp_build_error_object(Error *err)
                               error_get_pretty(err));
 }
 
+/*
+ * Detect whether a request should be run out-of-band, by quickly
+ * peeking at whether we have: { "control": { "run-oob": True } }. By
+ * default commands are run in-band.
+ */
+bool qmp_is_oob(const QObject *request)
+{
+    QDict *dict;
+    QBool *bool_obj;
+
+    dict = qobject_to_qdict(request);
+    if (!dict) {
+        return false;
+    }
+
+    dict = qdict_get_qdict(dict, "control");
+    if (!dict) {
+        return false;
+    }
+
+    bool_obj = qobject_to_qbool(qdict_get(dict, "run-oob"));
+    if (!bool_obj) {
+        return false;
+    }
+
+    return qbool_get_bool(bool_obj);
+}
+
 QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request)
 {
     Error *err = NULL;
diff --git a/trace-events b/trace-events
index 1f50f56..f7900a6 100644
--- a/trace-events
+++ b/trace-events
@@ -47,6 +47,8 @@  monitor_protocol_event_emit(uint32_t event, void *data) "event=%d data=%p"
 monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) "event=%d data=%p rate=%" PRId64
 handle_hmp_command(void *mon, const char *cmdline) "mon %p cmdline: %s"
 handle_qmp_command(void *mon, const char *req) "mon %p req: %s"
+monitor_qmp_cmd_in_band(const char *id) "%s"
+monitor_qmp_cmd_out_of_band(const char *id) "%s"
 
 # dma-helpers.c
 dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"