| Message ID | 20200909151149.490589-1-kwolf@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | monitor: Optionally run handlers in coroutines | expand |
Patchew URL: https://patchew.org/QEMU/20200909151149.490589-1-kwolf@redhat.com/ Hi, This series failed build test on FreeBSD host. Please find the details below. === TEST SCRIPT BEGIN === #!/bin/bash # Testing script will be invoked under the git checkout with # HEAD pointing to a commit that has the patches applied on top of "base" # branch if qemu-system-x86_64 --help >/dev/null 2>&1; then QEMU=qemu-system-x86_64 elif /usr/libexec/qemu-kvm --help >/dev/null 2>&1; then QEMU=/usr/libexec/qemu-kvm else exit 1 fi make vm-build-freebsd J=21 QEMU=$QEMU exit 0 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200909151149.490589-1-kwolf@redhat.com/testing.FreeBSD/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On Wed, Sep 09, 2020 at 05:11:36PM +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. Please clarify the following in the QAPI documentation: * Is the QMP monitor suspended while the command is pending? * Are QMP events reported while the command is pending? Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
Stefan Hajnoczi <stefanha@redhat.com> writes: > On Wed, Sep 09, 2020 at 05:11:36PM +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. > > Please clarify the following in the QAPI documentation: > * Is the QMP monitor suspended while the command is pending? > * Are QMP events reported while the command is pending? Good points. Kevin, I'd be willing to take this as a follow-up patch, if that's more convenient for you. > Acked-by: Stefan Hajnoczi <stefanha@redhat.com> Stefan, I could use your proper review of PATCH 11-13. Pretty-please?
On Mon, Sep 14, 2020 at 05:09:49PM +0200, Markus Armbruster wrote: > Stefan Hajnoczi <stefanha@redhat.com> writes: > > > On Wed, Sep 09, 2020 at 05:11:36PM +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. > > > > Please clarify the following in the QAPI documentation: > > * Is the QMP monitor suspended while the command is pending? > > * Are QMP events reported while the command is pending? > > Good points. Kevin, I'd be willing to take this as a follow-up patch, > if that's more convenient for you. > > > Acked-by: Stefan Hajnoczi <stefanha@redhat.com> > > Stefan, I could use your proper review of PATCH 11-13. Pretty-please? Sounds good. I have reviewed the patches 11-13 and left questions for Kevin.
Am 10.09.2020 um 15:24 hat Stefan Hajnoczi geschrieben: > On Wed, Sep 09, 2020 at 05:11:36PM +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. > > Please clarify the following in the QAPI documentation: > * Is the QMP monitor suspended while the command is pending? Suspended as in monitor_suspend()? No. > * Are QMP events reported while the command is pending? Hm, I don't know to be honest. But I think so. Does it matter, though? I don't think events have a defined order compared to command results, and the client can't respond to the event anyway until the current command has completed. Kevin
On Fri, Sep 25, 2020 at 07:15:41PM +0200, Kevin Wolf wrote: > Am 10.09.2020 um 15:24 hat Stefan Hajnoczi geschrieben: > > On Wed, Sep 09, 2020 at 05:11:36PM +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. > > > > Please clarify the following in the QAPI documentation: > > * Is the QMP monitor suspended while the command is pending? > > Suspended as in monitor_suspend()? No. > > > * Are QMP events reported while the command is pending? > > Hm, I don't know to be honest. But I think so. > > Does it matter, though? I don't think events have a defined order > compared to command results, and the client can't respond to the event > anyway until the current command has completed. You're right, I don't think it matters because the client must expect QMP events at any time. I was trying to understand the semantics of coroutine monitor commands from two perspectives: 1. The QMP client - do coroutine commands behave differently from non-coroutine commands? I think the answer is no. The monitor will not process more commands until the coroutine finishes? 2. The command implementation - which thread does the coroutine run in? I guess it runs in the main loop thread with the BQL and the AioContext acquired?
Kevin Wolf <kwolf@redhat.com> writes: > Am 10.09.2020 um 15:24 hat Stefan Hajnoczi geschrieben: >> On Wed, Sep 09, 2020 at 05:11:36PM +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. >> >> Please clarify the following in the QAPI documentation: >> * Is the QMP monitor suspended while the command is pending? > > Suspended as in monitor_suspend()? No. A suspended monitor doesn't read monitor input. We suspend * a QMP monitor while the request queue is full * an HMP monitor while it executes a command * a multiplexed HMP monitor while the "mux-focus" is elsewhere * an HMP monitor when it executes command "quit", forever * an HMP monitor while it executes command "migrate" without -d Let me explain the first item in a bit more detail. Before OOB, a QMP monitor was also suspended while it executed a command. To make OOB work, we moved the QMP monitors to an I/O thread and added a request queue, drained by the main loop. QMP monitors continue to read commands, execute right away if OOB, else queue, suspend when queue gets full, resume when it gets non-full. The "run command in coroutine context" feature does not affect any of this. qapi-code-gen.txt does not talk about monitor suspension at all. It's instead discussed in qmp-spec.txt section 2.3.1 Out-of-band execution. Stefan, what would you like us to clarify, and where? >> * Are QMP events reported while the command is pending? > > Hm, I don't know to be honest. But I think so. Yes, events should be reported while a command is being executed. Sending events takes locks. Their critical sections are all short-lived. Another possible delay is the underlying character device failing the send with EAGAIN. That's all. Fine print: qapi_event_emit() takes @monitor_lock. It sends to each QMP monitor with qmp_send_response(), which uses monitor_puts(), which takes the monitor's @mon_lock. The "run command in coroutine context" feature does not affect any of this. > Does it matter, though? I don't think events have a defined order > compared to command results, and the client can't respond to the event > anyway until the current command has completed. Stefan, what would you like us to clarify, and where?
Am 28.09.2020 um 10:46 hat Stefan Hajnoczi geschrieben: > On Fri, Sep 25, 2020 at 07:15:41PM +0200, Kevin Wolf wrote: > > Am 10.09.2020 um 15:24 hat Stefan Hajnoczi geschrieben: > > > On Wed, Sep 09, 2020 at 05:11:36PM +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. > > > > > > Please clarify the following in the QAPI documentation: > > > * Is the QMP monitor suspended while the command is pending? > > > > Suspended as in monitor_suspend()? No. > > > > > * Are QMP events reported while the command is pending? > > > > Hm, I don't know to be honest. But I think so. > > > > Does it matter, though? I don't think events have a defined order > > compared to command results, and the client can't respond to the event > > anyway until the current command has completed. > > You're right, I don't think it matters because the client must expect > QMP events at any time. > > I was trying to understand the semantics of coroutine monitor commands > from two perspectives: > > 1. The QMP client - do coroutine commands behave differently from > non-coroutine commands? I think the answer is no. The monitor will > not process more commands until the coroutine finishes? No, on the wire, things should look exactly the same. If you consider more than the QMP traffic and the client communicates with the guest, it might see that the guest isn't blocked any more, of course. > 2. The command implementation - which thread does the coroutine run in? > I guess it runs in the main loop thread with the BQL and the > AioContext acquired? By default, yes. But the coroutine can reschedule itself to another thread. Block-related handlers will want to reschedule themselves to bs->aio_context because you can't just hold the AioContext lock from another thread across yields. This is what block_resize does after this series. Kevin