| Message ID | 20200218154036.28562-4-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qmp: Optionally run handlers in coroutines | expand |
Kevin Wolf <kwolf@redhat.com> writes: > This moves the QMP dispatcher to a coroutine and runs all QMP command > handlers that declare 'coroutine': true in coroutine context so they > can avoid blocking the main loop while doing I/O or waiting for other > events. > > For commands that are not declared safe to run in a coroutine, the > dispatcher drops out of coroutine context by calling the QMP command > handler from a bottom half. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/qapi/qmp/dispatch.h | 1 + > monitor/monitor-internal.h | 6 +- > monitor/monitor.c | 55 +++++++++++++--- > monitor/qmp.c | 122 +++++++++++++++++++++++++++--------- > qapi/qmp-dispatch.c | 44 ++++++++++++- > qapi/qmp-registry.c | 3 + > util/aio-posix.c | 7 ++- > 7 files changed, 196 insertions(+), 42 deletions(-) > > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h > index d6ce9efc8e..6812e49b5f 100644 > --- a/include/qapi/qmp/dispatch.h > +++ b/include/qapi/qmp/dispatch.h > @@ -30,6 +30,7 @@ typedef enum QmpCommandOptions > typedef struct QmpCommand > { > const char *name; > + /* Runs in coroutine context if QCO_COROUTINE is set */ > QmpCommandFunc *fn; > QmpCommandOptions options; > QTAILQ_ENTRY(QmpCommand) node; > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h > index 3e6baba88f..f8123b151a 100644 > --- a/monitor/monitor-internal.h > +++ b/monitor/monitor-internal.h > @@ -155,7 +155,9 @@ static inline bool monitor_is_qmp(const Monitor *mon) > > typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList; > extern IOThread *mon_iothread; > -extern QEMUBH *qmp_dispatcher_bh; > +extern Coroutine *qmp_dispatcher_co; > +extern bool qmp_dispatcher_co_shutdown; > +extern bool qmp_dispatcher_co_busy; > extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands; > extern QemuMutex monitor_lock; > extern MonitorList mon_list; > @@ -173,7 +175,7 @@ void monitor_fdsets_cleanup(void); > > void qmp_send_response(MonitorQMP *mon, const QDict *rsp); > void monitor_data_destroy_qmp(MonitorQMP *mon); > -void monitor_qmp_bh_dispatcher(void *data); > +void coroutine_fn monitor_qmp_dispatcher_co(void *data); > > int get_monitor_def(int64_t *pval, const char *name); > void help_cmd(Monitor *mon, const char *name); > diff --git a/monitor/monitor.c b/monitor/monitor.c > index c1a6c4460f..72d57b5cd2 100644 > --- a/monitor/monitor.c > +++ b/monitor/monitor.c > @@ -53,8 +53,32 @@ typedef struct { > /* Shared monitor I/O thread */ > IOThread *mon_iothread; > > -/* Bottom half to dispatch the requests received from I/O thread */ > -QEMUBH *qmp_dispatcher_bh; > +/* Coroutine to dispatch the requests received from I/O thread */ > +Coroutine *qmp_dispatcher_co; > + > +/* Set to true when the dispatcher coroutine should terminate */ > +bool qmp_dispatcher_co_shutdown; > + > +/* > + * qmp_dispatcher_co_busy is used for synchronisation between the > + * monitor thread and the main thread to ensure that the dispatcher > + * coroutine never gets scheduled a second time when it's already > + * scheduled (scheduling the same coroutine twice is forbidden). > + * > + * It is true if the coroutine is active and processing requests. > + * Additional requests may then be pushed onto a mon->qmp_requests, > + * and @qmp_dispatcher_co_shutdown may be set without further ado. > + * @qmp_dispatcher_co_busy must not be woken up in this case. > + * > + * If false, you also have to set @qmp_dispatcher_co_busy to true and > + * wake up @qmp_dispatcher_co after pushing the new requests. Also after setting @qmp_dispatcher_co_shutdown, right? Happy to tweak the comment without a respin. > + * > + * The coroutine will automatically change this variable back to false > + * before it yields. Nobody else may set the variable to false. > + * > + * Access must be atomic for thread safety. > + */ > +bool qmp_dispatcher_co_busy; > > /* Protects mon_list, monitor_qapi_event_state, monitor_destroyed. */ > QemuMutex monitor_lock; [...] Reviewed-by: Markus Armbruster <armbru@redhat.com>
Am 03.03.2020 um 09:49 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > This moves the QMP dispatcher to a coroutine and runs all QMP command > > handlers that declare 'coroutine': true in coroutine context so they > > can avoid blocking the main loop while doing I/O or waiting for other > > events. > > > > For commands that are not declared safe to run in a coroutine, the > > dispatcher drops out of coroutine context by calling the QMP command > > handler from a bottom half. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > include/qapi/qmp/dispatch.h | 1 + > > monitor/monitor-internal.h | 6 +- > > monitor/monitor.c | 55 +++++++++++++--- > > monitor/qmp.c | 122 +++++++++++++++++++++++++++--------- > > qapi/qmp-dispatch.c | 44 ++++++++++++- > > qapi/qmp-registry.c | 3 + > > util/aio-posix.c | 7 ++- > > 7 files changed, 196 insertions(+), 42 deletions(-) > > > > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h > > index d6ce9efc8e..6812e49b5f 100644 > > --- a/include/qapi/qmp/dispatch.h > > +++ b/include/qapi/qmp/dispatch.h > > @@ -30,6 +30,7 @@ typedef enum QmpCommandOptions > > typedef struct QmpCommand > > { > > const char *name; > > + /* Runs in coroutine context if QCO_COROUTINE is set */ > > QmpCommandFunc *fn; > > QmpCommandOptions options; > > QTAILQ_ENTRY(QmpCommand) node; > > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h > > index 3e6baba88f..f8123b151a 100644 > > --- a/monitor/monitor-internal.h > > +++ b/monitor/monitor-internal.h > > @@ -155,7 +155,9 @@ static inline bool monitor_is_qmp(const Monitor *mon) > > > > typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList; > > extern IOThread *mon_iothread; > > -extern QEMUBH *qmp_dispatcher_bh; > > +extern Coroutine *qmp_dispatcher_co; > > +extern bool qmp_dispatcher_co_shutdown; > > +extern bool qmp_dispatcher_co_busy; > > extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands; > > extern QemuMutex monitor_lock; > > extern MonitorList mon_list; > > @@ -173,7 +175,7 @@ void monitor_fdsets_cleanup(void); > > > > void qmp_send_response(MonitorQMP *mon, const QDict *rsp); > > void monitor_data_destroy_qmp(MonitorQMP *mon); > > -void monitor_qmp_bh_dispatcher(void *data); > > +void coroutine_fn monitor_qmp_dispatcher_co(void *data); > > > > int get_monitor_def(int64_t *pval, const char *name); > > void help_cmd(Monitor *mon, const char *name); > > diff --git a/monitor/monitor.c b/monitor/monitor.c > > index c1a6c4460f..72d57b5cd2 100644 > > --- a/monitor/monitor.c > > +++ b/monitor/monitor.c > > @@ -53,8 +53,32 @@ typedef struct { > > /* Shared monitor I/O thread */ > > IOThread *mon_iothread; > > > > -/* Bottom half to dispatch the requests received from I/O thread */ > > -QEMUBH *qmp_dispatcher_bh; > > +/* Coroutine to dispatch the requests received from I/O thread */ > > +Coroutine *qmp_dispatcher_co; > > + > > +/* Set to true when the dispatcher coroutine should terminate */ > > +bool qmp_dispatcher_co_shutdown; > > + > > +/* > > + * qmp_dispatcher_co_busy is used for synchronisation between the > > + * monitor thread and the main thread to ensure that the dispatcher > > + * coroutine never gets scheduled a second time when it's already > > + * scheduled (scheduling the same coroutine twice is forbidden). > > + * > > + * It is true if the coroutine is active and processing requests. > > + * Additional requests may then be pushed onto a mon->qmp_requests, > > + * and @qmp_dispatcher_co_shutdown may be set without further ado. > > + * @qmp_dispatcher_co_busy must not be woken up in this case. > > + * > > + * If false, you also have to set @qmp_dispatcher_co_busy to true and > > + * wake up @qmp_dispatcher_co after pushing the new requests. > > Also after setting @qmp_dispatcher_co_shutdown, right? > > Happy to tweak the comment without a respin. I understood a shutdown request as just another kind of request, but if you want the comment to be more detailed, feel free. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > This moves the QMP dispatcher to a coroutine and runs all QMP command > handlers that declare 'coroutine': true in coroutine context so they > can avoid blocking the main loop while doing I/O or waiting for other > events. > > For commands that are not declared safe to run in a coroutine, the > dispatcher drops out of coroutine context by calling the QMP command > handler from a bottom half. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> Uh, what about @cur_mon? @cur_mon points to the current monitor while a command executes. Initial value is null. It is set in three places (not counting unit tests), and all three save, set, do something that may use @cur_mon, restore: * monitor_qmp_dispatch(), for use within qmp_dispatch() * monitor_read(), for use within handle_hmp_command() * qmp_human_monitor_command(), also for use within handle_hmp_command() Therefore, @cur_mon is null unless we're running within qmp_dispatch() or handle_hmp_command(). Example of use: error_report() & friends print "to current monitor if we have one, else to stderr." Makes sharing code between HMP and CLI easier. Uses @cur_mon under the hood. @cur_mon is thread-local. I'm afraid we have to save, clear and restore @cur_mon around a yield.
Am 18.03.2020 um 16:36 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > This moves the QMP dispatcher to a coroutine and runs all QMP command > > handlers that declare 'coroutine': true in coroutine context so they > > can avoid blocking the main loop while doing I/O or waiting for other > > events. > > > > For commands that are not declared safe to run in a coroutine, the > > dispatcher drops out of coroutine context by calling the QMP command > > handler from a bottom half. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > Uh, what about @cur_mon? > > @cur_mon points to the current monitor while a command executes. > Initial value is null. It is set in three places (not counting unit > tests), and all three save, set, do something that may use @cur_mon, > restore: > > * monitor_qmp_dispatch(), for use within qmp_dispatch() > * monitor_read(), for use within handle_hmp_command() > * qmp_human_monitor_command(), also for use within handle_hmp_command() > > Therefore, @cur_mon is null unless we're running within qmp_dispatch() > or handle_hmp_command(). Can we make it NULL for coroutine-enabled handlers? > Example of use: error_report() & friends print "to current monitor if we > have one, else to stderr." Makes sharing code between HMP and CLI > easier. Uses @cur_mon under the hood. error_report() eventually prints to stderr both for cur_mon == NULL and for QMP monitors. Is there an important difference between both cases? There is error_vprintf_unless_qmp(), which behaves differently for both cases. However, it is only used in VNC code, so that code would have to keep coroutines disabled. Is cur_mon used much in other functions called by QMP handlers? > @cur_mon is thread-local. > > I'm afraid we have to save, clear and restore @cur_mon around a yield. That sounds painful enough that I'd rather avoid it. Kevin
Hi On Mon, Mar 23, 2020 at 6:41 PM Kevin Wolf <kwolf@redhat.com> wrote: > > Am 18.03.2020 um 16:36 hat Markus Armbruster geschrieben: > > Kevin Wolf <kwolf@redhat.com> writes: > > > > > This moves the QMP dispatcher to a coroutine and runs all QMP command > > > handlers that declare 'coroutine': true in coroutine context so they > > > can avoid blocking the main loop while doing I/O or waiting for other > > > events. > > > > > > For commands that are not declared safe to run in a coroutine, the > > > dispatcher drops out of coroutine context by calling the QMP command > > > handler from a bottom half. > > > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > > Uh, what about @cur_mon? > > > > @cur_mon points to the current monitor while a command executes. > > Initial value is null. It is set in three places (not counting unit > > tests), and all three save, set, do something that may use @cur_mon, > > restore: > > > > * monitor_qmp_dispatch(), for use within qmp_dispatch() > > * monitor_read(), for use within handle_hmp_command() > > * qmp_human_monitor_command(), also for use within handle_hmp_command() > > > > Therefore, @cur_mon is null unless we're running within qmp_dispatch() > > or handle_hmp_command(). > > Can we make it NULL for coroutine-enabled handlers? fwiw, I think /dev/fdset doesn't care about cur_mon. However, qmp handlers that use monitor_get_fd() usually depend on cur_mon. Note: I wonder if add-fd (fdsets) and getfd (monitor fds) deserve to co-exist. > > > Example of use: error_report() & friends print "to current monitor if we > > have one, else to stderr." Makes sharing code between HMP and CLI > > easier. Uses @cur_mon under the hood. > > error_report() eventually prints to stderr both for cur_mon == NULL and > for QMP monitors. Is there an important difference between both cases? > > There is error_vprintf_unless_qmp(), which behaves differently for both > cases. However, it is only used in VNC code, so that code would have to > keep coroutines disabled. > > Is cur_mon used much in other functions called by QMP handlers? > > > @cur_mon is thread-local. > > > > I'm afraid we have to save, clear and restore @cur_mon around a yield. > > That sounds painful enough that I'd rather avoid it. > > Kevin >
Kevin Wolf <kwolf@redhat.com> writes: > Am 18.03.2020 um 16:36 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > This moves the QMP dispatcher to a coroutine and runs all QMP command >> > handlers that declare 'coroutine': true in coroutine context so they >> > can avoid blocking the main loop while doing I/O or waiting for other >> > events. >> > >> > For commands that are not declared safe to run in a coroutine, the >> > dispatcher drops out of coroutine context by calling the QMP command >> > handler from a bottom half. >> > >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> >> Uh, what about @cur_mon? >> >> @cur_mon points to the current monitor while a command executes. >> Initial value is null. It is set in three places (not counting unit >> tests), and all three save, set, do something that may use @cur_mon, >> restore: >> >> * monitor_qmp_dispatch(), for use within qmp_dispatch() >> * monitor_read(), for use within handle_hmp_command() >> * qmp_human_monitor_command(), also for use within handle_hmp_command() >> >> Therefore, @cur_mon is null unless we're running within qmp_dispatch() >> or handle_hmp_command(). > > Can we make it NULL for coroutine-enabled handlers? Sets up and arms a bear trap, I'm afraid. We can rely on code review to ensure a handler we want to coroutine-enable does not access @cur_mon. But how can we ensure no uses creep back? Not even on error paths, where testing tends to be ineffective? @cur_mon goes back to Jan's commit 376253ece48 (March 2009). Meant to be merely a stop gap: For the case that monitor outputs so far happen without clearly identifiable context, the global variable cur_mon is introduced that shall once provide a pointer either to the current active monitor (while processing commands) or to the default one. On the mid or long term, those use case will be obsoleted so that this variable can be removed again. It's been eleven years, and we haven't really gotten closer to getting them "obsoleted". I can't stop you if you want to try. Myself, I'd rather give up and eliminate the Monitor * parameter passing. >> Example of use: error_report() & friends print "to current monitor if we >> have one, else to stderr." Makes sharing code between HMP and CLI >> easier. Uses @cur_mon under the hood. > > error_report() eventually prints to stderr both for cur_mon == NULL and > for QMP monitors. Is there an important difference between both cases? This goes back to commit 4ad417baa43424b6b988c52b83989fd95670c113 Author: Cole Robinson <crobinso@redhat.com> Date: Fri Mar 21 19:42:24 2014 -0400 error: Print error_report() to stderr if using qmp monitor_printf will drop the requested output if cur_mon is qmp (for good reason). However these messages are often helpful for debugging issues with via libvirt. If we know the message won't hit the monitor, send it to stderr. Cc: Luiz Capitulino <lcapitulino@redhat.com> Cc: Markus Armbruster <armbru@redhat.com> Signed-off-by: Cole Robinson <crobinso@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> Use of error_report() in QMP context is wrong more often than not. Still, spewing the message to stderr is less bad than throwing it away. > There is error_vprintf_unless_qmp(), which behaves differently for both > cases. However, it is only used in VNC code, so that code would have to > keep coroutines disabled. It has seen other users over the years. error_printf_unless_qmp() is a convenient way to print hints for humans. In HMP context, you reach the human by printing to @cur_mon. Outside monitor context, you reach the human by printing to stdout or stderr. In QMP context, we're talking to a machine. > Is cur_mon used much in other functions called by QMP handlers? Marc-André pointed to the file-descriptor passing stuff. To be sure that's all, we'd have to review all uses of @cur_mon. >> @cur_mon is thread-local. >> >> I'm afraid we have to save, clear and restore @cur_mon around a yield. > > That sounds painful enough that I'd rather avoid it. I've seen coroutine implementations for C that provide coroutine-local variables of sorts by managing a thread-local opaque pointer.
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Mon, Mar 23, 2020 at 6:41 PM Kevin Wolf <kwolf@redhat.com> wrote: >> >> Am 18.03.2020 um 16:36 hat Markus Armbruster geschrieben: >> > Kevin Wolf <kwolf@redhat.com> writes: >> > >> > > This moves the QMP dispatcher to a coroutine and runs all QMP command >> > > handlers that declare 'coroutine': true in coroutine context so they >> > > can avoid blocking the main loop while doing I/O or waiting for other >> > > events. >> > > >> > > For commands that are not declared safe to run in a coroutine, the >> > > dispatcher drops out of coroutine context by calling the QMP command >> > > handler from a bottom half. >> > > >> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> > >> > Uh, what about @cur_mon? >> > >> > @cur_mon points to the current monitor while a command executes. >> > Initial value is null. It is set in three places (not counting unit >> > tests), and all three save, set, do something that may use @cur_mon, >> > restore: >> > >> > * monitor_qmp_dispatch(), for use within qmp_dispatch() >> > * monitor_read(), for use within handle_hmp_command() >> > * qmp_human_monitor_command(), also for use within handle_hmp_command() >> > >> > Therefore, @cur_mon is null unless we're running within qmp_dispatch() >> > or handle_hmp_command(). >> >> Can we make it NULL for coroutine-enabled handlers? > > fwiw, I think /dev/fdset doesn't care about cur_mon. However, qmp > handlers that use monitor_get_fd() usually depend on cur_mon. > > Note: I wonder if add-fd (fdsets) and getfd (monitor fds) deserve to co-exist. Beats me. If one of them is more general, we could consider deprecating the other one.
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index d6ce9efc8e..6812e49b5f 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -30,6 +30,7 @@ typedef enum QmpCommandOptions typedef struct QmpCommand { const char *name; + /* Runs in coroutine context if QCO_COROUTINE is set */ QmpCommandFunc *fn; QmpCommandOptions options; QTAILQ_ENTRY(QmpCommand) node; diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index 3e6baba88f..f8123b151a 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -155,7 +155,9 @@ static inline bool monitor_is_qmp(const Monitor *mon) typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList; extern IOThread *mon_iothread; -extern QEMUBH *qmp_dispatcher_bh; +extern Coroutine *qmp_dispatcher_co; +extern bool qmp_dispatcher_co_shutdown; +extern bool qmp_dispatcher_co_busy; extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands; extern QemuMutex monitor_lock; extern MonitorList mon_list; @@ -173,7 +175,7 @@ void monitor_fdsets_cleanup(void); void qmp_send_response(MonitorQMP *mon, const QDict *rsp); void monitor_data_destroy_qmp(MonitorQMP *mon); -void monitor_qmp_bh_dispatcher(void *data); +void coroutine_fn monitor_qmp_dispatcher_co(void *data); int get_monitor_def(int64_t *pval, const char *name); void help_cmd(Monitor *mon, const char *name); diff --git a/monitor/monitor.c b/monitor/monitor.c index c1a6c4460f..72d57b5cd2 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -53,8 +53,32 @@ typedef struct { /* Shared monitor I/O thread */ IOThread *mon_iothread; -/* Bottom half to dispatch the requests received from I/O thread */ -QEMUBH *qmp_dispatcher_bh; +/* Coroutine to dispatch the requests received from I/O thread */ +Coroutine *qmp_dispatcher_co; + +/* Set to true when the dispatcher coroutine should terminate */ +bool qmp_dispatcher_co_shutdown; + +/* + * qmp_dispatcher_co_busy is used for synchronisation between the + * monitor thread and the main thread to ensure that the dispatcher + * coroutine never gets scheduled a second time when it's already + * scheduled (scheduling the same coroutine twice is forbidden). + * + * It is true if the coroutine is active and processing requests. + * Additional requests may then be pushed onto a mon->qmp_requests, + * and @qmp_dispatcher_co_shutdown may be set without further ado. + * @qmp_dispatcher_co_busy must not be woken up in this case. + * + * If false, you also have to set @qmp_dispatcher_co_busy to true and + * wake up @qmp_dispatcher_co after pushing the new requests. + * + * The coroutine will automatically change this variable back to false + * before it yields. Nobody else may set the variable to false. + * + * Access must be atomic for thread safety. + */ +bool qmp_dispatcher_co_busy; /* Protects mon_list, monitor_qapi_event_state, monitor_destroyed. */ QemuMutex monitor_lock; @@ -579,9 +603,24 @@ void monitor_cleanup(void) } qemu_mutex_unlock(&monitor_lock); - /* QEMUBHs needs to be deleted before destroying the I/O thread */ - qemu_bh_delete(qmp_dispatcher_bh); - qmp_dispatcher_bh = NULL; + /* + * The dispatcher needs to stop before destroying the I/O thread. + * + * We need to poll both qemu_aio_context and iohandler_ctx to make + * sure that the dispatcher coroutine keeps making progress and + * eventually terminates. qemu_aio_context is automatically + * polled by calling AIO_WAIT_WHILE on it, but we must poll + * iohandler_ctx manually. + */ + qmp_dispatcher_co_shutdown = true; + if (!atomic_xchg(&qmp_dispatcher_co_busy, true)) { + aio_co_wake(qmp_dispatcher_co); + } + + AIO_WAIT_WHILE(qemu_get_aio_context(), + (aio_poll(iohandler_get_aio_context(), false), + atomic_mb_read(&qmp_dispatcher_co_busy))); + if (mon_iothread) { iothread_destroy(mon_iothread); mon_iothread = NULL; @@ -604,9 +643,9 @@ void monitor_init_globals_core(void) * have commands assuming that context. It would be nice to get * rid of those assumptions. */ - qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(), - monitor_qmp_bh_dispatcher, - NULL); + qmp_dispatcher_co = qemu_coroutine_create(monitor_qmp_dispatcher_co, NULL); + atomic_mb_set(&qmp_dispatcher_co_busy, true); + aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co); } int monitor_init_opts(QemuOpts *opts, Error **errp) diff --git a/monitor/qmp.c b/monitor/qmp.c index 8379c8f96e..d572d92e09 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -133,6 +133,10 @@ static void monitor_qmp_respond(MonitorQMP *mon, QDict *rsp) } } +/* + * Runs outside of coroutine context for OOB commands, but in + * coroutine context for everything else. + */ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req) { Monitor *old_mon; @@ -211,43 +215,99 @@ static QMPRequest *monitor_qmp_requests_pop_any_with_lock(void) return req_obj; } -void monitor_qmp_bh_dispatcher(void *data) +void coroutine_fn monitor_qmp_dispatcher_co(void *data) { - QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock(); + QMPRequest *req_obj = NULL; QDict *rsp; bool need_resume; MonitorQMP *mon; - if (!req_obj) { - return; - } + while (true) { + assert(atomic_mb_read(&qmp_dispatcher_co_busy) == true); - mon = req_obj->mon; - /* qmp_oob_enabled() might change after "qmp_capabilities" */ - need_resume = !qmp_oob_enabled(mon) || - mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1; - qemu_mutex_unlock(&mon->qmp_queue_lock); - if (req_obj->req) { - QDict *qdict = qobject_to(QDict, req_obj->req); - QObject *id = qdict ? qdict_get(qdict, "id") : NULL; - trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: ""); - monitor_qmp_dispatch(mon, req_obj->req); - } else { - assert(req_obj->err); - rsp = qmp_error_response(req_obj->err); - req_obj->err = NULL; - monitor_qmp_respond(mon, rsp); - qobject_unref(rsp); - } + /* + * Mark the dispatcher as not busy already here so that we + * don't miss any new requests coming in the middle of our + * processing. + */ + atomic_mb_set(&qmp_dispatcher_co_busy, false); + + while (!(req_obj = monitor_qmp_requests_pop_any_with_lock())) { + /* + * No more requests to process. Wait to be reentered from + * handle_qmp_command() when it pushes more requests, or + * from monitor_cleanup() when it requests shutdown. + */ + if (!qmp_dispatcher_co_shutdown) { + qemu_coroutine_yield(); + + /* + * busy must be set to true again by whoever + * rescheduled us to avoid double scheduling + */ + assert(atomic_xchg(&qmp_dispatcher_co_busy, false) == true); + } + + /* + * qmp_dispatcher_co_shutdown may have changed if we + * yielded and were reentered from monitor_cleanup() + */ + if (qmp_dispatcher_co_shutdown) { + return; + } + } - if (need_resume) { - /* Pairs with the monitor_suspend() in handle_qmp_command() */ - monitor_resume(&mon->common); - } - qmp_request_free(req_obj); + if (atomic_xchg(&qmp_dispatcher_co_busy, true) == true) { + /* + * Someone rescheduled us (probably because a new requests + * came in), but we didn't actually yield. Do that now, + * only to be immediately reentered and removed from the + * list of scheduled coroutines. + */ + qemu_coroutine_yield(); + } - /* Reschedule instead of looping so the main loop stays responsive */ - qemu_bh_schedule(qmp_dispatcher_bh); + /* + * Move the coroutine from iohandler_ctx to qemu_aio_context for + * executing the command handler so that it can make progress if it + * involves an AIO_WAIT_WHILE(). + */ + aio_co_schedule(qemu_get_aio_context(), qmp_dispatcher_co); + qemu_coroutine_yield(); + + mon = req_obj->mon; + /* qmp_oob_enabled() might change after "qmp_capabilities" */ + need_resume = !qmp_oob_enabled(mon) || + mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1; + qemu_mutex_unlock(&mon->qmp_queue_lock); + if (req_obj->req) { + QDict *qdict = qobject_to(QDict, req_obj->req); + QObject *id = qdict ? qdict_get(qdict, "id") : NULL; + trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: ""); + monitor_qmp_dispatch(mon, req_obj->req); + } else { + assert(req_obj->err); + rsp = qmp_error_response(req_obj->err); + req_obj->err = NULL; + monitor_qmp_respond(mon, rsp); + qobject_unref(rsp); + } + + if (need_resume) { + /* Pairs with the monitor_suspend() in handle_qmp_command() */ + monitor_resume(&mon->common); + } + qmp_request_free(req_obj); + + /* + * Yield and reschedule so the main loop stays responsive. + * + * Move back to iohandler_ctx so that nested event loops for + * qemu_aio_context don't start new monitor commands. + */ + aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co); + qemu_coroutine_yield(); + } } static void handle_qmp_command(void *opaque, QObject *req, Error *err) @@ -308,7 +368,9 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err) qemu_mutex_unlock(&mon->qmp_queue_lock); /* Kick the dispatcher routine */ - qemu_bh_schedule(qmp_dispatcher_bh); + if (!atomic_xchg(&qmp_dispatcher_co_busy, true)) { + aio_co_wake(qmp_dispatcher_co); + } } static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size) diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index bc264b3c9b..eef09d15bc 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -12,12 +12,16 @@ */ #include "qemu/osdep.h" + +#include "block/aio.h" #include "qapi/error.h" #include "qapi/qmp/dispatch.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qjson.h" #include "sysemu/runstate.h" #include "qapi/qmp/qbool.h" +#include "qemu/coroutine.h" +#include "qemu/main-loop.h" static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob, Error **errp) @@ -75,6 +79,25 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob, return dict; } +typedef struct QmpDispatchBH { + QmpCommand *cmd; + QDict *args; + QObject **ret; + Error **errp; + Coroutine *co; +} QmpDispatchBH; + +static void do_qmp_dispatch_bh(void *opaque) +{ + QmpDispatchBH *data = opaque; + data->cmd->fn(data->args, data->ret, data->errp); + aio_co_wake(data->co); +} + +/* + * Runs outside of coroutine context for OOB commands, but in coroutine context + * for everything else. + */ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request, bool allow_oob, Error **errp) { @@ -129,7 +152,22 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request, qobject_ref(args); } - cmd->fn(args, &ret, &local_err); + assert(!(oob && qemu_in_coroutine())); + if ((cmd->options & QCO_COROUTINE) || !qemu_in_coroutine()) { + cmd->fn(args, &ret, &local_err); + } else { + /* Must drop out of coroutine context for this one */ + QmpDispatchBH data = { + .cmd = cmd, + .args = args, + .ret = &ret, + .errp = &local_err, + .co = qemu_coroutine_self(), + }; + aio_bh_schedule_oneshot(qemu_get_aio_context(), do_qmp_dispatch_bh, + &data); + qemu_coroutine_yield(); + } if (local_err) { error_propagate(errp, local_err); } else if (cmd->options & QCO_NO_SUCCESS_RESP) { @@ -164,6 +202,10 @@ bool qmp_is_oob(const QDict *dict) && !qdict_haskey(dict, "execute"); } +/* + * Runs outside of coroutine context for OOB commands, but in coroutine context + * for everything else. + */ QDict *qmp_dispatch(QmpCommandList *cmds, QObject *request, bool allow_oob) { diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c index ca00f74795..1b5b9f1ccc 100644 --- a/qapi/qmp-registry.c +++ b/qapi/qmp-registry.c @@ -20,6 +20,9 @@ void qmp_register_command(QmpCommandList *cmds, const char *name, { QmpCommand *cmd = g_malloc0(sizeof(*cmd)); + /* QCO_COROUTINE and QCO_ALLOW_OOB are incompatible for now */ + assert(!((options & QCO_COROUTINE) && (options & QCO_ALLOW_OOB))); + cmd->name = name; cmd->fn = fn; cmd->enabled = true; diff --git a/util/aio-posix.c b/util/aio-posix.c index a4977f538e..223de08b91 100644 --- a/util/aio-posix.c +++ b/util/aio-posix.c @@ -15,6 +15,7 @@ #include "qemu/osdep.h" #include "block/block.h" +#include "qemu/main-loop.h" #include "qemu/rcu_queue.h" #include "qemu/sockets.h" #include "qemu/cutils.h" @@ -616,7 +617,11 @@ bool aio_poll(AioContext *ctx, bool blocking) int64_t timeout; int64_t start = 0; - assert(in_aio_context_home_thread(ctx)); + /* aio_poll() may only be called in the AioContext's thread. iohandler_ctx + * is special in that it runs in the main thread, but that thread's context + * is qemu_aio_context. */ + assert(in_aio_context_home_thread(ctx == iohandler_get_aio_context() ? + qemu_get_aio_context() : ctx)); /* aio_notify can avoid the expensive event_notifier_set if * everything (file descriptors, bottom halves, timers) will
This moves the QMP dispatcher to a coroutine and runs all QMP command handlers that declare 'coroutine': true in coroutine context so they can avoid blocking the main loop while doing I/O or waiting for other events. For commands that are not declared safe to run in a coroutine, the dispatcher drops out of coroutine context by calling the QMP command handler from a bottom half. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qapi/qmp/dispatch.h | 1 + monitor/monitor-internal.h | 6 +- monitor/monitor.c | 55 +++++++++++++--- monitor/qmp.c | 122 +++++++++++++++++++++++++++--------- qapi/qmp-dispatch.c | 44 ++++++++++++- qapi/qmp-registry.c | 3 + util/aio-posix.c | 7 ++- 7 files changed, 196 insertions(+), 42 deletions(-)