Re: [PATCH v2 08/16] migration: Use migration_transferred_bytes() to calculate rate_limit
On Fri, May 26, 2023 at 5:17 AM Juan Quintela wrote:
>
> Leonardo Brás wrote:
> > On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> >> Signed-off-by: Juan Quintela
> >> Reviewed-by: Cédric Le Goater
> >> ---
> >> migration/migration-stats.h | 8 +++-
> >> migration/migration-stats.c | 7 +--
> >> migration/migration.c | 2 +-
> >> 3 files changed, 13 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> >> index 91fda378d3..f1465c2ebe 100644
> >> --- a/migration/migration-stats.h
> >> +++ b/migration/migration-stats.h
> >> @@ -81,6 +81,10 @@ typedef struct {
> >> * Number of bytes sent during precopy stage.
> >> */
> >> Stat64 precopy_bytes;
> >> +/*
> >> + * Amount of transferred data at the start of current cycle.
> >> + */
> >> +Stat64 rate_limit_start;
> >> /*
> >> * Maximum amount of data we can send in a cycle.
> >> */
> >> @@ -136,8 +140,10 @@ uint64_t migration_rate_get(void);
> >> * migration_rate_reset: Reset the rate limit counter.
> >> *
> >> * This is called when we know we start a new transfer cycle.
> >> + *
> >> + * @f: QEMUFile used for main migration channel
> >> */
> >> -void migration_rate_reset(void);
> >> +void migration_rate_reset(QEMUFile *f);
> >>
> >> /**
> >> * migration_rate_set: Set the maximum amount that can be transferred.
> >> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> >> index 301392d208..da2bb69a15 100644
> >> --- a/migration/migration-stats.c
> >> +++ b/migration/migration-stats.c
> >> @@ -31,7 +31,9 @@ bool migration_rate_exceeded(QEMUFile *f)
> >> return true;
> >> }
> >>
> >> -uint64_t rate_limit_used = stat64_get(_stats.rate_limit_used);
> >> +uint64_t rate_limit_start = stat64_get(_stats.rate_limit_start);
> >> +uint64_t rate_limit_current = migration_transferred_bytes(f);
> >> +uint64_t rate_limit_used = rate_limit_current - rate_limit_start;
> >> uint64_t rate_limit_max = stat64_get(_stats.rate_limit_max);
> >
> > So, IIUC, instead of updating mig_stats.rate_limit_used every time data is
> > sent,
> > the idea is to 'reset' it to migration_transferred_bytes() at the beginning
> > of a
> > cycle, and read migration_transferred_bytes() again for checking if the
> > limit
> > was not crossed.
> >
> > Its a nice change since there is no need to update 2 counters, when 1 is
> > enough.
> >
> > I think it would look nicer if squashed with 9/16, though. It would make it
> > more
> > clear this is being added to replace migration_rate_account() strategy.
> >
> > What do you think?
>
> Already in tree.
My bad.
After I ended up reviewing the patchset I noticed a lot of it was
already in the PULL request.
>
> Done this way because on my tree there was an intermediate patch that
> did something like:
>
>
> uint64_t rate_limit_used = stat64_get(_stats.rate_limit_used);
> uint64_t rate_limit_start = stat64_get(_stats.rate_limit_start);
> uint64_t rate_limit_current = migration_transferred_bytes(f);
> uint64_t rate_limit_used_new = rate_limit_current - rate_limit_start;
>
> if (rate_limit_used_new != rate_limit_used) {
> printf("rate_limit old %lu new %lu\n", ...);
> }
>
> So I was sure that the counter that I was replacing had the same value
> that the new one.
Oh, I see.
You kept both to verify the implementation.
Makes sense
>
> This is the reason why I fixed transferred atomic in the previous patch,
> not because it mattered on the big scheme of things (migration_test was
> missing something like 100KB for the normal stage when I started, that
> for calculations don't matter). But to check if I was doing the things
> right it mattered. With that patch my replacement counter was exact,
> and none of the if's triggered.
>
> Except for the device transffer stages, there I missed something like
> 900KB, but it made no sense to go all over the tree to fix a counter
> that I was going to remove later.
Yeah, it makes no sense to invest time on stuff that will be removed later.
Thanks for helping me understand this :)
>
> Regards, Juan.
>
Re: [PATCH v2 08/16] migration: Use migration_transferred_bytes() to calculate rate_limit
Leonardo Brás wrote:
> On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela
>> Reviewed-by: Cédric Le Goater
>> ---
>> migration/migration-stats.h | 8 +++-
>> migration/migration-stats.c | 7 +--
>> migration/migration.c | 2 +-
>> 3 files changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
>> index 91fda378d3..f1465c2ebe 100644
>> --- a/migration/migration-stats.h
>> +++ b/migration/migration-stats.h
>> @@ -81,6 +81,10 @@ typedef struct {
>> * Number of bytes sent during precopy stage.
>> */
>> Stat64 precopy_bytes;
>> +/*
>> + * Amount of transferred data at the start of current cycle.
>> + */
>> +Stat64 rate_limit_start;
>> /*
>> * Maximum amount of data we can send in a cycle.
>> */
>> @@ -136,8 +140,10 @@ uint64_t migration_rate_get(void);
>> * migration_rate_reset: Reset the rate limit counter.
>> *
>> * This is called when we know we start a new transfer cycle.
>> + *
>> + * @f: QEMUFile used for main migration channel
>> */
>> -void migration_rate_reset(void);
>> +void migration_rate_reset(QEMUFile *f);
>>
>> /**
>> * migration_rate_set: Set the maximum amount that can be transferred.
>> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
>> index 301392d208..da2bb69a15 100644
>> --- a/migration/migration-stats.c
>> +++ b/migration/migration-stats.c
>> @@ -31,7 +31,9 @@ bool migration_rate_exceeded(QEMUFile *f)
>> return true;
>> }
>>
>> -uint64_t rate_limit_used = stat64_get(_stats.rate_limit_used);
>> +uint64_t rate_limit_start = stat64_get(_stats.rate_limit_start);
>> +uint64_t rate_limit_current = migration_transferred_bytes(f);
>> +uint64_t rate_limit_used = rate_limit_current - rate_limit_start;
>> uint64_t rate_limit_max = stat64_get(_stats.rate_limit_max);
>
> So, IIUC, instead of updating mig_stats.rate_limit_used every time data is
> sent,
> the idea is to 'reset' it to migration_transferred_bytes() at the beginning
> of a
> cycle, and read migration_transferred_bytes() again for checking if the limit
> was not crossed.
>
> Its a nice change since there is no need to update 2 counters, when 1 is
> enough.
>
> I think it would look nicer if squashed with 9/16, though. It would make it
> more
> clear this is being added to replace migration_rate_account() strategy.
>
> What do you think?
Already in tree.
Done this way because on my tree there was an intermediate patch that
did something like:
uint64_t rate_limit_used = stat64_get(_stats.rate_limit_used);
uint64_t rate_limit_start = stat64_get(_stats.rate_limit_start);
uint64_t rate_limit_current = migration_transferred_bytes(f);
uint64_t rate_limit_used_new = rate_limit_current - rate_limit_start;
if (rate_limit_used_new != rate_limit_used) {
printf("rate_limit old %lu new %lu\n", ...);
}
So I was sure that the counter that I was replacing had the same value
that the new one.
This is the reason why I fixed transferred atomic in the previous patch,
not because it mattered on the big scheme of things (migration_test was
missing something like 100KB for the normal stage when I started, that
for calculations don't matter). But to check if I was doing the things
right it mattered. With that patch my replacement counter was exact,
and none of the if's triggered.
Except for the device transffer stages, there I missed something like
900KB, but it made no sense to go all over the tree to fix a counter
that I was going to remove later.
Regards, Juan.
Re: [PATCH v2 08/16] migration: Use migration_transferred_bytes() to calculate rate_limit
On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela
> Reviewed-by: Cédric Le Goater
> ---
> migration/migration-stats.h | 8 +++-
> migration/migration-stats.c | 7 +--
> migration/migration.c | 2 +-
> 3 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index 91fda378d3..f1465c2ebe 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -81,6 +81,10 @@ typedef struct {
> * Number of bytes sent during precopy stage.
> */
> Stat64 precopy_bytes;
> +/*
> + * Amount of transferred data at the start of current cycle.
> + */
> +Stat64 rate_limit_start;
> /*
> * Maximum amount of data we can send in a cycle.
> */
> @@ -136,8 +140,10 @@ uint64_t migration_rate_get(void);
> * migration_rate_reset: Reset the rate limit counter.
> *
> * This is called when we know we start a new transfer cycle.
> + *
> + * @f: QEMUFile used for main migration channel
> */
> -void migration_rate_reset(void);
> +void migration_rate_reset(QEMUFile *f);
>
> /**
> * migration_rate_set: Set the maximum amount that can be transferred.
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index 301392d208..da2bb69a15 100644
> --- a/migration/migration-stats.c
> +++ b/migration/migration-stats.c
> @@ -31,7 +31,9 @@ bool migration_rate_exceeded(QEMUFile *f)
> return true;
> }
>
> -uint64_t rate_limit_used = stat64_get(_stats.rate_limit_used);
> +uint64_t rate_limit_start = stat64_get(_stats.rate_limit_start);
> +uint64_t rate_limit_current = migration_transferred_bytes(f);
> +uint64_t rate_limit_used = rate_limit_current - rate_limit_start;
> uint64_t rate_limit_max = stat64_get(_stats.rate_limit_max);
So, IIUC, instead of updating mig_stats.rate_limit_used every time data is sent,
the idea is to 'reset' it to migration_transferred_bytes() at the beginning of a
cycle, and read migration_transferred_bytes() again for checking if the limit
was not crossed.
Its a nice change since there is no need to update 2 counters, when 1 is enough.
I think it would look nicer if squashed with 9/16, though. It would make it more
clear this is being added to replace migration_rate_account() strategy.
What do you think?
Other than that,
Reviewed-by: Leonardo Bras
>
> if (rate_limit_max == RATE_LIMIT_MAX) {
> @@ -58,9 +60,10 @@ void migration_rate_set(uint64_t limit)
> stat64_set(_stats.rate_limit_max, limit / XFER_LIMIT_RATIO);
> }
>
> -void migration_rate_reset(void)
> +void migration_rate_reset(QEMUFile *f)
> {
> stat64_set(_stats.rate_limit_used, 0);
> +stat64_set(_stats.rate_limit_start, migration_transferred_bytes(f));
> }
>
> void migration_rate_account(uint64_t len)
> diff --git a/migration/migration.c b/migration/migration.c
> index 39ff538046..e48dd593ed 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2691,7 +2691,7 @@ static void migration_update_counters(MigrationState *s,
> stat64_get(_stats.dirty_bytes_last_sync) / bandwidth;
> }
>
> -migration_rate_reset();
> +migration_rate_reset(s->to_dst_file);
>
> update_iteration_initial_status(s);
>
[PATCH v2 08/16] migration: Use migration_transferred_bytes() to calculate rate_limit
Signed-off-by: Juan Quintela
Reviewed-by: Cédric Le Goater
---
migration/migration-stats.h | 8 +++-
migration/migration-stats.c | 7 +--
migration/migration.c | 2 +-
3 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index 91fda378d3..f1465c2ebe 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -81,6 +81,10 @@ typedef struct {
* Number of bytes sent during precopy stage.
*/
Stat64 precopy_bytes;
+/*
+ * Amount of transferred data at the start of current cycle.
+ */
+Stat64 rate_limit_start;
/*
* Maximum amount of data we can send in a cycle.
*/
@@ -136,8 +140,10 @@ uint64_t migration_rate_get(void);
* migration_rate_reset: Reset the rate limit counter.
*
* This is called when we know we start a new transfer cycle.
+ *
+ * @f: QEMUFile used for main migration channel
*/
-void migration_rate_reset(void);
+void migration_rate_reset(QEMUFile *f);
/**
* migration_rate_set: Set the maximum amount that can be transferred.
diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index 301392d208..da2bb69a15 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -31,7 +31,9 @@ bool migration_rate_exceeded(QEMUFile *f)
return true;
}
-uint64_t rate_limit_used = stat64_get(_stats.rate_limit_used);
+uint64_t rate_limit_start = stat64_get(_stats.rate_limit_start);
+uint64_t rate_limit_current = migration_transferred_bytes(f);
+uint64_t rate_limit_used = rate_limit_current - rate_limit_start;
uint64_t rate_limit_max = stat64_get(_stats.rate_limit_max);
if (rate_limit_max == RATE_LIMIT_MAX) {
@@ -58,9 +60,10 @@ void migration_rate_set(uint64_t limit)
stat64_set(_stats.rate_limit_max, limit / XFER_LIMIT_RATIO);
}
-void migration_rate_reset(void)
+void migration_rate_reset(QEMUFile *f)
{
stat64_set(_stats.rate_limit_used, 0);
+stat64_set(_stats.rate_limit_start, migration_transferred_bytes(f));
}
void migration_rate_account(uint64_t len)
diff --git a/migration/migration.c b/migration/migration.c
index 39ff538046..e48dd593ed 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2691,7 +2691,7 @@ static void migration_update_counters(MigrationState *s,
stat64_get(_stats.dirty_bytes_last_sync) / bandwidth;
}
-migration_rate_reset();
+migration_rate_reset(s->to_dst_file);
update_iteration_initial_status(s);
--
2.40.1
