From: Stefan Hajnoczi <stefanha@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: marcandre.lureau@gmail.com, armbru@redhat.com,
qemu-block@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH v8 00/14] monitor: Optionally run handlers in coroutines
Date: Thu, 8 Oct 2020 16:53:13 +0100 [thread overview]
Message-ID: <20201008155313.GB31472@stefanha-x1.localdomain> (raw)
In-Reply-To: <20201005155855.256490-1-kwolf@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 6920 bytes --]
On Mon, Oct 05, 2020 at 05:58:41PM +0200, 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.
>
> v8:
> - Replaced bdrv_co_move_to_aio_context() with bdrv_co_enter()/leave(),
> added bdrv_co_lock()/unlock() and fixed the iothread support in
> qmp_block_resize() with the new functions [Stefan]
> - More detailed documentation of 'coroutine' flag for commands [Markus]
> - Improved comment for dropping out of coroutine context in
> qmp_dispatch() [Markus]
> - Coding style changes [Markus]
> - Resolve some more merge conflicts on rebase
>
> v7:
> - Added patch 2 to add a Monitor parameter to monitor_get_cpu_index(),
> too [Markus]
> - Let monitor_set_cur() return the old monitor [Markus]
> - Fixed comment about linking stub objects in test-util-sockets [Markus]
> - More detailed commit message for per-coroutine current monitor and
> document that monitor_set_cur(NULL) must be called eventually [Markus]
> - Resolve some merge conflicts on rebase
>
> v6:
> - Fixed cur_mon behaviour: It should always point to the Monitor while
> we're in the handler coroutine, but be NULL while the handler
> coroutines has yielded. [Markus]
> - Give HMP handlers the coroutine option, too, because they will call
> QMP handlers, and life is easier when we know whether we are in
> coroutine context or not.
> - Fixed block_resize for block devices in iothreads and for HMP
> - Resolved some merge conflict with QAPI generator and monitor
> refactorings that were merged in the meantime
>
> v5:
> - Improved comments and documentation [Markus]
>
> v4:
> - Forbid 'oob': true, 'coroutine': true [Markus]
> - Removed Python type hints [Markus]
> - Introduced separate bool qmp_dispatcher_co_shutdown to make it clearer
> how a shutdown request is signalled to the dispatcher [Markus]
> - Allow using aio_poll() with iohandler_ctx and use that instead of
> aio_bh_poll() [Markus]
> - Removed coroutine_fn from qmp_block_resize() again because at least
> one caller (HMP) calls it outside of coroutine context [Markus]
> - Tried to make the synchronisation between dispatcher and the monitor
> thread clearer, and fixed a race condition
> - Improved documentation and comments
>
> v3:
> - Fix race between monitor thread and dispatcher that could schedule the
> dispatcher coroutine twice if a second requests comes in before the
> dispatcher can wake up [Patchew]
>
> 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 (14):
> monitor: Add Monitor parameter to monitor_set_cpu()
> monitor: Add Monitor parameter to monitor_get_cpu_index()
> monitor: Use getter/setter functions for cur_mon
> hmp: Update current monitor only in handle_hmp_command()
> qmp: Assert that no other monitor is active
> qmp: Call monitor_set_cur() only in qmp_dispatch()
> monitor: Make current monitor a per-coroutine property
> qapi: Add a 'coroutine' flag for commands
> qmp: Move dispatcher to a coroutine
> hmp: Add support for coroutine command handlers
> util/async: Add aio_co_reschedule_self()
> block: Add bdrv_co_enter()/leave()
> block: Add bdrv_lock()/unlock()
> block: Convert 'block_resize' to coroutine
>
> qapi/block-core.json | 3 +-
> docs/devel/qapi-code-gen.txt | 29 ++++++
> docs/sphinx/qapidoc.py | 2 +-
> include/block/aio.h | 10 ++
> include/block/block.h | 31 ++++++
> include/monitor/monitor.h | 7 +-
> include/qapi/qmp/dispatch.h | 5 +-
> monitor/monitor-internal.h | 7 +-
> audio/wavcapture.c | 8 +-
> block.c | 50 +++++++++
> blockdev.c | 16 +--
> dump/dump.c | 2 +-
> hw/core/machine-hmp-cmds.c | 2 +-
> hw/scsi/vhost-scsi.c | 2 +-
> hw/virtio/vhost-vsock.c | 2 +-
> migration/fd.c | 4 +-
> monitor/hmp-cmds.c | 4 +-
> monitor/hmp.c | 44 ++++++--
> monitor/misc.c | 38 +++----
> monitor/monitor.c | 101 ++++++++++++++++--
> monitor/qmp-cmds-control.c | 2 +
> monitor/qmp-cmds.c | 2 +-
> monitor/qmp.c | 131 +++++++++++++++++-------
> net/socket.c | 2 +-
> net/tap.c | 6 +-
> qapi/qmp-dispatch.c | 65 +++++++++++-
> qapi/qmp-registry.c | 3 +
> qga/main.c | 2 +-
> softmmu/cpus.c | 2 +-
> stubs/monitor-core.c | 10 +-
> tests/test-qmp-cmds.c | 10 +-
> tests/test-util-sockets.c | 12 +--
> trace/control.c | 2 +-
> util/aio-posix.c | 8 +-
> util/async.c | 30 ++++++
> util/qemu-error.c | 6 +-
> util/qemu-print.c | 3 +-
> util/qemu-sockets.c | 1 +
> scripts/qapi/commands.py | 10 +-
> scripts/qapi/expr.py | 11 +-
> scripts/qapi/introspect.py | 2 +-
> scripts/qapi/schema.py | 13 ++-
> tests/qapi-schema/test-qapi.py | 7 +-
> hmp-commands.hx | 1 +
> tests/qapi-schema/meson.build | 1 +
> tests/qapi-schema/oob-coroutine.err | 2 +
> tests/qapi-schema/oob-coroutine.json | 2 +
> tests/qapi-schema/oob-coroutine.out | 0
> tests/qapi-schema/qapi-schema-test.json | 1 +
> tests/qapi-schema/qapi-schema-test.out | 2 +
> 50 files changed, 573 insertions(+), 143 deletions(-)
> create mode 100644 tests/qapi-schema/oob-coroutine.err
> create mode 100644 tests/qapi-schema/oob-coroutine.json
> create mode 100644 tests/qapi-schema/oob-coroutine.out
>
> --
> 2.25.4
>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
prev parent reply other threads:[~2020-10-08 15:57 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-05 15:58 [PATCH v8 00/14] monitor: Optionally run handlers in coroutines Kevin Wolf
2020-10-05 15:58 ` [PATCH v8 01/14] monitor: Add Monitor parameter to monitor_set_cpu() Kevin Wolf
2020-10-05 15:58 ` [PATCH v8 02/14] monitor: Add Monitor parameter to monitor_get_cpu_index() Kevin Wolf
2020-10-05 16:34 ` Eric Blake
2020-10-05 15:58 ` [PATCH v8 03/14] monitor: Use getter/setter functions for cur_mon Kevin Wolf
2020-10-05 15:58 ` [PATCH v8 04/14] hmp: Update current monitor only in handle_hmp_command() Kevin Wolf
2020-10-05 15:58 ` [PATCH v8 05/14] qmp: Assert that no other monitor is active Kevin Wolf
2020-10-05 15:58 ` [PATCH v8 06/14] qmp: Call monitor_set_cur() only in qmp_dispatch() Kevin Wolf
2020-10-05 15:58 ` [PATCH v8 07/14] monitor: Make current monitor a per-coroutine property Kevin Wolf
2020-10-05 15:58 ` [PATCH v8 08/14] qapi: Add a 'coroutine' flag for commands Kevin Wolf
2020-10-05 15:58 ` [PATCH v8 09/14] qmp: Move dispatcher to a coroutine Kevin Wolf
2020-10-05 15:58 ` [PATCH v8 10/14] hmp: Add support for coroutine command handlers Kevin Wolf
2020-10-05 15:58 ` [PATCH v8 11/14] util/async: Add aio_co_reschedule_self() Kevin Wolf
2020-10-05 15:58 ` [PATCH v8 12/14] block: Add bdrv_co_enter()/leave() Kevin Wolf
2020-10-05 15:58 ` [PATCH v8 13/14] block: Add bdrv_lock()/unlock() Kevin Wolf
2020-10-05 15:58 ` [PATCH v8 14/14] block: Convert 'block_resize' to coroutine Kevin Wolf
2020-10-06 6:29 ` [PATCH v8 00/14] monitor: Optionally run handlers in coroutines Markus Armbruster
2020-10-08 15:53 ` Stefan Hajnoczi [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201008155313.GB31472@stefanha-x1.localdomain \
--to=stefanha@redhat.com \
--cc=armbru@redhat.com \
--cc=kwolf@redhat.com \
--cc=marcandre.lureau@gmail.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).