Message ID | 20191108150123.12213-1-marcandre.lureau@redhat.com (mailing list archive) |
---|---|
Headers | show |
Series | monitor: add asynchronous command type | expand |
Am 08.11.2019 um 16:00 hat Marc-André Lureau geschrieben: > The following series implements an internal async command solution > instead. By introducing a session context and a command return > handler, QMP handlers can: > - defer the return, allowing the mainloop to reenter > - return only to the caller (instead of the broadcast event reply) > - optionnally allow cancellation when the client is gone > - track on-going qapi command(s) per session This requires major changes to existing QMP command handlers. Did you consider at least optionally providing a way where instead of using the new async_fn, QMP already creates a coroutine in which the handler is executed? At least for some of the block layer commands, we could simply enable this without changing any of the code and would automatically get rid of blocking the guest while the command is doing I/O. If we need to implement .async_fn, in contrast, it would be quite a bit of boiler plate code for each single handler to create a coroutine for the real handler and to wrap all parameters in a struct. I started playing a bit with this and it didn't look too bad, but we can't move every command handler to a coroutine without auditing it, so I would have had to add a new option to the QAPI schema - and at that point I thought that I might end up duplicating a lot of your code if I continued. So I'm now trying to get your opinion or advice before I continue with anything in that direction. > This does not introduce new QMP APIs or client visible changes, the > command are handled in order, and the reply still come in order (even > when handlers finish out of order). > > Existing qemu commands can be gradually replaced by async:true > variants when needed, while carefully reviewing the concurrency > aspects. The async:true commands marshaller helpers are splitted in > half, the calling and return functions. The command is called with a > QmpReturn context, that can return immediately or later, using the > generated return helper. This part would certainly become simpler with coroutines (the marshaller could stay unchanged). > The screendump command is converted to an async:true version to solve > rhbz#1230527. The command shows basic cancellation (this could be > extended if needed). It could be further improved to do asynchronous > IO writes as well. After converting it to QIOChannel like you already do, I/O would automatically become asynchronous when run in a coroutine. If you don't want this yet, but only fix the BZ, I guess you could delay that patch until later and just have a single yield and reenter of the command handler coroutine like this: co = qemu_coroutine_self(); aio_co_schedule(qemu_coroutine_get_aio_context(co), co); qemu_coroutine_yield(); Kevin
Hi On Fri, Dec 13, 2019 at 8:04 PM Kevin Wolf <kwolf@redhat.com> wrote: > > Am 08.11.2019 um 16:00 hat Marc-André Lureau geschrieben: > > The following series implements an internal async command solution > > instead. By introducing a session context and a command return > > handler, QMP handlers can: > > - defer the return, allowing the mainloop to reenter > > - return only to the caller (instead of the broadcast event reply) > > - optionnally allow cancellation when the client is gone > > - track on-going qapi command(s) per session > > This requires major changes to existing QMP command handlers. Did you > consider at least optionally providing a way where instead of using the > new async_fn, QMP already creates a coroutine in which the handler is > executed? Yes, but I don't see how this could be done without the basic callback infrastructure I propose here. Also forcing existing code to be translated to coroutine-aware is probably even more complicated. > > At least for some of the block layer commands, we could simply enable > this without changing any of the code and would automatically get rid of > blocking the guest while the command is doing I/O. If we need to > implement .async_fn, in contrast, it would be quite a bit of boiler > plate code for each single handler to create a coroutine for the real > handler and to wrap all parameters in a struct. We could have the generator do that for you eventually, and spawn the coroutine. > > I started playing a bit with this and it didn't look too bad, but we > can't move every command handler to a coroutine without auditing it, so > I would have had to add a new option to the QAPI schema - and at that > point I thought that I might end up duplicating a lot of your code if I > continued. So I'm now trying to get your opinion or advice before I > continue with anything in that direction. thanks for looking at this old series! > > > This does not introduce new QMP APIs or client visible changes, the > > command are handled in order, and the reply still come in order (even > > when handlers finish out of order). > > > > Existing qemu commands can be gradually replaced by async:true > > variants when needed, while carefully reviewing the concurrency > > aspects. The async:true commands marshaller helpers are splitted in > > half, the calling and return functions. The command is called with a > > QmpReturn context, that can return immediately or later, using the > > generated return helper. > > This part would certainly become simpler with coroutines (the marshaller > could stay unchanged). That's not much change honestly. I am not sure sneaking a coroutine behind its back is going to be simpler, I would need to look at it. > > > The screendump command is converted to an async:true version to solve > > rhbz#1230527. The command shows basic cancellation (this could be > > extended if needed). It could be further improved to do asynchronous > > IO writes as well. > > After converting it to QIOChannel like you already do, I/O would > automatically become asynchronous when run in a coroutine. > > If you don't want this yet, but only fix the BZ, I guess you could delay > that patch until later and just have a single yield and reenter of the > command handler coroutine like this: > > co = qemu_coroutine_self(); > aio_co_schedule(qemu_coroutine_get_aio_context(co), co); > qemu_coroutine_yield(); > If various places of code start doing that, we are in trouble, the stack may grow, cancellation becomes hairy. Furthermore, in the case of screendump, IO is not necessarily within the coroutine context. In this case, we need to wait for the QXL device to "flush" the screen. Communicating this event back to the coroutine isn't simpler than what I propose here. Thanks!
Am 13.12.2019 um 17:28 hat Marc-André Lureau geschrieben: > On Fri, Dec 13, 2019 at 8:04 PM Kevin Wolf <kwolf@redhat.com> wrote: > > > > Am 08.11.2019 um 16:00 hat Marc-André Lureau geschrieben: > > > The following series implements an internal async command solution > > > instead. By introducing a session context and a command return > > > handler, QMP handlers can: > > > - defer the return, allowing the mainloop to reenter > > > - return only to the caller (instead of the broadcast event reply) > > > - optionnally allow cancellation when the client is gone > > > - track on-going qapi command(s) per session > > > > This requires major changes to existing QMP command handlers. Did you > > consider at least optionally providing a way where instead of using the > > new async_fn, QMP already creates a coroutine in which the handler is > > executed? > > Yes, but I don't see how this could be done without the basic callback > infrastructure I propose here. Also forcing existing code to be > translated to coroutine-aware is probably even more complicated. I'll attach what I hacked up last week after discussing a problem with Markus and Max. As you probably expected, screendump isn't really my main motivation to look into this. The specific command we discussed was block_resize, which can potentially block the monitor for a while, but I'm sure that many more block commands have the same problem. What my patch does is moving everything into a coroutine. This is wrong because not everything can be run in a coroutine, so it needs to be made optional (like you did with your async flag). The problem isn't with completely coroutine-unaware code, though: That one would just work, even if not taking advantage from the coroutine. A potential problem exists with code that behaves differently when run in a coroutine or outside of coroutine context (generally by checking qemu_in_coroutine())), or calls of coroutine-unaware code into such functions. Running some command handlers outside of coroutine context wouldn't be hard to add to my patch (basically just a BH), but I haven't looked into the QAPI side of making it an option. > > At least for some of the block layer commands, we could simply enable > > this without changing any of the code and would automatically get rid of > > blocking the guest while the command is doing I/O. If we need to > > implement .async_fn, in contrast, it would be quite a bit of boiler > > plate code for each single handler to create a coroutine for the real > > handler and to wrap all parameters in a struct. > > We could have the generator do that for you eventually, and spawn the > coroutine. Yes, if we need both, that's an option. I'd like to explore first if the callback-based approach is actually needed, though. > > > The screendump command is converted to an async:true version to solve > > > rhbz#1230527. The command shows basic cancellation (this could be > > > extended if needed). It could be further improved to do asynchronous > > > IO writes as well. > > > > After converting it to QIOChannel like you already do, I/O would > > automatically become asynchronous when run in a coroutine. > > > > If you don't want this yet, but only fix the BZ, I guess you could delay > > that patch until later and just have a single yield and reenter of the > > command handler coroutine like this: > > > > co = qemu_coroutine_self(); > > aio_co_schedule(qemu_coroutine_get_aio_context(co), co); > > qemu_coroutine_yield(); (This specific code is wrong, I misread your patches. You don't want to immediately reenter the coroutine, but only in graphic_hw_update_done().) > If various places of code start doing that, we are in trouble, the > stack may grow, cancellation becomes hairy. I don't understand. How does the coroutine-based approach differ from what your series does? Basically, instead of splitting qmp_screendump() in two parts, you keep the existing single function, just with a qemu_coroutine_yield() in the middle, and instead of calling qmp_screendump_finish() you wake up the coroutine. Yes, instead of malloc'ing a struct qmp_screendump, you would keep things on the stack, but that doesn't make the stack grow as these things are already on the stack in qmp_screendump(). > Furthermore, in the case of screendump, IO is not necessarily within > the coroutine context. In this case, we need to wait for the QXL > device to "flush" the screen. Communicating this event back to the > coroutine isn't simpler than what I propose here. Waiting for something in a coroutine means calling qemu_coroutine_yield() and then letting the event handler call aio_co_wake() to resume the coroutine. It's really simple. I'm pretty sure that if qmp_screendump() is called in a coroutine, your I/O would automatically end up in the coroutine context unless you do something specifically to avoid it. Kevin From 8d4b8ba34d33d3bbf3f0a21703928eb68895e169 Mon Sep 17 00:00:00 2001 From: Kevin Wolf <kwolf@redhat.com> Date: Thu, 12 Dec 2019 16:32:27 +0100 Subject: [PATCH] qmp: Move dispatcher to a coroutine Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qapi/qmp/dispatch.h | 2 + monitor/monitor-internal.h | 5 ++- monitor/monitor.c | 24 +++++++---- monitor/qmp.c | 80 +++++++++++++++++++++++-------------- qapi/qmp-dispatch.c | 4 ++ 5 files changed, 75 insertions(+), 40 deletions(-) diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h index 9aa426a398..4088255463 100644 --- a/include/qapi/qmp/dispatch.h +++ b/include/qapi/qmp/dispatch.h @@ -29,6 +29,8 @@ typedef enum QmpCommandOptions typedef struct QmpCommand { const char *name; + /* Runs outside of coroutine context for OOB commands, but in coroutine context + * for everything else. */ QmpCommandFunc *fn; QmpCommandOptions options; QTAILQ_ENTRY(QmpCommand) node; diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h index d78f5ca190..7389b6a56c 100644 --- a/monitor/monitor-internal.h +++ b/monitor/monitor-internal.h @@ -154,7 +154,8 @@ 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_busy; extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands; extern QemuMutex monitor_lock; extern MonitorList mon_list; @@ -172,7 +173,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 12898b6448..8f90d0b46c 100644 --- a/monitor/monitor.c +++ b/monitor/monitor.c @@ -53,8 +53,9 @@ 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; +bool qmp_dispatcher_co_busy; /* Protects mon_list, monitor_qapi_event_state, monitor_destroyed. */ QemuMutex monitor_lock; @@ -579,9 +580,16 @@ 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 */ + if (!atomic_mb_read(&qmp_dispatcher_co_busy)) { + aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co); + qmp_dispatcher_co = NULL; + } + + AIO_WAIT_WHILE(qemu_get_aio_context(), + aio_bh_poll(iohandler_get_aio_context()) || + atomic_mb_read(&qmp_dispatcher_co_busy)); + if (mon_iothread) { iothread_destroy(mon_iothread); mon_iothread = NULL; @@ -604,9 +612,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); } QemuOptsList qemu_mon_opts = { diff --git a/monitor/qmp.c b/monitor/qmp.c index b67a8e7d1f..f5dc77cd0a 100644 --- a/monitor/qmp.c +++ b/monitor/qmp.c @@ -133,6 +133,8 @@ 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 +213,59 @@ 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); + + while (!(req_obj = monitor_qmp_requests_pop_any_with_lock())) { + /* Wait to be reentered from handle_qmp_command, or terminate if + * qmp_dispatcher_co has been reset to NULL */ + atomic_mb_set(&qmp_dispatcher_co_busy, false); + if (qmp_dispatcher_co) { + qemu_coroutine_yield(); + } else { + return; + } + atomic_mb_set(&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); - } + 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); + if (need_resume) { + /* Pairs with the monitor_suspend() in handle_qmp_command() */ + monitor_resume(&mon->common); + } + qmp_request_free(req_obj); - /* Reschedule instead of looping so the main loop stays responsive */ - qemu_bh_schedule(qmp_dispatcher_bh); + /* Reschedule instead of looping so the main loop stays responsive */ + 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 +326,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_mb_read(&qmp_dispatcher_co_busy)) { + 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..1dbc152307 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -75,6 +75,8 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, bool allow_oob, return dict; } +/* 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) { @@ -164,6 +166,8 @@ 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) {
Hi Kevin, On Mon, Dec 16, 2019 at 4:07 PM Kevin Wolf <kwolf@redhat.com> wrote: > > Am 13.12.2019 um 17:28 hat Marc-André Lureau geschrieben: > > On Fri, Dec 13, 2019 at 8:04 PM Kevin Wolf <kwolf@redhat.com> wrote: > > > > > > Am 08.11.2019 um 16:00 hat Marc-André Lureau geschrieben: > > > > The following series implements an internal async command solution > > > > instead. By introducing a session context and a command return > > > > handler, QMP handlers can: > > > > - defer the return, allowing the mainloop to reenter > > > > - return only to the caller (instead of the broadcast event reply) > > > > - optionnally allow cancellation when the client is gone > > > > - track on-going qapi command(s) per session > > > > > > This requires major changes to existing QMP command handlers. Did you > > > consider at least optionally providing a way where instead of using the > > > new async_fn, QMP already creates a coroutine in which the handler is > > > executed? > > > > Yes, but I don't see how this could be done without the basic callback > > infrastructure I propose here. Also forcing existing code to be > > translated to coroutine-aware is probably even more complicated. > > I'll attach what I hacked up last week after discussing a problem with > Markus and Max. As you probably expected, screendump isn't really my > main motivation to look into this. The specific command we discussed was > block_resize, which can potentially block the monitor for a while, but > I'm sure that many more block commands have the same problem. Thanks, that helps me to understand your proposal (I had something more complicated in mind) > What my patch does is moving everything into a coroutine. This is wrong > because not everything can be run in a coroutine, so it needs to be made > optional (like you did with your async flag). "everything" is a bit too much ;) You proposal is to replace qmp_dispatch_bh by a coroutine version (except for OOB commands). This is nice because indeed, it allows to reenter the mainloop with a simple yield in QMP commands. It is also simpler than my "async" proposal, because some of the state is part of the coroutine, and because it doesn't allow QMP commands concurrency (beside existing OOB). Iow, coroutine (for async) + oob (for concurrency) make my proposal kinda obsolete. I can only regret that a simple callback-based solution looked simpler to me than one that mixes both threads & coroutines, but I don't mind if everybody likes it better :) I can definitely see the point for block commands, which rely on coroutines anyway, and qemu is already that complex in general. > > The problem isn't with completely coroutine-unaware code, though: That > one would just work, even if not taking advantage from the coroutine. A > potential problem exists with code that behaves differently when run in > a coroutine or outside of coroutine context (generally by checking > qemu_in_coroutine())), or calls of coroutine-unaware code into such > functions. > > Running some command handlers outside of coroutine context wouldn't be > hard to add to my patch (basically just a BH), but I haven't looked into > the QAPI side of making it an option. Yes, I think we should have a 'coroutine': true, for commands that should be run with a coroutine. Or perhaps replace existing allow-oob with 'dispatch': - 'bh' (default) - 'coroutine' - 'allow-oob' (oob + bh fallback, since oob don't have coroutine - at this point) Your patch looks quite good to me, but make check hangs. Have you looked at it?
Am 06.01.2020 um 19:21 hat Marc-André Lureau geschrieben: > > What my patch does is moving everything into a coroutine. This is wrong > > because not everything can be run in a coroutine, so it needs to be made > > optional (like you did with your async flag). > > "everything" is a bit too much ;) You proposal is to replace > qmp_dispatch_bh by a coroutine version (except for OOB commands). This > is nice because indeed, it allows to reenter the mainloop with a > simple yield in QMP commands. It is also simpler than my "async" > proposal, because some of the state is part of the coroutine, and > because it doesn't allow QMP commands concurrency (beside existing > OOB). > > Iow, coroutine (for async) + oob (for concurrency) make my proposal > kinda obsolete. I can only regret that a simple callback-based > solution looked simpler to me than one that mixes both threads & > coroutines, but I don't mind if everybody likes it better :) I can > definitely see the point for block commands, which rely on coroutines > anyway, and qemu is already that complex in general. Callbacks are indeed simple enough for implementing the infrastructure, but for the users they only look simple as long as they do trivial things. :-) Anyway, now that you have seen my POC hack, do you agree that this should help solving the screendump problem, too? > > The problem isn't with completely coroutine-unaware code, though: That > > one would just work, even if not taking advantage from the coroutine. A > > potential problem exists with code that behaves differently when run in > > a coroutine or outside of coroutine context (generally by checking > > qemu_in_coroutine())), or calls of coroutine-unaware code into such > > functions. > > > > Running some command handlers outside of coroutine context wouldn't be > > hard to add to my patch (basically just a BH), but I haven't looked into > > the QAPI side of making it an option. > > Yes, I think we should have a 'coroutine': true, for commands that > should be run with a coroutine. > > Or perhaps replace existing allow-oob with 'dispatch': > - 'bh' (default) > - 'coroutine' > - 'allow-oob' (oob + bh fallback, since oob don't have coroutine - at > this point) If it's "at this point", then making it two separate bools would make more sense. But I seem to remember that OOB handlers are fundamentally not supposed to block, so coroutine support would be pointless for them and an enum could work. I'll defer to Markus on this one. > Your patch looks quite good to me, but make check hangs. Have you > looked at it? I'm not sure if I was looking at some qemu-iotests cases or make check, but yes, I did see a hang. My case was a QMP command that just doesn't work correctly inside a coroutine without modifications, so requiring 'coroutine': true would fix it. Kevin
Hi On Tue, Jan 7, 2020 at 9:17 AM Kevin Wolf <kwolf@redhat.com> wrote: > > Am 06.01.2020 um 19:21 hat Marc-André Lureau geschrieben: > > > What my patch does is moving everything into a coroutine. This is wrong > > > because not everything can be run in a coroutine, so it needs to be made > > > optional (like you did with your async flag). > > > > "everything" is a bit too much ;) You proposal is to replace > > qmp_dispatch_bh by a coroutine version (except for OOB commands). This > > is nice because indeed, it allows to reenter the mainloop with a > > simple yield in QMP commands. It is also simpler than my "async" > > proposal, because some of the state is part of the coroutine, and > > because it doesn't allow QMP commands concurrency (beside existing > > OOB). > > > > Iow, coroutine (for async) + oob (for concurrency) make my proposal > > kinda obsolete. I can only regret that a simple callback-based > > solution looked simpler to me than one that mixes both threads & > > coroutines, but I don't mind if everybody likes it better :) I can > > definitely see the point for block commands, which rely on coroutines > > anyway, and qemu is already that complex in general. > > Callbacks are indeed simple enough for implementing the infrastructure, > but for the users they only look simple as long as they do trivial > things. :-) > > Anyway, now that you have seen my POC hack, do you agree that this > should help solving the screendump problem, too? Yes, and I will work on it as soon as you have a working patch series or branch :) > > > > The problem isn't with completely coroutine-unaware code, though: That > > > one would just work, even if not taking advantage from the coroutine. A > > > potential problem exists with code that behaves differently when run in > > > a coroutine or outside of coroutine context (generally by checking > > > qemu_in_coroutine())), or calls of coroutine-unaware code into such > > > functions. > > > > > > Running some command handlers outside of coroutine context wouldn't be > > > hard to add to my patch (basically just a BH), but I haven't looked into > > > the QAPI side of making it an option. > > > > Yes, I think we should have a 'coroutine': true, for commands that > > should be run with a coroutine. > > > > Or perhaps replace existing allow-oob with 'dispatch': > > - 'bh' (default) > > - 'coroutine' > > - 'allow-oob' (oob + bh fallback, since oob don't have coroutine - at > > this point) > > If it's "at this point", then making it two separate bools would make > more sense. But I seem to remember that OOB handlers are fundamentally > not supposed to block, so coroutine support would be pointless for them > and an enum could work. I think so too > > I'll defer to Markus on this one. Yup, Markus should take a look at your proposal and give some guidance. And hopefully, it won't take >2y. > > > Your patch looks quite good to me, but make check hangs. Have you > > looked at it? > > I'm not sure if I was looking at some qemu-iotests cases or make check, > but yes, I did see a hang. My case was a QMP command that just doesn't > work correctly inside a coroutine without modifications, so requiring > 'coroutine': true would fix it. ok, then I suggest you do a "minimal" patch series that works. thanks!
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Tue, Jan 7, 2020 at 9:17 AM Kevin Wolf <kwolf@redhat.com> wrote: >> >> Am 06.01.2020 um 19:21 hat Marc-André Lureau geschrieben: >> > > What my patch does is moving everything into a coroutine. This is wrong >> > > because not everything can be run in a coroutine, so it needs to be made >> > > optional (like you did with your async flag). >> > >> > "everything" is a bit too much ;) You proposal is to replace >> > qmp_dispatch_bh by a coroutine version (except for OOB commands). This >> > is nice because indeed, it allows to reenter the mainloop with a >> > simple yield in QMP commands. It is also simpler than my "async" >> > proposal, because some of the state is part of the coroutine, and >> > because it doesn't allow QMP commands concurrency (beside existing >> > OOB). >> > >> > Iow, coroutine (for async) + oob (for concurrency) make my proposal >> > kinda obsolete. I can only regret that a simple callback-based >> > solution looked simpler to me than one that mixes both threads & >> > coroutines, but I don't mind if everybody likes it better :) I can >> > definitely see the point for block commands, which rely on coroutines >> > anyway, and qemu is already that complex in general. >> >> Callbacks are indeed simple enough for implementing the infrastructure, >> but for the users they only look simple as long as they do trivial >> things. :-) >> >> Anyway, now that you have seen my POC hack, do you agree that this >> should help solving the screendump problem, too? > > Yes, and I will work on it as soon as you have a working patch series > or branch :) > >> >> > > The problem isn't with completely coroutine-unaware code, though: That >> > > one would just work, even if not taking advantage from the coroutine. A >> > > potential problem exists with code that behaves differently when run in >> > > a coroutine or outside of coroutine context (generally by checking >> > > qemu_in_coroutine())), or calls of coroutine-unaware code into such >> > > functions. >> > > >> > > Running some command handlers outside of coroutine context wouldn't be >> > > hard to add to my patch (basically just a BH), but I haven't looked into >> > > the QAPI side of making it an option. >> > >> > Yes, I think we should have a 'coroutine': true, for commands that >> > should be run with a coroutine. >> > >> > Or perhaps replace existing allow-oob with 'dispatch': >> > - 'bh' (default) >> > - 'coroutine' >> > - 'allow-oob' (oob + bh fallback, since oob don't have coroutine - at >> > this point) >> >> If it's "at this point", then making it two separate bools would make >> more sense. But I seem to remember that OOB handlers are fundamentally >> not supposed to block, so coroutine support would be pointless for them >> and an enum could work. > > I think so too > >> >> I'll defer to Markus on this one. > > Yup, Markus should take a look at your proposal and give some > guidance. And hopefully, it won't take >2y. Is it "[PATCH 0/4] qmp: Optionally run handlers in coroutines"? [...]
Am 13.01.2020 um 16:58 hat Markus Armbruster geschrieben: > Marc-André Lureau <marcandre.lureau@gmail.com> writes: > > > Hi > > > > On Tue, Jan 7, 2020 at 9:17 AM Kevin Wolf <kwolf@redhat.com> wrote: > >> > >> Am 06.01.2020 um 19:21 hat Marc-André Lureau geschrieben: > >> > > What my patch does is moving everything into a coroutine. This is wrong > >> > > because not everything can be run in a coroutine, so it needs to be made > >> > > optional (like you did with your async flag). > >> > > >> > "everything" is a bit too much ;) You proposal is to replace > >> > qmp_dispatch_bh by a coroutine version (except for OOB commands). This > >> > is nice because indeed, it allows to reenter the mainloop with a > >> > simple yield in QMP commands. It is also simpler than my "async" > >> > proposal, because some of the state is part of the coroutine, and > >> > because it doesn't allow QMP commands concurrency (beside existing > >> > OOB). > >> > > >> > Iow, coroutine (for async) + oob (for concurrency) make my proposal > >> > kinda obsolete. I can only regret that a simple callback-based > >> > solution looked simpler to me than one that mixes both threads & > >> > coroutines, but I don't mind if everybody likes it better :) I can > >> > definitely see the point for block commands, which rely on coroutines > >> > anyway, and qemu is already that complex in general. > >> > >> Callbacks are indeed simple enough for implementing the infrastructure, > >> but for the users they only look simple as long as they do trivial > >> things. :-) > >> > >> Anyway, now that you have seen my POC hack, do you agree that this > >> should help solving the screendump problem, too? > > > > Yes, and I will work on it as soon as you have a working patch series > > or branch :) > > > >> > >> > > The problem isn't with completely coroutine-unaware code, though: That > >> > > one would just work, even if not taking advantage from the coroutine. A > >> > > potential problem exists with code that behaves differently when run in > >> > > a coroutine or outside of coroutine context (generally by checking > >> > > qemu_in_coroutine())), or calls of coroutine-unaware code into such > >> > > functions. > >> > > > >> > > Running some command handlers outside of coroutine context wouldn't be > >> > > hard to add to my patch (basically just a BH), but I haven't looked into > >> > > the QAPI side of making it an option. > >> > > >> > Yes, I think we should have a 'coroutine': true, for commands that > >> > should be run with a coroutine. > >> > > >> > Or perhaps replace existing allow-oob with 'dispatch': > >> > - 'bh' (default) > >> > - 'coroutine' > >> > - 'allow-oob' (oob + bh fallback, since oob don't have coroutine - at > >> > this point) > >> > >> If it's "at this point", then making it two separate bools would make > >> more sense. But I seem to remember that OOB handlers are fundamentally > >> not supposed to block, so coroutine support would be pointless for them > >> and an enum could work. > > > > I think so too > > > >> > >> I'll defer to Markus on this one. > > > > Yup, Markus should take a look at your proposal and give some > > guidance. And hopefully, it won't take >2y. > > Is it "[PATCH 0/4] qmp: Optionally run handlers in coroutines"? Yes, that's the one. Kevin