Re: [Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

2017-09-09 Thread Paolo Bonzini 
On 06/09/2017 15:40, Dr. David Alan Gilbert wrote:
>> There's a race if someone does a 'stop' near the end of migrate;
>> the migration process goes through two runstates:
>> 'finish migrate'
>> 'postmigrate'
>>
>> If the user issues a 'stop' between the two we end up with invalid
>> state transitions.
>> Add the transitions as valid.
>>
>> Signed-off-by: Dr. David Alan Gilbert 
> Queued
> 
>> ---
>>  vl.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/vl.c b/vl.c
>> index 99fcfa0442..bacb03f49d 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -621,6 +621,7 @@ static const RunStateTransition 
>> runstate_transitions_def[] = {
>>  
>>  { RUN_STATE_PAUSED, RUN_STATE_RUNNING },
>>  { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE },
>> +{ RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE },

There is:

if (s->state == MIGRATION_STATUS_COMPLETED) {
...
runstate_set(RUN_STATE_POSTMIGRATE);
} else {
...
if (old_vm_running && !entered_postcopy) {
vm_start();
} else {
if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
runstate_set(RUN_STATE_POSTMIGRATE);
}
}
}

Maybe the runstate_check should be in the "then" branch too, instead
of adding this transition?

Paolo

>>  { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
>>  { RUN_STATE_PAUSED, RUN_STATE_COLO},
>>  
>> @@ -633,6 +634,7 @@ static const RunStateTransition 
>> runstate_transitions_def[] = {
>>  { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
>>  
>>  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
>> +{ RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED },
>>  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
>>  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH },
>>  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_COLO},
>> -- 
>> 2.13.3
>>
>>

Re: [Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

2017-09-06 Thread Dr. David Alan Gilbert 
* Dr. David Alan Gilbert (git) (dgilb...@redhat.com) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> There's a race if someone does a 'stop' near the end of migrate;
> the migration process goes through two runstates:
> 'finish migrate'
> 'postmigrate'
> 
> If the user issues a 'stop' between the two we end up with invalid
> state transitions.
> Add the transitions as valid.
> 
> Signed-off-by: Dr. David Alan Gilbert 

Queued

> ---
>  vl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/vl.c b/vl.c
> index 99fcfa0442..bacb03f49d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -621,6 +621,7 @@ static const RunStateTransition 
> runstate_transitions_def[] = {
>  
>  { RUN_STATE_PAUSED, RUN_STATE_RUNNING },
>  { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE },
> +{ RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE },
>  { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
>  { RUN_STATE_PAUSED, RUN_STATE_COLO},
>  
> @@ -633,6 +634,7 @@ static const RunStateTransition 
> runstate_transitions_def[] = {
>  { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
>  
>  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
> +{ RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED },
>  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
>  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH },
>  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_COLO},
> -- 
> 2.13.3
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

2017-08-08 Thread Peter Xu 
On Tue, Aug 08, 2017 at 09:02:54AM +0200, Juan Quintela wrote:
> "Dr. David Alan Gilbert (git)"  wrote:
> > From: "Dr. David Alan Gilbert" 
> >
> > There's a race if someone does a 'stop' near the end of migrate;
> > the migration process goes through two runstates:
> > 'finish migrate'
> > 'postmigrate'
> >
> > If the user issues a 'stop' between the two we end up with invalid
> > state transitions.
> > Add the transitions as valid.
> >
> > Signed-off-by: Dr. David Alan Gilbert 
> 
> Reviewed-by: Juan Quintela 
> 
> To answer Peter question:
> 
> int vm_stop(RunState state)
> {
>  we don't care about this case 
> 
> return do_vm_stop(state);
> }
> 
> static int do_vm_stop(RunState state)
> {
> int ret = 0;
> 
> if (runstate_is_running()) {
> cpu_disable_ticks();
> pause_all_vcpus();
> runstate_set(state);
> vm_state_notify(0, state);
> qapi_event_send_stop(_abort);
> }
> 
> bdrv_drain_all();
> replay_disable_events();
> ret = bdrv_flush_all();
> 
> return ret;
> }
> 
> 
> int runstate_is_running(void)
> {
> return runstate_check(RUN_STATE_RUNNING);
> }
> 
> 
> So, "stop" only changes states when we are in RUNNING state.
> The idea was that after migration, the only valid command (as in the
> film "it should do something")is "run".

Thanks for the details. :)

-- 
Peter Xu

Re: [Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

2017-08-08 Thread Juan Quintela 
"Dr. David Alan Gilbert (git)"  wrote:
> From: "Dr. David Alan Gilbert" 
>
> There's a race if someone does a 'stop' near the end of migrate;
> the migration process goes through two runstates:
> 'finish migrate'
> 'postmigrate'
>
> If the user issues a 'stop' between the two we end up with invalid
> state transitions.
> Add the transitions as valid.
>
> Signed-off-by: Dr. David Alan Gilbert 

Reviewed-by: Juan Quintela 

To answer Peter question:

int vm_stop(RunState state)
{
 we don't care about this case 

return do_vm_stop(state);
}

static int do_vm_stop(RunState state)
{
int ret = 0;

if (runstate_is_running()) {
cpu_disable_ticks();
pause_all_vcpus();
runstate_set(state);
vm_state_notify(0, state);
qapi_event_send_stop(_abort);
}

bdrv_drain_all();
replay_disable_events();
ret = bdrv_flush_all();

return ret;
}


int runstate_is_running(void)
{
return runstate_check(RUN_STATE_RUNNING);
}


So, "stop" only changes states when we are in RUNNING state.
The idea was that after migration, the only valid command (as in the
film "it should do something")is "run".

Later, Juan.

Re: [Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

2017-08-07 Thread Peter Xu 
On Mon, Aug 07, 2017 at 01:25:29PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > On Fri, Aug 04, 2017 at 06:50:11PM +0100, Dr. David Alan Gilbert (git) 
> > wrote:
> > > From: "Dr. David Alan Gilbert" 
> > > 
> > > There's a race if someone does a 'stop' near the end of migrate;
> > > the migration process goes through two runstates:
> > > 'finish migrate'
> > > 'postmigrate'
> > > 
> > > If the user issues a 'stop' between the two we end up with invalid
> > > state transitions.
> > > Add the transitions as valid.
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert 
> > > ---
> > >  vl.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/vl.c b/vl.c
> > > index 99fcfa0442..bacb03f49d 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -621,6 +621,7 @@ static const RunStateTransition 
> > > runstate_transitions_def[] = {
> > >  
> > >  { RUN_STATE_PAUSED, RUN_STATE_RUNNING },
> > >  { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE },
> > > +{ RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE },
> > >  { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
> > >  { RUN_STATE_PAUSED, RUN_STATE_COLO},
> > >  
> > > @@ -633,6 +634,7 @@ static const RunStateTransition 
> > > runstate_transitions_def[] = {
> > >  { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> > >  
> > >  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
> > > +{ RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED },
> > >  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
> > >  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH },
> > >  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_COLO},
> > 
> > Do we need this as well?
> > 
> > { RUN_STATE_POSTMIGRATE, RUN_STATE_PAUSED },
> 
> Apparently not:
> 
> (qemu) migrate "exec:cat > /dev/null"
> (qemu) infomigrate
> unknown command: 'infomigrate'
> (qemu) info status
> VM status: paused (postmigrate)
> (qemu) stop
> (qemu) info status
> VM status: paused (postmigrate)
> (qemu)
> 
> 
> so doing a stop at that point doesn't seem to cause any problem.

Yes. Thanks.

Reviewed-by: Peter Xu 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

2017-08-07 Thread Dr. David Alan Gilbert 
* Peter Xu (pet...@redhat.com) wrote:
> On Fri, Aug 04, 2017 at 06:50:11PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" 
> > 
> > There's a race if someone does a 'stop' near the end of migrate;
> > the migration process goes through two runstates:
> > 'finish migrate'
> > 'postmigrate'
> > 
> > If the user issues a 'stop' between the two we end up with invalid
> > state transitions.
> > Add the transitions as valid.
> > 
> > Signed-off-by: Dr. David Alan Gilbert 
> > ---
> >  vl.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/vl.c b/vl.c
> > index 99fcfa0442..bacb03f49d 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -621,6 +621,7 @@ static const RunStateTransition 
> > runstate_transitions_def[] = {
> >  
> >  { RUN_STATE_PAUSED, RUN_STATE_RUNNING },
> >  { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE },
> > +{ RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE },
> >  { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
> >  { RUN_STATE_PAUSED, RUN_STATE_COLO},
> >  
> > @@ -633,6 +634,7 @@ static const RunStateTransition 
> > runstate_transitions_def[] = {
> >  { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> >  
> >  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
> > +{ RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED },
> >  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
> >  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH },
> >  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_COLO},
> 
> Do we need this as well?
> 
> { RUN_STATE_POSTMIGRATE, RUN_STATE_PAUSED },

Apparently not:

(qemu) migrate "exec:cat > /dev/null"
(qemu) infomigrate
unknown command: 'infomigrate'
(qemu) info status
VM status: paused (postmigrate)
(qemu) stop
(qemu) info status
VM status: paused (postmigrate)
(qemu)


so doing a stop at that point doesn't seem to cause any problem.

Dave

> -- 
> Peter Xu
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

2017-08-07 Thread Peter Xu 
On Fri, Aug 04, 2017 at 06:50:11PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> There's a race if someone does a 'stop' near the end of migrate;
> the migration process goes through two runstates:
> 'finish migrate'
> 'postmigrate'
> 
> If the user issues a 'stop' between the two we end up with invalid
> state transitions.
> Add the transitions as valid.
> 
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  vl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/vl.c b/vl.c
> index 99fcfa0442..bacb03f49d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -621,6 +621,7 @@ static const RunStateTransition 
> runstate_transitions_def[] = {
>  
>  { RUN_STATE_PAUSED, RUN_STATE_RUNNING },
>  { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE },
> +{ RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE },
>  { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
>  { RUN_STATE_PAUSED, RUN_STATE_COLO},
>  
> @@ -633,6 +634,7 @@ static const RunStateTransition 
> runstate_transitions_def[] = {
>  { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
>  
>  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
> +{ RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED },
>  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
>  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH },
>  { RUN_STATE_FINISH_MIGRATE, RUN_STATE_COLO},

Do we need this as well?

{ RUN_STATE_POSTMIGRATE, RUN_STATE_PAUSED },

-- 
Peter Xu

[Qemu-devel] [PATCH f0r 2.11] runstate/migrate: Two more transitions

2017-08-04 Thread Dr. David Alan Gilbert (git) 
From: "Dr. David Alan Gilbert" 

There's a race if someone does a 'stop' near the end of migrate;
the migration process goes through two runstates:
'finish migrate'
'postmigrate'

If the user issues a 'stop' between the two we end up with invalid
state transitions.
Add the transitions as valid.

Signed-off-by: Dr. David Alan Gilbert 
---
 vl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/vl.c b/vl.c
index 99fcfa0442..bacb03f49d 100644
--- a/vl.c
+++ b/vl.c
@@ -621,6 +621,7 @@ static const RunStateTransition runstate_transitions_def[] 
= {
 
 { RUN_STATE_PAUSED, RUN_STATE_RUNNING },
 { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE },
+{ RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE },
 { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
 { RUN_STATE_PAUSED, RUN_STATE_COLO},
 
@@ -633,6 +634,7 @@ static const RunStateTransition runstate_transitions_def[] 
= {
 { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
 
 { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
+{ RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED },
 { RUN_STATE_FINISH_MIGRATE, RUN_STATE_POSTMIGRATE },
 { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PRELAUNCH },
 { RUN_STATE_FINISH_MIGRATE, RUN_STATE_COLO},
-- 
2.13.3