Re: [PATCH v2 03/16] migration: Move setup_time to mig_stats
On Fri, May 26, 2023 at 5:07 AM Juan Quintela wrote:
>
> Leonardo Brás 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
> >>
> >> ---
> >>
> >> 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(>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(_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.
>
Re: [PATCH v2 03/16] migration: Move setup_time to mig_stats
Leonardo Brás 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
>>
>> ---
>>
>> 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(>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(_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.
Re: [PATCH v2 03/16] migration: Move setup_time to mig_stats
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
>
> ---
>
> 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(>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
> 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(_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(_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(_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(_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(_stats, setup_start);
>
>
Re: [PATCH v2 03/16] migration: Move setup_time to mig_stats
Juan Quintela writes:
> David Edmondson wrote:
>> Juan Quintela 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
>>>
>>> ---
>>>
>>> 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.
--
You can't hide from the flipside.
Re: [PATCH v2 03/16] migration: Move setup_time to mig_stats
David Edmondson wrote:
> Juan Quintela 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
>>
>> ---
>>
>> 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.
Re: [PATCH v2 03/16] migration: Move setup_time to mig_stats
Juan Quintela 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
>
> ---
>
> 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(>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(_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(_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(_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(_stats, setup_start);
>
> trace_migration_thread_setup_complete();
> s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> --
> 2.40.1
--
Walk without rhythm and it won't attract the worm.
[PATCH v2 03/16] migration: Move setup_time to mig_stats
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
---
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(>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(_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(_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(_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(_stats, setup_start);
trace_migration_thread_setup_complete();
s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
--
2.40.1
