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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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.

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