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

[Juan Quintela <quintela@redhat.com>]

Leonardo Brás <leobras@redhat.com> wrote:
> 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?

Dropped this.
Other reviewer commented that this was not a counter, what is right.  So
I left the times for future work (it don't interfere with current
cleanups).


> 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?

Yeap.  All migration_stats() are initialized to zero at the start of
qemu, or when we start a migration.

After a migration, it don't matter if it finished with/without error,
they are there with the right value until we start another migration (in
the case of error, of course).

Later, Juan.