Re: [Qemu-devel] [PATCH COLO-Frame v9 05/32] migration: Integrate COLO checkpoint process into migration

[zhanghailiang <zhang.zhanghailiang@huawei.com>]

On 2015/10/10 0:53, Dr. David Alan Gilbert wrote:
> * zhanghailiang (zhang.zhanghailiang@huawei.com) wrote:
>> Add a migrate state: MIGRATION_STATUS_COLO, enter this migration state
>> after the first live migration successfully finished.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>
> If I understand this correctly, I think what you're doing is:
>       migration_thread
>             does initial migrate
>             init_checkpointer
>               creates a bh
>
>       main thread
>         bh
>           start_checkpointer
>               creates colo_thread
>
>       colo thread
>         outbound side loop
>
> Why not just keep the migration thread going and reuse that?
> Couldn't you just call the colo_thread() function at the place you
> currently call colo_init_checkpointer, and add the init code
> to the start of colo_thread?
> It seems simpler.
>

Of course, it is really a good idea to reuse migration thread,
i will fix it like that. and the helper function colo_thread will be renamed :)

Thanks,
zhanghailiang

> Other than that it's OK.
>
> Dave
>
>
>> ---
>>   include/migration/colo.h |  3 +++
>>   migration/colo.c         | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   migration/migration.c    | 23 ++++++++++++++-----
>>   qapi-schema.json         |  2 +-
>>   stubs/migration-colo.c   |  9 ++++++++
>>   trace-events             |  3 +++
>>   6 files changed, 92 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/migration/colo.h b/include/migration/colo.h
>> index 9b6662d..dface19 100644
>> --- a/include/migration/colo.h
>> +++ b/include/migration/colo.h
>> @@ -19,4 +19,7 @@
>>   bool colo_supported(void);
>>   void colo_info_mig_init(void);
>>
>> +void colo_init_checkpointer(MigrationState *s);
>> +bool migration_in_colo_state(void);
>> +
>>   #endif
>> diff --git a/migration/colo.c b/migration/colo.c
>> index 2c40d2e..97e64a3 100644
>> --- a/migration/colo.c
>> +++ b/migration/colo.c
>> @@ -10,9 +10,67 @@
>>    * later.  See the COPYING file in the top-level directory.
>>    */
>>
>> +#include "sysemu/sysemu.h"
>>   #include "migration/colo.h"
>> +#include "trace.h"
>> +
>> +static QEMUBH *colo_bh;
>>
>>   bool colo_supported(void)
>>   {
>>       return true;
>>   }
>> +
>> +bool migration_in_colo_state(void)
>> +{
>> +    MigrationState *s = migrate_get_current();
>> +
>> +    return (s->state == MIGRATION_STATUS_COLO);
>> +}
>> +
>> +static void *colo_thread(void *opaque)
>> +{
>> +    MigrationState *s = opaque;
>> +
>> +    qemu_mutex_lock_iothread();
>> +    vm_start();
>> +    qemu_mutex_unlock_iothread();
>> +    trace_colo_vm_state_change("stop", "run");
>> +
>> +    /*TODO: COLO checkpoint savevm loop*/
>> +
>> +    migrate_set_state(&s->state, MIGRATION_STATUS_COLO,
>> +                      MIGRATION_STATUS_COMPLETED);
>> +
>> +    qemu_mutex_lock_iothread();
>> +    qemu_bh_schedule(s->cleanup_bh);
>> +    qemu_mutex_unlock_iothread();
>> +
>> +    return NULL;
>> +}
>> +
>> +static void colo_start_checkpointer(void *opaque)
>> +{
>> +    MigrationState *s = opaque;
>> +
>> +    if (colo_bh) {
>> +        qemu_bh_delete(colo_bh);
>> +        colo_bh = NULL;
>> +    }
>> +
>> +    qemu_mutex_unlock_iothread();
>> +    qemu_thread_join(&s->thread);
>> +    qemu_mutex_lock_iothread();
>> +
>> +    migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
>> +                      MIGRATION_STATUS_COLO);
>> +
>> +    qemu_thread_create(&s->thread, "colo", colo_thread, s,
>> +                       QEMU_THREAD_JOINABLE);
>> +}
>> +
>> +void colo_init_checkpointer(MigrationState *s)
>> +{
>> +    colo_bh = qemu_bh_new(colo_start_checkpointer, s);
>> +    qemu_bh_schedule(colo_bh);
>> +}
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 98133f1..bee61aa 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -446,6 +446,10 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>>
>>           get_xbzrle_cache_stats(info);
>>           break;
>> +    case MIGRATION_STATUS_COLO:
>> +        info->has_status = true;
>> +        /* TODO: display COLO specific information (checkpoint info etc.) */
>> +        break;
>>       case MIGRATION_STATUS_COMPLETED:
>>           get_xbzrle_cache_stats(info);
>>
>> @@ -731,7 +735,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>
>>       if (s->state == MIGRATION_STATUS_ACTIVE ||
>>           s->state == MIGRATION_STATUS_SETUP ||
>> -        s->state == MIGRATION_STATUS_CANCELLING) {
>> +        s->state == MIGRATION_STATUS_CANCELLING ||
>> +        s->state == MIGRATION_STATUS_COLO) {
>>           error_setg(errp, QERR_MIGRATION_ACTIVE);
>>           return;
>>       }
>> @@ -948,6 +953,7 @@ static void *migration_thread(void *opaque)
>>       int64_t max_size = 0;
>>       int64_t start_time = initial_time;
>>       bool old_vm_running = false;
>> +    bool enable_colo = migrate_enable_colo();
>>
>>       rcu_register_thread();
>>
>> @@ -992,8 +998,10 @@ static void *migration_thread(void *opaque)
>>                   }
>>
>>                   if (!qemu_file_get_error(s->file)) {
>> -                    migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
>> -                                      MIGRATION_STATUS_COMPLETED);
>> +                    if (!enable_colo) {
>> +                        migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
>> +                                          MIGRATION_STATUS_COMPLETED);
>> +                    }
>>                       break;
>>                   }
>>               }
>> @@ -1044,11 +1052,16 @@ static void *migration_thread(void *opaque)
>>           }
>>           runstate_set(RUN_STATE_POSTMIGRATE);
>>       } else {
>> -        if (old_vm_running) {
>> +        if (s->state == MIGRATION_STATUS_ACTIVE && enable_colo) {
>> +            colo_init_checkpointer(s);
>> +        } else if (old_vm_running) {
>>               vm_start();
>>           }
>>       }
>> -    qemu_bh_schedule(s->cleanup_bh);
>> +
>> +    if (!enable_colo || s->state != MIGRATION_STATUS_ACTIVE) {
>> +        qemu_bh_schedule(s->cleanup_bh);
>> +    }
>>       qemu_mutex_unlock_iothread();
>>
>>       rcu_unregister_thread();
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index b14d1f4..6dd5c7c 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -433,7 +433,7 @@
>>   ##
>>   { 'enum': 'MigrationStatus',
>>     'data': [ 'none', 'setup', 'cancelling', 'cancelled',
>> -            'active', 'completed', 'failed' ] }
>> +            'active', 'completed', 'failed', 'colo' ] }
>>
>>   ##
>>   # @MigrationInfo
>> diff --git a/stubs/migration-colo.c b/stubs/migration-colo.c
>> index 3d817df..51b8f66 100644
>> --- a/stubs/migration-colo.c
>> +++ b/stubs/migration-colo.c
>> @@ -16,3 +16,12 @@ bool colo_supported(void)
>>   {
>>       return false;
>>   }
>> +
>> +bool migration_in_colo_state(void)
>> +{
>> +    return false;
>> +}
>> +
>> +void colo_init_checkpointer(MigrationState *s)
>> +{
>> +}
>> diff --git a/trace-events b/trace-events
>> index 8f9614a..487d1c7 100644
>> --- a/trace-events
>> +++ b/trace-events
>> @@ -1472,6 +1472,9 @@ rdma_start_incoming_migration_after_rdma_listen(void) ""
>>   rdma_start_outgoing_migration_after_rdma_connect(void) ""
>>   rdma_start_outgoing_migration_after_rdma_source_init(void) ""
>>
>> +# migration/colo.c
>> +colo_vm_state_change(const char *old, const char *new) "Change '%s' => '%s'"
>> +
>>   # kvm-all.c
>>   kvm_ioctl(int type, void *arg) "type 0x%x, arg %p"
>>   kvm_vm_ioctl(int type, void *arg) "type 0x%x, arg %p"
>> --
>> 1.8.3.1
>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
> .
>