Re: QMP (without OOB) function running in thread different from the main thread as part of aio_poll

[Markus Armbruster]

Fiona Ebner <f.ebner@proxmox.com> writes:

> Hi,
> while debugging a completely different issue, I was surprised to see
> do_qmp_dispatch_bh being run in a vCPU thread. I was under the
> impression that QMP functions are supposed to be executed in the main
> thread. Is that wrong?
>
> I managed to reproduced the scenario with a build of upstream QEMU
> v8.0.0-rc4 once more (again with GDB open), but it's not a simple
> reproducer and racy. The backtrace is below[0].
>
> My attempt at explaining the situation is:
> 1. In qapi/qmp-dispatch.c, the main thread schedules do_qmp_dispatch_bh,
> because the coroutine context doesn't match.
> 2. The host OS switches to the vCPU thread.
> 3. Something in the vCPU thread triggers aio_poll with the main thread's
> AioContext (in my case, a write to a pflash drive).
> 4. do_qmp_dispatch_bh is executed in the vCPU thread.
>
> Could this be an artifact of running with a debugger?
>
> I CC'ed the maintainers of util/aio-posix.c and qapi/qmp-dispatch.c
> hoping that is not too far off.

Fiona, thanks for reporting this!

The monitor core has become awfully complex.

Instead of trying to explain everything at once, let me recount its
history.  If you're familiar with the monitor, you might want to skip
ahead to "Paolo wrote".

In the beginning, there was just one monitor, and it ran entirely in the
main loop (callback when input is available).  To keep the main loop
going, monitor commands better complete quickly.

Then we got multiple monitors.  Same story, just multiple input streams,
each with a callback.

We also got additional threads.  When I say "main loop", I mean the main
thread's main loop.

"Must complete quickly" means no blocking I/O and such.  Writing code
that way is somewhere between hard and impractical.  Much code called by
monitor commands wasn't written that way.

"Doctor, doctor, running code in the main loop hurts".  Sadly, the
doctor's recommended remedy "don't do that then" is really hard to
apply: a lot of code has been written assuming "running in the main
loop, with the big QEMU lock held".

The first small step towards it was taken to enable the "out-of-band"
feature.  We moved the QMP monitor proper out of the main loop into a
monitor I/O thread.  The monitor commands get funneled to the main loop.
Instead of the main loop calling the monitor when a file descriptor has
input, it now calls the command dispatcher when a funnel queue has a
command.  This is implemented with a bottom half (merge commit
4bdc24fa018, March 2018).

Why bother?  Because now we can execute special "out-of-band" commands
right away, in the I/O thread, regardless of the main loop's state.
Peter Xu wanted this badly enough for postcopy recovery to code it up.
It was hard.  It's not generally useful, as the restriction on what OOB
commands can do are severe.

The next step was the coroutine feature.  Quite a few of the problematic
monitor commands are actually running coroutine-capable code: when
running in coroutine context, the code yields instead of blocking.
Running such commands in monitor context improves things from "blocks
the main loop" to "blocks all monitor commands".

Sadly, code exists that falls apart in coroutine context.  So we had to
make running in coroutine context opt-in.  Right now only two commands
opt in: block_resize and screendump.  Hopefully, we can get to the point
where most or all do.

Until all do, the dispatcher needs to run some commands coroutine
context, and others outside coroutine context.  How?

The QMP command dispatcher runs in a coroutine in the main loop (HMP
works differently, but let's ignore it here).

If the command can be executed in coroutine context, the dispatcher
calls its handler as before.  Right now, we take this path just for
block_resize and screendump.

Else, we create a bottom half that calls the handler, and schedule it to
run in the main loop.  Right now, we take this path for all the other
commands.

This if / else is in qmp_dispatch().  The else is the only way to get
do_qmp_dispatch_bh() to run.  Merge commit b7092cda1b3, October 2020.

Paolo wrote:

    While monitor commands in general start only in the main thread,
    bottom halves can also run (via aio_poll) in the vCPU threads,
    always under the BQL.

Today I learned...

So, splicing in a bottom half unmoored monitor commands from the main
loop.  We weren't aware of that, as our commit messages show.

I guess the commands themselves don't care; all they need is the BQL.

However, did we unwittingly change what can get blocked?  Before,
monitor commands could block only the main thread.  Now they can also
block vCPU threads.  Impact?