Re: [PATCH v4 1/4] qapi: Add a 'coroutine' flag for commands

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 22.01.2020 um 07:32 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> writes:
>> 
>> > This patch adds a new 'coroutine' flag to QMP command definitions that
>> > tells the QMP dispatcher that the command handler is safe to be run in a
>> > coroutine.
>> 
>> I'm afraid I missed this question in my review of v3: when is a handler
>> *not* safe to be run in a coroutine?
>
> That's a hard one to answer fully.
>
> Basically, I think the biggest problem is with calling functions that
> change their behaviour if run in a coroutine compared to running them
> outside of coroutine context. In most cases the differences like having
> a nested event loop instead of yielding are just fine, but they are
> still subtly different.
>
> I know this is vague, but I can assure you that problematic cases exist.
> I hit one of them with my initial hack that just moved everything into a
> coroutine. It was related to graph modifications and bdrv_drain and
> resulted in a hang. For the specifics, I would have to try and reproduce
> the problem again.

I'm afraid it's even more complicated.

Monitors (HMP and QMP) run in the main loop.  Before this patch, HMP and
QMP commands run start to finish, one after the other.

After this patch, QMP commands may elect to yield.  QMP commands still
run one after the other (the shared dispatcher ensures this even when we
have multiple QMP monitors).

However, *HMP* commands can now run interleaved with a coroutine-enabled
QMP command, i.e. between a yield and its re-enter.

Consider an HMP screendump running in the middle of a coroutine-enabled
QMP screendump, using Marc-André's patch.  As far as I can tell, it
works, because:

1. HMP does not run in a coroutine.  If it did, and both dumps dumped
the same @con, then it would overwrite con->screndump_co.  If we ever
decide to make HMP coroutine-capable so it doesn't block the main loop,
this will become unsafe in a nasty way.

2. graphic_hw_update() is safe to call even while another
graphic_hw_update() runs.  qxl_render_update() appears to ensure that
with the help of qxl->ssd.lock.

3. There is no 3[*].

My point is: this is a non-trivial argument.  Whether a QMP command
handler is safe to run in a coroutine depends on how it interacts with
all the stuff that can run interleaved with it.  Typically includes
itself via its HMP buddy.


[*] I'm less confident in 3. than in 1. and 2.