[PATCH v2 0/4] qmp: Optionally run handlers in coroutines

Kevin Wolf posted 4 patches 4 years, 2 months ago
Test docker-mingw@fedora passed
Test checkpatch passed
Test docker-quick@centos7 failed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200114182735.5553-1-kwolf@redhat.com
Maintainers: "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Max Reitz <mreitz@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Michael Roth <mdroth@linux.vnet.ibm.com>
There is a newer version of this series
qapi/block-core.json                    |  3 +-
tests/qapi-schema/qapi-schema-test.json |  1 +
docs/devel/qapi-code-gen.txt            |  4 ++
include/qapi/qmp/dispatch.h             |  3 +
monitor/monitor-internal.h              |  5 +-
blockdev.c                              |  6 +-
monitor/monitor.c                       | 24 ++++---
monitor/qmp.c                           | 83 ++++++++++++++++---------
qapi/qmp-dispatch.c                     | 38 ++++++++++-
tests/test-qmp-cmds.c                   |  4 ++
vl.c                                    | 10 +--
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 ++-
18 files changed, 158 insertions(+), 66 deletions(-)
[PATCH v2 0/4] qmp: Optionally run handlers in coroutines
Posted by Kevin Wolf 4 years, 2 months ago
Some QMP command handlers can block the main loop for a relatively long
time, for example because they perform some I/O. This is quite nasty.
Allowing such handlers to run in a coroutine where they can yield (and
therefore release the BQL) while waiting for an event such as I/O
completion solves the problem.

This series adds the infrastructure to allow this and switches
block_resize to run in a coroutine as a first example.

This is an alternative solution to Marc-André's "monitor: add
asynchronous command type" series.

v2:
- Fix typo in a commit message [Eric]
- Use hyphen instead of underscore for the test command [Eric]
- Mark qmp_block_resize() as coroutine_fn [Stefan]


Kevin Wolf (4):
  qapi: Add a 'coroutine' flag for commands
  vl: Initialise main loop earlier
  qmp: Move dispatcher to a coroutine
  block: Mark 'block_resize' as coroutine

 qapi/block-core.json                    |  3 +-
 tests/qapi-schema/qapi-schema-test.json |  1 +
 docs/devel/qapi-code-gen.txt            |  4 ++
 include/qapi/qmp/dispatch.h             |  3 +
 monitor/monitor-internal.h              |  5 +-
 blockdev.c                              |  6 +-
 monitor/monitor.c                       | 24 ++++---
 monitor/qmp.c                           | 83 ++++++++++++++++---------
 qapi/qmp-dispatch.c                     | 38 ++++++++++-
 tests/test-qmp-cmds.c                   |  4 ++
 vl.c                                    | 10 +--
 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 ++-
 18 files changed, 158 insertions(+), 66 deletions(-)

-- 
2.20.1


Re: [PATCH v2 0/4] qmp: Optionally run handlers in coroutines
Posted by Daniel P. Berrangé 4 years, 2 months ago
On Tue, Jan 14, 2020 at 07:27:31PM +0100, Kevin Wolf wrote:
> Some QMP command handlers can block the main loop for a relatively long
> time, for example because they perform some I/O. This is quite nasty.
> Allowing such handlers to run in a coroutine where they can yield (and
> therefore release the BQL) while waiting for an event such as I/O
> completion solves the problem.

Am I right that with this approach, there's no functional difference
to QMP from a mgmt app POV ? ie this is purely focused on avoiding
stalls in the main event loop which impact the guest OS and other
parts of QEMU ?

IOW, the response to the QMP command being executed will get sent
back to the mgmt app as normal when the command completes, and the
QMP monitor still serializes execution of commands ?

> This series adds the infrastructure to allow this and switches
> block_resize to run in a coroutine as a first example.
> 
> This is an alternative solution to Marc-André's "monitor: add
> asynchronous command type" series.

Where as this is an actual functional improvement to QMP from
the mgmt app POV in allowing background commands & thus
concurrent execution ?

If this is correct, then the two proposals are somewhat
complementary 

> 
> v2:
> - Fix typo in a commit message [Eric]
> - Use hyphen instead of underscore for the test command [Eric]
> - Mark qmp_block_resize() as coroutine_fn [Stefan]
> 
> 
> Kevin Wolf (4):
>   qapi: Add a 'coroutine' flag for commands
>   vl: Initialise main loop earlier
>   qmp: Move dispatcher to a coroutine
>   block: Mark 'block_resize' as coroutine
> 
>  qapi/block-core.json                    |  3 +-
>  tests/qapi-schema/qapi-schema-test.json |  1 +
>  docs/devel/qapi-code-gen.txt            |  4 ++
>  include/qapi/qmp/dispatch.h             |  3 +
>  monitor/monitor-internal.h              |  5 +-
>  blockdev.c                              |  6 +-
>  monitor/monitor.c                       | 24 ++++---
>  monitor/qmp.c                           | 83 ++++++++++++++++---------
>  qapi/qmp-dispatch.c                     | 38 ++++++++++-
>  tests/test-qmp-cmds.c                   |  4 ++
>  vl.c                                    | 10 +--
>  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 ++-
>  18 files changed, 158 insertions(+), 66 deletions(-)
> 
> -- 
> 2.20.1
> 
> 

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: [PATCH v2 0/4] qmp: Optionally run handlers in coroutines
Posted by Kevin Wolf 4 years, 2 months ago
Am 14.01.2020 um 19:45 hat Daniel P. Berrangé geschrieben:
> On Tue, Jan 14, 2020 at 07:27:31PM +0100, Kevin Wolf wrote:
> > Some QMP command handlers can block the main loop for a relatively long
> > time, for example because they perform some I/O. This is quite nasty.
> > Allowing such handlers to run in a coroutine where they can yield (and
> > therefore release the BQL) while waiting for an event such as I/O
> > completion solves the problem.
> 
> Am I right that with this approach, there's no functional difference
> to QMP from a mgmt app POV ? ie this is purely focused on avoiding
> stalls in the main event loop which impact the guest OS and other
> parts of QEMU ?
> 
> IOW, the response to the QMP command being executed will get sent
> back to the mgmt app as normal when the command completes, and the
> QMP monitor still serializes execution of commands ?

