mbox series

[v6,00/12] monitor: Optionally run handlers in coroutines

Message ID 20200528153742.274164-1-kwolf@redhat.com (mailing list archive)
Headers show
Series monitor: Optionally run handlers in coroutines | expand

Message

Kevin Wolf May 28, 2020, 3:37 p.m. UTC 
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.

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 (12):
  monitor: Add Monitor parameter to monitor_set_cpu()
  monitor: Use getter/setter functions for cur_mon
  hmp: Set cur_mon 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_move_to_aio_context()
  block: Convert 'block_resize' to coroutine

 qapi/block-core.json                    |   3 +-
 docs/devel/qapi-code-gen.txt            |  12 +++
 include/block/aio.h                     |  10 ++
 include/block/block.h                   |   6 ++
 include/monitor/monitor.h               |   5 +-
 include/qapi/qmp/dispatch.h             |   5 +-
 monitor/monitor-internal.h              |   7 +-
 audio/wavcapture.c                      |   8 +-
 block.c                                 |  10 ++
 blockdev.c                              |  13 ++-
 dump/dump.c                             |   2 +-
 hw/scsi/vhost-scsi.c                    |   2 +-
 hw/virtio/vhost-vsock.c                 |   2 +-
 migration/fd.c                          |   4 +-
 monitor/hmp-cmds.c                      |   2 +-
 monitor/hmp.c                           |  51 +++++++---
 monitor/misc.c                          |  21 ++--
 monitor/monitor.c                       |  87 ++++++++++++++--
 monitor/qmp-cmds-control.c              |   2 +
 monitor/qmp-cmds.c                      |   2 +-
 monitor/qmp.c                           | 130 +++++++++++++++++-------
 net/socket.c                            |   2 +-
 net/tap.c                               |   6 +-
 qapi/qmp-dispatch.c                     |  52 +++++++++-
 qapi/qmp-registry.c                     |   3 +
 qga/main.c                              |   2 +-
 stubs/monitor-core.c                    |   9 +-
 tests/test-qmp-cmds.c                   |  10 +-
 tests/test-util-sockets.c               |  22 ++--
 trace/control.c                         |   2 +-
 util/aio-posix.c                        |   8 +-
 util/async.c                            |  30 ++++++
 util/qemu-error.c                       |   4 +-
 util/qemu-print.c                       |   3 +-
 util/qemu-sockets.c                     |   1 +
 scripts/qapi/commands.py                |  10 +-
 scripts/qapi/doc.py                     |   2 +-
 scripts/qapi/expr.py                    |  10 +-
 scripts/qapi/introspect.py              |   2 +-
 scripts/qapi/schema.py                  |  12 ++-
 tests/qapi-schema/test-qapi.py          |   7 +-
 hmp-commands.hx                         |   1 +
 tests/Makefile.include                  |   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 +
 48 files changed, 454 insertions(+), 136 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

Comments

Markus Armbruster Aug. 4, 2020, 11:16 a.m. UTC | #1 
I let this series slide to get my Error API rework done, along with much
else.  My sincere apologies!

Unsurprisingly, it needs a rebase now.  I suggest to let me review it as
is first.
Markus Armbruster Aug. 5, 2020, 11:34 a.m. UTC | #2 
Markus Armbruster <armbru@redhat.com> writes:

> I let this series slide to get my Error API rework done, along with much
> else.  My sincere apologies!
>
> Unsurprisingly, it needs a rebase now.  I suggest to let me review it as
> is first.

I'm done with v6.  Summary:

* A few trivial things to correct here and there.

* A few ideas to improve things in relatively minor ways.

* PATCH 03 looks "why bother" to me until PATCH 09 makes me suspect you
  did the former to enable the latter.  If you had captured that in your
  commit message back then, like you did for the similar PATCH 05, I
  wouldn't be scratching my head now :)

* I dislike PATCH 06, and would like to explore an alternative idea.

* PATCH 08 makes hairy monitor code even hairier, but I don't have
  better ideas.

* I don't feel comfortable as a sole reviewer of the AIO magic in PATCH
  10-12.  Let's ask Stefan for an eye-over.

