Re: [PATCH v6 00/25] monitor: add asynchronous command type

[Marc-André Lureau]

Hi Kevin,

On Mon, Dec 16, 2019 at 4:07 PM Kevin Wolf <address@hidden> wrote:
>
> Am 13.12.2019 um 17:28 hat Marc-André Lureau geschrieben:
> > On Fri, Dec 13, 2019 at 8:04 PM Kevin Wolf <address@hidden> 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?

-- 
Marc-André Lureau