[Qemu-devel] [PATCH v8 00/23] QMP: out-of-band (OOB) execution support

Peter Xu posted 23 patches 6 years, 1 month ago
Failed in applying to current master (apply log)
docs/devel/qapi-code-gen.txt   |  65 +++-
docs/interop/qmp-spec.txt      |  30 +-
include/monitor/monitor.h      |   2 +-
include/qapi/qmp/dispatch.h    |   7 +-
include/qapi/qmp/qstring.h     |   2 +
monitor.c                      | 695 ++++++++++++++++++++++++++++++++++++-----
qapi/introspect.json           |   6 +-
qapi/misc.json                 |  87 +++++-
qapi/qmp-dispatch.c            |  33 +-
qmp.c                          |  16 +
qobject/qstring.c              |  21 ++
qom/object.c                   |   9 +-
scripts/qapi/commands.py       |  18 +-
scripts/qapi/common.py         |  15 +-
scripts/qapi/doc.py            |   2 +-
scripts/qapi/introspect.py     |  10 +-
tests/qapi-schema/test-qapi.py |   2 +-
tests/qmp-test.c               |  97 +++++-
trace-events                   |   3 +
vl.c                           |   7 +-
20 files changed, 1010 insertions(+), 117 deletions(-)
[Qemu-devel] [PATCH v8 00/23] QMP: out-of-band (OOB) execution support
Posted by Peter Xu 6 years, 1 month ago
Based-on: <20180306053320.15401-1-peterx@redhat.com>

The series is based on the following series:

  [PATCH v2 0/9] chardev: qio related non-default context support
  (actually patch 9 is based on version 2.1, Dan's version)

This version only contains some trivial changes, most of the work is
posted separately in either the QIO or CHARDEV series.

Four days until soft freeze.  Hmm...

Please review.  Thanks,

v8:
- fix up doc patch as suggested [Eric]
- rename functions in form of X_get_Y() or X_bh() [Stefan]
- English fixes [Stefan]
- make sure to kick iothread after suspend count update [Stefan]
- remove the hack to remove fd in monitor_init(), since now we have
  the QIO/CHARDEV series to solve the root problem.

v7:
- add some r-bs, and remove some.
- remove the chardev fix since already queued by Paolo
- use local var in qemu_chr_fe_add_watch [Stefan]
- move doc patch to front, mention it in some patches [Eric]
- Quite a few of English fixes [Eric]
- fix unlock missing in handle_qmp_command [Stefan]
- squash some patches according to the review comments
- don't break gdbserver usage on HMP non-interactive mode by fixing up
  the suspend/resume logic [Fam, Stefan]
- move the qemu_chr_fe_set_handlers() call in monitor_init() into a
  bottom half to avoid race between the call itself and
  iothread. [Stefan]
- spent quite a lot of time debugging another assertion failure in
  io_watch_poll_finalize() after above change is made (ouch! I really
  hoped we always have the latest glib): when QEMU inits chardevs in
  chardev_init_func() it's possible that QEMU registers the chardev
  handlers there, even before CharBackend is connected to that chardev
  in monitor_init().  Then, when we reach monitor_init() we must make
  sure we unregister that old one first, or there can have one orphan
  GSource still in default gcontext (note that this can really happen
  when we start to use QEMUBH to setup chardev frontends, which is
  above change).

v6:
- add r-bs
- s/negociate/negotiate/ [Dave]
- let mon_global be anonymous struct [Stefan]
- in monitor_init(): call qemu_chr_fe_set_handlers() later than init
  parser, then it's safe [Stefan]
- drop patch "qjson: add "opaque" field to JSONMessageParser",
  re-write the following one to use container_of(). [Stefan]
- keep get_qmp_greeting() the old way, add cap list [Stefan, Fam]
- fix the iothread_stop() comment since that's not really related to
  the glib bug [Stefan]
- when do qmp greeting, don't expose oob capability if iothread not
  used [Stefan, Fam]
- squash "qmp: introduce some capability helpers" into patch "monitor:
  separate QMP parser and dispatcher" [Fam]
- tune comments for monitor_qmp_respond(). [Stefan]
- use atomic_mb_{read|set}() where proper instead of no-mb ones
  [Stefan]
- add patch to let monitor suspend/resume really support QMP.  Notify
  iothread when needed, in both suspend/resume. [Stefan]
- add comment for qmp_queue_lock on lock taking ordering. [Stefan]
- refactor monitor_qmp_bh_dispatcher() to not loop, instead queue
  itself after each request. [Stefan]
- rename "request" into "command" in the new qmp event [Stefan]
- in handle_qmp_command() protect the queue length fetch with
  qmp_queue_lock [Stefan]
- one more s/2.11/2.12/.  [Fam]
- when isolating response queue, not only flush the queue, but also
  flush the monitor out buffer [Stefan]
- document rewrite [Stefan]
- some other touch-ups that I forgot to mention and some tiny new
  patches... anyway, rbs will be gone if any of the patch is touched
  up.  Please have a look!

v5
- rename "monitor_iothread" to "mon_iothread" [Dave]
- add comment in monitor_cleanup(), note that when the hacks can be
  removed. [Dan]
- add a note section in qmp-spec.txt, mentioning about how to migrate
  existing QMP command to oob-capable command. [Dave]
- drop patch "qmp: let migrate-incoming allow out-of-band".  All
  migration related changes will all be put into postcopy-recovery
  series.

v4:
- drop first patch to fix IOWatchPool [Stefan, Dan]
- add s-o-b where missing, and newly got r-bs
- fix English error in commit msg [Fam]
- some tunes on request-dropped event:  [Stefan]
  - firstly let 'id' be any type, meanwhile make sure "id" is there as
    long as OOB is enabled for the monitor.
  - some comments fix
- add new command "x-oob-test" for testing oob commands [Stefan]
- simplify the test codes to use new x-oob-test
- flush response queue before cleanup monitors

This series was born from this one:

  https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html

The idea comes from Markus Armbruster and the discussion we had in the
thread.  It's not a super ideal solution (I believe Markus had been
thinking hard to keep everything in order meanwhile trying to satisfy
the migration requirement), but AFAIU it's currently the best.

