Re: [PATCH v2 03/16] migration: Move setup_time to mig_stats

["Leonardo Brás" <leobras@redhat.com>]

On Mon, 2023-05-15 at 21:56 +0200, Juan Quintela wrote:
> It is a time that needs to be cleaned each time cancel migration.
> Once there create migration_time_since() to calculate how time since a
> time in the past.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> ---
> 
> Rename to migration_time_since (cédric)
> ---
>  migration/migration-stats.h | 13 +++++++++++++
>  migration/migration.h       |  1 -
>  migration/migration-stats.c |  7 +++++++
>  migration/migration.c       |  9 ++++-----
>  4 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index e782f1b0df..21402af9e4 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -75,6 +75,10 @@ typedef struct {
>       * Number of bytes sent during precopy stage.
>       */
>      Stat64 precopy_bytes;
> +    /*
> +     * How long has the setup stage took.
> +     */
> +    Stat64 setup_time;
>      /*
>       * Total number of bytes transferred.
>       */
> @@ -87,4 +91,13 @@ typedef struct {
>  
>  extern MigrationAtomicStats mig_stats;
>  
> +/**
> + * migration_time_since: Calculate how much time has passed
> + *
> + * @stats: migration stats
> + * @since: reference time since we want to calculate
> + *
> + * Returns: Nothing.  The time is stored in val.
> + */
> +void migration_time_since(MigrationAtomicStats *stats, int64_t since);
>  #endif
> diff --git a/migration/migration.h b/migration/migration.h
> index 48a46123a0..27aa3b1035 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -316,7 +316,6 @@ struct MigrationState {
>      int64_t downtime;
>      int64_t expected_downtime;
>      bool capabilities[MIGRATION_CAPABILITY__MAX];
> -    int64_t setup_time;
>      /*
>       * Whether guest was running when we enter the completion stage.
>       * If migration is interrupted by any reason, we need to continue
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index 2f2cea965c..3431453c90 100644
> --- a/migration/migration-stats.c
> +++ b/migration/migration-stats.c
> @@ -12,6 +12,13 @@
>  
>  #include "qemu/osdep.h"
>  #include "qemu/stats64.h"
> +#include "qemu/timer.h"
>  #include "migration-stats.h"
>  
>  MigrationAtomicStats mig_stats;
> +
> +void migration_time_since(MigrationAtomicStats *stats, int64_t since)
> +{
> +    int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> +    stat64_set(&stats->setup_time, now - since);
> +}

IIUC this calculates a time delta and saves on stats->setup_time, is that right?

It took me some time to understand that, since the function name is
migration_time_since(), which seems more generic.

Would not be more intuitive to name it migration_setup_time_set() or so?

Anyway,
Reviewed-by: Leonardo Bras <leobras@redhat.com>


> diff --git a/migration/migration.c b/migration/migration.c
> index c41c7491bb..e9466273bb 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -887,7 +887,7 @@ static void populate_time_info(MigrationInfo *info, MigrationState *s)
>  {
>      info->has_status = true;
>      info->has_setup_time = true;
> -    info->setup_time = s->setup_time;
> +    info->setup_time = stat64_get(&mig_stats.setup_time);
>  
>      if (s->state == MIGRATION_STATUS_COMPLETED) {
>          info->has_total_time = true;
> @@ -1390,7 +1390,6 @@ void migrate_init(MigrationState *s)
>      s->pages_per_second = 0.0;
>      s->downtime = 0;
>      s->expected_downtime = 0;
> -    s->setup_time = 0;


I could not see MigrationState->setup_time being initialized as 0 in this patch.
In a quick look in the code I noticed there is no initialization of this struct,
but on qemu_savevm_state() and migrate_prepare() we have:

memset(&mig_stats, 0, sizeof(mig_stats));

I suppose this is enough, right?


>      s->start_postcopy = false;
>      s->postcopy_after_devices = false;
>      s->migration_thread_running = false;
> @@ -2647,7 +2646,7 @@ static void migration_calculate_complete(MigrationState *s)
>          s->downtime = end_time - s->downtime_start;
>      }
>  
> -    transfer_time = s->total_time - s->setup_time;
> +    transfer_time = s->total_time - stat64_get(&mig_stats.setup_time);
>      if (transfer_time) {
>          s->mbps = ((double) bytes * 8.0) / transfer_time / 1000;
>      }
> @@ -2969,7 +2968,7 @@ static void *migration_thread(void *opaque)
>      qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
>                                 MIGRATION_STATUS_ACTIVE);
>  
> -    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> +    migration_time_since(&mig_stats, setup_start);
>  
>      trace_migration_thread_setup_complete();
>  
> @@ -3081,7 +3080,7 @@ static void *bg_migration_thread(void *opaque)
>      qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
>                                 MIGRATION_STATUS_ACTIVE);
>  
> -    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> +    migration_time_since(&mig_stats, setup_start);
>  
>      trace_migration_thread_setup_complete();
>      s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);