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

[Juan Quintela]

"Zhang, Chen" <chen.zhang@intel.com> wrote:
>> -----Original Message-----
>> From: Juan Quintela <quintela@redhat.com>
>> Sent: Wednesday, November 17, 2021 12:28 AM
>> To: Zhang, Chen <chen.zhang@intel.com>
>> Cc: Hailiang Zhang <zhang.zhanghailiang@huawei.com>; Dr . David Alan
>> Gilbert <dgilbert@redhat.com>; qemu-dev <qemu-devel@nongnu.org>
>> Subject: Re: [PATCH 1/2] migration/colo: Optimize COLO start code path
>> 
>> Zhang Chen <chen.zhang@intel.com> wrote:
>> > There is no need to start COLO through MIGRATION_STATUS_ACTIVE.
>> 
>> Hi
>> 
>> I don't understand what you are trying to do.  In my reading, at least the
>> commit message is wrong:
>> 
>> void migrate_start_colo_process(MigrationState *s) {
>>     ...
>>     migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
>>                       MIGRATION_STATUS_COLO);
>>     ...
>> }
>> 
>> and
>> 
>> void *colo_process_incoming_thread(void *opaque) {
>>     ...
>>     migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
>>                       MIGRATION_STATUS_COLO);
>> 
>> So colo starts with MIGRATION_STATUS_ACTIVE.
>
> Yes, this patch just optimized COLO primary code 
> path(migrate_start_colo_process()).
> We can see this patch removed the 
>  migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
>                       MIGRATION_STATUS_COLO);
> In the migrate_start_colo_process().
>
> Current COLO status path:
>  MIGRATION_STATUS_XXX   --->   MIGRATION_STATUS_ACTIVE ---> 
> MIGRATION_STATUS_COLO ---> MIGRATION_STATUS_COMPLETED
>
> This patch try to remove redundant " MIGRATION_STATUS_ACTIVE " in COLO start. 
> MIGRATION_STATUS_XXX   ---> MIGRATION_STATUS_COLO ---> 
> MIGRATION_STATUS_COMPLETED
>
> Actually COLO primary code did nothing when running on 
> "MIGRATION_STATUS_ACTIVE".
> But for COLO secondary (void *colo_process_incoming_thread()), it shared some 
> code with normal migration. No need to do this.
>
> So, I will fix commit message to:
> Optimize COLO primary start path to:
> MIGRATION_STATUS_XXX   ---> MIGRATION_STATUS_COLO ---> 
> MIGRATION_STATUS_COMPLETED
> No need to start primary COLO through "MIGRATION_STATUS_ACTIVE".
>
> How about it?

Much better, thank.s

>> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> > ---
>> >  migration/colo.c      |  2 --
>> >  migration/migration.c | 18 +++++++++++-------
>> >  2 files changed, 11 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/migration/colo.c b/migration/colo.c index
>> > 2415325262..ad1a4426b3 100644
>> > --- a/migration/colo.c
>> > +++ b/migration/colo.c
>> > @@ -667,8 +667,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
>> > abaf6f9e3d..4c8662a839 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -3222,7 +3222,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);
>> >      }
>> 
>> This moves the setup to MIGRATION_STATUS_COLO to completion time
>> instead of the beggining of the process.  I have no clue why.  I guess you 
>> can
>> put a comment/commit message to say what you ar.e trynig to do.
>
> You are right, no need to setup here.
> I will remove this in next version.

Thanks.

>> > @@ -3607,12 +3610,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__); @@ -3622,6
>> > +3620,12 @@ 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.
>> >           */
>> > +         /* Fallthrough */
>> > +    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:
>> 
>> I guess this change is related to the previous one, but I don't understand 
>> colo
>> enough to review it.
>
> I think this patch is the general code, little background needed.
> You can simple understand COLO is two VMs(primary node and secondary node) 
> entered a state of cyclic migration.
> Thanks your comments.

Later, Juan.