| Message ID | 20230515195709.63843-4-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Migration: More migration atomic counters | expand |
Juan Quintela <quintela@redhat.com> writes: > 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; Is this really a Stat64? It doesn't appear to need the atomic update feature. > /* > * 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. "stored in stats->setup_time" > + */ > +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); > +} > diff --git a/migration/migration.c b/migration/migration.c > index c41c7491bb..e9466273bb 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -887,7 +887,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; > @@ -1390,7 +1390,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; > @@ -2647,7 +2646,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; > } > @@ -2969,7 +2968,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; > + migration_time_since(&mig_stats, setup_start); > > trace_migration_thread_setup_complete(); > > @@ -3081,7 +3080,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; > + migration_time_since(&mig_stats, setup_start); > > trace_migration_thread_setup_complete(); > s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); > -- > 2.40.1
David Edmondson <david.edmondson@oracle.com> wrote: > Juan Quintela <quintela@redhat.com> writes: > >> 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; > > Is this really a Stat64? It doesn't appear to need the atomic update > feature. What this whole Migration Atomic Counters series try to do is that everything becomes atomic and then we can use everything everywhere. Before this series we had (I am simplifying here): - transferred, precopy_bytes, postcopy_bytes, downtime_bytes -> atomic, you can use it anywhere - qemu_file transferred -> you can only use it from the main migration thread - qemu_file rate_limit -> you can only use it from the main migration thread And we had to update the three counters in every place that we did a write wehad to update all of them. You can see the contorsions that we go to to update the rate_limit and the qemu_file transferred fields. After the series (you need to get what it is already on the tree, this series, QEMUFileHooks cleanup, and another serie on my tree waiting for this to be commited), you got three counters: - qemu_file: atomic, everytime we do a qemu_file write we update it - multifd_bytes: atomic, everytime that we do a write in a multifd channel, we update it. - rdma_bytes: atomic, everytime we do a write through RDMA we update it. And that is it. Both rate_limit and transferred are derived from these three counters: - at any point in time migration_transferred_bytes() returns the amount of bytes written since the start of the migration: qemu_file_bytes + multifd_bytes + rdma_bytes. - transferred on this period: at_start_of_period = migration_transferred_bytes(). trasferred_in_this_period = migration_transferred_bytes() - at_start_of_period; - Similar for precopy_bytes, postcopy_bytes and downtime_bytes. When we move from one stage to the next, we store what is the value of the previous stage. The counters that we use to calculate the rate limit are updated around 10 times per second (can be a bit bigger at the end of periods, iterations, ...) So performance is not extra critical. But as we have way less atomic operations (really one per real write), we don't really care a lot if we do some atomic operations when a normal operation will do. I.e. I think we have two options: - have the remaining counters that are only used in the main migration thread not be atomic. Document them and remember to do the correct thing everytime we use it. If we need to use it in another thread, just change it to atomic. - Make all counters atomic. No need to document anything. And you can call any operation/counter/... in migration-stats.c from anywhere. I think that the second option is better. But I can hear reasons from people that think that the 1st one is better. Comments? Later, Juan.
Juan Quintela <quintela@redhat.com> writes: > David Edmondson <david.edmondson@oracle.com> wrote: >> Juan Quintela <quintela@redhat.com> writes: >> >>> 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; >> >> Is this really a Stat64? It doesn't appear to need the atomic update >> feature. > > What this whole Migration Atomic Counters series try to do is that > everything becomes atomic and then we can use everything everywhere. > > Before this series we had (I am simplifying here): > > - transferred, precopy_bytes, postcopy_bytes, downtime_bytes -> atomic, > you can use it anywhere > > - qemu_file transferred -> you can only use it from the main migration > thread > > - qemu_file rate_limit -> you can only use it from the main migration > thread > > And we had to update the three counters in every place that we did a > write wehad to update all of them. > > You can see the contorsions that we go to to update the rate_limit and > the qemu_file transferred fields. > > After the series (you need to get what it is already on the tree, this > series, QEMUFileHooks cleanup, and another serie on my tree waiting for > this to be commited), you got three counters: > > - qemu_file: atomic, everytime we do a qemu_file write we update it > - multifd_bytes: atomic, everytime that we do a write in a multifd > channel, we update it. > - rdma_bytes: atomic, everytime we do a write through RDMA we update it. > > And that is it. > > Both rate_limit and transferred are derived from these three counters: > > - at any point in time migration_transferred_bytes() returns the amount > of bytes written since the start of the migration: > qemu_file_bytes + multifd_bytes + rdma_bytes. > > - transferred on this period: > at_start_of_period = migration_transferred_bytes(). > trasferred_in_this_period = migration_transferred_bytes() - at_start_of_period; > > - Similar for precopy_bytes, postcopy_bytes and downtime_bytes. When we > move from one stage to the next, we store what is the value of the > previous stage. > > The counters that we use to calculate the rate limit are updated around > 10 times per second (can be a bit bigger at the end of periods, > iterations, ...) So performance is not extra critical. > > But as we have way less atomic operations (really one per real write), > we don't really care a lot if we do some atomic operations when a normal > operation will do. > > I.e. I think we have two options: > > - have the remaining counters that are only used in the main migration > thread not be atomic. Document them and remember to do the correct > thing everytime we use it. If we need to use it in another thread, > just change it to atomic. > > - Make all counters atomic. No need to document anything. And you can > call any operation/counter/... in migration-stats.c from anywhere. > > I think that the second option is better. But I can hear reasons from > people that think that the 1st one is better. For the counters, no argument - making them all atomic seems like the right way forward. start_time isn't a counter, and isn't manipulated at multiple points in the code by different actors. I don't hate it being a Stat64, it just seems odd when the other 'time' related variables are not. > Comments? > > Later, Juan.
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? Anyway, Reviewed-by: Leonardo Bras <leobras@redhat.com> > diff --git a/migration/migration.c b/migration/migration.c > index c41c7491bb..e9466273bb 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -887,7 +887,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; > @@ -1390,7 +1390,6 @@ void migrate_init(MigrationState *s) > s->pages_per_second = 0.0; > s->downtime = 0; > s->expected_downtime = 0; > - s->setup_time = 0; 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? > s->start_postcopy = false; > s->postcopy_after_devices = false; > s->migration_thread_running = false; > @@ -2647,7 +2646,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; > } > @@ -2969,7 +2968,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; > + migration_time_since(&mig_stats, setup_start); > > trace_migration_thread_setup_complete(); > > @@ -3081,7 +3080,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; > + migration_time_since(&mig_stats, setup_start); > > trace_migration_thread_setup_complete(); > s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
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.
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. >
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); +} diff --git a/migration/migration.c b/migration/migration.c index c41c7491bb..e9466273bb 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -887,7 +887,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; @@ -1390,7 +1390,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; @@ -2647,7 +2646,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; } @@ -2969,7 +2968,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; + migration_time_since(&mig_stats, setup_start); trace_migration_thread_setup_complete(); @@ -3081,7 +3080,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; + migration_time_since(&mig_stats, setup_start); trace_migration_thread_setup_complete(); s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
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(-)