Yes, the QMP dispatcher still processes one request after another. The
only visible effect should be that the guest and other background tasks
aren't blocked.

> > This series adds the infrastructure to allow this and switches
> > block_resize to run in a coroutine as a first example.
> > 
> > This is an alternative solution to Marc-André's "monitor: add
> > asynchronous command type" series.
> 
> Where as this is an actual functional improvement to QMP from
> the mgmt app POV in allowing background commands & thus
> concurrent execution ?
> 
> If this is correct, then the two proposals are somewhat
> complementary 

Marc-André's first proposal (maybe two years ago) actually added real
asynchronous commands to the protocol. But his latest versions gave up
on this and made commands only internally asynchronous, with pretty much
the same effect as this series.

If we ever do want to extend the protocol to have async commands on the
protocol level, this can still be done on top. There is no fundamental
problem that would prevent just launching a coroutine for each parallel
request. In fact, this series is probably the first step that you would
make anyway to even have something that can be parallelised.

Kevin


Re: [PATCH v2 0/4] qmp: Optionally run handlers in coroutines
Posted by no-reply@patchew.org 4 years, 2 months ago
Patchew URL: https://patchew.org/QEMU/20200114182735.5553-1-kwolf@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

aio_co_schedule: Co-routine was already scheduled in 'aio_co_schedule'
Broken pipe
/tmp/qemu-test/src/tests/qtest/libqtest.c:149: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
ERROR - too few tests run (expected 4, got 1)
make: *** [check-qtest-aarch64] Error 1
make: *** Waiting for unfinished jobs....
  TEST    iotest-qcow2: 117
  TEST    iotest-qcow2: 120
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=7a16af349113413b993a610a9cc5ed59', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-0jve889m/src/docker-src.2020-01-14-15.32.21.3694:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=7a16af349113413b993a610a9cc5ed59
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-0jve889m/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    9m18.352s
user    0m9.409s


The full log is available at
http://patchew.org/logs/20200114182735.5553-1-kwolf@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [PATCH v2 0/4] qmp: Optionally run handlers in coroutines
Posted by Kevin Wolf 4 years, 2 months ago
Am 14.01.2020 um 21:41 hat no-reply@patchew.org geschrieben:
> Patchew URL: https://patchew.org/QEMU/20200114182735.5553-1-kwolf@redhat.com/
> 
> Hi,
> 
> This series failed the docker-quick@centos7 build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-centos7 V=1 NETWORK=1
> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
> aio_co_schedule: Co-routine was already scheduled in 'aio_co_schedule'
> Broken pipe
> /tmp/qemu-test/src/tests/qtest/libqtest.c:149: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
> ERROR - too few tests run (expected 4, got 1)
> make: *** [check-qtest-aarch64] Error 1
> make: *** Waiting for unfinished jobs....

This was a real bug in the series, v3 incoming.

Kevin


Re: [PATCH v2 0/4] qmp: Optionally run handlers in coroutines
Posted by Stefan Hajnoczi 4 years, 2 months ago
On Tue, Jan 14, 2020 at 07:27:31PM +0100, Kevin Wolf wrote:
> Some QMP command handlers can block the main loop for a relatively long
> time, for example because they perform some I/O. This is quite nasty.
> Allowing such handlers to run in a coroutine where they can yield (and
> therefore release the BQL) while waiting for an event such as I/O
> completion solves the problem.
> 
> This series adds the infrastructure to allow this and switches
> block_resize to run in a coroutine as a first example.
> 
> This is an alternative solution to Marc-André's "monitor: add
> asynchronous command type" series.
> 
> v2:
> - Fix typo in a commit message [Eric]
> - Use hyphen instead of underscore for the test command [Eric]
> - Mark qmp_block_resize() as coroutine_fn [Stefan]
> 
> 
> Kevin Wolf (4):
>   qapi: Add a 'coroutine' flag for commands
>   vl: Initialise main loop earlier
>   qmp: Move dispatcher to a coroutine
>   block: Mark 'block_resize' as coroutine
> 
>  qapi/block-core.json                    |  3 +-
>  tests/qapi-schema/qapi-schema-test.json |  1 +
>  docs/devel/qapi-code-gen.txt            |  4 ++
>  include/qapi/qmp/dispatch.h             |  3 +
>  monitor/monitor-internal.h              |  5 +-
>  blockdev.c                              |  6 +-
>  monitor/monitor.c                       | 24 ++++---
>  monitor/qmp.c                           | 83 ++++++++++++++++---------
>  qapi/qmp-dispatch.c                     | 38 ++++++++++-
>  tests/test-qmp-cmds.c                   |  4 ++
>  vl.c                                    | 10 +--
>  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 ++-
>  18 files changed, 158 insertions(+), 66 deletions(-)
> 
> -- 
> 2.20.1
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>