Re: [PATCH V2 1/2] migration/colo: Optimize COLO boot code path

[Lukas Straub]

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:

pgpcfDO6fejOz.pgp
Description: OpenPGP digital signature