Re: [PATCH v2 08/16] migration: Use migration_transferred_bytes() to calculate rate_limit

[Juan Quintela]

Leonardo Brás <leobras@redhat.com> wrote:
> On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Cédric Le Goater <clg@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.