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

[Leonardo Bras Soares Passos]

On Fri, May 26, 2023 at 5:07 AM Juan Quintela <quintela@redhat.com> wrote:
>
> 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).

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