What is OOB?
============

It's the shortcut of Out-Of-Band execution, its name is given by
Markus.  It's a way to quickly execute a QMP request.  Say, originally
QMP is going throw these steps:

      JSON Parser --> QMP Dispatcher --> Respond
          /|\    (2)                (3)     |
       (1) |                               \|/ (4)
           +---------  main thread  --------+

The requests are executed by the so-called QMP-dispatcher after the
JSON is parsed.  If OOB is on, we run the command directly in the
parser and quickly returns.

This series changed the QMP handling logic by moving the command
parsing and responding phases into IOThreads, so to be more accurate,
after the series the above graph would change into this:

               queue/kick              queue/kick
     JSON Parser ======> QMP Dispatcher =====> Responder
         /|\ |     (3)       /|\  |      (4)      | /|\
      (1) |  | (2)            |   |               |  |
          |  |                |  \|/           (6)|  |(5)
          |  |            main thread             |  |
          |  |                                    |  |
          |  +--------> monitor IO thread <-------+  |
          +-----------/                   \----------+

New Interfaces
==============

QMP Introspection Changes
-------------------------

When do query-qmp-schema, we were getting something like:

  {"name": "migrate-incoming", "ret-type": "17",
   "meta-type": "command", "arg-type": "89"}

Now we will get a new key named "allow-oob":

  {"name": "migrate-incoming", "ret-type": "17", "allow-oob": true,
   "meta-type": "command", "arg-type": "89"}

Which shows whether a command supports OOB.

QMP Negociation Changes
-----------------------

We were running "qmp_capabilities" always without parameters like:

  {"execute": "qmp_capabilities"}

