Re: [PATCH v2 01/16] migration: Don't use INT64_MAX for unlimited rate

[Juan Quintela <quintela@redhat.com>]

David Edmondson <david.edmondson@oracle.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> Define and use RATE_LIMIT_MAX instead.
>
> Suggest "RATE_LIMIT_MAX_NONE".

Then even better

RATE_LIMIT_DISABLED?
RATE_LIMIT_NONE?

Using MAX and NONE at the same time looks strange.

>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/migration-stats.h | 6 ++++++
>>  migration/migration.c       | 4 ++--
>>  migration/qemu-file.c       | 6 +++++-
>>  3 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
>> index cf8a4f0410..e782f1b0df 100644
>> --- a/migration/migration-stats.h
>> +++ b/migration/migration-stats.h
>> @@ -15,6 +15,12 @@
>>  
>>  #include "qemu/stats64.h"
>>  
>> +/*
>> + * If rate_limit_max is 0, there is special code to remove the rate
>> + * limit.
>> + */
>> +#define RATE_LIMIT_MAX 0
>> +
>>  /*
>>   * These are the ram migration statistic counters.  It is loosely
>>   * based on MigrationStats.  We change to Stat64 any counter that
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 039bba4804..c41c7491bb 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -2304,7 +2304,7 @@ static void migration_completion(MigrationState *s)
>>                   * them if migration fails or is cancelled.
>>                   */
>>                  s->block_inactive = !migrate_colo();
>> -                qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
>> +                qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX);
>>                  ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
>>                                                           s->block_inactive);
>>              }
>> @@ -3048,7 +3048,7 @@ static void *bg_migration_thread(void *opaque)
>>      rcu_register_thread();
>>      object_ref(OBJECT(s));
>>  
>> -    qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
>> +    qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX);
>>  
>>      setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
>>      /*
>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>> index 597054759d..4bc875b452 100644
>> --- a/migration/qemu-file.c
>> +++ b/migration/qemu-file.c
>> @@ -27,6 +27,7 @@
>>  #include "qemu/error-report.h"
>>  #include "qemu/iov.h"
>>  #include "migration.h"
>> +#include "migration-stats.h"
>>  #include "qemu-file.h"
>>  #include "trace.h"
>>  #include "options.h"
>> @@ -732,7 +733,10 @@ int qemu_file_rate_limit(QEMUFile *f)
>
> Given that qemu_file_rate_limit() is really a boolean, could it be
> declared as such?

I wanted to do on this patch justn $Subject.

You can see that when I move this function to
migration/migration-stats.c already do the type change.

That is patch:

[PATCH v2 05/16] migration: Move rate_limit_max and rate_limit_used to
migration_stats

diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index 3431453c90..1b16edae7d 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -22,3 +23,46 @@ void migration_time_since(MigrationAtomicStats *stats, int64_t since)
     int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST);
     stat64_set(&stats->setup_time, now - since);
 }
+
+bool migration_rate_exceeded(QEMUFile *f)
+{
+    if (qemu_file_get_error(f)) {
+        return true;
+    }
+
+    uint64_t rate_limit_used = stat64_get(&mig_stats.rate_limit_used);
+    uint64_t rate_limit_max = stat64_get(&mig_stats.rate_limit_max);
+
+    if (rate_limit_max == RATE_LIMIT_MAX) {
+        return false;
+    }
+    if (rate_limit_max > 0 && rate_limit_used > rate_limit_max) {
+        return true;
+    }
+    return false;
+}

Thanks, Juan.