Re: [Qemu-devel] [PATCH 2/6] Introduce monitor 'wait' command (v2)

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
> > Anthony Liguori wrote:
> > > The wait command will pause the monitor the command was issued in 
> > > until a new
> > > event becomes available.  Events are queued if there isn't a waiter 
> > > present.
> > > The wait command completes after a single event is available.
> > >   
> >
> > How do you stop a wait if there are no pending events?
>
> You mean, cancel a wait?  You cannot.  I thought about whether it was 
> a problem or not.  I'm not sure.

A management agent might want to detach from guests, upgrade, restart, 
and reattach.

> You could introduce a wait-cancel command, but then you need a way to 
> identify which wait you want to cancel.  I can't think of a simple way 
> to do that today.

This, as well as the queueing that's necessary with this model, 
indicates (to me) that it is too complicated.

> > > Today, we queue events indefinitely but in the future, I suspect 
> > > we'll drop
> > > events that are older than a certain amount of time to avoid infinitely
> > > allocating memory for long running VMs.
> > >   
> >
> > This queueing plug the race where an event happens immediately after 
> > a wait completes.  But it could be avoided completely by having 
> > asynchronous notifications on a single monitor.
>
> There are multiple things I think are being confused: asynchronous 
> completion of monitor commands, events, monitor multiplexing, and 
> non-human mode.
>
> There can only be one command active at any given time on a Monitor 
> context.  We can have many Monitor contexts.  There is currently only 
> one Monitor context connected to a character device at a given time.

Don't think of 'migrate -d' as a command to perform migration, instead 
it's a command to start migration.

I also object to exposing internal qemu implementation details like 
monitor contexts to the user (by forcing them to have multiple 
connections).  If we can't have more than one monitor command, we need 
to fix that.

> I think what you want to see is something like this:
>
> <input> tag=4: info cpus
> <input> tag=5: info kvm
> <output> tag=5,rc=0: kvm enabled
> <output> tag=4,rc=0: eip = 0x0000000444
> <ouput> rc=0,class=vm-state,name=start: vm started
>
> To me, this is a combination of events, which is introduced by this 
> patch, a non-human monitor mode, and finally multiplexing multiple 
> monitors into a single session.
>
> Right now, you can have the same functional thing by having three 
> monitors.  In the first monitor you'd see:
>
> (qemu) info cpus
> eip = 0x000000444
> (qemu)
>
> In the second you'd see:
>
> (qemu) info kvm
> kvm enabled
> (qemu)
>
> In the third you'd see:
>
> (qemu) wait
> 23423423.23423: vm-state: start: vm started
> (qemu)
>
> Even those the two info commands today are synchronous, there's 
> nothing requiring them to be (see migrate as an example).  So I think 
> we're in agreement but you just want to jump ahead 6 months ;-)

Commands which are inherently synchronous should remain so.  Commands 
which are inherently async should be coded like that.  It was a mistake 
IMO to have 'migrate' be a synchronous command, it should have always 
behaved as if -d is given.

Having tagged replies is a good idea, but IMO, introducing multiple 
monitors will create a lot of subtle problems, several of which we've 
already identified.

--
Do not meddle in the internals of kernels, for they are subtle and quick to 
panic.