Re: [Qemu-devel] [PATCH] runstate: do not discard runstate changes when paused

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 5 Oct 2011 15:49:42 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Wed, 05 Oct 2011 20:02:04 +0200
> Avi Kivity <avi@redhat.com> wrote:
> 
> > On 10/05/2011 07:39 PM, Luiz Capitulino wrote:
> > > On Wed, 05 Oct 2011 19:23:21 +0200
> > > Avi Kivity<avi@redhat.com>  wrote:
> > >
> > > >  On 10/05/2011 07:02 PM, Luiz Capitulino wrote:
> > > >  >  On Wed, 05 Oct 2011 18:37:51 +0200
> > > >  >  Avi Kivity<avi@redhat.com>   wrote:
> > > >  >
> > > >  >  >   On 10/05/2011 06:31 PM, Jan Kiszka wrote:
> > > >  >  >   >   >>
> > > >  >  >   >   >
> > > >  >  >   >   >    vm_start() should be symmetric with vm_stop().  That is, if a piece of
> > > >  >  >   >   >    code wants to execute with vcpus stopped, it should just run inside a
> > > >  >  >   >   >    stop/start pair.
> > > >  >  >   >   >
> > > >  >  >   >   >    The only confusion can come from the user, if he sees multiple stop
> > > >  >  >   >   >    events and expects that just one cont will continue the vm.  For the
> > > >  >  >   >   >    machine monitor, we should just document that the you have to issue one
> > > >  >  >   >   >    cont for every stop event you see (plus any stops you issue).  It's not
> > > >  >  >   >   >    unnatural - the code that handles a stop_due_to_enospace can work to fix
> > > >  >  >   >   >    the error and issue a cont, disregarding any other stops in progress
> > > >  >  >   >   >    (due to a user pressing the stop button, or migration, or cpu hotplug,
> > > >  >  >   >   >    or whatever).  For the human monitor, it's not so intuitive, but the
> > > >  >  >   >   >    situation is so rare we can just rely on the user to issue cont again.
> > > >  >  >   >
> > > >  >  >   >   Making this kind of user-visible change would be a bad idea.
> > > >  >  >
> > > >  >  >   The current situation is a bad idea.
> > > >  >
> > > >  >  Let's take the migration use-case as an example (ie. the user stops the VM
> > > >  >  before performing the migration). Today, if migration fails,
> > > >  >  migrate_fd_put_ready() will call vm_start() which will put the VM to
> > > >  >  run again.
> > > >  >
> > > >  >  But if we implement the ref count idea, then vm_start() will just "unlock"
> > > >  >  migrate_fd_put_ready()'s own call to vm_stop(), that's, the user stop will
> > > >  >  remain and the user is required to do a 'cont'.
> > > >  >
> > > >  >  I'd probably agree that that's the ideal semantics, but chances are we're
> > > >  >  going to break qmp clients here.
> > > >
> > > >  There are two questions here.  Is this autostart desirable?  (IMO no,
> > > >  but haven't given it much thought).
> > >
> > > It should be configurable at migration time I think.
> > 
> > Why? autostart can easily be emulated by non-autostart, but not vice versa.
> 
> What I meant is to give the user the choice to use.
> 
> > > >  If yes, we should provide it
> > > >  somehow.  If not, we should default to providing it, but switch to
> > > >  non-autostart if a newer client indicates it understands the new semantics.
> > >
> > > That's only one example. You mention another one above: if qemu stops
> > > itself twice, will the user be required to run 'cont' twice?
> > 
> > Yes.  That's how the user tells qemu it saw the two events and handled 
> > both of them.  Otherwise there is a race condition and an event goes 
> > unhandled (or rather, it is handled while the guest is running instead 
> > of stopped).
> 
> Breaks compatibility.
> 
> > > I'm not exactly against the semantics you're proposing, but they don't
> > > seem to fit today's qemu.
> > 
> > Today's qemu is broken here.
> 
> For me it's broken because it will abort() if you migrate a paused vm, for
> you it seems to be broken at the semantic level.
> 
> We can fix the semantics without breaking compatibility.

s/We can/ We can't