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

[Jan Kiszka]

On 2011-10-05 17:43, Avi Kivity wrote:
> On 10/05/2011 04:37 PM, Luiz Capitulino wrote:
>> Now, we have three options to fix this but I don't know which one to
>> choose:
>>
>>   1. We could just add the transition RSTATE_PAUSED -> 
>> RSTATE_POST_MIGRATE
>>      as valid. Not sure this is a good thing to do though, as it seems
>> a silly
>>      workaround for the fact that the transition to RSTATE_PRE_MIGRATE
>> has
>>      never occurred
>>
>>   2. This patch makes vm_stop() do the state transition even if the VM
>>      is already stopped. Seems good enough, except that I fear two
>> things.
>>      First, today we know that vm_stop() is a no-op if the VM is already
>>      stopped, so there's a semantic change that could turn out to be
>> trap.
>>      Second, I also fear people using vm_stop() as a way to change the
>>      VM status, just like runstate_set() (which can also become an
>> horrible
>>      trap)
>>
>>   3. Avi suggested we should keep a reference count, so that states are
>>      not discarded:
>>
>>     http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg00595.html
>>
>>      That solution seemed to be the perfect one, except for one important
>>      detail: how should we implement vm_start() (and thus 'cont')?
>>
>>      In order to maintain how we behave with the external world, the only
>>      option is that vm_start() will set the stop count to 0. Ie, doesn't
>>      matter if we have stopped the VM 500 times at some point, a
>> vm_start()
>>      call will discard all stored states.
>>
>>      Not sure if that's what you expected, but the first time I read
>> Avi's
>>      idea I had the impression that it would be a good idea that
>> vm_start()
>>      decremented the ref count only once, ie. vm_stop() and vm_start()
>> calls
>>      have to match.
>>
> 
> 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.

We are talking about multiple stop states here, but only a single
function (vm_stop) to enter them - maybe that's not optimal. But the
point is that we were missing one stop-to-stop transition. And that
needs to be fixed, either inside vm_stop or when it is called.

If you want to lock the VM into paused state, add a new symmetric API
that does precisely this. That API would send the VM into RSTATE_LOCKED
if it is not yet stopped on lock or is still locked on resume. That
would avoid redefining stop states that have no use for lock-counting
semantics.

Jan

signature.asc
Description: OpenPGP digital signature