I'd like to proceed as follows.  You rebase, and address "easy" review
comments (you decide what's easy).  Post as v7, cc'ing Stefan for the
AIO magic and David Gilbert for HMP.  While they review (hopefully), I
explore a replacement for PATCH 06.  And then we touch bases and decide
how to get this thing wrapped.

Okay?
Markus Armbruster Sept. 3, 2020, 10:49 a.m. UTC | #3 
Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> I let this series slide to get my Error API rework done, along with much
>> else.  My sincere apologies!
>>
>> Unsurprisingly, it needs a rebase now.  I suggest to let me review it as
>> is first.
>
> I'm done with v6.  Summary:
>
> * A few trivial things to correct here and there.
>
> * A few ideas to improve things in relatively minor ways.
>
> * PATCH 03 looks "why bother" to me until PATCH 09 makes me suspect you
>   did the former to enable the latter.  If you had captured that in your
>   commit message back then, like you did for the similar PATCH 05, I
>   wouldn't be scratching my head now :)
>
> * I dislike PATCH 06, and would like to explore an alternative idea.
>
> * PATCH 08 makes hairy monitor code even hairier, but I don't have
>   better ideas.
>
> * I don't feel comfortable as a sole reviewer of the AIO magic in PATCH
>   10-12.  Let's ask Stefan for an eye-over.
>
> I'd like to proceed as follows.  You rebase, and address "easy" review
> comments (you decide what's easy).  Post as v7, cc'ing Stefan for the
> AIO magic and David Gilbert for HMP.  While they review (hopefully), I
> explore a replacement for PATCH 06.  And then we touch bases and decide
> how to get this thing wrapped.

I explored:

    Subject: Ways to do per-coroutine properties (was: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property)
    Date: Fri, 07 Aug 2020 15:09:19 +0200 (3 weeks, 5 days, 21 hours ago)
    Message-ID: <87a6z6wqkg.fsf_-_@dusky.pond.sub.org>

May I have v7?  Feel free to keep your PATCH 06.  If I decide to replace
it, I can do it myself, possibly on top.
Kevin Wolf Sept. 3, 2020, 12:45 p.m. UTC | #4 
Am 03.09.2020 um 12:49 hat Markus Armbruster geschrieben:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Markus Armbruster <armbru@redhat.com> writes:
> >
> >> I let this series slide to get my Error API rework done, along with much
> >> else.  My sincere apologies!
> >>
> >> Unsurprisingly, it needs a rebase now.  I suggest to let me review it as
> >> is first.
> >
> > I'm done with v6.  Summary:
> >
> > * A few trivial things to correct here and there.
> >
> > * A few ideas to improve things in relatively minor ways.
> >
> > * PATCH 03 looks "why bother" to me until PATCH 09 makes me suspect you
> >   did the former to enable the latter.  If you had captured that in your
> >   commit message back then, like you did for the similar PATCH 05, I
> >   wouldn't be scratching my head now :)
> >
> > * I dislike PATCH 06, and would like to explore an alternative idea.
> >
> > * PATCH 08 makes hairy monitor code even hairier, but I don't have
> >   better ideas.
> >
> > * I don't feel comfortable as a sole reviewer of the AIO magic in PATCH
> >   10-12.  Let's ask Stefan for an eye-over.
> >
> > I'd like to proceed as follows.  You rebase, and address "easy" review
> > comments (you decide what's easy).  Post as v7, cc'ing Stefan for the
> > AIO magic and David Gilbert for HMP.  While they review (hopefully), I
> > explore a replacement for PATCH 06.  And then we touch bases and decide
> > how to get this thing wrapped.
> 
> I explored:
> 
>     Subject: Ways to do per-coroutine properties (was: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property)
>     Date: Fri, 07 Aug 2020 15:09:19 +0200 (3 weeks, 5 days, 21 hours ago)
>     Message-ID: <87a6z6wqkg.fsf_-_@dusky.pond.sub.org>
> 
> May I have v7?  Feel free to keep your PATCH 06.  If I decide to replace
> it, I can do it myself, possibly on top.

It's one of the next things on my list. I can't promise anything more
specific, though.

Kevin
Markus Armbruster Sept. 3, 2020, 2:17 p.m. UTC | #5 
Kevin Wolf <kwolf@redhat.com> writes:

> Am 03.09.2020 um 12:49 hat Markus Armbruster geschrieben:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>> > Markus Armbruster <armbru@redhat.com> writes:
>> >
>> >> I let this series slide to get my Error API rework done, along with much
>> >> else.  My sincere apologies!
>> >>
>> >> Unsurprisingly, it needs a rebase now.  I suggest to let me review it as
>> >> is first.
>> >
>> > I'm done with v6.  Summary:
>> >
>> > * A few trivial things to correct here and there.
>> >
>> > * A few ideas to improve things in relatively minor ways.
>> >
>> > * PATCH 03 looks "why bother" to me until PATCH 09 makes me suspect you
>> >   did the former to enable the latter.  If you had captured that in your
>> >   commit message back then, like you did for the similar PATCH 05, I
>> >   wouldn't be scratching my head now :)
>> >
>> > * I dislike PATCH 06, and would like to explore an alternative idea.
>> >
>> > * PATCH 08 makes hairy monitor code even hairier, but I don't have
>> >   better ideas.
>> >
>> > * I don't feel comfortable as a sole reviewer of the AIO magic in PATCH
>> >   10-12.  Let's ask Stefan for an eye-over.
>> >
>> > I'd like to proceed as follows.  You rebase, and address "easy" review
>> > comments (you decide what's easy).  Post as v7, cc'ing Stefan for the
>> > AIO magic and David Gilbert for HMP.  While they review (hopefully), I
>> > explore a replacement for PATCH 06.  And then we touch bases and decide
>> > how to get this thing wrapped.
>> 
>> I explored:
>> 
>>     Subject: Ways to do per-coroutine properties (was: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property)
>>     Date: Fri, 07 Aug 2020 15:09:19 +0200 (3 weeks, 5 days, 21 hours ago)
>>     Message-ID: <87a6z6wqkg.fsf_-_@dusky.pond.sub.org>
>> 
>> May I have v7?  Feel free to keep your PATCH 06.  If I decide to replace
>> it, I can do it myself, possibly on top.
>
> It's one of the next things on my list. I can't promise anything more
> specific, though.

Take your time.  I've certainly taken mine and then some.