Re: [PATCH 08/21] migration: Move setup_time to mig_stats

[Cédric Le Goater]

On 5/8/23 15:08, Juan Quintela wrote:
> It is a time that needs to be cleaned each time cancel migration.
> Once there ccreate calculate_time_since() to calculate how time since
> a time in the past.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>   migration/migration-stats.c |  7 +++++++
>   migration/migration-stats.h | 14 ++++++++++++++
>   migration/migration.c       |  9 ++++-----
>   migration/migration.h       |  1 -
>   4 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index 2f2cea965c..5278c6c821 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 calculate_time_since(Stat64 *val, int64_t since)
> +{
> +    int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> +    stat64_set(val, now - since);
> +}
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index cf8a4f0410..73c73d75b9 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -69,6 +69,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.
>        */
> @@ -81,4 +85,14 @@ typedef struct {
>   
>   extern MigrationAtomicStats mig_stats;
>   
> +/**
> + * calculate_time_since: Calculate how much time has passed
> + *
> + * @val: stat64 where to store the time
> + * @since: reference time since we want to calculate
> + *
> + * Returns: Nothing.  The time is stored in val.
> + */
> +
> +void calculate_time_since(Stat64 *val, int64_t since);

Since this routine is in the "migration" namespace, I would rename it to

  void migration_time_since(Stat64 *val, int64_t since);

of even

  void migration_time_since(MigrationAtomicStats *stat, int64_t since);

Do you need it elsewhere than in migration.c ?

Thanks,

C.

>   #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index b1cfb56523..72286de969 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -884,7 +884,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;
> @@ -1387,7 +1387,6 @@ void migrate_init(MigrationState *s)
>       s->pages_per_second = 0.0;
>       s->downtime = 0;
>       s->expected_downtime = 0;
> -    s->setup_time = 0;
>       s->start_postcopy = false;
>       s->postcopy_after_devices = false;
>       s->migration_thread_running = false;
> @@ -2640,7 +2639,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;
>       }
> @@ -2965,7 +2964,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;
> +    calculate_time_since(&mig_stats.setup_time, setup_start);
>   
>       trace_migration_thread_setup_complete();
>   
> @@ -3077,7 +3076,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;
> +    calculate_time_since(&mig_stats.setup_time, setup_start);
>   
>       trace_migration_thread_setup_complete();
>       s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> diff --git a/migration/migration.h b/migration/migration.h
> index 3a918514e7..7f554455ac 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -311,7 +311,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