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

[David Edmondson]

Juan Quintela <quintela@redhat.com> writes:

> 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?

RATE_LIMIT_NONE sounds good to me.

>
> 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.

Thank you 👍

> 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.
-- 
I was better off when I was on your side, and I was holding on.