Re: [PULL 09/12] migration: Use migration_transferred_bytes() to calculate rate_limit

[Juan Quintela <quintela@redhat.com>]

Fiona Ebner <f.ebner@proxmox.com> wrote:
> Am 18.05.23 um 19:13 schrieb Juan Quintela:
>> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
>> index feec7d7369..97759a45f3 100644
>> --- a/migration/migration-stats.c
>> +++ b/migration/migration-stats.c
>> @@ -24,7 +24,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);
>>  
>>      if (rate_limit_max == RATE_LIMIT_DISABLED) {
>
> Hi,
> just wanted to let you know that the call to
> migration_transferred_bytes(f) here can introduce a huge performance
> penalty when taking a snapshot. I ran into the issue when testing
> something else, with a single-disk snapshot. Without this call it takes
> about two seconds, with the call about two minutes.

Ouch.

Now that everything is reviewed for that series I can sent the new set
of patches.  As I drop the counter it should just get the speed back.

New series comming that removed rate_limit counter altogether.

Can you take a look after I send it?

Thanks for the report.

And now that we are at it.  How are you testing this?

As you appears to be the bigger user of snapshots (or at least the
louder).  Creating tests/qtest/snapshot-test.c could be a good idea.

1st to check this kind of breakage.
2nd so I be sure that we don't "pesimize" your use case.

Hint, hint.

2 seconds vs 2 minutes.

A more detailed explanation that what are you doing would be great.
I.e. you are taking lots of snapshots by second or what?

Later, Juan.