| Message ID | 20200604085533.7769-2-chen.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration/colo: Optimize COLO framework code | expand |
On Thu, 4 Jun 2020 16:55:32 +0800 Zhang Chen <chen.zhang@intel.com > wrote: > From: Zhang Chen <chen.zhang@intel.com> > > No need to reuse MIGRATION_STATUS_ACTIVE boot COLO. > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > --- > migration/colo.c | 2 -- > migration/migration.c | 17 ++++++++++------- > 2 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/migration/colo.c b/migration/colo.c > index ea7d1e9d4e..91c76789fa 100644 > --- a/migration/colo.c > +++ b/migration/colo.c > @@ -670,8 +670,6 @@ void migrate_start_colo_process(MigrationState *s) > colo_checkpoint_notify, s); > > qemu_sem_init(&s->colo_exit_sem, 0); > - migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE, > - MIGRATION_STATUS_COLO); > colo_process_checkpoint(s); > qemu_mutex_lock_iothread(); > } > diff --git a/migration/migration.c b/migration/migration.c > index b63ad91d34..7f3f82a715 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -2972,7 +2972,10 @@ static void migration_completion(MigrationState *s) > goto fail_invalidate; > } > > - if (!migrate_colo_enabled()) { > + if (migrate_colo_enabled()) { > + migrate_set_state(&s->state, current_active_state, > + MIGRATION_STATUS_COLO); > + } else { > migrate_set_state(&s->state, current_active_state, > MIGRATION_STATUS_COMPLETED); > } > @@ -3304,12 +3307,7 @@ static void migration_iteration_finish(MigrationState *s) > migration_calculate_complete(s); > runstate_set(RUN_STATE_POSTMIGRATE); > break; > - > - case MIGRATION_STATUS_ACTIVE: > - /* > - * We should really assert here, but since it's during > - * migration, let's try to reduce the usage of assertions. > - */ > + case MIGRATION_STATUS_COLO: > if (!migrate_colo_enabled()) { > error_report("%s: critical error: calling COLO code without " > "COLO enabled", __func__); > @@ -3319,6 +3317,11 @@ static void migration_iteration_finish(MigrationState *s) > * Fixme: we will run VM in COLO no matter its old running state. > * After exited COLO, we will keep running. > */ > + case MIGRATION_STATUS_ACTIVE: > + /* > + * We should really assert here, but since it's during > + * migration, let's try to reduce the usage of assertions. > + */ I think this case should be removed, because arriving here with MIGRATION_STATUS_ACTIVE is invalid. It should be handled by the default case. Regards, Lukas Straub > s->vm_was_running = true; > /* Fallthrough */ > case MIGRATION_STATUS_FAILED:
> -----Original Message----- > From: Lukas Straub <lukasstraub2@web.de> > Sent: Sunday, June 7, 2020 3:57 AM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: Dr . David Alan Gilbert <dgilbert@redhat.com>; qemu-dev <qemu- > devel@nongnu.org>; Zhanghailiang <zhang.zhanghailiang@huawei.com>; > Zhang Chen <zhangckid@gmail.com> > Subject: Re: [PATCH V2 1/2] migration/colo: Optimize COLO boot code path > > On Thu, 4 Jun 2020 16:55:32 +0800 > Zhang Chen <chen.zhang@intel.com > wrote: > > > From: Zhang Chen <chen.zhang@intel.com> > > > > No need to reuse MIGRATION_STATUS_ACTIVE boot COLO. > > > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > > Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > > --- > > migration/colo.c | 2 -- > > migration/migration.c | 17 ++++++++++------- > > 2 files changed, 10 insertions(+), 9 deletions(-) > > > > diff --git a/migration/colo.c b/migration/colo.c index > > ea7d1e9d4e..91c76789fa 100644 > > --- a/migration/colo.c > > +++ b/migration/colo.c > > @@ -670,8 +670,6 @@ void migrate_start_colo_process(MigrationState *s) > > colo_checkpoint_notify, s); > > > > qemu_sem_init(&s->colo_exit_sem, 0); > > - migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE, > > - MIGRATION_STATUS_COLO); > > colo_process_checkpoint(s); > > qemu_mutex_lock_iothread(); > > } > > diff --git a/migration/migration.c b/migration/migration.c index > > b63ad91d34..7f3f82a715 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -2972,7 +2972,10 @@ static void migration_completion(MigrationState > *s) > > goto fail_invalidate; > > } > > > > - if (!migrate_colo_enabled()) { > > + if (migrate_colo_enabled()) { > > + migrate_set_state(&s->state, current_active_state, > > + MIGRATION_STATUS_COLO); > > + } else { > > migrate_set_state(&s->state, current_active_state, > > MIGRATION_STATUS_COMPLETED); > > } > > @@ -3304,12 +3307,7 @@ static void > migration_iteration_finish(MigrationState *s) > > migration_calculate_complete(s); > > runstate_set(RUN_STATE_POSTMIGRATE); > > break; > > - > > - case MIGRATION_STATUS_ACTIVE: > > - /* > > - * We should really assert here, but since it's during > > - * migration, let's try to reduce the usage of assertions. > > - */ > > + case MIGRATION_STATUS_COLO: > > if (!migrate_colo_enabled()) { > > error_report("%s: critical error: calling COLO code without " > > "COLO enabled", __func__); @@ -3319,6 > > +3317,11 @@ static void migration_iteration_finish(MigrationState *s) > > * Fixme: we will run VM in COLO no matter its old running state. > > * After exited COLO, we will keep running. > > */ > > + case MIGRATION_STATUS_ACTIVE: > > + /* > > + * We should really assert here, but since it's during > > + * migration, let's try to reduce the usage of assertions. > > + */ > > I think this case should be removed, because arriving here with > MIGRATION_STATUS_ACTIVE is invalid. It should be handled by the default > case. OK, looks good here. I will add this part in V3. Thanks Zhang Chen > > Regards, > Lukas Straub > > > s->vm_was_running = true; > > /* Fallthrough */ > > case MIGRATION_STATUS_FAILED:
diff --git a/migration/colo.c b/migration/colo.c index ea7d1e9d4e..91c76789fa 100644 --- a/migration/colo.c +++ b/migration/colo.c @@ -670,8 +670,6 @@ void migrate_start_colo_process(MigrationState *s) colo_checkpoint_notify, s); qemu_sem_init(&s->colo_exit_sem, 0); - migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE, - MIGRATION_STATUS_COLO); colo_process_checkpoint(s); qemu_mutex_lock_iothread(); } diff --git a/migration/migration.c b/migration/migration.c index b63ad91d34..7f3f82a715 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2972,7 +2972,10 @@ static void migration_completion(MigrationState *s) goto fail_invalidate; } - if (!migrate_colo_enabled()) { + if (migrate_colo_enabled()) { + migrate_set_state(&s->state, current_active_state, + MIGRATION_STATUS_COLO); + } else { migrate_set_state(&s->state, current_active_state, MIGRATION_STATUS_COMPLETED); } @@ -3304,12 +3307,7 @@ static void migration_iteration_finish(MigrationState *s) migration_calculate_complete(s); runstate_set(RUN_STATE_POSTMIGRATE); break; - - case MIGRATION_STATUS_ACTIVE: - /* - * We should really assert here, but since it's during - * migration, let's try to reduce the usage of assertions. - */ + case MIGRATION_STATUS_COLO: if (!migrate_colo_enabled()) { error_report("%s: critical error: calling COLO code without " "COLO enabled", __func__); @@ -3319,6 +3317,11 @@ static void migration_iteration_finish(MigrationState *s) * Fixme: we will run VM in COLO no matter its old running state. * After exited COLO, we will keep running. */ + case MIGRATION_STATUS_ACTIVE: + /* + * We should really assert here, but since it's during + * migration, let's try to reduce the usage of assertions. + */ s->vm_was_running = true; /* Fallthrough */ case MIGRATION_STATUS_FAILED: