Leonardo Brás <leob...@redhat.com> wrote:
> On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela <quint...@redhat.com>
>> Reviewed-by: Cédric Le Goater <c...@kaod.org>
>> ---
>> 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(&mig_stats.rate_limit_used);
>> + uint64_t rate_limit_start = stat64_get(&mig_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(&mig_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(&mig_stats.rate_limit_used);
uint64_t rate_limit_start = stat64_get(&mig_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.