Now we can enable capabilities (we don't have any capability before
this series) like OOB using:

  {"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}}

Only after we explicitly enable OOB capability can we send requests in
OOB manner.  Otherwise we'll have exactly the same QMP session as
before, just like when OOB is not there.

When OOB is enabled, it's possible that OOB reply reaches faster than
previous command, so clients should be prepared.

Trigger OOB execution
---------------------

Let's take migrate-incoming as example.  The old command looks like:

  {"execute": "migrate-incoming", "arguments" : {"uri": "xxxxxx"}}

To execute a command with OOB execution, we need to specify it in the
QMP request in the extra "control" key:

  {"execute": "migrate-incoming", "arguments" : {"uri": "xxxxxx"},
   "control" { "run-oob": true } }

Then the command will be run in parser, and it can preempt other
commands.

Others
=================

The last patch of OOB test may need some attention.  I used
dump-guest-memory as a time-consuming command to test whether OOB is
working, and the only command I can test now is "migrate-incoming".  I
hope that is a "okay" solution for unit tests.  Any other suggestions
on that would be welcomed.

Please review.  Thanks.

Peter Xu (23):
  docs: update QMP documents for OOB commands
  qobject: introduce qstring_get_try_str()
  qobject: introduce qobject_get_try_str()
  qobject: let object_property_get_str() use new API
  monitor: move skip_flush into monitor_data_init
  monitor: move the cur_mon hack deeper for QMP
  monitor: unify global init
  monitor: let mon_list be tail queue
  monitor: allow using IO thread for parsing
  qmp: introduce QMPCapability
  monitor: introduce monitor_qmp_respond()
  monitor: let suspend_cnt be thread safe
  monitor: let suspend/resume work even with QMPs
  monitor: separate QMP parser and dispatcher
  qmp: add new event "command-dropped"
  monitor: send event when command queue full
  qapi: introduce new cmd option "allow-oob"
  qmp: support out-of-band (oob) execution
  qmp: isolate responses into io thread
  monitor: enable IO thread for (qmp & !mux) typed
  qmp: add command "x-oob-test"
  tests: qmp-test: verify command batching
  tests: qmp-test: add oob test

 docs/devel/qapi-code-gen.txt   |  65 +++-
 docs/interop/qmp-spec.txt      |  30 +-
 include/monitor/monitor.h      |   2 +-
 include/qapi/qmp/dispatch.h    |   7 +-
 include/qapi/qmp/qstring.h     |   2 +
 monitor.c                      | 695 ++++++++++++++++++++++++++++++++++++-----
 qapi/introspect.json           |   6 +-
 qapi/misc.json                 |  87 +++++-
 qapi/qmp-dispatch.c            |  33 +-
 qmp.c                          |  16 +
 qobject/qstring.c              |  21 ++
 qom/object.c                   |   9 +-
 scripts/qapi/commands.py       |  18 +-
 scripts/qapi/common.py         |  15 +-
 scripts/qapi/doc.py            |   2 +-
 scripts/qapi/introspect.py     |  10 +-
 tests/qapi-schema/test-qapi.py |   2 +-
 tests/qmp-test.c               |  97 +++++-
 trace-events                   |   3 +
 vl.c                           |   7 +-
 20 files changed, 1010 insertions(+), 117 deletions(-)

-- 
2.14.3


Re: [Qemu-devel] [PATCH v8 00/23] QMP: out-of-band (OOB) execution support
Posted by Eric Blake 6 years, 1 month ago
On 03/09/2018 02:59 AM, Peter Xu wrote:
> Based-on: <20180306053320.15401-1-peterx@redhat.com>
> 
> The series is based on the following series:
> 
>    [PATCH v2 0/9] chardev: qio related non-default context support
>    (actually patch 9 is based on version 2.1, Dan's version)
> 
> This version only contains some trivial changes, most of the work is
> posted separately in either the QIO or CHARDEV series.
> 
> Four days until soft freeze.  Hmm...
> 
> Please review.  Thanks,
> 
> v8:
> - fix up doc patch as suggested [Eric]
> - rename functions in form of X_get_Y() or X_bh() [Stefan]
> - English fixes [Stefan]
> - make sure to kick iothread after suspend count update [Stefan]
> - remove the hack to remove fd in monitor_init(), since now we have
>    the QIO/CHARDEV series to solve the root problem.

Still a lot that I had to clean up (in part due to conflicts with other 
pending patches that also touch qapi), but given the utility of this 
feature, and the impending deadline for freeze, I've gone ahead and done 
the cleanup work.  Your patches are now stages on my QAPI tree.

git://repo.or.cz/qemu/ericb.git qapi
http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi

Note that I had some ideas for followup patches (mainly strengthening 
the testsuite, which is typically safe early on in freeze because it is 
not introducing features but strengthening the quality of the release).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v8 00/23] QMP: out-of-band (OOB) execution support
Posted by Peter Xu 6 years, 1 month ago
On Sat, Mar 10, 2018 at 08:59:38PM -0600, Eric Blake wrote:
> On 03/09/2018 02:59 AM, Peter Xu wrote:
> > Based-on: <20180306053320.15401-1-peterx@redhat.com>
> > 
> > The series is based on the following series:
> > 
> >    [PATCH v2 0/9] chardev: qio related non-default context support
> >    (actually patch 9 is based on version 2.1, Dan's version)

[1]

> > 
> > This version only contains some trivial changes, most of the work is
> > posted separately in either the QIO or CHARDEV series.
> > 
> > Four days until soft freeze.  Hmm...
> > 
> > Please review.  Thanks,
> > 
> > v8:
> > - fix up doc patch as suggested [Eric]
> > - rename functions in form of X_get_Y() or X_bh() [Stefan]
> > - English fixes [Stefan]
> > - make sure to kick iothread after suspend count update [Stefan]
> > - remove the hack to remove fd in monitor_init(), since now we have
> >    the QIO/CHARDEV series to solve the root problem.
> 
> Still a lot that I had to clean up (in part due to conflicts with other
> pending patches that also touch qapi), but given the utility of this
> feature, and the impending deadline for freeze, I've gone ahead and done the
> cleanup work.  Your patches are now stages on my QAPI tree.
> 
> git://repo.or.cz/qemu/ericb.git qapi
> http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi
> 
> Note that I had some ideas for followup patches (mainly strengthening the
> testsuite, which is typically safe early on in freeze because it is not
> introducing features but strengthening the quality of the release).

I really appreciate very much on your work and time on the series.

I've noted down all the review comments, including the test
enhancements (which I'll do in a proper way soon).  I'll avoid
replying to every review comments since all of them are sane.

Note that this series will depend on the chardev fixes ([1] above) in
Paolo's tree. They'll possibly not be needed for compilation, but
might be needed for further tests.  Hope that won't be a big problem.

Thanks again!

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v8 00/23] QMP: out-of-band (OOB) execution support
Posted by Eric Blake 6 years, 1 month ago
On 03/09/2018 02:59 AM, Peter Xu wrote:
> Based-on: <20180306053320.15401-1-peterx@redhat.com>
> 
> The series is based on the following series:
> 
>    [PATCH v2 0/9] chardev: qio related non-default context support
>    (actually patch 9 is based on version 2.1, Dan's version)

I see Dan already submitted a pull request for (some of) your qio work; 
see commit 3ef91576.  Are there any more outstanding qio patches that 
have to land first?

> 
> This series was born from this one:
> 
>    https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
> 
> The idea comes from Markus Armbruster and the discussion we had in the
> thread.  It's not a super ideal solution (I believe Markus had been
> thinking hard to keep everything in order meanwhile trying to satisfy
> the migration requirement), but AFAIU it's currently the best.
> 
> What is OOB?
> ============
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v8 00/23] QMP: out-of-band (OOB) execution support
Posted by Daniel P. Berrangé 6 years, 1 month ago
On Mon, Mar 12, 2018 at 07:01:50AM -0500, Eric Blake wrote:
> On 03/09/2018 02:59 AM, Peter Xu wrote:
> > Based-on: <20180306053320.15401-1-peterx@redhat.com>
> > 
> > The series is based on the following series:
> > 
> >    [PATCH v2 0/9] chardev: qio related non-default context support
> >    (actually patch 9 is based on version 2.1, Dan's version)
> 
> I see Dan already submitted a pull request for (some of) your qio work; see
> commit 3ef91576.  Are there any more outstanding qio patches that have to
> land first?

AFAIK, there isn't anymore qio stuff pending merge - just the chardev
series Paolo has queued.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v8 00/23] QMP: out-of-band (OOB) execution support
Posted by Peter Xu 6 years, 1 month ago
On Mon, Mar 12, 2018 at 12:55:28PM +0000, Daniel P. Berrangé wrote:
> On Mon, Mar 12, 2018 at 07:01:50AM -0500, Eric Blake wrote:
> > On 03/09/2018 02:59 AM, Peter Xu wrote:
> > > Based-on: <20180306053320.15401-1-peterx@redhat.com>
> > > 
> > > The series is based on the following series:
> > > 
> > >    [PATCH v2 0/9] chardev: qio related non-default context support
> > >    (actually patch 9 is based on version 2.1, Dan's version)
> > 
> > I see Dan already submitted a pull request for (some of) your qio work; see
> > commit 3ef91576.  Are there any more outstanding qio patches that have to
> > land first?
> 
> AFAIK, there isn't anymore qio stuff pending merge - just the chardev
> series Paolo has queued.

Yes, there are still chardev patches that used the new QIO code queued
in Paolo's tree.  There can be either 8/9 patches there, dpending on
whether Paolo has queued the 9th patch from Dan.

Thanks,

-- 
Peter Xu