Re: [Qemu-devel] [PATCH v4 00/20] monitor: add asynchronous command type

[Marc-André Lureau]

Hi

On Thu, May 23, 2019 at 9:52 AM Markus Armbruster <address@hidden> wrote:
> I'm not sure how asynchronous commands could support reconnect and
> resume.

The same way as current commands, including job commands.

>
> >> I'm ignoring "etc" unless you expand it into something specific.
> >>
> >> I'm also not taking the "weird" bait :)
> >> > The following series implements an async command solution instead. By
> >> > introducing a session context and a command return handler, it can:
> >> > - defer the return, allowing the mainloop to reenter
> >> > - return only to the caller (instead of broadcast events for reply)
> >> > - optionnally allow cancellation when the client is gone
> >> > - track on-going qapi command(s) per client/session
> >> >
> >> > and without introduction of new QMP APIs or client visible change.
> >>
> >> What do async commands provide that jobs lack?
> >>
> >> Why do we want both?
> >
> > They are different things, last we discussed it: jobs are geared
> > toward block device operations,
>
> Historical accident.  We've discussed using them for non-blocky stuff,
> such as migration.  Of course, discussions are cheap, code is what
> counts.

Using job API means providing new (& more complex) APIs to client.

The screendump fix here doesn't need new API, it needs new internal
dispatch of QMP commands: the purpose of this series.

Whenever we can solve things on qemu side, I would rather not
deprecate current API.

> >                                 and do not provide simple qmp-level
> > facilities that I listed above. What I introduce is a way for an
> > *existing* QMP command to be splitted, so it can re-enter the main
> > loop sanely (and not by introducing new commands or signals or making
> > things unnecessarily more complicated).
> >
> > My proposal is fairly small:
> >   27 files changed, 877 insertions(+), 260 deletions(-)
> >
> > Including test, and the qxl screendump fix, which account for about
> > 1/3 of the series.
> >
> >> I started to write a feature-by-feature comparison, but realized I don't
> >> have the time to figure out either jobs or async from their (rather
> >> sparse) documentation, let alone from code.
> >>
> >> > Existing qemu commands can be gradually replaced by async:true
> >> > variants when needed, while carefully reviewing the concurrency
> >> > aspects. The async:true commands marshaller helpers are splitted in
> >> > half, the calling and return functions. The command is called with a
> >> > QmpReturn context, that can return immediately or later, using the
> >> > generated return helper, which allows for a step-by-step conversion.
> >> >
> >> > The screendump command is converted to an async:true version to solve
> >> > rhbz#1230527. The command shows basic cancellation (this could be
> >> > extended if needed). It could be further improved to do asynchronous
> >> > IO writes as well.
> >>
> >> What is "basic cancellation"?
> >> What extension(s) do you have in mind?
> >
> > It checks for cancellation in a few places, between IO. Full
> > cancellation would allow to cancel at any time.
> >
> >>
> >> What's the impact of screendump writing synchronously?
> >
> > It can be pretty bad, think about 4k screens. It is 33177600 bytes,
> > written in PPM format, blocking the main loop..
>
> My question was specifically about "could be further improved to do
> asynchronous IO writes as well".  What's the impact of not having this
> improvement?  I *guess* it means that even with the asynchronous
> command, the synchronous writes still block "something", but I'm not
> sure what "something" may be, and how it could impact behavior.  Hence
> my question.

It blocks many things since the BQL is taken.

The goal is not to improve responsiveness at this point, but to fix
the QXL screendump bug, by introducing a split dispatch in QMP
commands: callback for starting, and a separate return function. This
is not rocket science. See below.

>
> > QMP operation doing large IO (dumps), or blocking on events, could be
> > switched to this async form without introducing user-visible change,
>
> Letting the next QMP command start before the current one is done is a
> user-visible change.  We can discuss whether the change is harmless.

Agree, from cover letter:
Existing qemu commands can be gradually replaced by async:true
variants when needed, while carefully reviewing the concurrency
aspects.

>
> > and with minimal effort compared to jobs.
>
> To gauge the difference in effort, we'd need actual code to compare.

It's a no-go to me, you don't want to teach all users out there with
new job API for existing commands when you can improve or fix things
in QEMU.

The QEMU change for the command can't really be simpler than what I
propose. You go from:

qmp_foo() {
  // do foo synchronously and
  return something
}

to:

qmp_foo_async(QmpReturn *r) {
  // do foo asynchronously (or return synchronously)
}

foo_done() {
  qmp_foo_async_return(r, something)
}

See "scripts: learn 'async' qapi commands" for the details.

thanks

-- 
Marc-André Lureau