Re: [Qemu-devel] [RFC v5 24/26] docs: update QMP documents for OOB commands

[Peter Xu]

On Thu, Dec 14, 2017 at 02:30:19PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 05, 2017 at 01:51:58PM +0800, Peter Xu wrote:
> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> > index f04c63fe82..8597fdb087 100644
> > --- a/docs/devel/qapi-code-gen.txt
> > +++ b/docs/devel/qapi-code-gen.txt
> > @@ -556,7 +556,8 @@ following example objects:
> >  
> >  Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
> >           '*returns': TYPE-NAME, '*boxed': true,
> > -         '*gen': false, '*success-response': false }
> > +         '*gen': false, '*success-response': false,
> > +         '*allow-oob': false }
> >  
> >  Commands are defined by using a dictionary containing several members,
> >  where three members are most common.  The 'command' member is a
> > @@ -636,6 +637,44 @@ possible, the command expression should include the 
> > optional key
> >  'success-response' with boolean value false.  So far, only QGA makes
> >  use of this member.
> >  
> > +Most of the QMP commands are handled sequentially in such a order:
> > +Firstly, the JSON Parser parses the command request into some internal
> > +message, delivers the message to QMP dispatchers. Secondly, the QMP
> > +dispatchers will handle the commands one by one in time order, respond
> > +when necessary.
> 
> The important points to cover about normal commands:
> 1. They execute in order
> 2. They run the main loop
> 3. They run under the BQL
> 
> The other stuff about parsing requests into internal messages,
> dispatchers, etc is not relevant.  It's better not to include it in
> documentation because it can change and could also confuse readers
> (since they don't need this info).

Ah yes.  I'm thinking whether I should just remove most of the changes
to this file (qapi-code-gen.txt) since most of them are really not
related to code gen at all...  Maybe somewhere in qmp-spec.txt as
well?

> 
> > For some commands that always complete "quickly" can
> 
> I've mentioned before that "quickly" is misleading and not what oob
> commands are about.  I suggest changing this to:
> 
>   Certain urgent commands can
> 
> I've made similar comments further down where I think the text focusses
> on words like "quickly" or "asynchronous" too much.

I'll be more careful on the wording in next version on this.

> 
> > +instead be executed directly during parsing, at the QMP client's
> > +request.  This kind of commands that allow direct execution is called
> > +"out-of-band" ("oob" as shortcut) commands. The response can overtake
> > +prior in-band commands' responses.  By default, commands are always
> > +in-band.  We need to explicitly specify "allow-oob" to "True" to show
> 
> s/"True"/true/  (JSON is case-sensitive)

Ok.

> 
> > +that one command can be run out-of-band.
> > +
> > +One thing to mention for developers is that, although out-of-band
> > +execution of commands benefit from quick and asynchronous execution,
> > +it need to satisfy at least the following:
> > +
> > +(1) It is extremely quick and never blocks, so that its execution will
> > +    not block parsing routine of any other monitors.
> > +
> > +(2) It does not need BQL, since the parser can be run without BQL,
> > +    while the dispatcher is always with BQL held.
> 
> These conditions are not sufficient for postcopy recovery.  I suggest
> rephrasing this section as follows:
> 
>   An out-of-band command must:
> 
>   1. Complete extremely quickly
>   2. Not take locks
>   3. Not invoke blocking system calls
>   4. Not access guest RAM or memory that blocks when userfaultfd is
>      enabled for postcopy live migration
> 
> We could make #2 less strict by saying "Not take locks except for
> spinlocks (or mutexes that could be spinlocks because the critical
> regions are tiny) or indirectly via g_malloc()".

Ok.  I'm thinking whether I should also move these lines out too.

> 
> > Whether a command is
> > +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.
> 
> This is relevant to client authors, not QEMU developers learning about
> qapi.  I suggest dropping it.

Ok.

> 
> > +
> > +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.
> 
> This is relevant to client authors, not QEMU developers learning about
> qapi.  I suggest instead explaining how qmp functions need to test for
> qmp_is_oob() so that they know which mode they are executing in.
> 
> >  2.2.1 Capabilities
> >  ------------------
> >  
> > -As of the date this document was last revised, no server or client
> > -capability strings have been defined.
> > -
> > +Currently supported capabilities are:
> > +
> > +- "oob": it means the QMP server supports "Out-Of-Band" command
> > +  execution.  For more detail, please see "run-oob" parameter in
> > +  "Issuing Commands" section below.  Not all commands allow this "oob"
> > +  execution.  One can know whether one command supports "oob" by
> > +  "query-qmp-schema" command.
> > +
> > +  NOTE: Converting an existing QMP command to be OOB-capable can be
> > +  either very easy or extremely hard.  The most important thing is
> > +  that OOB-capable command should never be blocked for a long time.
> > +  Some bad examples: (1) doing IOs, especially when the backend can be
> > +  an NFS storage; or (2) accessing guest memories, which can be simply
> > +  blocked for a very long time when it triggers a page fault, which
> > +  may not be handled immediately.  It's still legal to take a mutex in
> > +  an OOB-capable command handler, however, we need to make sure that
> > +  all the other code paths that are protected by the same lock won't
> > +  be blocked very long as well, otherwise the OOB handler might be
> > +  blocked too when it tries to take the lock.
> 
> Please drop this paragraph, this is the qmp-spec.txt document so the
> implementation of QEMU's QMP commands shouldn't be discussed here.

Ok.

> 
> > For some commands that always complete
> > +  "quickly" can be executed directly during parsing at the QMP
> > +  client's request.
> 
> Please drop this sentence.  "quickly" isn't the important quality, it's
> urgency.  Also the description of execution in the QMP parser isn't
> relevant for qmp-spec.txt, what matters is that oob commands can execute
> while a normal monitor command is still running (this is described next
> so this whole sentence can be dropped).

Ok.

I'll try to re-arrange the sentences better in my next post.  Thanks,

-- 
Peter Xu