Re: [PATCH v4 3/4] qmp: Move dispatcher to a coroutine
Am 19.02.2020 um 15:21 hat Markus Armbruster geschrieben: > I think we need to talk about AioContext in qapi-code-gen.txt. PATCH 1 > now adds > > Member 'coroutine' tells the QMP dispatcher whether the command handler > is safe to be run in a coroutine. It defaults to false. If it is true, > the command handler is called from coroutine context and may yield while > waiting for an external event (such as I/O completion) in order to avoid > blocking the guest and other background operations. > > What about "is called from a coroutine running in the main loop with > @qemu_aio_context, and may yield"? > > Likewise for the commit message of PATCH 3: > > 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. > > "calls all ... in a coroutine running in the main loop with > @qemu_aio_context, so they can". > > Speaking of PATCH 1: > > It is an error to specify both 'coroutine': true and 'allow-oob': true > for a command. (We don't currently have a use case for both together and > without a use case, it's not entirely clear what the semantics should be.) > > Let's lose the parenthesis around the last sentence. > > If you agree with my proposed tweaks, and nothing else comes up, I can > try to do them in my tree. Works for me. In the v5 thread, dropping patch 2 came up. I think it may not be needed any more in the current version and 'make check' passes without it. Kevin
Re: [PATCH v4 3/4] qmp: Move dispatcher to a coroutine
Markus Armbruster writes: [...] > If you agree with my proposed tweaks, and nothing else comes up, I can > try to do them in my tree. I'll tweak your v5, of course.
Re: [PATCH v4 3/4] qmp: Move dispatcher to a coroutine
Kevin Wolf writes: > Am 19.02.2020 um 10:03 hat Markus Armbruster geschrieben: >> >> >> @@ -246,8 +287,15 @@ void monitor_qmp_bh_dispatcher(void *data) >> >> >>} >> >> >>qmp_request_free(req_obj); >> >> >> >> >> >> -/* Reschedule instead of looping so the main loop stays >> >> >> responsive */ >> >> >> -qemu_bh_schedule(qmp_dispatcher_bh); >> >> >> +/* >> >> >> + * 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. >> >> >> >> >> >> Can you explain this sentence for dummies? >> >> > >> >> > Nested event loops (i.e. AIO_WAIT_WHILE) poll qemu_aio_context, so if we >> >> > are scheduled there, the next iteration of the monitor dispatcher loop >> >> > could start from a nested event loop. If we are scheduled in >> >> > iohandler_ctx, then only the actual main loop will reenter the coroutine >> >> > and nested event loops ignore it. >> >> > >> >> > I'm not sure if or why this is actually important, but this matches >> >> > scheduling the dispatcher BH in iohandler_ctx in the code before this >> >> > patch. >> >> > >> >> > If we didn't do this, we could end up starting monitor requests in more >> >> > places than before, and who knows what that would mean. >> >> >> >> Let me say it in my own words, to make sure I got it. I'm going to >> >> ignore special cases like "not using I/O thread" and exec-oob. >> >> >> >> QMP monitor I/O happens in mon_iothread, in iohandler_ctx (I think). >> >> This pushes requests onto the monitor's qmp_requests queue. >> > >> > mon_iothread (like all iothreads) has a separate AioContext, which >> > doesn't have a name, but can be accessed with >> > iothread_get_aio_context(mon_iothread). >> >> Got it. >> >> @iohandler_ctx and @qemu_aio_context both belong to the main loop. >> >> @qemu_aio_context is for general "run in the main loop" use. It is >> polled both in the actual main loop and event loops nested in it. I >> figure "both" to keep things responsive. >> >> @iohandler_ctx is for "I/O handlers" (whatever those are). It is polled >> only in the actual main loop. I figure this means "I/O handlers" may >> get delayed while a nested event loop runs. But better starve a bit >> than run in unexpected places. >> >> >> Before this patch, the dispatcher runs in a bottom half in the main >> >> thread, in qemu_aio_context. >> > >> > The dispatcher BH is what is scheduled in iohandler_ctx. This means that >> > the BH is run from the main loop, but not from nested event loops. >> >> Got it. >> >> >> The patch moves it to a coroutine running in the main thread. It runs >> >> in iohandler_ctx, but switches to qemu_aio_context for executing command >> >> handlers. >> >> >> >> We want to keep command handlers running in qemu_aio_context, as before >> >> this patch. >> > >> > "Running in AioContext X" is kind of a sloppy term, and I'm afraid it >> > might be more confusing than helpful here. What this means is that the >> > code is run while holding the lock of the AioContext, and it registers >> > its BHs, callbacks etc. in that AioContext so it will be called from the >> > event loop of the respective thread. >> > >> > Before this patch, command handlers can't really use callbacks without a >> > nested event loop because they are synchronous. >> >> I figure an example for such a nested event loop is BDRV_POLL_WHILE() in >> bdrv_truncate(), which runs in the command handler for block_resize. >> True? > > Yes. I think most nested event loops are in the block layer, and they > use the BDRV_POLL_WHILE() or AIO_WAIT_WHILE() macros today. > >> > The BQL is held, which >> > is equivalent to holding the qemu_aio_context lock. >> >> Why is it equivalent? Are they always taken together? > > I guess ideally they would be. In practice, we neglect the > qemu_aio_context lock and rely on the BQL for everything in the main > thread. So maybe I should say the BQL replaces the AioContext lock for > the main context rather than being equivalent. Sounds a bit sloppy. It works... >> > But other than that, >> > all of the definition of "running in an AioContext" doesn't really apply. >> > >> > Now with coroutines, you assign them an AioContext, which is the context >> > in which BHs reentering the coroutine will be scheduled, e.g. from >> > explicit aio_co_schedule(), after qemu_co_sleep_ns() or after waiting >> > for a lock like a CoMutex. >> > >> > qemu_aio_context is what most (if not all) of the existing QMP handlers >> > use when they run a nested event loop, >> >> bdrv_truncate() appears to use bdrv_get_aio_context(), which is the >> BlockDriverState's AioContext if set, else @qemu_aio_context. > > Right, the BDRV_POLL_WHILE() macro allows
Re: [PATCH v4 3/4] qmp: Move dispatcher to a coroutine
Am 19.02.2020 um 10:03 hat Markus Armbruster geschrieben: > >> >> @@ -246,8 +287,15 @@ void monitor_qmp_bh_dispatcher(void *data) > >> >>} > >> >>qmp_request_free(req_obj); > >> >> > >> >> -/* Reschedule instead of looping so the main loop stays > >> >> responsive */ > >> >> -qemu_bh_schedule(qmp_dispatcher_bh); > >> >> +/* > >> >> + * 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. > >> >> > >> >> Can you explain this sentence for dummies? > >> > > >> > Nested event loops (i.e. AIO_WAIT_WHILE) poll qemu_aio_context, so if we > >> > are scheduled there, the next iteration of the monitor dispatcher loop > >> > could start from a nested event loop. If we are scheduled in > >> > iohandler_ctx, then only the actual main loop will reenter the coroutine > >> > and nested event loops ignore it. > >> > > >> > I'm not sure if or why this is actually important, but this matches > >> > scheduling the dispatcher BH in iohandler_ctx in the code before this > >> > patch. > >> > > >> > If we didn't do this, we could end up starting monitor requests in more > >> > places than before, and who knows what that would mean. > >> > >> Let me say it in my own words, to make sure I got it. I'm going to > >> ignore special cases like "not using I/O thread" and exec-oob. > >> > >> QMP monitor I/O happens in mon_iothread, in iohandler_ctx (I think). > >> This pushes requests onto the monitor's qmp_requests queue. > > > > mon_iothread (like all iothreads) has a separate AioContext, which > > doesn't have a name, but can be accessed with > > iothread_get_aio_context(mon_iothread). > > Got it. > > @iohandler_ctx and @qemu_aio_context both belong to the main loop. > > @qemu_aio_context is for general "run in the main loop" use. It is > polled both in the actual main loop and event loops nested in it. I > figure "both" to keep things responsive. > > @iohandler_ctx is for "I/O handlers" (whatever those are). It is polled > only in the actual main loop. I figure this means "I/O handlers" may > get delayed while a nested event loop runs. But better starve a bit > than run in unexpected places. > > >> Before this patch, the dispatcher runs in a bottom half in the main > >> thread, in qemu_aio_context. > > > > The dispatcher BH is what is scheduled in iohandler_ctx. This means that > > the BH is run from the main loop, but not from nested event loops. > > Got it. > > >> The patch moves it to a coroutine running in the main thread. It runs > >> in iohandler_ctx, but switches to qemu_aio_context for executing command > >> handlers. > >> > >> We want to keep command handlers running in qemu_aio_context, as before > >> this patch. > > > > "Running in AioContext X" is kind of a sloppy term, and I'm afraid it > > might be more confusing than helpful here. What this means is that the > > code is run while holding the lock of the AioContext, and it registers > > its BHs, callbacks etc. in that AioContext so it will be called from the > > event loop of the respective thread. > > > > Before this patch, command handlers can't really use callbacks without a > > nested event loop because they are synchronous. > > I figure an example for such a nested event loop is BDRV_POLL_WHILE() in > bdrv_truncate(), which runs in the command handler for block_resize. > True? Yes. I think most nested event loops are in the block layer, and they use the BDRV_POLL_WHILE() or AIO_WAIT_WHILE() macros today. > > The BQL is held, which > > is equivalent to holding the qemu_aio_context lock. > > Why is it equivalent? Are they always taken together? I guess ideally they would be. In practice, we neglect the qemu_aio_context lock and rely on the BQL for everything in the main thread. So maybe I should say the BQL replaces the AioContext lock for the main context rather than being equivalent. > > But other than that, > > all of the definition of "running in an AioContext" doesn't really apply. > > > > Now with coroutines, you assign them an AioContext, which is the context > > in which BHs reentering the coroutine will be scheduled, e.g. from > > explicit aio_co_schedule(), after qemu_co_sleep_ns() or after waiting > > for a lock like a CoMutex. > > > > qemu_aio_context is what most (if not all) of the existing QMP handlers > > use when they run a nested event loop, > > bdrv_truncate() appears to use bdrv_get_aio_context(), which is the > BlockDriverState's AioContext if set, else @qemu_aio_context. Right, the BDRV_POLL_WHILE() macro allows waiting for something in a different AioContext from the main context. This works because of the aio_wait_kick() in bdrv_truncate_co_entry(), which schedules a BH in the main
Re: [PATCH v4 3/4] qmp: Move dispatcher to a coroutine
Kevin Wolf writes:
> Am 18.02.2020 um 15:12 hat Markus Armbruster geschrieben:
>> >> Regarding calling monitor_qmp_requests_pop_any_with_lock(): it needs
>> >> @monitor_lock and @mon_list to be valid. We just initialized
>> >> @monitor_lock, and @mon_list is empty.
>> >> monitor_qmp_requests_pop_any_with_lock() should be safe and return null.
>> >> monitor_qmp_dispatcher_co() should also be safe and yield without doing
>> >> work.
>> >>
>> >> Can we exploit that to make things a bit simpler? Separate patch would
>> >> be fine with me.
>> >
>> > If this is true, we could replace this line:
>> >
>> > aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
>> >
>> > with the following one:
>> >
>> > qemu_aio_coroutine_enter(iohandler_get_aio_context(),
>> > qmp_dispatcher_co);
>> >
>> > I'm not sure that this is any simpler.
>>
>> Naive question: what's the difference between "scheduling", "entering",
>> and "waking up" a coroutine?
>>
>> qemu_coroutine_enter() and qemu_aio_coroutine_enter() are in
>> coroutine.h.
>
> These are the low-level functions that just enter the coroutine (i.e. do
> a stack switch and jump to coroutine code) immediately when they are
> called. They must be called in the right thread with the right
> AioContext locks held. (What "right" means depends on the code run in
> the coroutine.)
I see; low level building blocks.
>> aio_co_schedule(), aio_co_wake() and aio_co_enter() are in aio.h.
>
> aio_co_schedule() never enters the coroutine directly. It only adds it
> to the list of scheduled coroutines and schedules a BH in the target
> AioContext. This BH in turn will actually enter the coroutine.
Makes sense.
> aio_co_enter() enters the coroutine immediately if it's being called
> outside of coroutine context and in the right thread for the given
> AioContext. If it's in the right thread, but in coroutine context then
> it will queue the given coroutine so that it runs whenever the current
> coroutine yields. If it's in the wrong thread, it uses aio_co_schedule()
> to get it run in the right thread.
Uff, sounds complicated. Lots of magic.
> aio_co_wake() takes just the coroutine as a parameter and calls
> aio_co_enter() with the context in which the coroutine was last run.
Complicated by extension.
> All of these functions make sure that the AioContext lock is taken when
> the coroutine is entered and that they run in the right thread.
>
>> qemu_coroutine_enter() calls qemu_aio_coroutine_enter().
>>
>> aio_co_wake() calls aio_co_enter() calls qemu_aio_coroutine_enter().
>>
>> aio_co_enter() seems to be independent.
>
> It's not. It calls either aio_co_schedule() or
> qemu_aio_coroutine_enter(), or inserts the coroutine ina queue that is
> processed in qemu_aio_coroutine_enter().
I was blind.
>> aio.h seems to be layered on top of coroutine.h. Should I prefer using
>> aio.h to coroutine.h?
>
> In the common case, using the aio.h functions is preferable because they
> just do "the right thing" without requiring as much thinking as the
> low-level functions.
Got it.
>> >> > }
>> >> >
>> >> > QemuOptsList qemu_mon_opts = {
>> >> > diff --git a/monitor/qmp.c b/monitor/qmp.c
>> >> > index 54c06ba824..9444de9fcf 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.
>> >> > + */
>> >>
>> >> Nitpick: wrap around column 70, please.
>> >>
>> >> Note to self: the precondition is asserted in do_qmp_dispatch() below.
>> >> Asserting here is impractical, because we don't know whether this is an
>> >> OOB command.
>> >>
>> >> > static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
>> >> > {
>> >> > Monitor *old_mon;
>> >> > @@ -211,43 +215,87 @@ 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(_dispatcher_co_busy) == true);
>> >>
>> >> Read and assert, then ...
>> >>
>> >> > +
>> >> > +/* 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(_dispatcher_co_busy, false);
>> >>
>> >> ... set. Would exchange, then assert be cleaner?
>> >
>> > Then you would ask me why the
Re: [PATCH v4 3/4] qmp: Move dispatcher to a coroutine
Am 18.02.2020 um 15:12 hat Markus Armbruster geschrieben:
> >> Regarding calling monitor_qmp_requests_pop_any_with_lock(): it needs
> >> @monitor_lock and @mon_list to be valid. We just initialized
> >> @monitor_lock, and @mon_list is empty.
> >> monitor_qmp_requests_pop_any_with_lock() should be safe and return null.
> >> monitor_qmp_dispatcher_co() should also be safe and yield without doing
> >> work.
> >>
> >> Can we exploit that to make things a bit simpler? Separate patch would
> >> be fine with me.
> >
> > If this is true, we could replace this line:
> >
> > aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
> >
> > with the following one:
> >
> > qemu_aio_coroutine_enter(iohandler_get_aio_context(),
> > qmp_dispatcher_co);
> >
> > I'm not sure that this is any simpler.
>
> Naive question: what's the difference between "scheduling", "entering",
> and "waking up" a coroutine?
>
> qemu_coroutine_enter() and qemu_aio_coroutine_enter() are in
> coroutine.h.
These are the low-level functions that just enter the coroutine (i.e. do
a stack switch and jump to coroutine code) immediately when they are
called. They must be called in the right thread with the right
AioContext locks held. (What "right" means depends on the code run in
the coroutine.)
> aio_co_schedule(), aio_co_wake() and aio_co_enter() are in aio.h.
aio_co_schedule() never enters the coroutine directly. It only adds it
to the list of scheduled coroutines and schedules a BH in the target
AioContext. This BH in turn will actually enter the coroutine.
aio_co_enter() enters the coroutine immediately if it's being called
outside of coroutine context and in the right thread for the given
AioContext. If it's in the right thread, but in coroutine context then
it will queue the given coroutine so that it runs whenever the current
coroutine yields. If it's in the wrong thread, it uses aio_co_schedule()
to get it run in the right thread.
aio_co_wake() takes just the coroutine as a parameter and calls
aio_co_enter() with the context in which the coroutine was last run.
All of these functions make sure that the AioContext lock is taken when
the coroutine is entered and that they run in the right thread.
> qemu_coroutine_enter() calls qemu_aio_coroutine_enter().
>
> aio_co_wake() calls aio_co_enter() calls qemu_aio_coroutine_enter().
>
> aio_co_enter() seems to be independent.
It's not. It calls either aio_co_schedule() or
qemu_aio_coroutine_enter(), or inserts the coroutine ina queue that is
processed in qemu_aio_coroutine_enter().
> aio.h seems to be layered on top of coroutine.h. Should I prefer using
> aio.h to coroutine.h?
In the common case, using the aio.h functions is preferable because they
just do "the right thing" without requiring as much thinking as the
low-level functions.
> >> > }
> >> >
> >> > QemuOptsList qemu_mon_opts = {
> >> > diff --git a/monitor/qmp.c b/monitor/qmp.c
> >> > index 54c06ba824..9444de9fcf 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.
> >> > + */
> >>
> >> Nitpick: wrap around column 70, please.
> >>
> >> Note to self: the precondition is asserted in do_qmp_dispatch() below.
> >> Asserting here is impractical, because we don't know whether this is an
> >> OOB command.
> >>
> >> > static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
> >> > {
> >> > Monitor *old_mon;
> >> > @@ -211,43 +215,87 @@ 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(_dispatcher_co_busy) == true);
> >>
> >> Read and assert, then ...
> >>
> >> > +
> >> > +/* 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(_dispatcher_co_busy, false);
> >>
> >> ... set. Would exchange, then assert be cleaner?
> >
> > Then you would ask me why the exchange has to be atomic. :-)
>
> Possibly :)
>
> > More practically, I would need a temporary variable so that I don't get
> > code with side effects in assert() (which may be compiled out with
> > NDEBUG). The temporary variable would never be read outside the assert
> > and would be unused with NDEBUG.
>
Re: [PATCH v4 3/4] qmp: Move dispatcher to a coroutine
Never been closer...
Kevin Wolf writes:
> Am 17.02.2020 um 12:08 hat Markus Armbruster geschrieben:
>> This is the hairy one. Please bear with me while I try to grok it.
>>
>> Kevin Wolf 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
>> > ---
>> > include/qapi/qmp/dispatch.h | 1 +
>> > monitor/monitor-internal.h | 6 +-
>> > monitor/monitor.c | 33 ---
>> > monitor/qmp.c | 110 ++--
>> > qapi/qmp-dispatch.c | 44 ++-
>> > qapi/qmp-registry.c | 3 +
>> > util/aio-posix.c| 7 ++-
>> > 7 files changed, 162 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 d78f5ca190..f180d03368 100644
>> > --- a/monitor/monitor-internal.h
>> > +++ b/monitor/monitor-internal.h
>> > @@ -154,7 +154,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;
>> > @@ -172,7 +174,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..e753fa435d 100644
>> > --- a/monitor/monitor.c
>> > +++ b/monitor/monitor.c
>> > @@ -53,8 +53,18 @@ 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;
>> > +
>> > +/*
>> > + * true if the coroutine is active and processing requests. The coroutine
>> > may
>> > + * only be woken up externally (e.g. from the monitor thread) after
>> > changing
>> > + * qmp_dispatcher_co_busy from false to true (e.g. using atomic_xchg).
>> > + */
>>
>> I'm not sure what you mean by "externally".
>
> By anyone outside the coroutine itself. Maybe just dropping the word
> "externally" avoids the confusion because it's an implementation detail
> that the coroutine can schedule itself while it is marked busy.
Let's do that. For me, a coroutine scheduling itself is not covered by
"woken up".
>> Also mention how it changes from true to false?
>
> Somethin like: "The coroutine will automatically change it back to false
> after processing all pending requests"?
Works for me.
More below.
>> Note to self: monitor_qmp_dispatcher_co() checks busy is true on resume.
>>
>> Nitpick: wrap around column 70, two spaces between sentences for
>> consistency with other comments in this file, please.
>
> Any specific reason why comments (but not code) in this file use a
> different text width than everything else in QEMU? My editor is set to
> use 80 characters to conform to CODING_STYLE.rst.
Legibility. Humans tend to have trouble following long lines with their
eyes (I sure do). Typographic manuals suggest to limit columns to
roughly 60 characters for exactly that reason[*].
Code is special. It's typically indented, and long identifiers push it
further to the right, function arguments in particular. We compromised
at 80 columns.
Block comments are not code. They are typically not indented much.
This one isn't indented at all. Line length without
Re: [PATCH v4 3/4] qmp: Move dispatcher to a coroutine
Am 17.02.2020 um 12:08 hat Markus Armbruster geschrieben:
> This is the hairy one. Please bear with me while I try to grok it.
>
> Kevin Wolf 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
> > ---
> > include/qapi/qmp/dispatch.h | 1 +
> > monitor/monitor-internal.h | 6 +-
> > monitor/monitor.c | 33 ---
> > monitor/qmp.c | 110 ++--
> > qapi/qmp-dispatch.c | 44 ++-
> > qapi/qmp-registry.c | 3 +
> > util/aio-posix.c| 7 ++-
> > 7 files changed, 162 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 d78f5ca190..f180d03368 100644
> > --- a/monitor/monitor-internal.h
> > +++ b/monitor/monitor-internal.h
> > @@ -154,7 +154,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;
> > @@ -172,7 +174,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..e753fa435d 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -53,8 +53,18 @@ 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;
> > +
> > +/*
> > + * true if the coroutine is active and processing requests. The coroutine
> > may
> > + * only be woken up externally (e.g. from the monitor thread) after
> > changing
> > + * qmp_dispatcher_co_busy from false to true (e.g. using atomic_xchg).
> > + */
>
> I'm not sure what you mean by "externally".
By anyone outside the coroutine itself. Maybe just dropping the word
"externally" avoids the confusion because it's an implementation detail
that the coroutine can schedule itself while it is marked busy.
> Also mention how it changes from true to false?
Somethin like: "The coroutine will automatically change it back to false
after processing all pending requests"?
> Note to self: monitor_qmp_dispatcher_co() checks busy is true on resume.
>
> Nitpick: wrap around column 70, two spaces between sentences for
> consistency with other comments in this file, please.
Any specific reason why comments (but not code) in this file use a
different text width than everything else in QEMU? My editor is set to
use 80 characters to conform to CODING_STYLE.rst.
> > +bool qmp_dispatcher_co_busy;
> >
> > /* Protects mon_list, monitor_qapi_event_state, monitor_destroyed. */
> > QemuMutex monitor_lock;
> > @@ -579,9 +589,16 @@ void monitor_cleanup(void)
>
> monitor_cleanup() runs in the main thread.
>
> Coroutine qmp_dispatcher_co also runs in the main thread, right?
Yes.
> > }
> > qemu_mutex_unlock(_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 */
> > +qmp_dispatcher_co_shutdown = true;
>
> The coroutine switch ensures qmp_dispatcher_co sees this write, so no
> need for a
Re: [PATCH v4 3/4] qmp: Move dispatcher to a coroutine
This is the hairy one. Please bear with me while I try to grok it.
Kevin Wolf 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
> ---
> include/qapi/qmp/dispatch.h | 1 +
> monitor/monitor-internal.h | 6 +-
> monitor/monitor.c | 33 ---
> monitor/qmp.c | 110 ++--
> qapi/qmp-dispatch.c | 44 ++-
> qapi/qmp-registry.c | 3 +
> util/aio-posix.c| 7 ++-
> 7 files changed, 162 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 d78f5ca190..f180d03368 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -154,7 +154,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;
> @@ -172,7 +174,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..e753fa435d 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -53,8 +53,18 @@ 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;
> +
> +/*
> + * true if the coroutine is active and processing requests. The coroutine may
> + * only be woken up externally (e.g. from the monitor thread) after changing
> + * qmp_dispatcher_co_busy from false to true (e.g. using atomic_xchg).
> + */
I'm not sure what you mean by "externally".
Also mention how it changes from true to false?
Note to self: monitor_qmp_dispatcher_co() checks busy is true on resume.
Nitpick: wrap around column 70, two spaces between sentences for
consistency with other comments in this file, please.
> +bool qmp_dispatcher_co_busy;
>
> /* Protects mon_list, monitor_qapi_event_state, monitor_destroyed. */
> QemuMutex monitor_lock;
> @@ -579,9 +589,16 @@ void monitor_cleanup(void)
monitor_cleanup() runs in the main thread.
Coroutine qmp_dispatcher_co also runs in the main thread, right?
> }
> qemu_mutex_unlock(_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 */
> +qmp_dispatcher_co_shutdown = true;
The coroutine switch ensures qmp_dispatcher_co sees this write, so no
need for a barrier. Correct?
> +if (!atomic_xchg(_dispatcher_co_busy, true)) {
Why do we need atomic? I figure it's because qmp_dispatcher_co_busy is
accessed from multiple threads (main thread and mon_iothread), unlike
qmp_dispatcher_co_shutdown.
What kind of atomic? I'm asking because you use sequentially consistent
atomic_xchg() together with the weaker atomic_mb_set() and
atomic_mb_read().
> +aio_co_wake(qmp_dispatcher_co);
> +}
> +
> +AIO_WAIT_WHILE(qemu_get_aio_context(),
> + (aio_poll(iohandler_get_aio_context(), false),
> +atomic_mb_read(_dispatcher_co_busy)));
This waits for qmp_dispatcher_co_busy to become false again. While
waiting, pending AIO work is given a chance to progress, as long as it
doesn't block.
The only
[PATCH v4 3/4] qmp: Move dispatcher to a coroutine
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
---
include/qapi/qmp/dispatch.h | 1 +
monitor/monitor-internal.h | 6 +-
monitor/monitor.c | 33 ---
monitor/qmp.c | 110 ++--
qapi/qmp-dispatch.c | 44 ++-
qapi/qmp-registry.c | 3 +
util/aio-posix.c| 7 ++-
7 files changed, 162 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 d78f5ca190..f180d03368 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -154,7 +154,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;
@@ -172,7 +174,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..e753fa435d 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -53,8 +53,18 @@ 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;
+
+/*
+ * true if the coroutine is active and processing requests. The coroutine may
+ * only be woken up externally (e.g. from the monitor thread) after changing
+ * qmp_dispatcher_co_busy from false to true (e.g. using atomic_xchg).
+ */
+bool qmp_dispatcher_co_busy;
/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed. */
QemuMutex monitor_lock;
@@ -579,9 +589,16 @@ void monitor_cleanup(void)
}
qemu_mutex_unlock(_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 */
+qmp_dispatcher_co_shutdown = true;
+if (!atomic_xchg(_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(_dispatcher_co_busy)));
+
if (mon_iothread) {
iothread_destroy(mon_iothread);
mon_iothread = NULL;
@@ -604,9 +621,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(_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 54c06ba824..9444de9fcf 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,87 @@ static QMPRequest
*monitor_qmp_requests_pop_any_with_lock(void)
return req_obj;
}
-void monitor_qmp_bh_dispatcher(void *data)
+void coroutine_fn
