Leonardo Brás <leob...@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 <quint...@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.
