[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 09/13] qmp: Move dispatcher to a coroutine
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v7 09/13] qmp: Move dispatcher to a coroutine |
Date: |
Mon, 28 Sep 2020 10:24:33 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
Kevin Wolf <kwolf@redhat.com> writes:
> Am 14.09.2020 um 17:30 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>
>> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> > ---
[...]
>> > diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
>> > index 5677ba92ca..754f7b854c 100644
>> > --- a/qapi/qmp-dispatch.c
>> > +++ b/qapi/qmp-dispatch.c
[...]
>> > @@ -153,12 +181,35 @@ QDict *qmp_dispatch(const QmpCommandList *cmds,
>> > QObject *request,
>> > qobject_ref(args);
>> > }
>> >
>> > + assert(!(oob && qemu_in_coroutine()));
>> > assert(monitor_cur() == NULL);
>> > - monitor_set_cur(qemu_coroutine_self(), cur_mon);
>> > -
>> > - cmd->fn(args, &ret, &err);
>> > -
>> > - monitor_set_cur(qemu_coroutine_self(), NULL);
>> > + if (!!(cmd->options & QCO_COROUTINE) == qemu_in_coroutine()) {
>> > + monitor_set_cur(qemu_coroutine_self(), cur_mon);
>> > + cmd->fn(args, &ret, &err);
>> > + monitor_set_cur(qemu_coroutine_self(), NULL);
>> > + } else {
>> > + /*
>> > + * Not being in coroutine context implies that we're handling
>> > + * an OOB command, which must not have QCO_COROUTINE.
>> > + *
>> > + * This implies that we are in coroutine context, but the
>> > + * command doesn't have QCO_COROUTINE. We must drop out of
>> > + * coroutine context for this one.
>> > + */
>>
>> I had to read this several times to get it. The first sentence leads me
>> into coroutine context, and then the next sentence tells me the
>> opposite, throwing me into confusion.
>>
>> Perhaps something like this:
>>
>> /*
>> * Actual context doesn't match the one the command needs.
>> * Case 1: we are in coroutine context, but command does not
>> * have QCO_COROUTINE. We need to drop out of coroutine
>> * context for executing it.
>> * Case 2: we are outside coroutine context, but command has
>> * QCO_COROUTINE. Can't actually happen, because we get here
>> * outside coroutine context only when executing a command
>> * out of band, and OOB commands never have QCO_COROUTINE.
>> */
>
> Works for me. Can you squash this in while applying?
Sure!
[PATCH v7 11/13] util/async: Add aio_co_reschedule_self(), Kevin Wolf, 2020/09/09
[PATCH v7 10/13] hmp: Add support for coroutine command handlers, Kevin Wolf, 2020/09/09
[PATCH v7 12/13] block: Add bdrv_co_move_to_aio_context(), Kevin Wolf, 2020/09/09
[PATCH v7 13/13] block: Convert 'block_resize' to coroutine, Kevin Wolf, 2020/09/09