Re: [Qemu-devel] [PATCH 04/11] migration: split use of MigrationState.total_time

[Juan Quintela]

Peter Xu <address@hidden> wrote:
> It was used either to:
>
> 1. store initial timestamp of migration start, and
> 2. store total time used by last migration
>
> Let's provide two parameters for each of them.  Mix use of the two is
> slightly misleading.
>
> Signed-off-by: Peter Xu <address@hidden>

Reviewed-by: Juan Quintela <address@hidden>

If you have to respin, I would like to use the names:

start_time and total_time, i.e. without the mig_ preffix, because they
are in an struct that is clearly named migration O:-)

For the rest, good cleanup.

Later, Juan.


> ---
>  migration/migration.c | 13 +++++++------
>  migration/migration.h |  5 ++++-
>  2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 20f7565527..b684c2005d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -613,7 +613,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>          info->has_status = true;
>          info->has_total_time = true;
>          info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
> -            - s->total_time;
> +            - s->mig_start_time;
>          info->has_expected_downtime = true;
>          info->expected_downtime = s->expected_downtime;
>          info->has_setup_time = true;
> @@ -629,7 +629,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>      case MIGRATION_STATUS_COMPLETED:
>          info->has_status = true;
>          info->has_total_time = true;
> -        info->total_time = s->total_time;
> +        info->total_time = s->mig_total_time;
>          info->has_downtime = true;
>          info->downtime = s->downtime;
>          info->has_setup_time = true;
> @@ -1270,7 +1270,8 @@ MigrationState *migrate_init(void)
>  
>      migrate_set_state(&s->state, MIGRATION_STATUS_NONE, 
> MIGRATION_STATUS_SETUP);
>  
> -    s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    s->mig_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    s->mig_total_time = 0;
>      return s;
>  }
>  
> @@ -2293,13 +2294,13 @@ static void *migration_thread(void *opaque)
>      qemu_mutex_lock_iothread();
>      if (s->state == MIGRATION_STATUS_COMPLETED) {
>          uint64_t transferred_bytes = qemu_ftell(s->to_dst_file);
> -        s->total_time = end_time - s->total_time;
> +        s->mig_total_time = end_time - s->mig_start_time;
>          if (!entered_postcopy) {
>              s->downtime = end_time - start_time;
>          }
> -        if (s->total_time) {
> +        if (s->mig_total_time) {
>              s->mbps = (((double) transferred_bytes * 8.0) /
> -                       ((double) s->total_time)) / 1000;
> +                       ((double) s->mig_total_time)) / 1000;
>          }
>          runstate_set(RUN_STATE_POSTMIGRATE);
>      } else {
> diff --git a/migration/migration.h b/migration/migration.h
> index 663415fe48..ac74a12713 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -103,7 +103,10 @@ struct MigrationState
>      } rp_state;
>  
>      double mbps;
> -    int64_t total_time;
> +    /* Timestamp when recent migration starts (ms) */
> +    int64_t mig_start_time;
> +    /* Total time used by latest migration (ms) */
> +    int64_t mig_total_time;
>      int64_t downtime;
>      int64_t expected_downtime;
>      bool enabled_capabilities[MIGRATION_CAPABILITY__MAX];