Re: [Qemu-devel] [RFC v2 0/8] monitor: allow per-monitor thread

[Peter Xu]

On Thu, Sep 07, 2017 at 10:18:16AM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (address@hidden) wrote:
> > On Thu, Sep 7, 2017 at 9:13 AM, Peter Xu <address@hidden> wrote:
> > > On Wed, Sep 06, 2017 at 12:54:28PM +0100, Daniel P. Berrange wrote:
> > >> On Wed, Sep 06, 2017 at 12:31:58PM +0100, Dr. David Alan Gilbert wrote:
> > >> > * Daniel P. Berrange (address@hidden) wrote:
> > >> > > This does imply that you need a separate monitor I/O processing, 
> > >> > > from the
> > >> > > command execution thread, but I see no need for all commands to 
> > >> > > suddenly
> > >> > > become async. Just allowing interleaved replies is sufficient from 
> > >> > > the
> > >> > > POV of the protocol definition. This interleaving is easy to handle 
> > >> > > from
> > >> > > the client POV - just requires a unique 'serial' in the request by 
> > >> > > the
> > >> > > client, that is copied into the reply by QEMU.
> > >> >
> > >> > OK, so for that we can just take Marc-André's syntax and call it 'id':
> > >> >   https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg03634.html
> > >> >
> > >> > then it's upto the caller to ensure those id's are unique.
> > >>
> > >> Libvirt has in fact generated a unique 'id' for every monitor command
> > >> since day 1 of supporting QMP.
> > >>
> > >> > I do worry about two things:
> > >> >   a) With this the caller doesn't really know which commands could be
> > >> >   in parallel - for example if we've got a recovery command that's
> > >> >   executed by this non-locking thread that's OK, we expect that
> > >> >   to be doable in parallel.  If in the future though we do
> > >> >   what you initially suggested and have a bunch of commands get
> > >> >   routed to the migration thread (say) then those would suddenly
> > >> >   operate in parallel with other commands that we're previously
> > >> >   synchronous.
> > >>
> > >> We could still have an opt-in for async commands. eg default to executing
> > >> all commands in the main thread, unless the client issues an explicit
> > >> "make it async" command, to switch to allowing the migration thread to
> > >> process it async.
> > >>
> > >>  { "execute": "qmp_allow_async",
> > >>    "data": { "commands": [
> > >>        "migrate_cancel",
> > >>    ] } }
> > >>
> > >>
> > >>  { "return": { "commands": [
> > >>        "migrate_cancel",
> > >>    ] } }
> > >>
> > >> The server response contains the subset of commands from the request
> > >> for which async is supported.
> > >>
> > >> That gives good negotiation ability going forward as we incrementally
> > >> support async on more commands.
> > >
> > > I think this goes back to the discussion on which design we'd like to
> > > choose.  IMHO the whole async idea plus the per-command-id is indeed
> > > cleaner and nicer, and I believe that can benefit not only libvirt,
> > > but also other QMP users.  The problem is, I have no idea how long
> > > it'll take to let us have such a feature - I believe that will include
> > > QEMU and Libvirt to both support that.  And it'll be a pity if the
> > > postcopy recovery cannot work only because we cannot guarantee a
> > > stable monitor.
> > 
> > Please don't rush in a hack, they often introduce new bugs that we
> > have to support long-term when they are part of the QMP API.

Sorry, I wasn't meant to push anything.  I was trying to see what
would be the best way to go.

> > 
> > In your original email you mentioned "info cpus".  Have you considered
> > modifying this command so it does not sync the CPU?  I'm not sure
> > callers really need to sync the CPU, typically they just want to know
> > the vcpu numbers, thread IDs, and current state (halted, running,
> > etc).
> 
> But it has the pc as well, so that's actual state.

Yes.  Even if we don't need to sync pc regs for this single "info
cpus" command, I do feel slightly awkward if we don't allow things
like syncing CPU to happen in any command.  IMHO we just need to make
sure these commands may block.

> 
> Dave
> 
> > The next step after that would be to audit other monitor commands for
> > unnecessary vcpu synchronization.

It's really hard to do this for every single command, at least to me.

Comparing to this, I think now I more prefer what Dan has suggested in
the other reply to have an extra way to request async commands while
keep the rest of commands compatible (though obviously I misunderstood
the email when I was writting up previous reply...).

Thanks,

-- 
Peter Xu