On Fri, May 26, 2023 at 5:07 AM Juan Quintela <quint...@redhat.com> wrote:
>
> 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).
Oh, it makes sense.
>
>
> > 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).
That's great to simplify the code.
Thanks!
>
> Later, Juan.
>
