[v2] Migration: More migration atomic counters

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

Posted by Juan Quintela 2 years, 8 months ago

Define and use RATE_LIMIT_MAX instead.

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)
     if (qemu_file_get_error(f)) {
         return 1;
     }
-    if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) {
+    if (f->rate_limit_max == RATE_LIMIT_MAX) {
+        return 0;
+    }
+    if (f->rate_limit_used > f->rate_limit_max) {
         return 1;
     }
     return 0;
-- 
2.40.1

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

Posted by David Edmondson 2 years, 8 months ago

Juan Quintela <quintela@redhat.com> writes:

> Define and use RATE_LIMIT_MAX instead.

Suggest "RATE_LIMIT_MAX_NONE".

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

>      if (qemu_file_get_error(f)) {
>          return 1;
>      }
> -    if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) {
> +    if (f->rate_limit_max == RATE_LIMIT_MAX) {
> +        return 0;
> +    }
> +    if (f->rate_limit_used > f->rate_limit_max) {
>          return 1;
>      }
>      return 0;
> -- 
> 2.40.1
-- 
All those lines and circles, to me, a mystery.

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

Posted by Juan Quintela 2 years, 8 months ago

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.

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

Posted by Cédric Le Goater 2 years, 8 months ago

On 5/16/23 11:24, Juan Quintela wrote:
> 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?

I'd vote for RATE_LIMIT_DISABLED.

> RATE_LIMIT_NONE?
> 
> Using MAX and NONE at the same time looks strange.

Cheers,

C.

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

Posted by Leonardo Brás 2 years, 8 months ago

On Tue, 2023-05-16 at 14:47 +0200, Cédric Le Goater wrote:
> On 5/16/23 11:24, Juan Quintela wrote:
> > 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?
> 
> I'd vote for RATE_LIMIT_DISABLED.

Me too.

> 
> > RATE_LIMIT_NONE?
> > 
> > Using MAX and NONE at the same time looks strange.
> 
> Cheers,
> 
> C.
> 

Reviewed-by: Leonardo Bras <leobras@redhat.com>

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

Posted by David Edmondson 2 years, 8 months ago

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.

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

Posted by Harsh Prateek Bora 2 years, 8 months ago


On 5/16/23 01:26, Juan Quintela wrote:
> Define and use RATE_LIMIT_MAX instead.
> 
> 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
                         .. there is a check in qemu_file_rate_limit() ..
> + * limit.
> + */
> +#define RATE_LIMIT_MAX 0
> +
Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

>   /*
>    * 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)
>       if (qemu_file_get_error(f)) {
>           return 1;
>       }
> -    if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) {
> +    if (f->rate_limit_max == RATE_LIMIT_MAX) {
> +        return 0;
> +    }
> +    if (f->rate_limit_used > f->rate_limit_max) {
>           return 1;
>       }
>       return 0;