[PATCH v2 00/16] Migration: More migration atomic counters

[PATCH v2 00/16] Migration: More migration atomic counters

[Juan Quintela]

Hi

In this v2 series:
- More documentation here and there.
- Fix migration_rate_set() to really / XFER_LIMIT_RATIO
- All reviewed patches are in Migration PULL request 20230515
- There are later reviewed patches, but that depend on the first ones
  that are still not reviewed.

Please review.

Thanks, Juan.

Subject: [PULL 00/11] Migration 20230515 patches
Based-on: Message-Id: <20230515123334.58995-1-quintela@redhat.com>

[v1]
In this series:
- play with rate limit
  * document that a value of 0 means no rate-limit
  * change all users of INT64_MAX to use 0
  * Make sure that transferred value is right
    This gets transferred == multifd_bytes + qemu_file_transferred()
    until the completation stage.  Changing all devices is overkill and not useful.
  * Move all rate_limit calculations to use atomics instead of qemu_file_transferred().
    Use atomics for rate_limit.
  * RDMA
    Adjust counters here and there
    Change the "imaginary" 1 byte transfer to say if it has sent a page or not.
    More cleanups due to this changes
  * multifd: Adjust the number of transferred bytes in the right place and right amount
    right place: just after write, now with atomic counters we can
    right ammount: Now that we are in the right place, we can do it right also for compressing

Please review.

ToDo: Best described as ToSend:
- qemu_file_transfered() is based on atomics on my branch
- transferred atomic is not needed anymore

ToDo before my next send:

- downtime_bytes, precopy_bytes and postcopy_bytes should be based on
  migration_transfered_bytes and not need a counter of their own.

With that my cleanup would have finishing, moving from:
- total_transferred in QEMUFile (not atomic)
- rate_limit_used in QEMUFile (not atomic)
- multifd_bytes in mig_stats
- transferred in mig_stats (not updated everywhere needed, the
  following ones are based on this one)
- downtime_bytes in mig_stats
- precopy_bytes in mig_stats
- postcopy_bytes in mig_stats

To just:
- qemu_file_transferred in mig_stats
- multifd_bytes in mig_stats
- rdma_bytes in mig_stats

And for each transfer, we only update one of the three, everything
else is derived from this three values.

Later, Juan.

Juan Quintela (16):
  migration: Don't use INT64_MAX for unlimited rate
  migration: Correct transferred bytes value
  migration: Move setup_time to mig_stats
  qemu-file: Account for rate_limit usage on qemu_fflush()
  migration: Move rate_limit_max and rate_limit_used to migration_stats
  migration: Move migration_total_bytes() to migration-stats.c
  migration: Add a trace for migration_transferred_bytes
  migration: Use migration_transferred_bytes() to calculate rate_limit
  migration: We don't need the field rate_limit_used anymore
  migration: Don't abuse qemu_file transferred for RDMA
  migration/RDMA: It is accounting for zero/normal pages in two places
  migration/rdma: Remove QEMUFile parameter when not used
  migration/rdma: Don't use imaginary transfers
  migration: Remove unused qemu_file_credit_transfer()
  migration/rdma: Simplify the function that saves a page
  migration/multifd: Compute transferred bytes correctly

 include/migration/qemu-file-types.h | 12 ++++-
 migration/migration-stats.h         | 73 +++++++++++++++++++++++++++
 migration/migration.h               |  1 -
 migration/options.h                 |  7 ---
 migration/qemu-file.h               | 33 +++----------
 hw/ppc/spapr.c                      |  4 +-
 hw/s390x/s390-stattrib.c            |  2 +-
 migration/block-dirty-bitmap.c      |  2 +-
 migration/block.c                   |  5 +-
 migration/migration-stats.c         | 59 ++++++++++++++++++++++
 migration/migration.c               | 36 ++++++--------
 migration/multifd.c                 | 14 +++---
 migration/options.c                 |  7 ++-
 migration/qemu-file.c               | 77 ++++-------------------------
 migration/ram.c                     | 34 +++++++------
 migration/rdma.c                    | 64 +++++++++++++-----------
 migration/savevm.c                  | 21 ++++++--
 migration/vmstate.c                 |  3 ++
 migration/meson.build               |  2 +-
 migration/trace-events              |  3 ++
 20 files changed, 268 insertions(+), 191 deletions(-)

-- 
2.40.1

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

[Juan Quintela]

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

[Harsh Prateek Bora]

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;

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

[David Edmondson]

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

[Juan Quintela]

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

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

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

[Cédric Le Goater]

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

[Leonardo Brás]

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>

[PATCH v2 02/16] migration: Correct transferred bytes value

[Juan Quintela]

We forget several places to add to trasferred amount of data.  With
this fixes I get:

   qemu_file_transferred() + multifd_bytes == transferred

The only place whrer this is not true is during devices sending.  But
going all through the full tree searching for devices that use
QEMUFile directly is a bit too much.

Multifd, precopy and xbzrle work as expected. Postocpy still misses 35
bytes, but searching for them is getting complicated, so I stop here.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c       | 14 ++++++++++++++
 migration/savevm.c    | 19 +++++++++++++++++--
 migration/vmstate.c   |  3 +++
 migration/meson.build |  2 +-
 4 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index f69d8d42b0..fd5a8db0f8 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -337,6 +337,7 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file,
 
     g_free(le_bitmap);
 
+    stat64_add(&mig_stats.transferred, 8 + size + 8);
     if (qemu_file_get_error(file)) {
         return qemu_file_get_error(file);
     }
@@ -1392,6 +1393,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
                     return ret;
                 }
                 qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
+                stat64_add(&mig_stats.transferred, 8);
                 qemu_fflush(f);
             }
             /*
@@ -3020,6 +3022,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     RAMState **rsp = opaque;
     RAMBlock *block;
     int ret;
+    size_t size = 0;
 
     if (compress_threads_save_setup()) {
         return -1;
@@ -3038,16 +3041,20 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
         qemu_put_be64(f, ram_bytes_total_with_ignored()
                          | RAM_SAVE_FLAG_MEM_SIZE);
 
+        size += 8;
         RAMBLOCK_FOREACH_MIGRATABLE(block) {
             qemu_put_byte(f, strlen(block->idstr));
             qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr));
             qemu_put_be64(f, block->used_length);
+            size += 1 + strlen(block->idstr) + 8;
             if (migrate_postcopy_ram() && block->page_size !=
                                           qemu_host_page_size) {
                 qemu_put_be64(f, block->page_size);
+                size += 8;
             }
             if (migrate_ignore_shared()) {
                 qemu_put_be64(f, block->mr->addr);
+                size += 8;
             }
         }
     }
@@ -3064,11 +3071,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
 
     if (!migrate_multifd_flush_after_each_section()) {
         qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
+        size += 8;
     }
 
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
+    size += 8;
     qemu_fflush(f);
 
+    stat64_add(&mig_stats.transferred, size);
     return 0;
 }
 
@@ -3209,6 +3219,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
     RAMState **temp = opaque;
     RAMState *rs = *temp;
     int ret = 0;
+    size_t size = 0;
 
     rs->last_stage = !migration_in_colo_state();
 
@@ -3253,8 +3264,11 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
 
     if (!migrate_multifd_flush_after_each_section()) {
         qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
+        size += 8;
     }
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
+    size += 8;
+    stat64_add(&mig_stats.transferred, size);
     qemu_fflush(f);
 
     return 0;
diff --git a/migration/savevm.c b/migration/savevm.c
index e33788343a..c7af9050c2 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -952,6 +952,7 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se,
     qemu_put_byte(f, section_type);
     qemu_put_be32(f, se->section_id);
 
+    size_t size = 1 + 4;
     if (section_type == QEMU_VM_SECTION_FULL ||
         section_type == QEMU_VM_SECTION_START) {
         /* ID string */
@@ -961,7 +962,9 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se,
 
         qemu_put_be32(f, se->instance_id);
         qemu_put_be32(f, se->version_id);
+        size += 1 + len + 4 + 4;
     }
+    stat64_add(&mig_stats.transferred, size);
 }
 
 /*
@@ -973,6 +976,7 @@ static void save_section_footer(QEMUFile *f, SaveStateEntry *se)
     if (migrate_get_current()->send_section_footer) {
         qemu_put_byte(f, QEMU_VM_SECTION_FOOTER);
         qemu_put_be32(f, se->section_id);
+        stat64_add(&mig_stats.transferred, 1 + 4);
     }
 }
 
@@ -1032,6 +1036,7 @@ static void qemu_savevm_command_send(QEMUFile *f,
     qemu_put_be16(f, (uint16_t)command);
     qemu_put_be16(f, len);
     qemu_put_buffer(f, data, len);
+    stat64_add(&mig_stats.transferred, 1 + 2 + 2 + len);
     qemu_fflush(f);
 }
 
@@ -1212,11 +1217,13 @@ void qemu_savevm_state_header(QEMUFile *f)
     trace_savevm_state_header();
     qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
     qemu_put_be32(f, QEMU_VM_FILE_VERSION);
-
+    size_t size = 4 + 4;
     if (migrate_get_current()->send_configuration) {
         qemu_put_byte(f, QEMU_VM_CONFIGURATION);
+        size += 1;
         vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
     }
+    stat64_add(&mig_stats.transferred, size);
 }
 
 bool qemu_savevm_state_guest_unplug_pending(void)
@@ -1384,6 +1391,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
 {
     SaveStateEntry *se;
     int ret;
+    size_t size = 0;
 
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (!se->ops || !se->ops->save_live_complete_postcopy) {
@@ -1398,7 +1406,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
         /* Section type */
         qemu_put_byte(f, QEMU_VM_SECTION_END);
         qemu_put_be32(f, se->section_id);
-
+        size += 1 + 4;
         ret = se->ops->save_live_complete_postcopy(f, se->opaque);
         trace_savevm_section_end(se->idstr, se->section_id, ret);
         save_section_footer(f, se);
@@ -1409,6 +1417,8 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
     }
 
     qemu_put_byte(f, QEMU_VM_EOF);
+    size += 1;
+    stat64_add(&mig_stats.transferred, size);
     qemu_fflush(f);
 }
 
@@ -1484,6 +1494,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
     if (!in_postcopy) {
         /* Postcopy stream will still be going */
         qemu_put_byte(f, QEMU_VM_EOF);
+        stat64_add(&mig_stats.transferred, 1);
     }
 
     json_writer_end_array(vmdesc);
@@ -1664,15 +1675,18 @@ void qemu_savevm_live_state(QEMUFile *f)
     /* save QEMU_VM_SECTION_END section */
     qemu_savevm_state_complete_precopy(f, true, false);
     qemu_put_byte(f, QEMU_VM_EOF);
+    stat64_add(&mig_stats.transferred, 1);
 }
 
 int qemu_save_device_state(QEMUFile *f)
 {
     SaveStateEntry *se;
+    size_t size = 0;
 
     if (!migration_in_colo_state()) {
         qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
         qemu_put_be32(f, QEMU_VM_FILE_VERSION);
+        size = 4 + 4;
     }
     cpu_synchronize_all_states();
 
@@ -1690,6 +1704,7 @@ int qemu_save_device_state(QEMUFile *f)
 
     qemu_put_byte(f, QEMU_VM_EOF);
 
+    stat64_add(&mig_stats.transferred, size + 1);
     return qemu_file_get_error(f);
 }
 
diff --git a/migration/vmstate.c b/migration/vmstate.c
index af01d54b6f..ee3b70a6a8 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -12,6 +12,7 @@
 
 #include "qemu/osdep.h"
 #include "migration.h"
+#include "migration-stats.h"
 #include "migration/vmstate.h"
 #include "savevm.h"
 #include "qapi/qmp/json-writer.h"
@@ -394,6 +395,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                 written_bytes = qemu_file_transferred_fast(f) - old_offset;
                 vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes, i);
 
+                stat64_add(&mig_stats.transferred, written_bytes);
                 /* Compressed arrays only care about the first element */
                 if (vmdesc_loop && vmsd_can_compress(field)) {
                     vmdesc_loop = NULL;
@@ -517,6 +519,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
             qemu_put_byte(f, len);
             qemu_put_buffer(f, (uint8_t *)vmsdsub->name, len);
             qemu_put_be32(f, vmsdsub->version_id);
+            stat64_add(&mig_stats.transferred, 1 + 1 + len + 4);
             ret = vmstate_save_state(f, vmsdsub, opaque, vmdesc);
             if (ret) {
                 return ret;
diff --git a/migration/meson.build b/migration/meson.build
index dc8b1daef5..b3d0c537c8 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -1,5 +1,6 @@
 # Files needed by unit tests
 migration_files = files(
+  'migration-stats.c',
   'page_cache.c',
   'xbzrle.c',
   'vmstate-types.c',
@@ -18,7 +19,6 @@ softmmu_ss.add(files(
   'fd.c',
   'global_state.c',
   'migration-hmp-cmds.c',
-  'migration-stats.c',
   'migration.c',
   'multifd.c',
   'multifd-zlib.c',
-- 
2.40.1

Re: [PATCH v2 02/16] migration: Correct transferred bytes value

[David Edmondson]

Juan Quintela <quintela@redhat.com> writes:

> We forget several places to add to trasferred amount of data.  With

"transferred".

> this fixes I get:
>
>    qemu_file_transferred() + multifd_bytes == transferred
>
> The only place whrer this is not true is during devices sending.  But

"where"

> going all through the full tree searching for devices that use
> QEMUFile directly is a bit too much.
>
> Multifd, precopy and xbzrle work as expected. Postocpy still misses 35
> bytes, but searching for them is getting complicated, so I stop here.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/ram.c       | 14 ++++++++++++++
>  migration/savevm.c    | 19 +++++++++++++++++--
>  migration/vmstate.c   |  3 +++
>  migration/meson.build |  2 +-
>  4 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index f69d8d42b0..fd5a8db0f8 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -337,6 +337,7 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file,
>  
>      g_free(le_bitmap);
>  
> +    stat64_add(&mig_stats.transferred, 8 + size + 8);

Push this up after the qemu_fflush() call?

>      if (qemu_file_get_error(file)) {
>          return qemu_file_get_error(file);
>      }
> @@ -1392,6 +1393,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
>                      return ret;
>                  }
>                  qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
> +                stat64_add(&mig_stats.transferred, 8);
>                  qemu_fflush(f);
>              }
>              /*
> @@ -3020,6 +3022,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>      RAMState **rsp = opaque;
>      RAMBlock *block;
>      int ret;
> +    size_t size = 0;
>  
>      if (compress_threads_save_setup()) {
>          return -1;
> @@ -3038,16 +3041,20 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>          qemu_put_be64(f, ram_bytes_total_with_ignored()
>                           | RAM_SAVE_FLAG_MEM_SIZE);
>  
> +        size += 8;

Move the blank line down.

>          RAMBLOCK_FOREACH_MIGRATABLE(block) {
>              qemu_put_byte(f, strlen(block->idstr));
>              qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr));
>              qemu_put_be64(f, block->used_length);
> +            size += 1 + strlen(block->idstr) + 8;
>              if (migrate_postcopy_ram() && block->page_size !=
>                                            qemu_host_page_size) {
>                  qemu_put_be64(f, block->page_size);
> +                size += 8;
>              }
>              if (migrate_ignore_shared()) {
>                  qemu_put_be64(f, block->mr->addr);
> +                size += 8;
>              }
>          }
>      }
> @@ -3064,11 +3071,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>  
>      if (!migrate_multifd_flush_after_each_section()) {
>          qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
> +        size += 8;
>      }
>  
>      qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
> +    size += 8;
>      qemu_fflush(f);
>  
> +    stat64_add(&mig_stats.transferred, size);
>      return 0;
>  }
>  
> @@ -3209,6 +3219,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>      RAMState **temp = opaque;
>      RAMState *rs = *temp;
>      int ret = 0;
> +    size_t size = 0;
>  
>      rs->last_stage = !migration_in_colo_state();
>  
> @@ -3253,8 +3264,11 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>  
>      if (!migrate_multifd_flush_after_each_section()) {
>          qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
> +        size += 8;
>      }
>      qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
> +    size += 8;
> +    stat64_add(&mig_stats.transferred, size);

This is after qemu_fflush() in the other cases.

>      qemu_fflush(f);
>  
>      return 0;
> diff --git a/migration/savevm.c b/migration/savevm.c
> index e33788343a..c7af9050c2 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -952,6 +952,7 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se,
>      qemu_put_byte(f, section_type);
>      qemu_put_be32(f, se->section_id);
>  
> +    size_t size = 1 + 4;

Move the blank line down.

>      if (section_type == QEMU_VM_SECTION_FULL ||
>          section_type == QEMU_VM_SECTION_START) {
>          /* ID string */
> @@ -961,7 +962,9 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se,
>  
>          qemu_put_be32(f, se->instance_id);
>          qemu_put_be32(f, se->version_id);
> +        size += 1 + len + 4 + 4;
>      }
> +    stat64_add(&mig_stats.transferred, size);
>  }
>  
>  /*
> @@ -973,6 +976,7 @@ static void save_section_footer(QEMUFile *f, SaveStateEntry *se)
>      if (migrate_get_current()->send_section_footer) {
>          qemu_put_byte(f, QEMU_VM_SECTION_FOOTER);
>          qemu_put_be32(f, se->section_id);
> +        stat64_add(&mig_stats.transferred, 1 + 4);
>      }
>  }
>  
> @@ -1032,6 +1036,7 @@ static void qemu_savevm_command_send(QEMUFile *f,
>      qemu_put_be16(f, (uint16_t)command);
>      qemu_put_be16(f, len);
>      qemu_put_buffer(f, data, len);
> +    stat64_add(&mig_stats.transferred, 1 + 2 + 2 + len);
>      qemu_fflush(f);
>  }
>  
> @@ -1212,11 +1217,13 @@ void qemu_savevm_state_header(QEMUFile *f)
>      trace_savevm_state_header();
>      qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
>      qemu_put_be32(f, QEMU_VM_FILE_VERSION);
> -
> +    size_t size = 4 + 4;

Move the blank line down.

>      if (migrate_get_current()->send_configuration) {
>          qemu_put_byte(f, QEMU_VM_CONFIGURATION);
> +        size += 1;
>          vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
>      }
> +    stat64_add(&mig_stats.transferred, size);
>  }
>  
>  bool qemu_savevm_state_guest_unplug_pending(void)
> @@ -1384,6 +1391,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
>  {
>      SaveStateEntry *se;
>      int ret;
> +    size_t size = 0;
>  
>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>          if (!se->ops || !se->ops->save_live_complete_postcopy) {
> @@ -1398,7 +1406,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
>          /* Section type */
>          qemu_put_byte(f, QEMU_VM_SECTION_END);
>          qemu_put_be32(f, se->section_id);
> -
> +        size += 1 + 4;

Move the blank line down.

>          ret = se->ops->save_live_complete_postcopy(f, se->opaque);
>          trace_savevm_section_end(se->idstr, se->section_id, ret);
>          save_section_footer(f, se);
> @@ -1409,6 +1417,8 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
>      }
>  
>      qemu_put_byte(f, QEMU_VM_EOF);
> +    size += 1;
> +    stat64_add(&mig_stats.transferred, size);
>      qemu_fflush(f);
>  }
>  
> @@ -1484,6 +1494,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>      if (!in_postcopy) {
>          /* Postcopy stream will still be going */
>          qemu_put_byte(f, QEMU_VM_EOF);
> +        stat64_add(&mig_stats.transferred, 1);
>      }
>  
>      json_writer_end_array(vmdesc);
> @@ -1664,15 +1675,18 @@ void qemu_savevm_live_state(QEMUFile *f)
>      /* save QEMU_VM_SECTION_END section */
>      qemu_savevm_state_complete_precopy(f, true, false);
>      qemu_put_byte(f, QEMU_VM_EOF);
> +    stat64_add(&mig_stats.transferred, 1);
>  }
>  
>  int qemu_save_device_state(QEMUFile *f)
>  {
>      SaveStateEntry *se;
> +    size_t size = 0;
>  
>      if (!migration_in_colo_state()) {
>          qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
>          qemu_put_be32(f, QEMU_VM_FILE_VERSION);
> +        size = 4 + 4;
>      }
>      cpu_synchronize_all_states();
>  
> @@ -1690,6 +1704,7 @@ int qemu_save_device_state(QEMUFile *f)
>  
>      qemu_put_byte(f, QEMU_VM_EOF);
>  
> +    stat64_add(&mig_stats.transferred, size + 1);

Move the blank line down. In other cases the pattern has been to update
'size' and then call stat64_add() - make this one the same?

>      return qemu_file_get_error(f);
>  }
>  
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index af01d54b6f..ee3b70a6a8 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -12,6 +12,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "migration.h"
> +#include "migration-stats.h"
>  #include "migration/vmstate.h"
>  #include "savevm.h"
>  #include "qapi/qmp/json-writer.h"
> @@ -394,6 +395,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  written_bytes = qemu_file_transferred_fast(f) - old_offset;
>                  vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes, i);
>  
> +                stat64_add(&mig_stats.transferred, written_bytes);

Move the blank line down.

>                  /* Compressed arrays only care about the first element */
>                  if (vmdesc_loop && vmsd_can_compress(field)) {
>                      vmdesc_loop = NULL;
> @@ -517,6 +519,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
>              qemu_put_byte(f, len);
>              qemu_put_buffer(f, (uint8_t *)vmsdsub->name, len);
>              qemu_put_be32(f, vmsdsub->version_id);
> +            stat64_add(&mig_stats.transferred, 1 + 1 + len + 4);
>              ret = vmstate_save_state(f, vmsdsub, opaque, vmdesc);
>              if (ret) {
>                  return ret;
> diff --git a/migration/meson.build b/migration/meson.build
> index dc8b1daef5..b3d0c537c8 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -1,5 +1,6 @@
>  # Files needed by unit tests
>  migration_files = files(
> +  'migration-stats.c',
>    'page_cache.c',
>    'xbzrle.c',
>    'vmstate-types.c',
> @@ -18,7 +19,6 @@ softmmu_ss.add(files(
>    'fd.c',
>    'global_state.c',
>    'migration-hmp-cmds.c',
> -  'migration-stats.c',
>    'migration.c',
>    'multifd.c',
>    'multifd-zlib.c',
> -- 
> 2.40.1
-- 
Are you laughing at me now? May I please laugh along with you.

Re: [PATCH v2 02/16] migration: Correct transferred bytes value

[Leonardo Brás]

On Mon, 2023-05-15 at 21:56 +0200, Juan Quintela wrote:
> We forget several places to add to trasferred amount of data.  With
> this fixes I get:
> 
>    qemu_file_transferred() + multifd_bytes == transferred
> 
> The only place whrer this is not true is during devices sending.  But
> going all through the full tree searching for devices that use
> QEMUFile directly is a bit too much.
> 
> Multifd, precopy and xbzrle work as expected. Postocpy still misses 35
> bytes, but searching for them is getting complicated, so I stop here.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/ram.c       | 14 ++++++++++++++
>  migration/savevm.c    | 19 +++++++++++++++++--
>  migration/vmstate.c   |  3 +++
>  migration/meson.build |  2 +-
>  4 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index f69d8d42b0..fd5a8db0f8 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -337,6 +337,7 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file,
>  
>      g_free(le_bitmap);
>  
> +    stat64_add(&mig_stats.transferred, 8 + size + 8);
>      if (qemu_file_get_error(file)) {
>          return qemu_file_get_error(file);
>      }
> @@ -1392,6 +1393,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
>                      return ret;
>                  }
>                  qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
> +                stat64_add(&mig_stats.transferred, 8);
>                  qemu_fflush(f);
>              }
>              /*
> @@ -3020,6 +3022,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>      RAMState **rsp = opaque;
>      RAMBlock *block;
>      int ret;
> +    size_t size = 0;
>  
>      if (compress_threads_save_setup()) {
>          return -1;
> @@ -3038,16 +3041,20 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>          qemu_put_be64(f, ram_bytes_total_with_ignored()
>                           | RAM_SAVE_FLAG_MEM_SIZE);
>  
> +        size += 8;
>          RAMBLOCK_FOREACH_MIGRATABLE(block) {
>              qemu_put_byte(f, strlen(block->idstr));
>              qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr));
>              qemu_put_be64(f, block->used_length);
> +            size += 1 + strlen(block->idstr) + 8;

I was thinking some of them would look better with sizeof()s instead of given
literal number, such as:

size += sizeof(Byte) + strlen(block->idstr) + sizeof(block->used_length);

Maybe too much?

>              if (migrate_postcopy_ram() && block->page_size !=
>                                            qemu_host_page_size) {
>                  qemu_put_be64(f, block->page_size);
> +                size += 8;
>              }
>              if (migrate_ignore_shared()) {
>                  qemu_put_be64(f, block->mr->addr);
> +                size += 8;
>              }
>          }
>      }
> @@ -3064,11 +3071,14 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>  
>      if (!migrate_multifd_flush_after_each_section()) {
>          qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
> +        size += 8;

sizeof(uint64_t) here is probably too much.


Maybe, it would be nice to have qemu_put_* to return the value, and in this
case:

size += qemu_put_be64(...)

What do you think?

Anyway, 

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

>      }
>  
>      qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
> +    size += 8;
>      qemu_fflush(f);
>  
> +    stat64_add(&mig_stats.transferred, size);
>      return 0;
>  }
>  
> @@ -3209,6 +3219,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>      RAMState **temp = opaque;
>      RAMState *rs = *temp;
>      int ret = 0;
> +    size_t size = 0;
>  
>      rs->last_stage = !migration_in_colo_state();
>  
> @@ -3253,8 +3264,11 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>  
>      if (!migrate_multifd_flush_after_each_section()) {
>          qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
> +        size += 8;
>      }
>      qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
> +    size += 8;
> +    stat64_add(&mig_stats.transferred, size);
>      qemu_fflush(f);
>  
>      return 0;
> diff --git a/migration/savevm.c b/migration/savevm.c
> index e33788343a..c7af9050c2 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -952,6 +952,7 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se,
>      qemu_put_byte(f, section_type);
>      qemu_put_be32(f, se->section_id);
>  
> +    size_t size = 1 + 4;
>      if (section_type == QEMU_VM_SECTION_FULL ||
>          section_type == QEMU_VM_SECTION_START) {
>          /* ID string */
> @@ -961,7 +962,9 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se,
>  
>          qemu_put_be32(f, se->instance_id);
>          qemu_put_be32(f, se->version_id);
> +        size += 1 + len + 4 + 4;
>      }
> +    stat64_add(&mig_stats.transferred, size);
>  }
>  
>  /*
> @@ -973,6 +976,7 @@ static void save_section_footer(QEMUFile *f, SaveStateEntry *se)
>      if (migrate_get_current()->send_section_footer) {
>          qemu_put_byte(f, QEMU_VM_SECTION_FOOTER);
>          qemu_put_be32(f, se->section_id);
> +        stat64_add(&mig_stats.transferred, 1 + 4);
>      }
>  }
>  
> @@ -1032,6 +1036,7 @@ static void qemu_savevm_command_send(QEMUFile *f,
>      qemu_put_be16(f, (uint16_t)command);
>      qemu_put_be16(f, len);
>      qemu_put_buffer(f, data, len);
> +    stat64_add(&mig_stats.transferred, 1 + 2 + 2 + len);
>      qemu_fflush(f);
>  }
>  
> @@ -1212,11 +1217,13 @@ void qemu_savevm_state_header(QEMUFile *f)
>      trace_savevm_state_header();
>      qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
>      qemu_put_be32(f, QEMU_VM_FILE_VERSION);
> -
> +    size_t size = 4 + 4;
>      if (migrate_get_current()->send_configuration) {
>          qemu_put_byte(f, QEMU_VM_CONFIGURATION);
> +        size += 1;
>          vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
>      }
> +    stat64_add(&mig_stats.transferred, size);
>  }
>  
>  bool qemu_savevm_state_guest_unplug_pending(void)
> @@ -1384,6 +1391,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
>  {
>      SaveStateEntry *se;
>      int ret;
> +    size_t size = 0;
>  
>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>          if (!se->ops || !se->ops->save_live_complete_postcopy) {
> @@ -1398,7 +1406,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
>          /* Section type */
>          qemu_put_byte(f, QEMU_VM_SECTION_END);
>          qemu_put_be32(f, se->section_id);
> -
> +        size += 1 + 4;
>          ret = se->ops->save_live_complete_postcopy(f, se->opaque);
>          trace_savevm_section_end(se->idstr, se->section_id, ret);
>          save_section_footer(f, se);
> @@ -1409,6 +1417,8 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
>      }
>  
>      qemu_put_byte(f, QEMU_VM_EOF);
> +    size += 1;
> +    stat64_add(&mig_stats.transferred, size);
>      qemu_fflush(f);
>  }
>  
> @@ -1484,6 +1494,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>      if (!in_postcopy) {
>          /* Postcopy stream will still be going */
>          qemu_put_byte(f, QEMU_VM_EOF);
> +        stat64_add(&mig_stats.transferred, 1);
>      }
>  
>      json_writer_end_array(vmdesc);
> @@ -1664,15 +1675,18 @@ void qemu_savevm_live_state(QEMUFile *f)
>      /* save QEMU_VM_SECTION_END section */
>      qemu_savevm_state_complete_precopy(f, true, false);
>      qemu_put_byte(f, QEMU_VM_EOF);
> +    stat64_add(&mig_stats.transferred, 1);
>  }
>  
>  int qemu_save_device_state(QEMUFile *f)
>  {
>      SaveStateEntry *se;
> +    size_t size = 0;
>  
>      if (!migration_in_colo_state()) {
>          qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
>          qemu_put_be32(f, QEMU_VM_FILE_VERSION);
> +        size = 4 + 4;
>      }
>      cpu_synchronize_all_states();
>  
> @@ -1690,6 +1704,7 @@ int qemu_save_device_state(QEMUFile *f)
>  
>      qemu_put_byte(f, QEMU_VM_EOF);
>  
> +    stat64_add(&mig_stats.transferred, size + 1);
>      return qemu_file_get_error(f);
>  }
>  
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index af01d54b6f..ee3b70a6a8 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -12,6 +12,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "migration.h"
> +#include "migration-stats.h"
>  #include "migration/vmstate.h"
>  #include "savevm.h"
>  #include "qapi/qmp/json-writer.h"
> @@ -394,6 +395,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  written_bytes = qemu_file_transferred_fast(f) - old_offset;
>                  vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes, i);
>  
> +                stat64_add(&mig_stats.transferred, written_bytes);
>                  /* Compressed arrays only care about the first element */
>                  if (vmdesc_loop && vmsd_can_compress(field)) {
>                      vmdesc_loop = NULL;
> @@ -517,6 +519,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
>              qemu_put_byte(f, len);
>              qemu_put_buffer(f, (uint8_t *)vmsdsub->name, len);
>              qemu_put_be32(f, vmsdsub->version_id);
> +            stat64_add(&mig_stats.transferred, 1 + 1 + len + 4);
>              ret = vmstate_save_state(f, vmsdsub, opaque, vmdesc);
>              if (ret) {
>                  return ret;
> diff --git a/migration/meson.build b/migration/meson.build
> index dc8b1daef5..b3d0c537c8 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -1,5 +1,6 @@
>  # Files needed by unit tests
>  migration_files = files(
> +  'migration-stats.c',
>    'page_cache.c',
>    'xbzrle.c',
>    'vmstate-types.c',
> @@ -18,7 +19,6 @@ softmmu_ss.add(files(
>    'fd.c',
>    'global_state.c',
>    'migration-hmp-cmds.c',
> -  'migration-stats.c',
>    'migration.c',
>    'multifd.c',
>    'multifd-zlib.c',

Re: [PATCH v2 02/16] migration: Correct transferred bytes value

[Juan Quintela]

Leonardo Brás <leobras@redhat.com> wrote:
> On Mon, 2023-05-15 at 21:56 +0200, Juan Quintela wrote:
>> We forget several places to add to trasferred amount of data.  With
>> this fixes I get:
>> 
>>    qemu_file_transferred() + multifd_bytes == transferred
>> 
>> The only place whrer this is not true is during devices sending.  But
>> going all through the full tree searching for devices that use
>> QEMUFile directly is a bit too much.
>> 
>> Multifd, precopy and xbzrle work as expected. Postocpy still misses 35
>> bytes, but searching for them is getting complicated, so I stop here.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/ram.c       | 14 ++++++++++++++
>>  migration/savevm.c    | 19 +++++++++++++++++--
>>  migration/vmstate.c   |  3 +++
>>  migration/meson.build |  2 +-
>>  4 files changed, 35 insertions(+), 3 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index f69d8d42b0..fd5a8db0f8 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -337,6 +337,7 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file,
>>  
>>      g_free(le_bitmap);
>>  
>> +    stat64_add(&mig_stats.transferred, 8 + size + 8);
>>      if (qemu_file_get_error(file)) {
>>          return qemu_file_get_error(file);
>>      }
>> @@ -1392,6 +1393,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
>>                      return ret;
>>                  }
>>                  qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
>> +                stat64_add(&mig_stats.transferred, 8);
>>                  qemu_fflush(f);
>>              }
>>              /*
>> @@ -3020,6 +3022,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>      RAMState **rsp = opaque;
>>      RAMBlock *block;
>>      int ret;
>> +    size_t size = 0;
>>  
>>      if (compress_threads_save_setup()) {
>>          return -1;
>> @@ -3038,16 +3041,20 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>          qemu_put_be64(f, ram_bytes_total_with_ignored()
>>                           | RAM_SAVE_FLAG_MEM_SIZE);
>>  
>> +        size += 8;
>>          RAMBLOCK_FOREACH_MIGRATABLE(block) {
>>              qemu_put_byte(f, strlen(block->idstr));
>>              qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr));
>>              qemu_put_be64(f, block->used_length);
>> +            size += 1 + strlen(block->idstr) + 8;
>
> I was thinking some of them would look better with sizeof()s instead of given
> literal number, such as:
>
> size += sizeof(Byte) + strlen(block->idstr) + sizeof(block->used_length);
>
> Maybe too much?

I dropped this patch for two reasons:

- reviewers gave me a bad time with it O:-)
- it was there only so if anyone was meassuring that new counters are
  the same that old counters.

But as I have already checked that, we don't need it.

I drop it on the next round that I send.
> Maybe, it would be nice to have qemu_put_* to return the value, and in this
> case:
>
> size += qemu_put_be64(...)
>
> What do you think?

Even more important than that is to return an error value, but that
is a very long project.

See on my next series that qemu_fflush() return errors, so code gets
simplifed:

qemu_fflush(file);
if (qemu_file_get_error(file)) {
    handle error;
}

to:

qemu_fflush(file);
if (qemu_file_get_error(file)) {
    handle error;
}

We need to do basically all qemu_put_*() and qemu_get_*() functions, but
it is a step on the right direction.

Later, Juan.

Re: [PATCH v2 02/16] migration: Correct transferred bytes value

[Leonardo Bras Soares Passos]

On Fri, May 26, 2023 at 5:04 AM Juan Quintela <quintela@redhat.com> wrote:
>
> Leonardo Brás <leobras@redhat.com> wrote:
> > On Mon, 2023-05-15 at 21:56 +0200, Juan Quintela wrote:
> >> We forget several places to add to trasferred amount of data.  With
> >> this fixes I get:
> >>
> >>    qemu_file_transferred() + multifd_bytes == transferred
> >>
> >> The only place whrer this is not true is during devices sending.  But
> >> going all through the full tree searching for devices that use
> >> QEMUFile directly is a bit too much.
> >>
> >> Multifd, precopy and xbzrle work as expected. Postocpy still misses 35
> >> bytes, but searching for them is getting complicated, so I stop here.
> >>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  migration/ram.c       | 14 ++++++++++++++
> >>  migration/savevm.c    | 19 +++++++++++++++++--
> >>  migration/vmstate.c   |  3 +++
> >>  migration/meson.build |  2 +-
> >>  4 files changed, 35 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index f69d8d42b0..fd5a8db0f8 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -337,6 +337,7 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file,
> >>
> >>      g_free(le_bitmap);
> >>
> >> +    stat64_add(&mig_stats.transferred, 8 + size + 8);
> >>      if (qemu_file_get_error(file)) {
> >>          return qemu_file_get_error(file);
> >>      }
> >> @@ -1392,6 +1393,7 @@ static int find_dirty_block(RAMState *rs, PageSearchStatus *pss)
> >>                      return ret;
> >>                  }
> >>                  qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
> >> +                stat64_add(&mig_stats.transferred, 8);
> >>                  qemu_fflush(f);
> >>              }
> >>              /*
> >> @@ -3020,6 +3022,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> >>      RAMState **rsp = opaque;
> >>      RAMBlock *block;
> >>      int ret;
> >> +    size_t size = 0;
> >>
> >>      if (compress_threads_save_setup()) {
> >>          return -1;
> >> @@ -3038,16 +3041,20 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> >>          qemu_put_be64(f, ram_bytes_total_with_ignored()
> >>                           | RAM_SAVE_FLAG_MEM_SIZE);
> >>
> >> +        size += 8;
> >>          RAMBLOCK_FOREACH_MIGRATABLE(block) {
> >>              qemu_put_byte(f, strlen(block->idstr));
> >>              qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr));
> >>              qemu_put_be64(f, block->used_length);
> >> +            size += 1 + strlen(block->idstr) + 8;
> >
> > I was thinking some of them would look better with sizeof()s instead of given
> > literal number, such as:
> >
> > size += sizeof(Byte) + strlen(block->idstr) + sizeof(block->used_length);
> >
> > Maybe too much?
>
> I dropped this patch for two reasons:
>
> - reviewers gave me a bad time with it O:-)
> - it was there only so if anyone was meassuring that new counters are
>   the same that old counters.
>
> But as I have already checked that, we don't need it.
>
> I drop it on the next round that I send.
> > Maybe, it would be nice to have qemu_put_* to return the value, and in this
> > case:
> >
> > size += qemu_put_be64(...)
> >
> > What do you think?
>
> Even more important than that is to return an error value, but that
> is a very long project.
>
> See on my next series that qemu_fflush() return errors, so code gets
> simplifed:
>
> qemu_fflush(file);
> if (qemu_file_get_error(file)) {
>     handle error;
> }
>
> to:
>
> qemu_fflush(file);
> if (qemu_file_get_error(file)) {
>     handle error;
> }
>

They look the same to me, what changed?

> We need to do basically all qemu_put_*() and qemu_get_*() functions, but
> it is a step on the right direction.
>
> Later, Juan.
>

Re: [PATCH v2 02/16] migration: Correct transferred bytes value

[Juan Quintela]

Leonardo Bras Soares Passos <leobras@redhat.com> wrote:
> On Fri, May 26, 2023 at 5:04 AM Juan Quintela <quintela@redhat.com> wrote:
>> > Maybe too much?
>>
>> I dropped this patch for two reasons:
>>
>> - reviewers gave me a bad time with it O:-)
>> - it was there only so if anyone was meassuring that new counters are
>>   the same that old counters.
>>
>> But as I have already checked that, we don't need it.
>>
>> I drop it on the next round that I send.
>> > Maybe, it would be nice to have qemu_put_* to return the value, and in this
>> > case:
>> >
>> > size += qemu_put_be64(...)
>> >
>> > What do you think?
>>
>> Even more important than that is to return an error value, but that
>> is a very long project.
>>
>> See on my next series that qemu_fflush() return errors, so code gets
>> simplifed:
>>
>> qemu_fflush(file);
>> if (qemu_file_get_error(file)) {
>>     handle error;
>> }
>>
>> to:
>>
>> qemu_fflush(file);
>> if (qemu_file_get_error(file)) {
>>     handle error;
>> }
>>
>
> They look the same to me, what changed?

I did copy paste without changing:

if (qemu_fflush(file)) {
    handle error;
}



>> We need to do basically all qemu_put_*() and qemu_get_*() functions, but
>> it is a step on the right direction.
>>
>> Later, Juan.
>>

[PATCH v2 03/16] migration: Move setup_time to mig_stats

[Juan Quintela]

It is a time that needs to be cleaned each time cancel migration.
Once there create migration_time_since() to calculate how time since a
time in the past.

Signed-off-by: Juan Quintela <quintela@redhat.com>

---

Rename to migration_time_since (cédric)
---
 migration/migration-stats.h | 13 +++++++++++++
 migration/migration.h       |  1 -
 migration/migration-stats.c |  7 +++++++
 migration/migration.c       |  9 ++++-----
 4 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index e782f1b0df..21402af9e4 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -75,6 +75,10 @@ typedef struct {
      * Number of bytes sent during precopy stage.
      */
     Stat64 precopy_bytes;
+    /*
+     * How long has the setup stage took.
+     */
+    Stat64 setup_time;
     /*
      * Total number of bytes transferred.
      */
@@ -87,4 +91,13 @@ typedef struct {
 
 extern MigrationAtomicStats mig_stats;
 
+/**
+ * migration_time_since: Calculate how much time has passed
+ *
+ * @stats: migration stats
+ * @since: reference time since we want to calculate
+ *
+ * Returns: Nothing.  The time is stored in val.
+ */
+void migration_time_since(MigrationAtomicStats *stats, int64_t since);
 #endif
diff --git a/migration/migration.h b/migration/migration.h
index 48a46123a0..27aa3b1035 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -316,7 +316,6 @@ struct MigrationState {
     int64_t downtime;
     int64_t expected_downtime;
     bool capabilities[MIGRATION_CAPABILITY__MAX];
-    int64_t setup_time;
     /*
      * Whether guest was running when we enter the completion stage.
      * If migration is interrupted by any reason, we need to continue
diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index 2f2cea965c..3431453c90 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -12,6 +12,13 @@
 
 #include "qemu/osdep.h"
 #include "qemu/stats64.h"
+#include "qemu/timer.h"
 #include "migration-stats.h"
 
 MigrationAtomicStats mig_stats;
+
+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);
+}
diff --git a/migration/migration.c b/migration/migration.c
index c41c7491bb..e9466273bb 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -887,7 +887,7 @@ static void populate_time_info(MigrationInfo *info, MigrationState *s)
 {
     info->has_status = true;
     info->has_setup_time = true;
-    info->setup_time = s->setup_time;
+    info->setup_time = stat64_get(&mig_stats.setup_time);
 
     if (s->state == MIGRATION_STATUS_COMPLETED) {
         info->has_total_time = true;
@@ -1390,7 +1390,6 @@ void migrate_init(MigrationState *s)
     s->pages_per_second = 0.0;
     s->downtime = 0;
     s->expected_downtime = 0;
-    s->setup_time = 0;
     s->start_postcopy = false;
     s->postcopy_after_devices = false;
     s->migration_thread_running = false;
@@ -2647,7 +2646,7 @@ static void migration_calculate_complete(MigrationState *s)
         s->downtime = end_time - s->downtime_start;
     }
 
-    transfer_time = s->total_time - s->setup_time;
+    transfer_time = s->total_time - stat64_get(&mig_stats.setup_time);
     if (transfer_time) {
         s->mbps = ((double) bytes * 8.0) / transfer_time / 1000;
     }
@@ -2969,7 +2968,7 @@ static void *migration_thread(void *opaque)
     qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
                                MIGRATION_STATUS_ACTIVE);
 
-    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
+    migration_time_since(&mig_stats, setup_start);
 
     trace_migration_thread_setup_complete();
 
@@ -3081,7 +3080,7 @@ static void *bg_migration_thread(void *opaque)
     qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
                                MIGRATION_STATUS_ACTIVE);
 
-    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
+    migration_time_since(&mig_stats, setup_start);
 
     trace_migration_thread_setup_complete();
     s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
-- 
2.40.1

Re: [PATCH v2 03/16] migration: Move setup_time to mig_stats

[David Edmondson]

Juan Quintela <quintela@redhat.com> writes:

> It is a time that needs to be cleaned each time cancel migration.
> Once there create migration_time_since() to calculate how time since a
> time in the past.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> ---
>
> Rename to migration_time_since (cédric)
> ---
>  migration/migration-stats.h | 13 +++++++++++++
>  migration/migration.h       |  1 -
>  migration/migration-stats.c |  7 +++++++
>  migration/migration.c       |  9 ++++-----
>  4 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index e782f1b0df..21402af9e4 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -75,6 +75,10 @@ typedef struct {
>       * Number of bytes sent during precopy stage.
>       */
>      Stat64 precopy_bytes;
> +    /*
> +     * How long has the setup stage took.
> +     */
> +    Stat64 setup_time;

Is this really a Stat64? It doesn't appear to need the atomic update
feature.

>      /*
>       * Total number of bytes transferred.
>       */
> @@ -87,4 +91,13 @@ typedef struct {
>  
>  extern MigrationAtomicStats mig_stats;
>  
> +/**
> + * migration_time_since: Calculate how much time has passed
> + *
> + * @stats: migration stats
> + * @since: reference time since we want to calculate
> + *
> + * Returns: Nothing.  The time is stored in val.

"stored in stats->setup_time"

> + */
> +void migration_time_since(MigrationAtomicStats *stats, int64_t since);
>  #endif
> diff --git a/migration/migration.h b/migration/migration.h
> index 48a46123a0..27aa3b1035 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -316,7 +316,6 @@ struct MigrationState {
>      int64_t downtime;
>      int64_t expected_downtime;
>      bool capabilities[MIGRATION_CAPABILITY__MAX];
> -    int64_t setup_time;
>      /*
>       * Whether guest was running when we enter the completion stage.
>       * If migration is interrupted by any reason, we need to continue
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index 2f2cea965c..3431453c90 100644
> --- a/migration/migration-stats.c
> +++ b/migration/migration-stats.c
> @@ -12,6 +12,13 @@
>  
>  #include "qemu/osdep.h"
>  #include "qemu/stats64.h"
> +#include "qemu/timer.h"
>  #include "migration-stats.h"
>  
>  MigrationAtomicStats mig_stats;
> +
> +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);
> +}
> diff --git a/migration/migration.c b/migration/migration.c
> index c41c7491bb..e9466273bb 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -887,7 +887,7 @@ static void populate_time_info(MigrationInfo *info, MigrationState *s)
>  {
>      info->has_status = true;
>      info->has_setup_time = true;
> -    info->setup_time = s->setup_time;
> +    info->setup_time = stat64_get(&mig_stats.setup_time);
>  
>      if (s->state == MIGRATION_STATUS_COMPLETED) {
>          info->has_total_time = true;
> @@ -1390,7 +1390,6 @@ void migrate_init(MigrationState *s)
>      s->pages_per_second = 0.0;
>      s->downtime = 0;
>      s->expected_downtime = 0;
> -    s->setup_time = 0;
>      s->start_postcopy = false;
>      s->postcopy_after_devices = false;
>      s->migration_thread_running = false;
> @@ -2647,7 +2646,7 @@ static void migration_calculate_complete(MigrationState *s)
>          s->downtime = end_time - s->downtime_start;
>      }
>  
> -    transfer_time = s->total_time - s->setup_time;
> +    transfer_time = s->total_time - stat64_get(&mig_stats.setup_time);
>      if (transfer_time) {
>          s->mbps = ((double) bytes * 8.0) / transfer_time / 1000;
>      }
> @@ -2969,7 +2968,7 @@ static void *migration_thread(void *opaque)
>      qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
>                                 MIGRATION_STATUS_ACTIVE);
>  
> -    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> +    migration_time_since(&mig_stats, setup_start);
>  
>      trace_migration_thread_setup_complete();
>  
> @@ -3081,7 +3080,7 @@ static void *bg_migration_thread(void *opaque)
>      qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
>                                 MIGRATION_STATUS_ACTIVE);
>  
> -    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> +    migration_time_since(&mig_stats, setup_start);
>  
>      trace_migration_thread_setup_complete();
>      s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> -- 
> 2.40.1
-- 
Walk without rhythm and it won't attract the worm.

Re: [PATCH v2 03/16] migration: Move setup_time to mig_stats

[Juan Quintela]

David Edmondson <david.edmondson@oracle.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> It is a time that needs to be cleaned each time cancel migration.
>> Once there create migration_time_since() to calculate how time since a
>> time in the past.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>
>> ---
>>
>> Rename to migration_time_since (cédric)
>> ---
>>  migration/migration-stats.h | 13 +++++++++++++
>>  migration/migration.h       |  1 -
>>  migration/migration-stats.c |  7 +++++++
>>  migration/migration.c       |  9 ++++-----
>>  4 files changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
>> index e782f1b0df..21402af9e4 100644
>> --- a/migration/migration-stats.h
>> +++ b/migration/migration-stats.h
>> @@ -75,6 +75,10 @@ typedef struct {
>>       * Number of bytes sent during precopy stage.
>>       */
>>      Stat64 precopy_bytes;
>> +    /*
>> +     * How long has the setup stage took.
>> +     */
>> +    Stat64 setup_time;
>
> Is this really a Stat64? It doesn't appear to need the atomic update
> feature.

What this whole Migration Atomic Counters series try to do is that
everything becomes atomic and then we can use everything everywhere.

Before this series we had (I am simplifying here):

- transferred, precopy_bytes, postcopy_bytes, downtime_bytes -> atomic,
  you can use it anywhere

- qemu_file transferred -> you can only use it from the main migration
  thread

- qemu_file rate_limit -> you can only use it from the main migration
  thread

And we had to update the three counters in every place that we did a
write wehad to update all of them.

You can see the contorsions that we go to to update the rate_limit and
the qemu_file transferred fields.

After the series (you need to get what it is already on the tree, this
series, QEMUFileHooks cleanup, and another serie on my tree waiting for
this to be commited), you got three counters:

- qemu_file: atomic, everytime we do a qemu_file write we update it
- multifd_bytes: atomic, everytime that we do a write in a multifd
  channel, we update it.
- rdma_bytes: atomic, everytime we do a write through RDMA we update it.

And that is it.

Both rate_limit and transferred are derived from these three counters:

- at any point in time migration_transferred_bytes() returns the amount
  of bytes written since the start of the migration:
     qemu_file_bytes + multifd_bytes + rdma_bytes.

- transferred on this period:
       at_start_of_period = migration_transferred_bytes().
       trasferred_in_this_period = migration_transferred_bytes() - at_start_of_period;

- Similar for precopy_bytes, postcopy_bytes and downtime_bytes.  When we
  move from one stage to the next, we store what is the value of the
  previous stage.

The counters that we use to calculate the rate limit are updated around
10 times per second (can be a bit bigger at the end of periods,
iterations, ...)  So performance is not extra critical.

But as we have way less atomic operations (really one per real write),
we don't really care a lot if we do some atomic operations when a normal
operation will do.

I.e. I think we have two options:

- have the remaining counters that are only used in the main migration
  thread not be atomic.  Document them and remember to do the correct
  thing everytime we use it.  If we need to use it in another thread,
  just change it to atomic.

- Make all counters atomic. No need to document anything.  And you can
  call any operation/counter/... in migration-stats.c from anywhere.

I think that the second option is better.  But I can hear reasons from
people that think that the 1st one is better.

Comments?

Later, Juan.

Re: [PATCH v2 03/16] migration: Move setup_time to mig_stats

[David Edmondson]

Juan Quintela <quintela@redhat.com> writes:

> David Edmondson <david.edmondson@oracle.com> wrote:
>> Juan Quintela <quintela@redhat.com> writes:
>>
>>> It is a time that needs to be cleaned each time cancel migration.
>>> Once there create migration_time_since() to calculate how time since a
>>> time in the past.
>>>
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>
>>> ---
>>>
>>> Rename to migration_time_since (cédric)
>>> ---
>>>  migration/migration-stats.h | 13 +++++++++++++
>>>  migration/migration.h       |  1 -
>>>  migration/migration-stats.c |  7 +++++++
>>>  migration/migration.c       |  9 ++++-----
>>>  4 files changed, 24 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
>>> index e782f1b0df..21402af9e4 100644
>>> --- a/migration/migration-stats.h
>>> +++ b/migration/migration-stats.h
>>> @@ -75,6 +75,10 @@ typedef struct {
>>>       * Number of bytes sent during precopy stage.
>>>       */
>>>      Stat64 precopy_bytes;
>>> +    /*
>>> +     * How long has the setup stage took.
>>> +     */
>>> +    Stat64 setup_time;
>>
>> Is this really a Stat64? It doesn't appear to need the atomic update
>> feature.
>
> What this whole Migration Atomic Counters series try to do is that
> everything becomes atomic and then we can use everything everywhere.
>
> Before this series we had (I am simplifying here):
>
> - transferred, precopy_bytes, postcopy_bytes, downtime_bytes -> atomic,
>   you can use it anywhere
>
> - qemu_file transferred -> you can only use it from the main migration
>   thread
>
> - qemu_file rate_limit -> you can only use it from the main migration
>   thread
>
> And we had to update the three counters in every place that we did a
> write wehad to update all of them.
>
> You can see the contorsions that we go to to update the rate_limit and
> the qemu_file transferred fields.
>
> After the series (you need to get what it is already on the tree, this
> series, QEMUFileHooks cleanup, and another serie on my tree waiting for
> this to be commited), you got three counters:
>
> - qemu_file: atomic, everytime we do a qemu_file write we update it
> - multifd_bytes: atomic, everytime that we do a write in a multifd
>   channel, we update it.
> - rdma_bytes: atomic, everytime we do a write through RDMA we update it.
>
> And that is it.
>
> Both rate_limit and transferred are derived from these three counters:
>
> - at any point in time migration_transferred_bytes() returns the amount
>   of bytes written since the start of the migration:
>      qemu_file_bytes + multifd_bytes + rdma_bytes.
>
> - transferred on this period:
>        at_start_of_period = migration_transferred_bytes().
>        trasferred_in_this_period = migration_transferred_bytes() - at_start_of_period;
>
> - Similar for precopy_bytes, postcopy_bytes and downtime_bytes.  When we
>   move from one stage to the next, we store what is the value of the
>   previous stage.
>
> The counters that we use to calculate the rate limit are updated around
> 10 times per second (can be a bit bigger at the end of periods,
> iterations, ...)  So performance is not extra critical.
>
> But as we have way less atomic operations (really one per real write),
> we don't really care a lot if we do some atomic operations when a normal
> operation will do.
>
> I.e. I think we have two options:
>
> - have the remaining counters that are only used in the main migration
>   thread not be atomic.  Document them and remember to do the correct
>   thing everytime we use it.  If we need to use it in another thread,
>   just change it to atomic.
>
> - Make all counters atomic. No need to document anything.  And you can
>   call any operation/counter/... in migration-stats.c from anywhere.
>
> I think that the second option is better.  But I can hear reasons from
> people that think that the 1st one is better.

For the counters, no argument - making them all atomic seems like the
right way forward.

start_time isn't a counter, and isn't manipulated at multiple points in
the code by different actors.

I don't hate it being a Stat64, it just seems odd when the other 'time'
related variables are not.

> Comments?
>
> Later, Juan.
-- 
You can't hide from the flipside.

Re: [PATCH v2 03/16] migration: Move setup_time to mig_stats

[Leonardo Brás]

On Mon, 2023-05-15 at 21:56 +0200, Juan Quintela wrote:
> It is a time that needs to be cleaned each time cancel migration.
> Once there create migration_time_since() to calculate how time since a
> time in the past.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> ---
> 
> Rename to migration_time_since (cédric)
> ---
>  migration/migration-stats.h | 13 +++++++++++++
>  migration/migration.h       |  1 -
>  migration/migration-stats.c |  7 +++++++
>  migration/migration.c       |  9 ++++-----
>  4 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index e782f1b0df..21402af9e4 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -75,6 +75,10 @@ typedef struct {
>       * Number of bytes sent during precopy stage.
>       */
>      Stat64 precopy_bytes;
> +    /*
> +     * How long has the setup stage took.
> +     */
> +    Stat64 setup_time;
>      /*
>       * Total number of bytes transferred.
>       */
> @@ -87,4 +91,13 @@ typedef struct {
>  
>  extern MigrationAtomicStats mig_stats;
>  
> +/**
> + * migration_time_since: Calculate how much time has passed
> + *
> + * @stats: migration stats
> + * @since: reference time since we want to calculate
> + *
> + * Returns: Nothing.  The time is stored in val.
> + */
> +void migration_time_since(MigrationAtomicStats *stats, int64_t since);
>  #endif
> diff --git a/migration/migration.h b/migration/migration.h
> index 48a46123a0..27aa3b1035 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -316,7 +316,6 @@ struct MigrationState {
>      int64_t downtime;
>      int64_t expected_downtime;
>      bool capabilities[MIGRATION_CAPABILITY__MAX];
> -    int64_t setup_time;
>      /*
>       * Whether guest was running when we enter the completion stage.
>       * If migration is interrupted by any reason, we need to continue
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index 2f2cea965c..3431453c90 100644
> --- a/migration/migration-stats.c
> +++ b/migration/migration-stats.c
> @@ -12,6 +12,13 @@
>  
>  #include "qemu/osdep.h"
>  #include "qemu/stats64.h"
> +#include "qemu/timer.h"
>  #include "migration-stats.h"
>  
>  MigrationAtomicStats mig_stats;
> +
> +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);
> +}

IIUC this calculates a time delta and saves on stats->setup_time, is that right?

It took me some time to understand that, since the function name is
migration_time_since(), which seems more generic.

Would not be more intuitive to name it migration_setup_time_set() or so?

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


> diff --git a/migration/migration.c b/migration/migration.c
> index c41c7491bb..e9466273bb 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -887,7 +887,7 @@ static void populate_time_info(MigrationInfo *info, MigrationState *s)
>  {
>      info->has_status = true;
>      info->has_setup_time = true;
> -    info->setup_time = s->setup_time;
> +    info->setup_time = stat64_get(&mig_stats.setup_time);
>  
>      if (s->state == MIGRATION_STATUS_COMPLETED) {
>          info->has_total_time = true;
> @@ -1390,7 +1390,6 @@ void migrate_init(MigrationState *s)
>      s->pages_per_second = 0.0;
>      s->downtime = 0;
>      s->expected_downtime = 0;
> -    s->setup_time = 0;


I could not see MigrationState->setup_time being initialized as 0 in this patch.
In a quick look in the code I noticed there is no initialization of this struct,
but on qemu_savevm_state() and migrate_prepare() we have:

memset(&mig_stats, 0, sizeof(mig_stats));

I suppose this is enough, right?


>      s->start_postcopy = false;
>      s->postcopy_after_devices = false;
>      s->migration_thread_running = false;
> @@ -2647,7 +2646,7 @@ static void migration_calculate_complete(MigrationState *s)
>          s->downtime = end_time - s->downtime_start;
>      }
>  
> -    transfer_time = s->total_time - s->setup_time;
> +    transfer_time = s->total_time - stat64_get(&mig_stats.setup_time);
>      if (transfer_time) {
>          s->mbps = ((double) bytes * 8.0) / transfer_time / 1000;
>      }
> @@ -2969,7 +2968,7 @@ static void *migration_thread(void *opaque)
>      qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
>                                 MIGRATION_STATUS_ACTIVE);
>  
> -    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> +    migration_time_since(&mig_stats, setup_start);
>  
>      trace_migration_thread_setup_complete();
>  
> @@ -3081,7 +3080,7 @@ static void *bg_migration_thread(void *opaque)
>      qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
>                                 MIGRATION_STATUS_ACTIVE);
>  
> -    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> +    migration_time_since(&mig_stats, setup_start);
>  
>      trace_migration_thread_setup_complete();
>      s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);

Re: [PATCH v2 03/16] migration: Move setup_time to mig_stats

[Juan Quintela]

Leonardo Brás <leobras@redhat.com> wrote:
> On Mon, 2023-05-15 at 21:56 +0200, Juan Quintela wrote:
>> It is a time that needs to be cleaned each time cancel migration.
>> Once there create migration_time_since() to calculate how time since a
>> time in the past.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> 
>> ---
>> 
>> Rename to migration_time_since (cédric)
>> ---
>>  migration/migration-stats.h | 13 +++++++++++++
>>  migration/migration.h       |  1 -
>>  migration/migration-stats.c |  7 +++++++
>>  migration/migration.c       |  9 ++++-----
>>  4 files changed, 24 insertions(+), 6 deletions(-)
>> 
>> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
>> index e782f1b0df..21402af9e4 100644
>> --- a/migration/migration-stats.h
>> +++ b/migration/migration-stats.h
>> @@ -75,6 +75,10 @@ typedef struct {
>>       * Number of bytes sent during precopy stage.
>>       */
>>      Stat64 precopy_bytes;
>> +    /*
>> +     * How long has the setup stage took.
>> +     */
>> +    Stat64 setup_time;
>>      /*
>>       * Total number of bytes transferred.
>>       */
>> @@ -87,4 +91,13 @@ typedef struct {
>>  
>>  extern MigrationAtomicStats mig_stats;
>>  
>> +/**
>> + * migration_time_since: Calculate how much time has passed
>> + *
>> + * @stats: migration stats
>> + * @since: reference time since we want to calculate
>> + *
>> + * Returns: Nothing.  The time is stored in val.
>> + */
>> +void migration_time_since(MigrationAtomicStats *stats, int64_t since);
>>  #endif
>> diff --git a/migration/migration.h b/migration/migration.h
>> index 48a46123a0..27aa3b1035 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -316,7 +316,6 @@ struct MigrationState {
>>      int64_t downtime;
>>      int64_t expected_downtime;
>>      bool capabilities[MIGRATION_CAPABILITY__MAX];
>> -    int64_t setup_time;
>>      /*
>>       * Whether guest was running when we enter the completion stage.
>>       * If migration is interrupted by any reason, we need to continue
>> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
>> index 2f2cea965c..3431453c90 100644
>> --- a/migration/migration-stats.c
>> +++ b/migration/migration-stats.c
>> @@ -12,6 +12,13 @@
>>  
>>  #include "qemu/osdep.h"
>>  #include "qemu/stats64.h"
>> +#include "qemu/timer.h"
>>  #include "migration-stats.h"
>>  
>>  MigrationAtomicStats mig_stats;
>> +
>> +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);
>> +}
>
> IIUC this calculates a time delta and saves on stats->setup_time, is that right?
>
> It took me some time to understand that, since the function name is
> migration_time_since(), which seems more generic.
>
> Would not be more intuitive to name it migration_setup_time_set() or so?

Dropped this.
Other reviewer commented that this was not a counter, what is right.  So
I left the times for future work (it don't interfere with current
cleanups).


> I could not see MigrationState->setup_time being initialized as 0 in this patch.
> In a quick look in the code I noticed there is no initialization of this struct,
> but on qemu_savevm_state() and migrate_prepare() we have:
>
> memset(&mig_stats, 0, sizeof(mig_stats));
>
> I suppose this is enough, right?

Yeap.  All migration_stats() are initialized to zero at the start of
qemu, or when we start a migration.

After a migration, it don't matter if it finished with/without error,
they are there with the right value until we start another migration (in
the case of error, of course).

Later, Juan.

Re: [PATCH v2 03/16] migration: Move setup_time to mig_stats

[Leonardo Bras Soares Passos]

On Fri, May 26, 2023 at 5:07 AM Juan Quintela <quintela@redhat.com> wrote:
>
> Leonardo Brás <leobras@redhat.com> wrote:
> > On Mon, 2023-05-15 at 21:56 +0200, Juan Quintela wrote:
> >> It is a time that needs to be cleaned each time cancel migration.
> >> Once there create migration_time_since() to calculate how time since a
> >> time in the past.
> >>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >>
> >> ---
> >>
> >> Rename to migration_time_since (cédric)
> >> ---
> >>  migration/migration-stats.h | 13 +++++++++++++
> >>  migration/migration.h       |  1 -
> >>  migration/migration-stats.c |  7 +++++++
> >>  migration/migration.c       |  9 ++++-----
> >>  4 files changed, 24 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> >> index e782f1b0df..21402af9e4 100644
> >> --- a/migration/migration-stats.h
> >> +++ b/migration/migration-stats.h
> >> @@ -75,6 +75,10 @@ typedef struct {
> >>       * Number of bytes sent during precopy stage.
> >>       */
> >>      Stat64 precopy_bytes;
> >> +    /*
> >> +     * How long has the setup stage took.
> >> +     */
> >> +    Stat64 setup_time;
> >>      /*
> >>       * Total number of bytes transferred.
> >>       */
> >> @@ -87,4 +91,13 @@ typedef struct {
> >>
> >>  extern MigrationAtomicStats mig_stats;
> >>
> >> +/**
> >> + * migration_time_since: Calculate how much time has passed
> >> + *
> >> + * @stats: migration stats
> >> + * @since: reference time since we want to calculate
> >> + *
> >> + * Returns: Nothing.  The time is stored in val.
> >> + */
> >> +void migration_time_since(MigrationAtomicStats *stats, int64_t since);
> >>  #endif
> >> diff --git a/migration/migration.h b/migration/migration.h
> >> index 48a46123a0..27aa3b1035 100644
> >> --- a/migration/migration.h
> >> +++ b/migration/migration.h
> >> @@ -316,7 +316,6 @@ struct MigrationState {
> >>      int64_t downtime;
> >>      int64_t expected_downtime;
> >>      bool capabilities[MIGRATION_CAPABILITY__MAX];
> >> -    int64_t setup_time;
> >>      /*
> >>       * Whether guest was running when we enter the completion stage.
> >>       * If migration is interrupted by any reason, we need to continue
> >> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> >> index 2f2cea965c..3431453c90 100644
> >> --- a/migration/migration-stats.c
> >> +++ b/migration/migration-stats.c
> >> @@ -12,6 +12,13 @@
> >>
> >>  #include "qemu/osdep.h"
> >>  #include "qemu/stats64.h"
> >> +#include "qemu/timer.h"
> >>  #include "migration-stats.h"
> >>
> >>  MigrationAtomicStats mig_stats;
> >> +
> >> +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);
> >> +}
> >
> > IIUC this calculates a time delta and saves on stats->setup_time, is that right?
> >
> > It took me some time to understand that, since the function name is
> > migration_time_since(), which seems more generic.
> >
> > Would not be more intuitive to name it migration_setup_time_set() or so?
>
> Dropped this.
> Other reviewer commented that this was not a counter, what is right.  So
> I left the times for future work (it don't interfere with current
> cleanups).

Oh, it makes sense.

>
>
> > I could not see MigrationState->setup_time being initialized as 0 in this patch.
> > In a quick look in the code I noticed there is no initialization of this struct,
> > but on qemu_savevm_state() and migrate_prepare() we have:
> >
> > memset(&mig_stats, 0, sizeof(mig_stats));
> >
> > I suppose this is enough, right?
>
> Yeap.  All migration_stats() are initialized to zero at the start of
> qemu, or when we start a migration.
>
> After a migration, it don't matter if it finished with/without error,
> they are there with the right value until we start another migration (in
> the case of error, of course).

That's great to simplify the code.
Thanks!

>
> Later, Juan.
>

[PATCH v2 04/16] qemu-file: Account for rate_limit usage on qemu_fflush()

[Juan Quintela]

That is the moment we know we have transferred something.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
---
 migration/qemu-file.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 4bc875b452..956bd2a580 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -302,7 +302,9 @@ void qemu_fflush(QEMUFile *f)
                                    &local_error) < 0) {
             qemu_file_set_error_obj(f, -EIO, local_error);
         } else {
-            f->total_transferred += iov_size(f->iov, f->iovcnt);
+            uint64_t size = iov_size(f->iov, f->iovcnt);
+            qemu_file_acct_rate_limit(f, size);
+            f->total_transferred += size;
         }
 
         qemu_iovec_release_ram(f);
@@ -519,7 +521,6 @@ void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size,
         return;
     }
 
-    f->rate_limit_used += size;
     add_to_iovec(f, buf, size, may_free);
 }
 
@@ -537,7 +538,6 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size)
             l = size;
         }
         memcpy(f->buf + f->buf_index, buf, l);
-        f->rate_limit_used += l;
         add_buf_to_iovec(f, l);
         if (qemu_file_get_error(f)) {
             break;
@@ -554,7 +554,6 @@ void qemu_put_byte(QEMUFile *f, int v)
     }
 
     f->buf[f->buf_index] = v;
-    f->rate_limit_used++;
     add_buf_to_iovec(f, 1);
 }
 
-- 
2.40.1

Re: [PATCH v2 04/16] qemu-file: Account for rate_limit usage on qemu_fflush()

[Leonardo Brás]

On Mon, 2023-05-15 at 21:56 +0200, Juan Quintela wrote:
> That is the moment we know we have transferred something.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
>  migration/qemu-file.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 4bc875b452..956bd2a580 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -302,7 +302,9 @@ void qemu_fflush(QEMUFile *f)
>                                     &local_error) < 0) {
>              qemu_file_set_error_obj(f, -EIO, local_error);
>          } else {
> -            f->total_transferred += iov_size(f->iov, f->iovcnt);
> +            uint64_t size = iov_size(f->iov, f->iovcnt);
> +            qemu_file_acct_rate_limit(f, size);
> +            f->total_transferred += size;
>          }
>  
>          qemu_iovec_release_ram(f);
> @@ -519,7 +521,6 @@ void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size,
>          return;
>      }
>  
> -    f->rate_limit_used += size;
>      add_to_iovec(f, buf, size, may_free);
>  }
>  
> @@ -537,7 +538,6 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size)
>              l = size;
>          }
>          memcpy(f->buf + f->buf_index, buf, l);
> -        f->rate_limit_used += l;
>          add_buf_to_iovec(f, l);
>          if (qemu_file_get_error(f)) {
>              break;
> @@ -554,7 +554,6 @@ void qemu_put_byte(QEMUFile *f, int v)
>      }
>  
>      f->buf[f->buf_index] = v;
> -    f->rate_limit_used++;
>      add_buf_to_iovec(f, 1);
>  }
>  

If we are counting transferred data at fflush, it makes sense to increase rate-
limit accounting at the same place. It may be less granular, but is more
efficient.

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

Re: [PATCH v2 04/16] qemu-file: Account for rate_limit usage on qemu_fflush()

[Juan Quintela]

Leonardo Brás <leobras@redhat.com> wrote:
> On Mon, 2023-05-15 at 21:56 +0200, Juan Quintela wrote:
>> That is the moment we know we have transferred something.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  migration/qemu-file.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>> 
>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>> index 4bc875b452..956bd2a580 100644
>> --- a/migration/qemu-file.c
>> +++ b/migration/qemu-file.c
>> @@ -302,7 +302,9 @@ void qemu_fflush(QEMUFile *f)
>>                                     &local_error) < 0) {
>>              qemu_file_set_error_obj(f, -EIO, local_error);
>>          } else {
>> -            f->total_transferred += iov_size(f->iov, f->iovcnt);
>> +            uint64_t size = iov_size(f->iov, f->iovcnt);
>> +            qemu_file_acct_rate_limit(f, size);
>> +            f->total_transferred += size;
>>          }
>>  
>>          qemu_iovec_release_ram(f);
>> @@ -519,7 +521,6 @@ void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size,
>>          return;
>>      }
>>  
>> -    f->rate_limit_used += size;
>>      add_to_iovec(f, buf, size, may_free);
>>  }
>>  
>> @@ -537,7 +538,6 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size)
>>              l = size;
>>          }
>>          memcpy(f->buf + f->buf_index, buf, l);
>> -        f->rate_limit_used += l;
>>          add_buf_to_iovec(f, l);
>>          if (qemu_file_get_error(f)) {
>>              break;
>> @@ -554,7 +554,6 @@ void qemu_put_byte(QEMUFile *f, int v)
>>      }
>>  
>>      f->buf[f->buf_index] = v;
>> -    f->rate_limit_used++;
>>      add_buf_to_iovec(f, 1);
>>  }
>>  
>
> If we are counting transferred data at fflush, it makes sense to increase rate-
> limit accounting at the same place. It may be less granular, but is more
> efficient.

Yeap, the whole point is that in my next series, rate_limit_used
dissapear, we just use transferred for both things(*).

Later, Juan.

*: It is a bit more complicated than that, but we go from three counters
 to a single counter.

Re: [PATCH v2 04/16] qemu-file: Account for rate_limit usage on qemu_fflush()

[Leonardo Bras Soares Passos]

On Fri, May 26, 2023 at 5:09 AM Juan Quintela <quintela@redhat.com> wrote:
>
> Leonardo Brás <leobras@redhat.com> wrote:
> > On Mon, 2023-05-15 at 21:56 +0200, Juan Quintela wrote:
> >> That is the moment we know we have transferred something.
> >>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  migration/qemu-file.c | 7 +++----
> >>  1 file changed, 3 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> >> index 4bc875b452..956bd2a580 100644
> >> --- a/migration/qemu-file.c
> >> +++ b/migration/qemu-file.c
> >> @@ -302,7 +302,9 @@ void qemu_fflush(QEMUFile *f)
> >>                                     &local_error) < 0) {
> >>              qemu_file_set_error_obj(f, -EIO, local_error);
> >>          } else {
> >> -            f->total_transferred += iov_size(f->iov, f->iovcnt);
> >> +            uint64_t size = iov_size(f->iov, f->iovcnt);
> >> +            qemu_file_acct_rate_limit(f, size);
> >> +            f->total_transferred += size;
> >>          }
> >>
> >>          qemu_iovec_release_ram(f);
> >> @@ -519,7 +521,6 @@ void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size,
> >>          return;
> >>      }
> >>
> >> -    f->rate_limit_used += size;
> >>      add_to_iovec(f, buf, size, may_free);
> >>  }
> >>
> >> @@ -537,7 +538,6 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size)
> >>              l = size;
> >>          }
> >>          memcpy(f->buf + f->buf_index, buf, l);
> >> -        f->rate_limit_used += l;
> >>          add_buf_to_iovec(f, l);
> >>          if (qemu_file_get_error(f)) {
> >>              break;
> >> @@ -554,7 +554,6 @@ void qemu_put_byte(QEMUFile *f, int v)
> >>      }
> >>
> >>      f->buf[f->buf_index] = v;
> >> -    f->rate_limit_used++;
> >>      add_buf_to_iovec(f, 1);
> >>  }
> >>
> >
> > If we are counting transferred data at fflush, it makes sense to increase rate-
> > limit accounting at the same place. It may be less granular, but is more
> > efficient.
>
> Yeap, the whole point is that in my next series, rate_limit_used
> dissapear, we just use transferred for both things(*).
>
> Later, Juan.
>
> *: It is a bit more complicated than that, but we go from three counters
>  to a single counter.
>

Seems great to simplify stuff.
Thanks!
Leo

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

[Juan Quintela]

These way we can make them atomic and use this functions from any
place.  I also moved all functions that use rate_limit to
migration-stats.

Functions got renamed, they are not qemu_file anymore.

qemu_file_rate_limit -> migration_rate_exceeded
qemu_file_set_rate_limit -> migration_rate_set
qemu_file_get_rate_limit -> migration_rate_get
qemu_file_reset_rate_limit -> migration_rate_reset
qemu_file_acct_rate_limit -> migration_rate_account.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

---

s/this/these/ (harsh)
If you have any good suggestion for better names, I am all ears.
Fix missing / XFER_LIMIT_RATIO in migration_rate_set(quintela)
---
 include/migration/qemu-file-types.h | 12 ++++++-
 migration/migration-stats.h         | 47 ++++++++++++++++++++++++++
 migration/options.h                 |  7 ----
 migration/qemu-file.h               | 11 ------
 hw/ppc/spapr.c                      |  4 +--
 hw/s390x/s390-stattrib.c            |  2 +-
 migration/block-dirty-bitmap.c      |  2 +-
 migration/block.c                   |  5 +--
 migration/migration-stats.c         | 44 ++++++++++++++++++++++++
 migration/migration.c               | 14 ++++----
 migration/multifd.c                 |  2 +-
 migration/options.c                 |  7 ++--
 migration/qemu-file.c               | 52 ++---------------------------
 migration/ram.c                     |  2 +-
 migration/savevm.c                  |  2 +-
 15 files changed, 124 insertions(+), 89 deletions(-)

diff --git a/include/migration/qemu-file-types.h b/include/migration/qemu-file-types.h
index 1436f9ce92..9ba163f333 100644
--- a/include/migration/qemu-file-types.h
+++ b/include/migration/qemu-file-types.h
@@ -165,6 +165,16 @@ size_t coroutine_mixed_fn qemu_get_counted_string(QEMUFile *f, char buf[256]);
 
 void qemu_put_counted_string(QEMUFile *f, const char *name);
 
-int qemu_file_rate_limit(QEMUFile *f);
+/**
+ * migration_rate_exceeded: Check if we have exceeded rate for this interval
+ *
+ * Checks if we have already transferred more data that we are allowed
+ * in the current interval.
+ *
+ * @f: QEMUFile used for main migration channel
+ *
+ * Returns if we should stop sending data for this interval.
+ */
+bool migration_rate_exceeded(QEMUFile *f);
 
 #endif
diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index 21402af9e4..e39c083245 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -15,6 +15,12 @@
 
 #include "qemu/stats64.h"
 
+/*
+ * Amount of time to allocate to each "chunk" of bandwidth-throttled
+ * data.
+ */
+#define BUFFER_DELAY     100
+
 /*
  * If rate_limit_max is 0, there is special code to remove the rate
  * limit.
@@ -75,6 +81,14 @@ typedef struct {
      * Number of bytes sent during precopy stage.
      */
     Stat64 precopy_bytes;
+    /*
+     * Maximum amount of data we can send in a cycle.
+     */
+    Stat64 rate_limit_max;
+    /*
+     * Amount of data we have sent in the current cycle.
+     */
+    Stat64 rate_limit_used;
     /*
      * How long has the setup stage took.
      */
@@ -100,4 +114,37 @@ extern MigrationAtomicStats mig_stats;
  * Returns: Nothing.  The time is stored in val.
  */
 void migration_time_since(MigrationAtomicStats *stats, int64_t since);
+
+/**
+ * migration_rate_account: Increase the number of bytes transferred.
+ *
+ * Report on a number of bytes the have been transferred that need to
+ * be applied to the rate limiting calcuations.
+ *
+ * @len: amount of bytes transferred
+ */
+void migration_rate_account(uint64_t len);
+
+/**
+ * migration_rate_get: Get the maximum amount that can be transferred.
+ *
+ * Returns the maximum number of bytes that can be transferred in a cycle.
+ */
+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.
+ */
+void migration_rate_reset(void);
+
+/**
+ * migration_rate_set: Set the maximum amount that can be transferred.
+ *
+ * Sets the maximum amount of bytes that can be transferred in one cycle.
+ *
+ * @new_rate: new maximum amount
+ */
+void migration_rate_set(uint64_t new_rate);
 #endif
diff --git a/migration/options.h b/migration/options.h
index 5cca3326d6..45991af3c2 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -17,13 +17,6 @@
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
 
-/* constants */
-
-/* Amount of time to allocate to each "chunk" of bandwidth-throttled
- * data. */
-#define BUFFER_DELAY     100
-#define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
-
 /* migration properties */
 
 extern Property migration_properties[];
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index bcc39081f2..e649718492 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -130,17 +130,6 @@ void qemu_file_skip(QEMUFile *f, int size);
  * accounting information tracks the total migration traffic.
  */
 void qemu_file_credit_transfer(QEMUFile *f, size_t size);
-void qemu_file_reset_rate_limit(QEMUFile *f);
-/*
- * qemu_file_acct_rate_limit:
- *
- * Report on a number of bytes the have been transferred
- * out of band from the main file object I/O methods, and
- * need to be applied to the rate limiting calcuations
- */
-void qemu_file_acct_rate_limit(QEMUFile *f, uint64_t len);
-void qemu_file_set_rate_limit(QEMUFile *f, uint64_t new_rate);
-uint64_t qemu_file_get_rate_limit(QEMUFile *f);
 int qemu_file_get_error_obj(QEMUFile *f, Error **errp);
 int qemu_file_get_error_obj_any(QEMUFile *f1, QEMUFile *f2, Error **errp);
 void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ddc9c7b1a1..1baea16c96 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2166,7 +2166,7 @@ static void htab_save_first_pass(QEMUFile *f, SpaprMachineState *spapr,
                 break;
             }
         }
-    } while ((index < htabslots) && !qemu_file_rate_limit(f));
+    } while ((index < htabslots) && !migration_rate_exceeded(f));
 
     if (index >= htabslots) {
         assert(index == htabslots);
@@ -2237,7 +2237,7 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
             assert(index == htabslots);
             index = 0;
         }
-    } while ((examined < htabslots) && (!qemu_file_rate_limit(f) || final));
+    } while ((examined < htabslots) && (!migration_rate_exceeded(f) || final));
 
     if (index >= htabslots) {
         assert(index == htabslots);
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index aed919ad7d..220e845d12 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -209,7 +209,7 @@ static int cmma_save(QEMUFile *f, void *opaque, int final)
         return -ENOMEM;
     }
 
-    while (final ? 1 : qemu_file_rate_limit(f) == 0) {
+    while (final ? 1 : migration_rate_exceeded(f) == 0) {
         reallen = sac->get_stattr(sas, &start_gfn, buflen, buf);
         if (reallen < 0) {
             g_free(buf);
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 20f36e6bd8..032fc5f405 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -706,7 +706,7 @@ static void bulk_phase(QEMUFile *f, DBMSaveState *s, bool limit)
     QSIMPLEQ_FOREACH(dbms, &s->dbms_list, entry) {
         while (!dbms->bulk_completed) {
             bulk_phase_send_chunk(f, s, dbms);
-            if (limit && qemu_file_rate_limit(f)) {
+            if (limit && migration_rate_exceeded(f)) {
                 return;
             }
         }
diff --git a/migration/block.c b/migration/block.c
index 12617b4152..b9580a6c7e 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -23,6 +23,7 @@
 #include "block/dirty-bitmap.h"
 #include "migration/misc.h"
 #include "migration.h"
+#include "migration-stats.h"
 #include "migration/register.h"
 #include "qemu-file.h"
 #include "migration/vmstate.h"
@@ -625,7 +626,7 @@ static int flush_blks(QEMUFile *f)
 
     blk_mig_lock();
     while ((blk = QSIMPLEQ_FIRST(&block_mig_state.blk_list)) != NULL) {
-        if (qemu_file_rate_limit(f)) {
+        if (migration_rate_exceeded(f)) {
             break;
         }
         if (blk->ret < 0) {
@@ -762,7 +763,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
     /* control the rate of transfer */
     blk_mig_lock();
     while (block_mig_state.read_done * BLK_MIG_BLOCK_SIZE <
-           qemu_file_get_rate_limit(f) &&
+           migration_rate_get() &&
            block_mig_state.submitted < MAX_PARALLEL_IO &&
            (block_mig_state.submitted + block_mig_state.read_done) <
            MAX_IO_BUFFERS) {
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
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "qemu/stats64.h"
 #include "qemu/timer.h"
+#include "qemu-file.h"
 #include "migration-stats.h"
 
 MigrationAtomicStats mig_stats;
@@ -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;
+}
+
+uint64_t migration_rate_get(void)
+{
+    return stat64_get(&mig_stats.rate_limit_max);
+}
+
+#define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
+
+void migration_rate_set(uint64_t limit)
+{
+    /*
+     * 'limit' is per second.  But we check it each BUFER_DELAY miliseconds.
+     */
+    stat64_set(&mig_stats.rate_limit_max, limit / XFER_LIMIT_RATIO);
+}
+
+void migration_rate_reset(void)
+{
+    stat64_set(&mig_stats.rate_limit_used, 0);
+}
+
+void migration_rate_account(uint64_t len)
+{
+    stat64_add(&mig_stats.rate_limit_used, len);
+}
diff --git a/migration/migration.c b/migration/migration.c
index e9466273bb..594709dbbc 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2119,7 +2119,7 @@ static int postcopy_start(MigrationState *ms)
      * will notice we're in POSTCOPY_ACTIVE and not actually
      * wrap their state up here
      */
-    qemu_file_set_rate_limit(ms->to_dst_file, bandwidth);
+    migration_rate_set(bandwidth);
     if (migrate_postcopy_ram()) {
         /* Ping just for debugging, helps line traces up */
         qemu_savevm_send_ping(ms->to_dst_file, 2);
@@ -2303,7 +2303,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, RATE_LIMIT_MAX);
+                migration_rate_set(RATE_LIMIT_MAX);
                 ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
                                                          s->block_inactive);
             }
@@ -2698,7 +2698,7 @@ static void migration_update_counters(MigrationState *s,
             stat64_get(&mig_stats.dirty_bytes_last_sync) / bandwidth;
     }
 
-    qemu_file_reset_rate_limit(s->to_dst_file);
+    migration_rate_reset();
 
     update_iteration_initial_status(s);
 
@@ -2851,7 +2851,7 @@ bool migration_rate_limit(void)
 
     bool urgent = false;
     migration_update_counters(s, now);
-    if (qemu_file_rate_limit(s->to_dst_file)) {
+    if (migration_rate_exceeded(s->to_dst_file)) {
 
         if (qemu_file_get_error(s->to_dst_file)) {
             return false;
@@ -2973,7 +2973,7 @@ static void *migration_thread(void *opaque)
     trace_migration_thread_setup_complete();
 
     while (migration_is_active(s)) {
-        if (urgent || !qemu_file_rate_limit(s->to_dst_file)) {
+        if (urgent || !migration_rate_exceeded(s->to_dst_file)) {
             MigIterateState iter_state = migration_iteration_run(s);
             if (iter_state == MIG_ITERATE_SKIP) {
                 continue;
@@ -3047,7 +3047,7 @@ static void *bg_migration_thread(void *opaque)
     rcu_register_thread();
     object_ref(OBJECT(s));
 
-    qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX);
+    migration_rate_set(RATE_LIMIT_MAX);
 
     setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
     /*
@@ -3219,7 +3219,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
         notifier_list_notify(&migration_state_notifiers, s);
     }
 
-    qemu_file_set_rate_limit(s->to_dst_file, rate_limit);
+    migration_rate_set(rate_limit);
     qemu_file_set_blocking(s->to_dst_file, true);
 
     /*
diff --git a/migration/multifd.c b/migration/multifd.c
index 5c4298eadf..5052091ce2 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -431,7 +431,7 @@ static int multifd_send_pages(QEMUFile *f)
     multifd_send_state->pages = p->pages;
     p->pages = pages;
     transferred = ((uint64_t) pages->num) * p->page_size + p->packet_len;
-    qemu_file_acct_rate_limit(f, transferred);
+    migration_rate_account(transferred);
     qemu_mutex_unlock(&p->mutex);
     stat64_add(&mig_stats.transferred, transferred);
     stat64_add(&mig_stats.multifd_bytes, transferred);
diff --git a/migration/options.c b/migration/options.c
index c2a278ee2d..b62ab30cd5 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -23,6 +23,7 @@
 #include "migration/colo.h"
 #include "migration/misc.h"
 #include "migration.h"
+#include "migration-stats.h"
 #include "qemu-file.h"
 #include "ram.h"
 #include "options.h"
@@ -1242,8 +1243,7 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
     if (params->has_max_bandwidth) {
         s->parameters.max_bandwidth = params->max_bandwidth;
         if (s->to_dst_file && !migration_in_postcopy()) {
-            qemu_file_set_rate_limit(s->to_dst_file,
-                                s->parameters.max_bandwidth);
+            migration_rate_set(s->parameters.max_bandwidth);
         }
     }
 
@@ -1272,8 +1272,7 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
     if (params->has_max_postcopy_bandwidth) {
         s->parameters.max_postcopy_bandwidth = params->max_postcopy_bandwidth;
         if (s->to_dst_file && migration_in_postcopy()) {
-            qemu_file_set_rate_limit(s->to_dst_file,
-                    s->parameters.max_postcopy_bandwidth);
+            migration_rate_set(s->parameters.max_postcopy_bandwidth);
         }
     }
     if (params->has_max_cpu_throttle) {
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 956bd2a580..9c67b52fe0 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -41,17 +41,6 @@ struct QEMUFile {
     QIOChannel *ioc;
     bool is_writable;
 
-    /*
-     * Maximum amount of data in bytes to transfer during one
-     * rate limiting time window
-     */
-    uint64_t rate_limit_max;
-    /*
-     * Total amount of data in bytes queued for transfer
-     * during this rate limiting time window
-     */
-    uint64_t rate_limit_used;
-
     /* The sum of bytes transferred on the wire */
     uint64_t total_transferred;
 
@@ -303,7 +292,7 @@ void qemu_fflush(QEMUFile *f)
             qemu_file_set_error_obj(f, -EIO, local_error);
         } else {
             uint64_t size = iov_size(f->iov, f->iovcnt);
-            qemu_file_acct_rate_limit(f, size);
+            migration_rate_account(size);
             f->total_transferred += size;
         }
 
@@ -356,7 +345,7 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
         int ret = f->hooks->save_page(f, block_offset,
                                       offset, size, bytes_sent);
         if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
-            qemu_file_acct_rate_limit(f, size);
+            migration_rate_account(size);
         }
 
         if (ret != RAM_SAVE_CONTROL_DELAYED &&
@@ -727,43 +716,6 @@ uint64_t qemu_file_transferred(QEMUFile *f)
     return f->total_transferred;
 }
 
-int qemu_file_rate_limit(QEMUFile *f)
-{
-    if (qemu_file_get_error(f)) {
-        return 1;
-    }
-    if (f->rate_limit_max == RATE_LIMIT_MAX) {
-        return 0;
-    }
-    if (f->rate_limit_used > f->rate_limit_max) {
-        return 1;
-    }
-    return 0;
-}
-
-uint64_t qemu_file_get_rate_limit(QEMUFile *f)
-{
-    return f->rate_limit_max;
-}
-
-void qemu_file_set_rate_limit(QEMUFile *f, uint64_t limit)
-{
-    /*
-     * 'limit' is per second.  But we check it each 100 miliseconds.
-     */
-    f->rate_limit_max = limit / XFER_LIMIT_RATIO;
-}
-
-void qemu_file_reset_rate_limit(QEMUFile *f)
-{
-    f->rate_limit_used = 0;
-}
-
-void qemu_file_acct_rate_limit(QEMUFile *f, uint64_t len)
-{
-    f->rate_limit_used += len;
-}
-
 void qemu_put_be16(QEMUFile *f, unsigned int v)
 {
     qemu_put_byte(f, v >> 8);
diff --git a/migration/ram.c b/migration/ram.c
index fd5a8db0f8..a706edecc0 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3126,7 +3126,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
 
         t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
         i = 0;
-        while ((ret = qemu_file_rate_limit(f)) == 0 ||
+        while ((ret = migration_rate_exceeded(f)) == 0 ||
                postcopy_has_request(rs)) {
             int pages;
 
diff --git a/migration/savevm.c b/migration/savevm.c
index c7af9050c2..6b783d9c39 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1345,7 +1345,7 @@ int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy)
             !(se->ops->has_postcopy && se->ops->has_postcopy(se->opaque))) {
             continue;
         }
-        if (qemu_file_rate_limit(f)) {
+        if (migration_rate_exceeded(f)) {
             return 0;
         }
         trace_savevm_section_start(se->idstr, se->section_id);
-- 
2.40.1

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

[Cédric Le Goater]

On 5/15/23 21:56, Juan Quintela wrote:
> These way we can make them atomic and use this functions from any
> place.  I also moved all functions that use rate_limit to
> migration-stats.
> 
> Functions got renamed, they are not qemu_file anymore.
> 
> qemu_file_rate_limit -> migration_rate_exceeded
> qemu_file_set_rate_limit -> migration_rate_set
> qemu_file_get_rate_limit -> migration_rate_get
> qemu_file_reset_rate_limit -> migration_rate_reset
> qemu_file_acct_rate_limit -> migration_rate_account.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
> 
> s/this/these/ (harsh)
> If you have any good suggestion for better names, I am all ears.
> Fix missing / XFER_LIMIT_RATIO in migration_rate_set(quintela)
> ---
>   include/migration/qemu-file-types.h | 12 ++++++-
>   migration/migration-stats.h         | 47 ++++++++++++++++++++++++++
>   migration/options.h                 |  7 ----
>   migration/qemu-file.h               | 11 ------
>   hw/ppc/spapr.c                      |  4 +--
>   hw/s390x/s390-stattrib.c            |  2 +-
>   migration/block-dirty-bitmap.c      |  2 +-
>   migration/block.c                   |  5 +--
>   migration/migration-stats.c         | 44 ++++++++++++++++++++++++
>   migration/migration.c               | 14 ++++----
>   migration/multifd.c                 |  2 +-
>   migration/options.c                 |  7 ++--
>   migration/qemu-file.c               | 52 ++---------------------------
>   migration/ram.c                     |  2 +-
>   migration/savevm.c                  |  2 +-
>   15 files changed, 124 insertions(+), 89 deletions(-)
> 
> diff --git a/include/migration/qemu-file-types.h b/include/migration/qemu-file-types.h
> index 1436f9ce92..9ba163f333 100644
> --- a/include/migration/qemu-file-types.h
> +++ b/include/migration/qemu-file-types.h
> @@ -165,6 +165,16 @@ size_t coroutine_mixed_fn qemu_get_counted_string(QEMUFile *f, char buf[256]);
>   
>   void qemu_put_counted_string(QEMUFile *f, const char *name);
>   
> -int qemu_file_rate_limit(QEMUFile *f);
> +/**
> + * migration_rate_exceeded: Check if we have exceeded rate for this interval
> + *
> + * Checks if we have already transferred more data that we are allowed
> + * in the current interval.
> + *
> + * @f: QEMUFile used for main migration channel
> + *
> + * Returns if we should stop sending data for this interval.
> + */
> +bool migration_rate_exceeded(QEMUFile *f);
>   
>   #endif
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index 21402af9e4..e39c083245 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -15,6 +15,12 @@
>   
>   #include "qemu/stats64.h"
>   
> +/*
> + * Amount of time to allocate to each "chunk" of bandwidth-throttled
> + * data.
> + */
> +#define BUFFER_DELAY     100
> +
>   /*
>    * If rate_limit_max is 0, there is special code to remove the rate
>    * limit.
> @@ -75,6 +81,14 @@ typedef struct {
>        * Number of bytes sent during precopy stage.
>        */
>       Stat64 precopy_bytes;
> +    /*
> +     * Maximum amount of data we can send in a cycle.
> +     */
> +    Stat64 rate_limit_max;
> +    /*
> +     * Amount of data we have sent in the current cycle.
> +     */
> +    Stat64 rate_limit_used;
>       /*
>        * How long has the setup stage took.
>        */
> @@ -100,4 +114,37 @@ extern MigrationAtomicStats mig_stats;
>    * Returns: Nothing.  The time is stored in val.
>    */
>   void migration_time_since(MigrationAtomicStats *stats, int64_t since);
> +
> +/**
> + * migration_rate_account: Increase the number of bytes transferred.
> + *
> + * Report on a number of bytes the have been transferred that need to
> + * be applied to the rate limiting calcuations.
> + *
> + * @len: amount of bytes transferred
> + */
> +void migration_rate_account(uint64_t len);
> +
> +/**
> + * migration_rate_get: Get the maximum amount that can be transferred.
> + *
> + * Returns the maximum number of bytes that can be transferred in a cycle.
> + */
> +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.
> + */
> +void migration_rate_reset(void);
> +
> +/**
> + * migration_rate_set: Set the maximum amount that can be transferred.
> + *
> + * Sets the maximum amount of bytes that can be transferred in one cycle.
> + *
> + * @new_rate: new maximum amount
> + */
> +void migration_rate_set(uint64_t new_rate);
>   #endif
> diff --git a/migration/options.h b/migration/options.h
> index 5cca3326d6..45991af3c2 100644
> --- a/migration/options.h
> +++ b/migration/options.h
> @@ -17,13 +17,6 @@
>   #include "hw/qdev-properties.h"
>   #include "hw/qdev-properties-system.h"
>   
> -/* constants */
> -
> -/* Amount of time to allocate to each "chunk" of bandwidth-throttled
> - * data. */
> -#define BUFFER_DELAY     100
> -#define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
> -
>   /* migration properties */
>   
>   extern Property migration_properties[];
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index bcc39081f2..e649718492 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -130,17 +130,6 @@ void qemu_file_skip(QEMUFile *f, int size);
>    * accounting information tracks the total migration traffic.
>    */
>   void qemu_file_credit_transfer(QEMUFile *f, size_t size);
> -void qemu_file_reset_rate_limit(QEMUFile *f);
> -/*
> - * qemu_file_acct_rate_limit:
> - *
> - * Report on a number of bytes the have been transferred
> - * out of band from the main file object I/O methods, and
> - * need to be applied to the rate limiting calcuations
> - */
> -void qemu_file_acct_rate_limit(QEMUFile *f, uint64_t len);
> -void qemu_file_set_rate_limit(QEMUFile *f, uint64_t new_rate);
> -uint64_t qemu_file_get_rate_limit(QEMUFile *f);
>   int qemu_file_get_error_obj(QEMUFile *f, Error **errp);
>   int qemu_file_get_error_obj_any(QEMUFile *f1, QEMUFile *f2, Error **errp);
>   void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ddc9c7b1a1..1baea16c96 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2166,7 +2166,7 @@ static void htab_save_first_pass(QEMUFile *f, SpaprMachineState *spapr,
>                   break;
>               }
>           }
> -    } while ((index < htabslots) && !qemu_file_rate_limit(f));
> +    } while ((index < htabslots) && !migration_rate_exceeded(f));
>   
>       if (index >= htabslots) {
>           assert(index == htabslots);
> @@ -2237,7 +2237,7 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
>               assert(index == htabslots);
>               index = 0;
>           }
> -    } while ((examined < htabslots) && (!qemu_file_rate_limit(f) || final));
> +    } while ((examined < htabslots) && (!migration_rate_exceeded(f) || final));
>   
>       if (index >= htabslots) {
>           assert(index == htabslots);
> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
> index aed919ad7d..220e845d12 100644
> --- a/hw/s390x/s390-stattrib.c
> +++ b/hw/s390x/s390-stattrib.c
> @@ -209,7 +209,7 @@ static int cmma_save(QEMUFile *f, void *opaque, int final)
>           return -ENOMEM;
>       }
>   
> -    while (final ? 1 : qemu_file_rate_limit(f) == 0) {
> +    while (final ? 1 : migration_rate_exceeded(f) == 0) {
>           reallen = sac->get_stattr(sas, &start_gfn, buflen, buf);
>           if (reallen < 0) {
>               g_free(buf);
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 20f36e6bd8..032fc5f405 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -706,7 +706,7 @@ static void bulk_phase(QEMUFile *f, DBMSaveState *s, bool limit)
>       QSIMPLEQ_FOREACH(dbms, &s->dbms_list, entry) {
>           while (!dbms->bulk_completed) {
>               bulk_phase_send_chunk(f, s, dbms);
> -            if (limit && qemu_file_rate_limit(f)) {
> +            if (limit && migration_rate_exceeded(f)) {
>                   return;
>               }
>           }
> diff --git a/migration/block.c b/migration/block.c
> index 12617b4152..b9580a6c7e 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -23,6 +23,7 @@
>   #include "block/dirty-bitmap.h"
>   #include "migration/misc.h"
>   #include "migration.h"
> +#include "migration-stats.h"
>   #include "migration/register.h"
>   #include "qemu-file.h"
>   #include "migration/vmstate.h"
> @@ -625,7 +626,7 @@ static int flush_blks(QEMUFile *f)
>   
>       blk_mig_lock();
>       while ((blk = QSIMPLEQ_FIRST(&block_mig_state.blk_list)) != NULL) {
> -        if (qemu_file_rate_limit(f)) {
> +        if (migration_rate_exceeded(f)) {
>               break;
>           }
>           if (blk->ret < 0) {
> @@ -762,7 +763,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>       /* control the rate of transfer */
>       blk_mig_lock();
>       while (block_mig_state.read_done * BLK_MIG_BLOCK_SIZE <
> -           qemu_file_get_rate_limit(f) &&
> +           migration_rate_get() &&
>              block_mig_state.submitted < MAX_PARALLEL_IO &&
>              (block_mig_state.submitted + block_mig_state.read_done) <
>              MAX_IO_BUFFERS) {
> 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
> @@ -13,6 +13,7 @@
>   #include "qemu/osdep.h"
>   #include "qemu/stats64.h"
>   #include "qemu/timer.h"
> +#include "qemu-file.h"
>   #include "migration-stats.h"
>   
>   MigrationAtomicStats mig_stats;
> @@ -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;
> +}
> +
> +uint64_t migration_rate_get(void)
> +{
> +    return stat64_get(&mig_stats.rate_limit_max);
> +}
> +
> +#define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
> +
> +void migration_rate_set(uint64_t limit)
> +{
> +    /*
> +     * 'limit' is per second.  But we check it each BUFER_DELAY miliseconds.
> +     */
> +    stat64_set(&mig_stats.rate_limit_max, limit / XFER_LIMIT_RATIO);
> +}
> +
> +void migration_rate_reset(void)
> +{
> +    stat64_set(&mig_stats.rate_limit_used, 0);
> +}
> +
> +void migration_rate_account(uint64_t len)
> +{
> +    stat64_add(&mig_stats.rate_limit_used, len);
> +}
> diff --git a/migration/migration.c b/migration/migration.c
> index e9466273bb..594709dbbc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2119,7 +2119,7 @@ static int postcopy_start(MigrationState *ms)
>        * will notice we're in POSTCOPY_ACTIVE and not actually
>        * wrap their state up here
>        */
> -    qemu_file_set_rate_limit(ms->to_dst_file, bandwidth);
> +    migration_rate_set(bandwidth);
>       if (migrate_postcopy_ram()) {
>           /* Ping just for debugging, helps line traces up */
>           qemu_savevm_send_ping(ms->to_dst_file, 2);
> @@ -2303,7 +2303,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, RATE_LIMIT_MAX);
> +                migration_rate_set(RATE_LIMIT_MAX);
>                   ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
>                                                            s->block_inactive);
>               }
> @@ -2698,7 +2698,7 @@ static void migration_update_counters(MigrationState *s,
>               stat64_get(&mig_stats.dirty_bytes_last_sync) / bandwidth;
>       }
>   
> -    qemu_file_reset_rate_limit(s->to_dst_file);
> +    migration_rate_reset();
>   
>       update_iteration_initial_status(s);
>   
> @@ -2851,7 +2851,7 @@ bool migration_rate_limit(void)
>   
>       bool urgent = false;
>       migration_update_counters(s, now);
> -    if (qemu_file_rate_limit(s->to_dst_file)) {
> +    if (migration_rate_exceeded(s->to_dst_file)) {
>   
>           if (qemu_file_get_error(s->to_dst_file)) {
>               return false;
> @@ -2973,7 +2973,7 @@ static void *migration_thread(void *opaque)
>       trace_migration_thread_setup_complete();
>   
>       while (migration_is_active(s)) {
> -        if (urgent || !qemu_file_rate_limit(s->to_dst_file)) {
> +        if (urgent || !migration_rate_exceeded(s->to_dst_file)) {
>               MigIterateState iter_state = migration_iteration_run(s);
>               if (iter_state == MIG_ITERATE_SKIP) {
>                   continue;
> @@ -3047,7 +3047,7 @@ static void *bg_migration_thread(void *opaque)
>       rcu_register_thread();
>       object_ref(OBJECT(s));
>   
> -    qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX);
> +    migration_rate_set(RATE_LIMIT_MAX);
>   
>       setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
>       /*
> @@ -3219,7 +3219,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>           notifier_list_notify(&migration_state_notifiers, s);
>       }
>   
> -    qemu_file_set_rate_limit(s->to_dst_file, rate_limit);
> +    migration_rate_set(rate_limit);
>       qemu_file_set_blocking(s->to_dst_file, true);
>   
>       /*
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 5c4298eadf..5052091ce2 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -431,7 +431,7 @@ static int multifd_send_pages(QEMUFile *f)
>       multifd_send_state->pages = p->pages;
>       p->pages = pages;
>       transferred = ((uint64_t) pages->num) * p->page_size + p->packet_len;
> -    qemu_file_acct_rate_limit(f, transferred);
> +    migration_rate_account(transferred);
>       qemu_mutex_unlock(&p->mutex);
>       stat64_add(&mig_stats.transferred, transferred);
>       stat64_add(&mig_stats.multifd_bytes, transferred);
> diff --git a/migration/options.c b/migration/options.c
> index c2a278ee2d..b62ab30cd5 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -23,6 +23,7 @@
>   #include "migration/colo.h"
>   #include "migration/misc.h"
>   #include "migration.h"
> +#include "migration-stats.h"
>   #include "qemu-file.h"
>   #include "ram.h"
>   #include "options.h"
> @@ -1242,8 +1243,7 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>       if (params->has_max_bandwidth) {
>           s->parameters.max_bandwidth = params->max_bandwidth;
>           if (s->to_dst_file && !migration_in_postcopy()) {
> -            qemu_file_set_rate_limit(s->to_dst_file,
> -                                s->parameters.max_bandwidth);
> +            migration_rate_set(s->parameters.max_bandwidth);
>           }
>       }
>   
> @@ -1272,8 +1272,7 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>       if (params->has_max_postcopy_bandwidth) {
>           s->parameters.max_postcopy_bandwidth = params->max_postcopy_bandwidth;
>           if (s->to_dst_file && migration_in_postcopy()) {
> -            qemu_file_set_rate_limit(s->to_dst_file,
> -                    s->parameters.max_postcopy_bandwidth);
> +            migration_rate_set(s->parameters.max_postcopy_bandwidth);
>           }
>       }
>       if (params->has_max_cpu_throttle) {
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 956bd2a580..9c67b52fe0 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -41,17 +41,6 @@ struct QEMUFile {
>       QIOChannel *ioc;
>       bool is_writable;
>   
> -    /*
> -     * Maximum amount of data in bytes to transfer during one
> -     * rate limiting time window
> -     */
> -    uint64_t rate_limit_max;
> -    /*
> -     * Total amount of data in bytes queued for transfer
> -     * during this rate limiting time window
> -     */
> -    uint64_t rate_limit_used;
> -
>       /* The sum of bytes transferred on the wire */
>       uint64_t total_transferred;
>   
> @@ -303,7 +292,7 @@ void qemu_fflush(QEMUFile *f)
>               qemu_file_set_error_obj(f, -EIO, local_error);
>           } else {
>               uint64_t size = iov_size(f->iov, f->iovcnt);
> -            qemu_file_acct_rate_limit(f, size);
> +            migration_rate_account(size);
>               f->total_transferred += size;
>           }
>   
> @@ -356,7 +345,7 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
>           int ret = f->hooks->save_page(f, block_offset,
>                                         offset, size, bytes_sent);
>           if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
> -            qemu_file_acct_rate_limit(f, size);
> +            migration_rate_account(size);
>           }
>   
>           if (ret != RAM_SAVE_CONTROL_DELAYED &&
> @@ -727,43 +716,6 @@ uint64_t qemu_file_transferred(QEMUFile *f)
>       return f->total_transferred;
>   }
>   
> -int qemu_file_rate_limit(QEMUFile *f)
> -{
> -    if (qemu_file_get_error(f)) {
> -        return 1;
> -    }
> -    if (f->rate_limit_max == RATE_LIMIT_MAX) {
> -        return 0;
> -    }
> -    if (f->rate_limit_used > f->rate_limit_max) {
> -        return 1;
> -    }
> -    return 0;
> -}
> -
> -uint64_t qemu_file_get_rate_limit(QEMUFile *f)
> -{
> -    return f->rate_limit_max;
> -}
> -
> -void qemu_file_set_rate_limit(QEMUFile *f, uint64_t limit)
> -{
> -    /*
> -     * 'limit' is per second.  But we check it each 100 miliseconds.
> -     */
> -    f->rate_limit_max = limit / XFER_LIMIT_RATIO;
> -}
> -
> -void qemu_file_reset_rate_limit(QEMUFile *f)
> -{
> -    f->rate_limit_used = 0;
> -}
> -
> -void qemu_file_acct_rate_limit(QEMUFile *f, uint64_t len)
> -{
> -    f->rate_limit_used += len;
> -}
> -
>   void qemu_put_be16(QEMUFile *f, unsigned int v)
>   {
>       qemu_put_byte(f, v >> 8);
> diff --git a/migration/ram.c b/migration/ram.c
> index fd5a8db0f8..a706edecc0 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3126,7 +3126,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>   
>           t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>           i = 0;
> -        while ((ret = qemu_file_rate_limit(f)) == 0 ||
> +        while ((ret = migration_rate_exceeded(f)) == 0 ||
>                  postcopy_has_request(rs)) {
>               int pages;
>   
> diff --git a/migration/savevm.c b/migration/savevm.c
> index c7af9050c2..6b783d9c39 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1345,7 +1345,7 @@ int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy)
>               !(se->ops->has_postcopy && se->ops->has_postcopy(se->opaque))) {
>               continue;
>           }
> -        if (qemu_file_rate_limit(f)) {
> +        if (migration_rate_exceeded(f)) {
>               return 0;
>           }
>           trace_savevm_section_start(se->idstr, se->section_id);

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

[Leonardo Brás]

On Mon, 2023-05-15 at 21:56 +0200, Juan Quintela wrote:
> These way we can make them atomic and use this functions from any
> place.  I also moved all functions that use rate_limit to
> migration-stats.
> 
> Functions got renamed, they are not qemu_file anymore.
> 
> qemu_file_rate_limit -> migration_rate_exceeded
> qemu_file_set_rate_limit -> migration_rate_set
> qemu_file_get_rate_limit -> migration_rate_get
> qemu_file_reset_rate_limit -> migration_rate_reset
> qemu_file_acct_rate_limit -> migration_rate_account.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> 
> ---
> 
> s/this/these/ (harsh)
> If you have any good suggestion for better names, I am all ears.
> Fix missing / XFER_LIMIT_RATIO in migration_rate_set(quintela)
> ---
>  include/migration/qemu-file-types.h | 12 ++++++-
>  migration/migration-stats.h         | 47 ++++++++++++++++++++++++++
>  migration/options.h                 |  7 ----
>  migration/qemu-file.h               | 11 ------
>  hw/ppc/spapr.c                      |  4 +--
>  hw/s390x/s390-stattrib.c            |  2 +-
>  migration/block-dirty-bitmap.c      |  2 +-
>  migration/block.c                   |  5 +--
>  migration/migration-stats.c         | 44 ++++++++++++++++++++++++
>  migration/migration.c               | 14 ++++----
>  migration/multifd.c                 |  2 +-
>  migration/options.c                 |  7 ++--
>  migration/qemu-file.c               | 52 ++---------------------------
>  migration/ram.c                     |  2 +-
>  migration/savevm.c                  |  2 +-
>  15 files changed, 124 insertions(+), 89 deletions(-)
> 
> diff --git a/include/migration/qemu-file-types.h b/include/migration/qemu-file-types.h
> index 1436f9ce92..9ba163f333 100644
> --- a/include/migration/qemu-file-types.h
> +++ b/include/migration/qemu-file-types.h
> @@ -165,6 +165,16 @@ size_t coroutine_mixed_fn qemu_get_counted_string(QEMUFile *f, char buf[256]);
>  
>  void qemu_put_counted_string(QEMUFile *f, const char *name);
>  
> -int qemu_file_rate_limit(QEMUFile *f);
> +/**
> + * migration_rate_exceeded: Check if we have exceeded rate for this interval
> + *
> + * Checks if we have already transferred more data that we are allowed
> + * in the current interval.
> + *
> + * @f: QEMUFile used for main migration channel
> + *
> + * Returns if we should stop sending data for this interval.
> + */
> +bool migration_rate_exceeded(QEMUFile *f);
>  
>  #endif
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index 21402af9e4..e39c083245 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -15,6 +15,12 @@
>  
>  #include "qemu/stats64.h"
>  
> +/*
> + * Amount of time to allocate to each "chunk" of bandwidth-throttled
> + * data.
> + */
> +#define BUFFER_DELAY     100
> +
>  /*
>   * If rate_limit_max is 0, there is special code to remove the rate
>   * limit.
> @@ -75,6 +81,14 @@ typedef struct {
>       * Number of bytes sent during precopy stage.
>       */
>      Stat64 precopy_bytes;
> +    /*
> +     * Maximum amount of data we can send in a cycle.
> +     */
> +    Stat64 rate_limit_max;
> +    /*
> +     * Amount of data we have sent in the current cycle.
> +     */
> +    Stat64 rate_limit_used;
>      /*
>       * How long has the setup stage took.
>       */
> @@ -100,4 +114,37 @@ extern MigrationAtomicStats mig_stats;
>   * Returns: Nothing.  The time is stored in val.
>   */
>  void migration_time_since(MigrationAtomicStats *stats, int64_t since);
> +
> +/**
> + * migration_rate_account: Increase the number of bytes transferred.
> + *
> + * Report on a number of bytes the have been transferred that need to
> + * be applied to the rate limiting calcuations.

s/calcuations/calculations

> + *
> + * @len: amount of bytes transferred
> + */
> +void migration_rate_account(uint64_t len);
> +
> +/**
> + * migration_rate_get: Get the maximum amount that can be transferred.
> + *
> + * Returns the maximum number of bytes that can be transferred in a cycle.
> + */
> +uint64_t migration_rate_get(void);

maybe migration_max_rate_get() ?

> +
> +/**
> + * migration_rate_reset: Reset the rate limit counter.
> + *
> + * This is called when we know we start a new transfer cycle.
> + */
> +void migration_rate_reset(void);
> +
> +/**
> + * migration_rate_set: Set the maximum amount that can be transferred.
> + *
> + * Sets the maximum amount of bytes that can be transferred in one cycle.
> + *
> + * @new_rate: new maximum amount
> + */
> +void migration_rate_set(uint64_t new_rate);

maybe migration_max_rate_set() ?

>  #endif
> diff --git a/migration/options.h b/migration/options.h
> index 5cca3326d6..45991af3c2 100644
> --- a/migration/options.h
> +++ b/migration/options.h
> @@ -17,13 +17,6 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/qdev-properties-system.h"
>  
> -/* constants */
> -
> -/* Amount of time to allocate to each "chunk" of bandwidth-throttled
> - * data. */
> -#define BUFFER_DELAY     100
> -#define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
> -
>  /* migration properties */
>  
>  extern Property migration_properties[];
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index bcc39081f2..e649718492 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -130,17 +130,6 @@ void qemu_file_skip(QEMUFile *f, int size);
>   * accounting information tracks the total migration traffic.
>   */
>  void qemu_file_credit_transfer(QEMUFile *f, size_t size);
> -void qemu_file_reset_rate_limit(QEMUFile *f);
> -/*
> - * qemu_file_acct_rate_limit:
> - *
> - * Report on a number of bytes the have been transferred
> - * out of band from the main file object I/O methods, and
> - * need to be applied to the rate limiting calcuations
> - */
> -void qemu_file_acct_rate_limit(QEMUFile *f, uint64_t len);
> -void qemu_file_set_rate_limit(QEMUFile *f, uint64_t new_rate);
> -uint64_t qemu_file_get_rate_limit(QEMUFile *f);
>  int qemu_file_get_error_obj(QEMUFile *f, Error **errp);
>  int qemu_file_get_error_obj_any(QEMUFile *f1, QEMUFile *f2, Error **errp);
>  void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ddc9c7b1a1..1baea16c96 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2166,7 +2166,7 @@ static void htab_save_first_pass(QEMUFile *f, SpaprMachineState *spapr,
>                  break;
>              }
>          }
> -    } while ((index < htabslots) && !qemu_file_rate_limit(f));
> +    } while ((index < htabslots) && !migration_rate_exceeded(f));
>  
>      if (index >= htabslots) {
>          assert(index == htabslots);
> @@ -2237,7 +2237,7 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
>              assert(index == htabslots);
>              index = 0;
>          }
> -    } while ((examined < htabslots) && (!qemu_file_rate_limit(f) || final));
> +    } while ((examined < htabslots) && (!migration_rate_exceeded(f) || final));
>  
>      if (index >= htabslots) {
>          assert(index == htabslots);
> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
> index aed919ad7d..220e845d12 100644
> --- a/hw/s390x/s390-stattrib.c
> +++ b/hw/s390x/s390-stattrib.c
> @@ -209,7 +209,7 @@ static int cmma_save(QEMUFile *f, void *opaque, int final)
>          return -ENOMEM;
>      }
>  
> -    while (final ? 1 : qemu_file_rate_limit(f) == 0) {
> +    while (final ? 1 : migration_rate_exceeded(f) == 0) {
>          reallen = sac->get_stattr(sas, &start_gfn, buflen, buf);
>          if (reallen < 0) {
>              g_free(buf);
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 20f36e6bd8..032fc5f405 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -706,7 +706,7 @@ static void bulk_phase(QEMUFile *f, DBMSaveState *s, bool limit)
>      QSIMPLEQ_FOREACH(dbms, &s->dbms_list, entry) {
>          while (!dbms->bulk_completed) {
>              bulk_phase_send_chunk(f, s, dbms);
> -            if (limit && qemu_file_rate_limit(f)) {
> +            if (limit && migration_rate_exceeded(f)) {
>                  return;
>              }
>          }
> diff --git a/migration/block.c b/migration/block.c
> index 12617b4152..b9580a6c7e 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -23,6 +23,7 @@
>  #include "block/dirty-bitmap.h"
>  #include "migration/misc.h"
>  #include "migration.h"
> +#include "migration-stats.h"
>  #include "migration/register.h"
>  #include "qemu-file.h"
>  #include "migration/vmstate.h"
> @@ -625,7 +626,7 @@ static int flush_blks(QEMUFile *f)
>  
>      blk_mig_lock();
>      while ((blk = QSIMPLEQ_FIRST(&block_mig_state.blk_list)) != NULL) {
> -        if (qemu_file_rate_limit(f)) {
> +        if (migration_rate_exceeded(f)) {
>              break;
>          }
>          if (blk->ret < 0) {
> @@ -762,7 +763,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>      /* control the rate of transfer */
>      blk_mig_lock();
>      while (block_mig_state.read_done * BLK_MIG_BLOCK_SIZE <
> -           qemu_file_get_rate_limit(f) &&
> +           migration_rate_get() &&
>             block_mig_state.submitted < MAX_PARALLEL_IO &&
>             (block_mig_state.submitted + block_mig_state.read_done) <
>             MAX_IO_BUFFERS) {
> 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
> @@ -13,6 +13,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/stats64.h"
>  #include "qemu/timer.h"
> +#include "qemu-file.h"
>  #include "migration-stats.h"
>  
>  MigrationAtomicStats mig_stats;
> @@ -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)

Really liked this name, and how it fits the usage :)

> +{
> +    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;
> +}
> +
> +uint64_t migration_rate_get(void)
> +{
> +    return stat64_get(&mig_stats.rate_limit_max);
> +}
> +
> +#define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
> +
> +void migration_rate_set(uint64_t limit)
> +{
> +    /*
> +     * 'limit' is per second.  But we check it each BUFER_DELAY miliseconds.
> +     */
> +    stat64_set(&mig_stats.rate_limit_max, limit / XFER_LIMIT_RATIO);
> +}
> +
> +void migration_rate_reset(void)
> +{
> +    stat64_set(&mig_stats.rate_limit_used, 0);
> +}
> +
> +void migration_rate_account(uint64_t len)
> +{
> +    stat64_add(&mig_stats.rate_limit_used, len);
> +}
> diff --git a/migration/migration.c b/migration/migration.c
> index e9466273bb..594709dbbc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2119,7 +2119,7 @@ static int postcopy_start(MigrationState *ms)
>       * will notice we're in POSTCOPY_ACTIVE and not actually
>       * wrap their state up here
>       */
> -    qemu_file_set_rate_limit(ms->to_dst_file, bandwidth);
> +    migration_rate_set(bandwidth);
>      if (migrate_postcopy_ram()) {
>          /* Ping just for debugging, helps line traces up */
>          qemu_savevm_send_ping(ms->to_dst_file, 2);
> @@ -2303,7 +2303,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, RATE_LIMIT_MAX);
> +                migration_rate_set(RATE_LIMIT_MAX);
>                  ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
>                                                           s->block_inactive);
>              }
> @@ -2698,7 +2698,7 @@ static void migration_update_counters(MigrationState *s,
>              stat64_get(&mig_stats.dirty_bytes_last_sync) / bandwidth;
>      }
>  
> -    qemu_file_reset_rate_limit(s->to_dst_file);
> +    migration_rate_reset();
>  
>      update_iteration_initial_status(s);
>  
> @@ -2851,7 +2851,7 @@ bool migration_rate_limit(void)
>  
>      bool urgent = false;
>      migration_update_counters(s, now);
> -    if (qemu_file_rate_limit(s->to_dst_file)) {
> +    if (migration_rate_exceeded(s->to_dst_file)) {
>  
>          if (qemu_file_get_error(s->to_dst_file)) {
>              return false;
> @@ -2973,7 +2973,7 @@ static void *migration_thread(void *opaque)
>      trace_migration_thread_setup_complete();
>  
>      while (migration_is_active(s)) {
> -        if (urgent || !qemu_file_rate_limit(s->to_dst_file)) {
> +        if (urgent || !migration_rate_exceeded(s->to_dst_file)) {
>              MigIterateState iter_state = migration_iteration_run(s);
>              if (iter_state == MIG_ITERATE_SKIP) {
>                  continue;
> @@ -3047,7 +3047,7 @@ static void *bg_migration_thread(void *opaque)
>      rcu_register_thread();
>      object_ref(OBJECT(s));
>  
> -    qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX);
> +    migration_rate_set(RATE_LIMIT_MAX);
>  
>      setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
>      /*
> @@ -3219,7 +3219,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>          notifier_list_notify(&migration_state_notifiers, s);
>      }
>  
> -    qemu_file_set_rate_limit(s->to_dst_file, rate_limit);
> +    migration_rate_set(rate_limit);
>      qemu_file_set_blocking(s->to_dst_file, true);
>  
>      /*
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 5c4298eadf..5052091ce2 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -431,7 +431,7 @@ static int multifd_send_pages(QEMUFile *f)
>      multifd_send_state->pages = p->pages;
>      p->pages = pages;
>      transferred = ((uint64_t) pages->num) * p->page_size + p->packet_len;
> -    qemu_file_acct_rate_limit(f, transferred);
> +    migration_rate_account(transferred);
>      qemu_mutex_unlock(&p->mutex);
>      stat64_add(&mig_stats.transferred, transferred);
>      stat64_add(&mig_stats.multifd_bytes, transferred);
> diff --git a/migration/options.c b/migration/options.c
> index c2a278ee2d..b62ab30cd5 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -23,6 +23,7 @@
>  #include "migration/colo.h"
>  #include "migration/misc.h"
>  #include "migration.h"
> +#include "migration-stats.h"
>  #include "qemu-file.h"
>  #include "ram.h"
>  #include "options.h"
> @@ -1242,8 +1243,7 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>      if (params->has_max_bandwidth) {
>          s->parameters.max_bandwidth = params->max_bandwidth;
>          if (s->to_dst_file && !migration_in_postcopy()) {
> -            qemu_file_set_rate_limit(s->to_dst_file,
> -                                s->parameters.max_bandwidth);
> +            migration_rate_set(s->parameters.max_bandwidth);
>          }
>      }
>  
> @@ -1272,8 +1272,7 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>      if (params->has_max_postcopy_bandwidth) {
>          s->parameters.max_postcopy_bandwidth = params->max_postcopy_bandwidth;
>          if (s->to_dst_file && migration_in_postcopy()) {
> -            qemu_file_set_rate_limit(s->to_dst_file,
> -                    s->parameters.max_postcopy_bandwidth);
> +            migration_rate_set(s->parameters.max_postcopy_bandwidth);
>          }
>      }
>      if (params->has_max_cpu_throttle) {
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 956bd2a580..9c67b52fe0 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -41,17 +41,6 @@ struct QEMUFile {
>      QIOChannel *ioc;
>      bool is_writable;
>  
> -    /*
> -     * Maximum amount of data in bytes to transfer during one
> -     * rate limiting time window
> -     */
> -    uint64_t rate_limit_max;
> -    /*
> -     * Total amount of data in bytes queued for transfer
> -     * during this rate limiting time window
> -     */
> -    uint64_t rate_limit_used;
> -
>      /* The sum of bytes transferred on the wire */
>      uint64_t total_transferred;
>  
> @@ -303,7 +292,7 @@ void qemu_fflush(QEMUFile *f)
>              qemu_file_set_error_obj(f, -EIO, local_error);
>          } else {
>              uint64_t size = iov_size(f->iov, f->iovcnt);
> -            qemu_file_acct_rate_limit(f, size);
> +            migration_rate_account(size);
>              f->total_transferred += size;
>          }
>  
> @@ -356,7 +345,7 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
>          int ret = f->hooks->save_page(f, block_offset,
>                                        offset, size, bytes_sent);
>          if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
> -            qemu_file_acct_rate_limit(f, size);
> +            migration_rate_account(size);
>          }
>  
>          if (ret != RAM_SAVE_CONTROL_DELAYED &&
> @@ -727,43 +716,6 @@ uint64_t qemu_file_transferred(QEMUFile *f)
>      return f->total_transferred;
>  }
>  
> -int qemu_file_rate_limit(QEMUFile *f)
> -{
> -    if (qemu_file_get_error(f)) {
> -        return 1;
> -    }
> -    if (f->rate_limit_max == RATE_LIMIT_MAX) {
> -        return 0;
> -    }
> -    if (f->rate_limit_used > f->rate_limit_max) {
> -        return 1;
> -    }
> -    return 0;
> -}
> -
> -uint64_t qemu_file_get_rate_limit(QEMUFile *f)
> -{
> -    return f->rate_limit_max;
> -}
> -
> -void qemu_file_set_rate_limit(QEMUFile *f, uint64_t limit)
> -{
> -    /*
> -     * 'limit' is per second.  But we check it each 100 miliseconds.
> -     */
> -    f->rate_limit_max = limit / XFER_LIMIT_RATIO;
> -}
> -
> -void qemu_file_reset_rate_limit(QEMUFile *f)
> -{
> -    f->rate_limit_used = 0;
> -}
> -
> -void qemu_file_acct_rate_limit(QEMUFile *f, uint64_t len)
> -{
> -    f->rate_limit_used += len;
> -}
> -
>  void qemu_put_be16(QEMUFile *f, unsigned int v)
>  {
>      qemu_put_byte(f, v >> 8);
> diff --git a/migration/ram.c b/migration/ram.c
> index fd5a8db0f8..a706edecc0 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3126,7 +3126,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>  
>          t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>          i = 0;
> -        while ((ret = qemu_file_rate_limit(f)) == 0 ||
> +        while ((ret = migration_rate_exceeded(f)) == 0 ||

migration_rate_exceeded() now returns bool, so it feels weird to compare it to
zero instead of false. 

Also, I noticed that after this ret is used in:
[...]
    if (ret >= 0
        && migration_is_setup_or_active(migrate_get_current()->state)) {
[...]

IIRC bool promotion to int will always be 0 or 1, and (ret >= 0) test will be
always true. Also, qemu_file_rate_limit() only returns 0 or 1, so either this
test is unnecessary, or this could be a bug, and (ret == 0) is the correct test.

If the test is unnecessary, it would be removed and we could have:

-        while ((ret = qemu_file_rate_limit(f)) == 0 ||
+        while (!migration_rate_exceeded(f) ||

(ret could be local to if (ret >= 0 ...) now)

If this is a bug, then an one-liner 'could' be:

-       while ((ret = qemu_file_rate_limit(f)) == 0 ||
+	while ((ret = migration_rate_exceeded(f) ? -1 : 0) == 0 ||

But I strongly advise on doing this instead:

#####
diff --git a/migration/ram.c b/migration/ram.c
index f69d8d42b0..1643dd564c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3084,7 +3084,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
 {
     RAMState **temp = opaque;
     RAMState *rs = *temp;
-    int ret = 0;
+    bool rate_exceeded = false;
     int i;
     int64_t t0;
     int done = 0;
@@ -3116,7 +3116,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
 
         t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
         i = 0;
-        while ((ret = qemu_file_rate_limit(f)) == 0 ||
+        while (!(rate_exceeded = migration_rate_exceeded(f)) ||
                postcopy_has_request(rs)) {
             int pages;
 
@@ -3172,8 +3172,10 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
     ram_control_after_iterate(f, RAM_CONTROL_ROUND);
 
 out:
-    if (ret >= 0
+    if (!rate_exceeded
         && migration_is_setup_or_active(migrate_get_current()->state)) {
+        int ret;
+
         if (migrate_multifd_flush_after_each_section()) {
             ret = multifd_send_sync_main(rs-
>pss[RAM_CHANNEL_PRECOPY].pss_channel);
             if (ret < 0) {
@@ -3186,9 +3188,10 @@ out:
         ram_transferred_add(8);
 
         ret = qemu_file_get_error(f);
-    }
-    if (ret < 0) {
-        return ret;
+
+        if (ret < 0) {
+            return ret;
+        }
     }
 
     return done;

#####

Notice I moved the if(ret < 0) inside the if (rate_exceeded >= 0 ...), reason
being neither qemu_file_rate_limit() nor (int) rate_exceeded will ever be < 0.


>                 postcopy_has_request(rs)) {
>              int pages;
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index c7af9050c2..6b783d9c39 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1345,7 +1345,7 @@ int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy)
>              !(se->ops->has_postcopy && se->ops->has_postcopy(se->opaque))) {
>              continue;
>          }
> -        if (qemu_file_rate_limit(f)) {
> +        if (migration_rate_exceeded(f)) {
>              return 0;
>          }
>          trace_savevm_section_start(se->idstr, se->section_id);

Other than that, it looks fine :)

Thanks!

[PATCH v2 06/16] migration: Move migration_total_bytes() to migration-stats.c

[Juan Quintela]

Once there rename it to migration_transferred_bytes() and pass a
QEMUFile instead of a migration object.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
---
 migration/migration-stats.h | 11 +++++++++++
 migration/migration-stats.c |  6 ++++++
 migration/migration.c       | 13 +++----------
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index e39c083245..91fda378d3 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -147,4 +147,15 @@ void migration_rate_reset(void);
  * @new_rate: new maximum amount
  */
 void migration_rate_set(uint64_t new_rate);
+
+/**
+ * migration_transferred_bytes: Return number of bytes transferred
+ *
+ * @f: QEMUFile used for main migration channel
+ *
+ * Returns how many bytes have we transferred since the beginning of
+ * the migration.  It accounts for bytes sent through any migration
+ * channel, multifd, qemu_file, rdma, ....
+ */
+uint64_t migration_transferred_bytes(QEMUFile *f);
 #endif
diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index 1b16edae7d..9bd97caa23 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -66,3 +66,9 @@ void migration_rate_account(uint64_t len)
 {
     stat64_add(&mig_stats.rate_limit_used, len);
 }
+
+uint64_t migration_transferred_bytes(QEMUFile *f)
+{
+    return qemu_file_transferred(f) + stat64_get(&mig_stats.multifd_bytes);
+}
+
diff --git a/migration/migration.c b/migration/migration.c
index 594709dbbc..39ff538046 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2624,16 +2624,9 @@ static MigThrError migration_detect_error(MigrationState *s)
     }
 }
 
-/* How many bytes have we transferred since the beginning of the migration */
-static uint64_t migration_total_bytes(MigrationState *s)
-{
-    return qemu_file_transferred(s->to_dst_file) +
-        stat64_get(&mig_stats.multifd_bytes);
-}
-
 static void migration_calculate_complete(MigrationState *s)
 {
-    uint64_t bytes = migration_total_bytes(s);
+    uint64_t bytes = migration_transferred_bytes(s->to_dst_file);
     int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     int64_t transfer_time;
 
@@ -2659,7 +2652,7 @@ static void update_iteration_initial_status(MigrationState *s)
      * wrong speed calculation.
      */
     s->iteration_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
-    s->iteration_initial_bytes = migration_total_bytes(s);
+    s->iteration_initial_bytes = migration_transferred_bytes(s->to_dst_file);
     s->iteration_initial_pages = ram_get_total_transferred_pages();
 }
 
@@ -2674,7 +2667,7 @@ static void migration_update_counters(MigrationState *s,
         return;
     }
 
-    current_bytes = migration_total_bytes(s);
+    current_bytes = migration_transferred_bytes(s->to_dst_file);
     transferred = current_bytes - s->iteration_initial_bytes;
     time_spent = current_time - s->iteration_start_time;
     bandwidth = (double)transferred / time_spent;
-- 
2.40.1

Re: [PATCH v2 06/16] migration: Move migration_total_bytes() to migration-stats.c

[Leonardo Brás]

On Mon, 2023-05-15 at 21:56 +0200, Juan Quintela wrote:
> Once there rename it to migration_transferred_bytes() and pass a
> QEMUFile instead of a migration object.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
>  migration/migration-stats.h | 11 +++++++++++
>  migration/migration-stats.c |  6 ++++++
>  migration/migration.c       | 13 +++----------
>  3 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index e39c083245..91fda378d3 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -147,4 +147,15 @@ void migration_rate_reset(void);
>   * @new_rate: new maximum amount
>   */
>  void migration_rate_set(uint64_t new_rate);
> +
> +/**
> + * migration_transferred_bytes: Return number of bytes transferred
> + *
> + * @f: QEMUFile used for main migration channel
> + *
> + * Returns how many bytes have we transferred since the beginning of
> + * the migration.  It accounts for bytes sent through any migration
> + * channel, multifd, qemu_file, rdma, ....
> + */
> +uint64_t migration_transferred_bytes(QEMUFile *f);
>  #endif
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index 1b16edae7d..9bd97caa23 100644
> --- a/migration/migration-stats.c
> +++ b/migration/migration-stats.c
> @@ -66,3 +66,9 @@ void migration_rate_account(uint64_t len)
>  {
>      stat64_add(&mig_stats.rate_limit_used, len);
>  }
> +
> +uint64_t migration_transferred_bytes(QEMUFile *f)
> +{
> +    return qemu_file_transferred(f) + stat64_get(&mig_stats.multifd_bytes);
> +}
> +
> diff --git a/migration/migration.c b/migration/migration.c
> index 594709dbbc..39ff538046 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2624,16 +2624,9 @@ static MigThrError migration_detect_error(MigrationState *s)
>      }
>  }
>  
> -/* How many bytes have we transferred since the beginning of the migration */
> -static uint64_t migration_total_bytes(MigrationState *s)
> -{
> -    return qemu_file_transferred(s->to_dst_file) +
> -        stat64_get(&mig_stats.multifd_bytes);
> -}
> -
>  static void migration_calculate_complete(MigrationState *s)
>  {
> -    uint64_t bytes = migration_total_bytes(s);
> +    uint64_t bytes = migration_transferred_bytes(s->to_dst_file);
>      int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>      int64_t transfer_time;
>  
> @@ -2659,7 +2652,7 @@ static void update_iteration_initial_status(MigrationState *s)
>       * wrong speed calculation.
>       */
>      s->iteration_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> -    s->iteration_initial_bytes = migration_total_bytes(s);
> +    s->iteration_initial_bytes = migration_transferred_bytes(s->to_dst_file);
>      s->iteration_initial_pages = ram_get_total_transferred_pages();
>  }
>  
> @@ -2674,7 +2667,7 @@ static void migration_update_counters(MigrationState *s,
>          return;
>      }
>  
> -    current_bytes = migration_total_bytes(s);
> +    current_bytes = migration_transferred_bytes(s->to_dst_file);
>      transferred = current_bytes - s->iteration_initial_bytes;
>      time_spent = current_time - s->iteration_start_time;
>      bandwidth = (double)transferred / time_spent;

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

[PATCH v2 07/16] migration: Add a trace for migration_transferred_bytes

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
---
 migration/migration-stats.c | 8 ++++++--
 migration/trace-events      | 3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index 9bd97caa23..301392d208 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -14,6 +14,7 @@
 #include "qemu/stats64.h"
 #include "qemu/timer.h"
 #include "qemu-file.h"
+#include "trace.h"
 #include "migration-stats.h"
 
 MigrationAtomicStats mig_stats;
@@ -69,6 +70,9 @@ void migration_rate_account(uint64_t len)
 
 uint64_t migration_transferred_bytes(QEMUFile *f)
 {
-    return qemu_file_transferred(f) + stat64_get(&mig_stats.multifd_bytes);
-}
+    uint64_t multifd = stat64_get(&mig_stats.multifd_bytes);
+    uint64_t qemu_file = qemu_file_transferred(f);
 
+    trace_migration_transferred_bytes(qemu_file, multifd);
+    return qemu_file + multifd;
+}
diff --git a/migration/trace-events b/migration/trace-events
index f39818c329..cdaef7a1ea 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -186,6 +186,9 @@ process_incoming_migration_co_end(int ret, int ps) "ret=%d postcopy-state=%d"
 process_incoming_migration_co_postcopy_end_main(void) ""
 postcopy_preempt_enabled(bool value) "%d"
 
+# migration-stats
+migration_transferred_bytes(uint64_t qemu_file, uint64_t multifd) "qemu_file %" PRIu64 " multifd %" PRIu64
+
 # channel.c
 migration_set_incoming_channel(void *ioc, const char *ioctype) "ioc=%p ioctype=%s"
 migration_set_outgoing_channel(void *ioc, const char *ioctype, const char *hostname, void *err)  "ioc=%p ioctype=%s hostname=%s err=%p"
-- 
2.40.1

Re: [PATCH v2 07/16] migration: Add a trace for migration_transferred_bytes

[Leonardo Brás]

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.c | 8 ++++++--
>  migration/trace-events      | 3 +++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index 9bd97caa23..301392d208 100644
> --- a/migration/migration-stats.c
> +++ b/migration/migration-stats.c
> @@ -14,6 +14,7 @@
>  #include "qemu/stats64.h"
>  #include "qemu/timer.h"
>  #include "qemu-file.h"
> +#include "trace.h"
>  #include "migration-stats.h"
>  
>  MigrationAtomicStats mig_stats;
> @@ -69,6 +70,9 @@ void migration_rate_account(uint64_t len)
>  
>  uint64_t migration_transferred_bytes(QEMUFile *f)
>  {
> -    return qemu_file_transferred(f) + stat64_get(&mig_stats.multifd_bytes);
> -}
> +    uint64_t multifd = stat64_get(&mig_stats.multifd_bytes);
> +    uint64_t qemu_file = qemu_file_transferred(f);
>  
> +    trace_migration_transferred_bytes(qemu_file, multifd);
> +    return qemu_file + multifd;
> +}
> diff --git a/migration/trace-events b/migration/trace-events
> index f39818c329..cdaef7a1ea 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -186,6 +186,9 @@ process_incoming_migration_co_end(int ret, int ps) "ret=%d postcopy-state=%d"
>  process_incoming_migration_co_postcopy_end_main(void) ""
>  postcopy_preempt_enabled(bool value) "%d"
>  
> +# migration-stats
> +migration_transferred_bytes(uint64_t qemu_file, uint64_t multifd) "qemu_file %" PRIu64 " multifd %" PRIu64
> +
>  # channel.c
>  migration_set_incoming_channel(void *ioc, const char *ioctype) "ioc=%p ioctype=%s"
>  migration_set_outgoing_channel(void *ioc, const char *ioctype, const char *hostname, void *err)  "ioc=%p ioctype=%s hostname=%s err=%p"

LGTM

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

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

[Juan Quintela]

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);
 
     if (rate_limit_max == RATE_LIMIT_MAX) {
@@ -58,9 +60,10 @@ void migration_rate_set(uint64_t limit)
     stat64_set(&mig_stats.rate_limit_max, limit / XFER_LIMIT_RATIO);
 }
 
-void migration_rate_reset(void)
+void migration_rate_reset(QEMUFile *f)
 {
     stat64_set(&mig_stats.rate_limit_used, 0);
+    stat64_set(&mig_stats.rate_limit_start, migration_transferred_bytes(f));
 }
 
 void migration_rate_account(uint64_t len)
diff --git a/migration/migration.c b/migration/migration.c
index 39ff538046..e48dd593ed 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2691,7 +2691,7 @@ static void migration_update_counters(MigrationState *s,
             stat64_get(&mig_stats.dirty_bytes_last_sync) / bandwidth;
     }
 
-    migration_rate_reset();
+    migration_rate_reset(s->to_dst_file);
 
     update_iteration_initial_status(s);
 
-- 
2.40.1

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

[Leonardo Brás]

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?

Other than that, 
Reviewed-by: Leonardo Bras <leobras@redhat.com>

>  
>      if (rate_limit_max == RATE_LIMIT_MAX) {
> @@ -58,9 +60,10 @@ void migration_rate_set(uint64_t limit)
>      stat64_set(&mig_stats.rate_limit_max, limit / XFER_LIMIT_RATIO);
>  }
>  
> -void migration_rate_reset(void)
> +void migration_rate_reset(QEMUFile *f)
>  {
>      stat64_set(&mig_stats.rate_limit_used, 0);
> +    stat64_set(&mig_stats.rate_limit_start, migration_transferred_bytes(f));
>  }
>  
>  void migration_rate_account(uint64_t len)
> diff --git a/migration/migration.c b/migration/migration.c
> index 39ff538046..e48dd593ed 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2691,7 +2691,7 @@ static void migration_update_counters(MigrationState *s,
>              stat64_get(&mig_stats.dirty_bytes_last_sync) / bandwidth;
>      }
>  
> -    migration_rate_reset();
> +    migration_rate_reset(s->to_dst_file);
>  
>      update_iteration_initial_status(s);
>  

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.

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

[Leonardo Bras Soares Passos]

On Fri, May 26, 2023 at 5:17 AM Juan Quintela <quintela@redhat.com> wrote:
>
> 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.

My bad.
After I ended up reviewing the patchset I noticed a lot of it was
already in the PULL request.

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

Oh, I see.
You kept both to verify the implementation.
Makes sense

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

Yeah, it makes no sense to invest time on stuff that will be removed later.

Thanks for helping me understand this :)

>
> Regards, Juan.
>

[PATCH v2 09/16] migration: We don't need the field rate_limit_used anymore

[Juan Quintela]

Since previous commit, we calculate how much data we have send with
migration_transferred_bytes() so no need to maintain this counter and
remember to always update it.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
---
 migration/migration-stats.h | 14 --------------
 migration/migration-stats.c |  6 ------
 migration/multifd.c         |  1 -
 migration/qemu-file.c       |  4 ----
 4 files changed, 25 deletions(-)

diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index f1465c2ebe..9568b5b473 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -89,10 +89,6 @@ typedef struct {
      * Maximum amount of data we can send in a cycle.
      */
     Stat64 rate_limit_max;
-    /*
-     * Amount of data we have sent in the current cycle.
-     */
-    Stat64 rate_limit_used;
     /*
      * How long has the setup stage took.
      */
@@ -119,16 +115,6 @@ extern MigrationAtomicStats mig_stats;
  */
 void migration_time_since(MigrationAtomicStats *stats, int64_t since);
 
-/**
- * migration_rate_account: Increase the number of bytes transferred.
- *
- * Report on a number of bytes the have been transferred that need to
- * be applied to the rate limiting calcuations.
- *
- * @len: amount of bytes transferred
- */
-void migration_rate_account(uint64_t len);
-
 /**
  * migration_rate_get: Get the maximum amount that can be transferred.
  *
diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index da2bb69a15..abf2d38b18 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -62,15 +62,9 @@ void migration_rate_set(uint64_t limit)
 
 void migration_rate_reset(QEMUFile *f)
 {
-    stat64_set(&mig_stats.rate_limit_used, 0);
     stat64_set(&mig_stats.rate_limit_start, migration_transferred_bytes(f));
 }
 
-void migration_rate_account(uint64_t len)
-{
-    stat64_add(&mig_stats.rate_limit_used, len);
-}
-
 uint64_t migration_transferred_bytes(QEMUFile *f)
 {
     uint64_t multifd = stat64_get(&mig_stats.multifd_bytes);
diff --git a/migration/multifd.c b/migration/multifd.c
index 5052091ce2..aabf9b6d98 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -431,7 +431,6 @@ static int multifd_send_pages(QEMUFile *f)
     multifd_send_state->pages = p->pages;
     p->pages = pages;
     transferred = ((uint64_t) pages->num) * p->page_size + p->packet_len;
-    migration_rate_account(transferred);
     qemu_mutex_unlock(&p->mutex);
     stat64_add(&mig_stats.transferred, transferred);
     stat64_add(&mig_stats.multifd_bytes, transferred);
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 9c67b52fe0..acc282654a 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -292,7 +292,6 @@ void qemu_fflush(QEMUFile *f)
             qemu_file_set_error_obj(f, -EIO, local_error);
         } else {
             uint64_t size = iov_size(f->iov, f->iovcnt);
-            migration_rate_account(size);
             f->total_transferred += size;
         }
 
@@ -344,9 +343,6 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
     if (f->hooks && f->hooks->save_page) {
         int ret = f->hooks->save_page(f, block_offset,
                                       offset, size, bytes_sent);
-        if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
-            migration_rate_account(size);
-        }
 
         if (ret != RAM_SAVE_CONTROL_DELAYED &&
             ret != RAM_SAVE_CONTROL_NOT_SUPP) {
-- 
2.40.1

Re: [PATCH v2 09/16] migration: We don't need the field rate_limit_used anymore

[Leonardo Brás]

On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> Since previous commit, we calculate how much data we have send with
> migration_transferred_bytes() so no need to maintain this counter and
> remember to always update it.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
>  migration/migration-stats.h | 14 --------------
>  migration/migration-stats.c |  6 ------
>  migration/multifd.c         |  1 -
>  migration/qemu-file.c       |  4 ----
>  4 files changed, 25 deletions(-)
> 
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index f1465c2ebe..9568b5b473 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -89,10 +89,6 @@ typedef struct {
>       * Maximum amount of data we can send in a cycle.
>       */
>      Stat64 rate_limit_max;
> -    /*
> -     * Amount of data we have sent in the current cycle.
> -     */
> -    Stat64 rate_limit_used;
>      /*
>       * How long has the setup stage took.
>       */
> @@ -119,16 +115,6 @@ extern MigrationAtomicStats mig_stats;
>   */
>  void migration_time_since(MigrationAtomicStats *stats, int64_t since);
>  
> -/**
> - * migration_rate_account: Increase the number of bytes transferred.
> - *
> - * Report on a number of bytes the have been transferred that need to
> - * be applied to the rate limiting calcuations.
> - *
> - * @len: amount of bytes transferred
> - */
> -void migration_rate_account(uint64_t len);
> -
>  /**
>   * migration_rate_get: Get the maximum amount that can be transferred.
>   *
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index da2bb69a15..abf2d38b18 100644
> --- a/migration/migration-stats.c
> +++ b/migration/migration-stats.c
> @@ -62,15 +62,9 @@ void migration_rate_set(uint64_t limit)
>  
>  void migration_rate_reset(QEMUFile *f)
>  {
> -    stat64_set(&mig_stats.rate_limit_used, 0);
>      stat64_set(&mig_stats.rate_limit_start, migration_transferred_bytes(f));
>  }
>  
> -void migration_rate_account(uint64_t len)
> -{
> -    stat64_add(&mig_stats.rate_limit_used, len);
> -}
> -
>  uint64_t migration_transferred_bytes(QEMUFile *f)
>  {
>      uint64_t multifd = stat64_get(&mig_stats.multifd_bytes);
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 5052091ce2..aabf9b6d98 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -431,7 +431,6 @@ static int multifd_send_pages(QEMUFile *f)
>      multifd_send_state->pages = p->pages;
>      p->pages = pages;
>      transferred = ((uint64_t) pages->num) * p->page_size + p->packet_len;
> -    migration_rate_account(transferred);
>      qemu_mutex_unlock(&p->mutex);
>      stat64_add(&mig_stats.transferred, transferred);
>      stat64_add(&mig_stats.multifd_bytes, transferred);
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 9c67b52fe0..acc282654a 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -292,7 +292,6 @@ void qemu_fflush(QEMUFile *f)
>              qemu_file_set_error_obj(f, -EIO, local_error);
>          } else {
>              uint64_t size = iov_size(f->iov, f->iovcnt);
> -            migration_rate_account(size);
>              f->total_transferred += size;
>          }
>  
> @@ -344,9 +343,6 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
>      if (f->hooks && f->hooks->save_page) {
>          int ret = f->hooks->save_page(f, block_offset,
>                                        offset, size, bytes_sent);
> -        if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
> -            migration_rate_account(size);
> -        }
>  
>          if (ret != RAM_SAVE_CONTROL_DELAYED &&
>              ret != RAM_SAVE_CONTROL_NOT_SUPP) {

I reviewed this one together with 8/16.
It makes sense for me to squash them together, but anyway:

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

Re: [PATCH v2 09/16] migration: We don't need the field rate_limit_used anymore

[Juan Quintela]

Leonardo Brás <leobras@redhat.com> wrote:
> On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
>> Since previous commit, we calculate how much data we have send with
>> migration_transferred_bytes() so no need to maintain this counter and
>> remember to always update it.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>

> I reviewed this one together with 8/16.
> It makes sense for me to squash them together, but anyway:

Already in tree.

See explanation for the split on previous patch.

The other reason for the split is that this part of the patch is trivial
to review O:-)

Later, Juan.

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

Re: [PATCH v2 09/16] migration: We don't need the field rate_limit_used anymore

[Leonardo Bras Soares Passos]

On Fri, May 26, 2023 at 5:18 AM Juan Quintela <quintela@redhat.com> wrote:
>
> Leonardo Brás <leobras@redhat.com> wrote:
> > On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> >> Since previous commit, we calculate how much data we have send with
> >> migration_transferred_bytes() so no need to maintain this counter and
> >> remember to always update it.
> >>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>
> > I reviewed this one together with 8/16.
> > It makes sense for me to squash them together, but anyway:
>
> Already in tree.
>
> See explanation for the split on previous patch.
>
> The other reason for the split is that this part of the patch is trivial
> to review O:-)

:-)

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

[PATCH v2 10/16] migration: Don't abuse qemu_file transferred for RDMA

[Juan Quintela]

Just create a variable for it, the same way that multifd does.  This
way it is safe to use for other thread, etc, etc.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-stats.h |  4 ++++
 migration/migration-stats.c |  5 +++--
 migration/rdma.c            | 22 ++++++++++++++++++++--
 migration/trace-events      |  2 +-
 4 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index 9568b5b473..2e3e894307 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -89,6 +89,10 @@ typedef struct {
      * Maximum amount of data we can send in a cycle.
      */
     Stat64 rate_limit_max;
+    /*
+     * Number of bytes sent through RDMA.
+     */
+    Stat64 rdma_bytes;
     /*
      * How long has the setup stage took.
      */
diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index abf2d38b18..4d8e9f93b7 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -68,8 +68,9 @@ void migration_rate_reset(QEMUFile *f)
 uint64_t migration_transferred_bytes(QEMUFile *f)
 {
     uint64_t multifd = stat64_get(&mig_stats.multifd_bytes);
+    uint64_t rdma = stat64_get(&mig_stats.rdma_bytes);
     uint64_t qemu_file = qemu_file_transferred(f);
 
-    trace_migration_transferred_bytes(qemu_file, multifd);
-    return qemu_file + multifd;
+    trace_migration_transferred_bytes(qemu_file, multifd, rdma);
+    return qemu_file + multifd + rdma;
 }
diff --git a/migration/rdma.c b/migration/rdma.c
index 2e4dcff1c9..074456f9df 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -2122,9 +2122,18 @@ retry:
                     return -EIO;
                 }
 
+                /*
+                 * TODO: Here we are sending something, but we are not
+                 * accounting for anything transferred.  The following is wrong:
+                 *
+                 * stat64_add(&mig_stats.rdma_bytes, sge.length);
+                 *
+                 * because we are using some kind of compression.  I
+                 * would think that head.len would be the more similar
+                 * thing to a correct value.
+                 */
                 stat64_add(&mig_stats.zero_pages,
                            sge.length / qemu_target_page_size());
-
                 return 1;
             }
 
@@ -2232,8 +2241,17 @@ retry:
 
     set_bit(chunk, block->transit_bitmap);
     stat64_add(&mig_stats.normal_pages, sge.length / qemu_target_page_size());
+    /*
+     * We are adding to transferred the amount of data written, but no
+     * overhead at all.  I will asume that RDMA is magicaly and don't
+     * need to transfer (at least) the addresses where it wants to
+     * write the pages.  Here it looks like it should be something
+     * like:
+     *     sizeof(send_wr) + sge.length
+     * but this being RDMA, who knows.
+     */
+    stat64_add(&mig_stats.rdma_bytes, sge.length);
     ram_transferred_add(sge.length);
-    qemu_file_credit_transfer(f, sge.length);
     rdma->total_writes++;
 
     return 0;
diff --git a/migration/trace-events b/migration/trace-events
index cdaef7a1ea..54ae5653fd 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -187,7 +187,7 @@ process_incoming_migration_co_postcopy_end_main(void) ""
 postcopy_preempt_enabled(bool value) "%d"
 
 # migration-stats
-migration_transferred_bytes(uint64_t qemu_file, uint64_t multifd) "qemu_file %" PRIu64 " multifd %" PRIu64
+migration_transferred_bytes(uint64_t qemu_file, uint64_t multifd, uint64_t rdma) "qemu_file %" PRIu64 " multifd %" PRIu64 " RDMA %" PRIu64
 
 # channel.c
 migration_set_incoming_channel(void *ioc, const char *ioctype) "ioc=%p ioctype=%s"
-- 
2.40.1

Re: [PATCH v2 10/16] migration: Don't abuse qemu_file transferred for RDMA

[Leonardo Brás]

On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> Just create a variable for it, the same way that multifd does.  This
> way it is safe to use for other thread, etc, etc.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration-stats.h |  4 ++++
>  migration/migration-stats.c |  5 +++--
>  migration/rdma.c            | 22 ++++++++++++++++++++--
>  migration/trace-events      |  2 +-
>  4 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index 9568b5b473..2e3e894307 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -89,6 +89,10 @@ typedef struct {
>       * Maximum amount of data we can send in a cycle.
>       */
>      Stat64 rate_limit_max;
> +    /*
> +     * Number of bytes sent through RDMA.
> +     */
> +    Stat64 rdma_bytes;
>      /*
>       * How long has the setup stage took.
>       */
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index abf2d38b18..4d8e9f93b7 100644
> --- a/migration/migration-stats.c
> +++ b/migration/migration-stats.c
> @@ -68,8 +68,9 @@ void migration_rate_reset(QEMUFile *f)
>  uint64_t migration_transferred_bytes(QEMUFile *f)
>  {
>      uint64_t multifd = stat64_get(&mig_stats.multifd_bytes);
> +    uint64_t rdma = stat64_get(&mig_stats.rdma_bytes);
>      uint64_t qemu_file = qemu_file_transferred(f);
>  
> -    trace_migration_transferred_bytes(qemu_file, multifd);
> -    return qemu_file + multifd;
> +    trace_migration_transferred_bytes(qemu_file, multifd, rdma);
> +    return qemu_file + multifd + rdma;
>  }
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 2e4dcff1c9..074456f9df 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2122,9 +2122,18 @@ retry:
>                      return -EIO;
>                  }
>  
> +                /*
> +                 * TODO: Here we are sending something, but we are not
> +                 * accounting for anything transferred.  The following is wrong:
> +                 *
> +                 * stat64_add(&mig_stats.rdma_bytes, sge.length);
> +                 *
> +                 * because we are using some kind of compression.  I
> +                 * would think that head.len would be the more similar
> +                 * thing to a correct value.
> +                 */
>                  stat64_add(&mig_stats.zero_pages,
>                             sge.length / qemu_target_page_size());
> -
>                  return 1;
>              }
>  
> @@ -2232,8 +2241,17 @@ retry:
>  
>      set_bit(chunk, block->transit_bitmap);
>      stat64_add(&mig_stats.normal_pages, sge.length / qemu_target_page_size());
> +    /*
> +     * We are adding to transferred the amount of data written, but no
> +     * overhead at all.  I will asume that RDMA is magicaly and don't
> +     * need to transfer (at least) the addresses where it wants to
> +     * write the pages.  Here it looks like it should be something
> +     * like:
> +     *     sizeof(send_wr) + sge.length
> +     * but this being RDMA, who knows.
> +     */
> +    stat64_add(&mig_stats.rdma_bytes, sge.length);
>      ram_transferred_add(sge.length);
> -    qemu_file_credit_transfer(f, sge.length);
>      rdma->total_writes++;
>  
>      return 0;
> diff --git a/migration/trace-events b/migration/trace-events
> index cdaef7a1ea..54ae5653fd 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -187,7 +187,7 @@ process_incoming_migration_co_postcopy_end_main(void) ""
>  postcopy_preempt_enabled(bool value) "%d"
>  
>  # migration-stats
> -migration_transferred_bytes(uint64_t qemu_file, uint64_t multifd) "qemu_file %" PRIu64 " multifd %" PRIu64
> +migration_transferred_bytes(uint64_t qemu_file, uint64_t multifd, uint64_t rdma) "qemu_file %" PRIu64 " multifd %" PRIu64 " RDMA %" PRIu64
>  
>  # channel.c
>  migration_set_incoming_channel(void *ioc, const char *ioctype) "ioc=%p ioctype=%s"


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

[PATCH v2 11/16] migration/RDMA: It is accounting for zero/normal pages in two places

[Juan Quintela]

Remove the one in control_save_page().

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index a706edecc0..67ed49b387 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1191,13 +1191,6 @@ static bool control_save_page(PageSearchStatus *pss, RAMBlock *block,
     if (ret == RAM_SAVE_CONTROL_DELAYED) {
         return true;
     }
-
-    if (bytes_xmit > 0) {
-        stat64_add(&mig_stats.normal_pages, 1);
-    } else if (bytes_xmit == 0) {
-        stat64_add(&mig_stats.zero_pages, 1);
-    }
-
     return true;
 }
 
-- 
2.40.1

Re: [PATCH v2 11/16] migration/RDMA: It is accounting for zero/normal pages in two places

[Leonardo Brás]

On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> Remove the one in control_save_page().
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/ram.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index a706edecc0..67ed49b387 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1191,13 +1191,6 @@ static bool control_save_page(PageSearchStatus *pss, RAMBlock *block,
>      if (ret == RAM_SAVE_CONTROL_DELAYED) {
>          return true;
>      }
> -
> -    if (bytes_xmit > 0) {
> -        stat64_add(&mig_stats.normal_pages, 1);
> -    } else if (bytes_xmit == 0) {
> -        stat64_add(&mig_stats.zero_pages, 1);
> -    }
> -
>      return true;
>  }
>  


ram_save_target_page_legacy() calls both control_save_page() and
save_zero_page() which increment zero-pages.

It also calls ram_save_page() which increments normal-pages.

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

[PATCH v2 12/16] migration/rdma: Remove QEMUFile parameter when not used

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/rdma.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 074456f9df..416dec00a2 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -2027,7 +2027,7 @@ static int qemu_rdma_exchange_recv(RDMAContext *rdma, RDMAControlHeader *head,
  * If we're using dynamic registration on the dest-side, we have to
  * send a registration command first.
  */
-static int qemu_rdma_write_one(QEMUFile *f, RDMAContext *rdma,
+static int qemu_rdma_write_one(RDMAContext *rdma,
                                int current_index, uint64_t current_addr,
                                uint64_t length)
 {
@@ -2263,7 +2263,7 @@ retry:
  * We support sending out multiple chunks at the same time.
  * Not all of them need to get signaled in the completion queue.
  */
-static int qemu_rdma_write_flush(QEMUFile *f, RDMAContext *rdma)
+static int qemu_rdma_write_flush(RDMAContext *rdma)
 {
     int ret;
 
@@ -2271,7 +2271,7 @@ static int qemu_rdma_write_flush(QEMUFile *f, RDMAContext *rdma)
         return 0;
     }
 
-    ret = qemu_rdma_write_one(f, rdma,
+    ret = qemu_rdma_write_one(rdma,
             rdma->current_index, rdma->current_addr, rdma->current_length);
 
     if (ret < 0) {
@@ -2344,7 +2344,7 @@ static inline int qemu_rdma_buffer_mergable(RDMAContext *rdma,
  *    and only require that a batch gets acknowledged in the completion
  *    queue instead of each individual chunk.
  */
-static int qemu_rdma_write(QEMUFile *f, RDMAContext *rdma,
+static int qemu_rdma_write(RDMAContext *rdma,
                            uint64_t block_offset, uint64_t offset,
                            uint64_t len)
 {
@@ -2355,7 +2355,7 @@ static int qemu_rdma_write(QEMUFile *f, RDMAContext *rdma,
 
     /* If we cannot merge it, we flush the current buffer first. */
     if (!qemu_rdma_buffer_mergable(rdma, current_addr, len)) {
-        ret = qemu_rdma_write_flush(f, rdma);
+        ret = qemu_rdma_write_flush(rdma);
         if (ret) {
             return ret;
         }
@@ -2377,7 +2377,7 @@ static int qemu_rdma_write(QEMUFile *f, RDMAContext *rdma,
 
     /* flush it if buffer is too large */
     if (rdma->current_length >= RDMA_MERGE_MAX) {
-        return qemu_rdma_write_flush(f, rdma);
+        return qemu_rdma_write_flush(rdma);
     }
 
     return 0;
@@ -2798,7 +2798,6 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
                                        Error **errp)
 {
     QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(ioc);
-    QEMUFile *f = rioc->file;
     RDMAContext *rdma;
     int ret;
     ssize_t done = 0;
@@ -2819,7 +2818,7 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
      * Push out any writes that
      * we're queued up for VM's ram.
      */
-    ret = qemu_rdma_write_flush(f, rdma);
+    ret = qemu_rdma_write_flush(rdma);
     if (ret < 0) {
         rdma->error_state = ret;
         error_setg(errp, "qemu_rdma_write_flush returned %d", ret);
@@ -2958,11 +2957,11 @@ static ssize_t qio_channel_rdma_readv(QIOChannel *ioc,
 /*
  * Block until all the outstanding chunks have been delivered by the hardware.
  */
-static int qemu_rdma_drain_cq(QEMUFile *f, RDMAContext *rdma)
+static int qemu_rdma_drain_cq(RDMAContext *rdma)
 {
     int ret;
 
-    if (qemu_rdma_write_flush(f, rdma) < 0) {
+    if (qemu_rdma_write_flush(rdma) < 0) {
         return -EIO;
     }
 
@@ -3272,7 +3271,7 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
      * is full, or the page doesn't belong to the current chunk,
      * an actual RDMA write will occur and a new chunk will be formed.
      */
-    ret = qemu_rdma_write(f, rdma, block_offset, offset, size);
+    ret = qemu_rdma_write(rdma, block_offset, offset, size);
     if (ret < 0) {
         error_report("rdma migration: write error! %d", ret);
         goto err;
@@ -3927,7 +3926,7 @@ static int qemu_rdma_registration_stop(QEMUFile *f,
     CHECK_ERROR_STATE();
 
     qemu_fflush(f);
-    ret = qemu_rdma_drain_cq(f, rdma);
+    ret = qemu_rdma_drain_cq(rdma);
 
     if (ret < 0) {
         goto err;
-- 
2.40.1

Re: [PATCH v2 12/16] migration/rdma: Remove QEMUFile parameter when not used

[Leonardo Brás]

On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/rdma.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 074456f9df..416dec00a2 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2027,7 +2027,7 @@ static int qemu_rdma_exchange_recv(RDMAContext *rdma, RDMAControlHeader *head,
>   * If we're using dynamic registration on the dest-side, we have to
>   * send a registration command first.
>   */
> -static int qemu_rdma_write_one(QEMUFile *f, RDMAContext *rdma,
> +static int qemu_rdma_write_one(RDMAContext *rdma,
>                                 int current_index, uint64_t current_addr,
>                                 uint64_t length)
>  {
> @@ -2263,7 +2263,7 @@ retry:
>   * We support sending out multiple chunks at the same time.
>   * Not all of them need to get signaled in the completion queue.
>   */
> -static int qemu_rdma_write_flush(QEMUFile *f, RDMAContext *rdma)
> +static int qemu_rdma_write_flush(RDMAContext *rdma)
>  {
>      int ret;
>  
> @@ -2271,7 +2271,7 @@ static int qemu_rdma_write_flush(QEMUFile *f, RDMAContext *rdma)
>          return 0;
>      }
>  
> -    ret = qemu_rdma_write_one(f, rdma,
> +    ret = qemu_rdma_write_one(rdma,
>              rdma->current_index, rdma->current_addr, rdma->current_length);
>  
>      if (ret < 0) {
> @@ -2344,7 +2344,7 @@ static inline int qemu_rdma_buffer_mergable(RDMAContext *rdma,
>   *    and only require that a batch gets acknowledged in the completion
>   *    queue instead of each individual chunk.
>   */
> -static int qemu_rdma_write(QEMUFile *f, RDMAContext *rdma,
> +static int qemu_rdma_write(RDMAContext *rdma,
>                             uint64_t block_offset, uint64_t offset,
>                             uint64_t len)
>  {
> @@ -2355,7 +2355,7 @@ static int qemu_rdma_write(QEMUFile *f, RDMAContext *rdma,
>  
>      /* If we cannot merge it, we flush the current buffer first. */
>      if (!qemu_rdma_buffer_mergable(rdma, current_addr, len)) {
> -        ret = qemu_rdma_write_flush(f, rdma);
> +        ret = qemu_rdma_write_flush(rdma);
>          if (ret) {
>              return ret;
>          }
> @@ -2377,7 +2377,7 @@ static int qemu_rdma_write(QEMUFile *f, RDMAContext *rdma,
>  
>      /* flush it if buffer is too large */
>      if (rdma->current_length >= RDMA_MERGE_MAX) {
> -        return qemu_rdma_write_flush(f, rdma);
> +        return qemu_rdma_write_flush(rdma);
>      }
>  
>      return 0;
> @@ -2798,7 +2798,6 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
>                                         Error **errp)
>  {
>      QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(ioc);
> -    QEMUFile *f = rioc->file;
>      RDMAContext *rdma;
>      int ret;
>      ssize_t done = 0;
> @@ -2819,7 +2818,7 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
>       * Push out any writes that
>       * we're queued up for VM's ram.
>       */
> -    ret = qemu_rdma_write_flush(f, rdma);
> +    ret = qemu_rdma_write_flush(rdma);
>      if (ret < 0) {
>          rdma->error_state = ret;
>          error_setg(errp, "qemu_rdma_write_flush returned %d", ret);
> @@ -2958,11 +2957,11 @@ static ssize_t qio_channel_rdma_readv(QIOChannel *ioc,
>  /*
>   * Block until all the outstanding chunks have been delivered by the hardware.
>   */
> -static int qemu_rdma_drain_cq(QEMUFile *f, RDMAContext *rdma)
> +static int qemu_rdma_drain_cq(RDMAContext *rdma)
>  {
>      int ret;
>  
> -    if (qemu_rdma_write_flush(f, rdma) < 0) {
> +    if (qemu_rdma_write_flush(rdma) < 0) {
>          return -EIO;
>      }
>  
> @@ -3272,7 +3271,7 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
>       * is full, or the page doesn't belong to the current chunk,
>       * an actual RDMA write will occur and a new chunk will be formed.
>       */
> -    ret = qemu_rdma_write(f, rdma, block_offset, offset, size);
> +    ret = qemu_rdma_write(rdma, block_offset, offset, size);
>      if (ret < 0) {
>          error_report("rdma migration: write error! %d", ret);
>          goto err;
> @@ -3927,7 +3926,7 @@ static int qemu_rdma_registration_stop(QEMUFile *f,
>      CHECK_ERROR_STATE();
>  
>      qemu_fflush(f);
> -    ret = qemu_rdma_drain_cq(f, rdma);
> +    ret = qemu_rdma_drain_cq(rdma);
>  
>      if (ret < 0) {
>          goto err;

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

[PATCH v2 13/16] migration/rdma: Don't use imaginary transfers

[Juan Quintela]

RDMA protocol is completely asynchronous, so in qemu_rdma_save_page()
they "invent" that a byte has been transferred.  And then they call
qemu_file_credit_transfer() and ram_transferred_add() with that byte.
Just remove that calls as nothing has been sent.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.c | 5 +----
 migration/ram.c       | 1 -
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index acc282654a..23a21e2331 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -346,13 +346,10 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
 
         if (ret != RAM_SAVE_CONTROL_DELAYED &&
             ret != RAM_SAVE_CONTROL_NOT_SUPP) {
-            if (bytes_sent && *bytes_sent > 0) {
-                qemu_file_credit_transfer(f, *bytes_sent);
-            } else if (ret < 0) {
+            if (ret < 0) {
                 qemu_file_set_error(f, ret);
             }
         }
-
         return ret;
     }
 
diff --git a/migration/ram.c b/migration/ram.c
index 67ed49b387..2d3927a15f 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1184,7 +1184,6 @@ static bool control_save_page(PageSearchStatus *pss, RAMBlock *block,
     }
 
     if (bytes_xmit) {
-        ram_transferred_add(bytes_xmit);
         *pages = 1;
     }
 
-- 
2.40.1

Re: [PATCH v2 13/16] migration/rdma: Don't use imaginary transfers

[Leonardo Brás]

On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> RDMA protocol is completely asynchronous, so in qemu_rdma_save_page()
> they "invent" that a byte has been transferred.  And then they call
> qemu_file_credit_transfer() and ram_transferred_add() with that byte.
> Just remove that calls as nothing has been sent.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/qemu-file.c | 5 +----
>  migration/ram.c       | 1 -
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index acc282654a..23a21e2331 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -346,13 +346,10 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
>  
>          if (ret != RAM_SAVE_CONTROL_DELAYED &&
>              ret != RAM_SAVE_CONTROL_NOT_SUPP) {
> -            if (bytes_sent && *bytes_sent > 0) {
> -                qemu_file_credit_transfer(f, *bytes_sent);
> -            } else if (ret < 0) {
> +            if (ret < 0) {
>                  qemu_file_set_error(f, ret);
>              }
>          }
> -
>          return ret;
>      }
>  
> diff --git a/migration/ram.c b/migration/ram.c
> index 67ed49b387..2d3927a15f 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1184,7 +1184,6 @@ static bool control_save_page(PageSearchStatus *pss, RAMBlock *block,
>      }
>  
>      if (bytes_xmit) {
> -        ram_transferred_add(bytes_xmit);
>          *pages = 1;
>      }
>  

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

[PATCH v2 14/16] migration: Remove unused qemu_file_credit_transfer()

[Juan Quintela]

After this change, nothing abuses QEMUFile to account for data
transferrefd during migration.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.h | 8 --------
 migration/qemu-file.c | 5 -----
 2 files changed, 13 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index e649718492..37f42315c7 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -122,14 +122,6 @@ bool qemu_file_buffer_empty(QEMUFile *file);
  */
 int coroutine_mixed_fn qemu_peek_byte(QEMUFile *f, int offset);
 void qemu_file_skip(QEMUFile *f, int size);
-/*
- * qemu_file_credit_transfer:
- *
- * Report on a number of bytes that have been transferred
- * out of band from the main file object I/O methods. This
- * accounting information tracks the total migration traffic.
- */
-void qemu_file_credit_transfer(QEMUFile *f, size_t size);
 int qemu_file_get_error_obj(QEMUFile *f, Error **errp);
 int qemu_file_get_error_obj_any(QEMUFile *f1, QEMUFile *f2, Error **errp);
 void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err);
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 23a21e2331..72e130631d 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -411,11 +411,6 @@ static ssize_t coroutine_mixed_fn qemu_fill_buffer(QEMUFile *f)
     return len;
 }
 
-void qemu_file_credit_transfer(QEMUFile *f, size_t size)
-{
-    f->total_transferred += size;
-}
-
 /** Closes the file
  *
  * Returns negative error value if any error happened on previous operations or
-- 
2.40.1

Re: [PATCH v2 14/16] migration: Remove unused qemu_file_credit_transfer()

[Leonardo Brás]

On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> After this change, nothing abuses QEMUFile to account for data
> transferrefd during migration.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/qemu-file.h | 8 --------
>  migration/qemu-file.c | 5 -----
>  2 files changed, 13 deletions(-)
> 
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index e649718492..37f42315c7 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -122,14 +122,6 @@ bool qemu_file_buffer_empty(QEMUFile *file);
>   */
>  int coroutine_mixed_fn qemu_peek_byte(QEMUFile *f, int offset);
>  void qemu_file_skip(QEMUFile *f, int size);
> -/*
> - * qemu_file_credit_transfer:
> - *
> - * Report on a number of bytes that have been transferred
> - * out of band from the main file object I/O methods. This
> - * accounting information tracks the total migration traffic.
> - */
> -void qemu_file_credit_transfer(QEMUFile *f, size_t size);
>  int qemu_file_get_error_obj(QEMUFile *f, Error **errp);
>  int qemu_file_get_error_obj_any(QEMUFile *f1, QEMUFile *f2, Error **errp);
>  void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err);
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 23a21e2331..72e130631d 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -411,11 +411,6 @@ static ssize_t coroutine_mixed_fn qemu_fill_buffer(QEMUFile *f)
>      return len;
>  }
>  
> -void qemu_file_credit_transfer(QEMUFile *f, size_t size)
> -{
> -    f->total_transferred += size;
> -}
> -
>  /** Closes the file
>   *
>   * Returns negative error value if any error happened on previous operations or


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

[PATCH v2 15/16] migration/rdma: Simplify the function that saves a page

[Juan Quintela]

When we sent a page through QEMUFile hooks (RDMA) there are three
posiblities:
- We are not using RDMA. return RAM_SAVE_CONTROL_DELAYED and
  control_save_page() returns false to let anything else to proceed.
- There is one error but we are using RDMA.  Then we return a negative
  value, control_save_page() needs to return true.
- Everything goes well and RDMA start the sent of the page
  asynchronously.  It returns RAM_SAVE_CONTROL_DELAYED and we need to
  return 1 for ram_save_page_legacy.

Clear?

I know, I know, the interfaz is as bad as it gets.  I think that now
it is a bit clearer, but this needs to be done some other way.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.h | 14 ++++++--------
 migration/qemu-file.c | 12 ++++++------
 migration/ram.c       | 10 +++-------
 migration/rdma.c      | 19 +++----------------
 4 files changed, 18 insertions(+), 37 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 37f42315c7..ed77996201 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -49,11 +49,10 @@ typedef int (QEMURamHookFunc)(QEMUFile *f, uint64_t flags, void *data);
  * This function allows override of where the RAM page
  * is saved (such as RDMA, for example.)
  */
-typedef size_t (QEMURamSaveFunc)(QEMUFile *f,
-                                 ram_addr_t block_offset,
-                                 ram_addr_t offset,
-                                 size_t size,
-                                 uint64_t *bytes_sent);
+typedef int (QEMURamSaveFunc)(QEMUFile *f,
+                              ram_addr_t block_offset,
+                              ram_addr_t offset,
+                              size_t size);
 
 typedef struct QEMUFileHooks {
     QEMURamHookFunc *before_ram_iterate;
@@ -146,9 +145,8 @@ void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data);
 #define RAM_SAVE_CONTROL_NOT_SUPP -1000
 #define RAM_SAVE_CONTROL_DELAYED  -2000
 
-size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
-                             ram_addr_t offset, size_t size,
-                             uint64_t *bytes_sent);
+int ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
+                          ram_addr_t offset, size_t size);
 QIOChannel *qemu_file_get_ioc(QEMUFile *file);
 
 #endif
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 72e130631d..32ef5e9651 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -336,14 +336,14 @@ void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data)
     }
 }
 
-size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
-                             ram_addr_t offset, size_t size,
-                             uint64_t *bytes_sent)
+int ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
+                          ram_addr_t offset, size_t size)
 {
     if (f->hooks && f->hooks->save_page) {
-        int ret = f->hooks->save_page(f, block_offset,
-                                      offset, size, bytes_sent);
-
+        int ret = f->hooks->save_page(f, block_offset, offset, size);
+        /*
+         * RAM_SAVE_CONTROL_* are negative values
+         */
         if (ret != RAM_SAVE_CONTROL_DELAYED &&
             ret != RAM_SAVE_CONTROL_NOT_SUPP) {
             if (ret < 0) {
diff --git a/migration/ram.c b/migration/ram.c
index 2d3927a15f..f9fcbb3bb8 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1173,23 +1173,19 @@ static int save_zero_page(PageSearchStatus *pss, QEMUFile *f, RAMBlock *block,
 static bool control_save_page(PageSearchStatus *pss, RAMBlock *block,
                               ram_addr_t offset, int *pages)
 {
-    uint64_t bytes_xmit = 0;
     int ret;
 
-    *pages = -1;
     ret = ram_control_save_page(pss->pss_channel, block->offset, offset,
-                                TARGET_PAGE_SIZE, &bytes_xmit);
+                                TARGET_PAGE_SIZE);
     if (ret == RAM_SAVE_CONTROL_NOT_SUPP) {
         return false;
     }
 
-    if (bytes_xmit) {
-        *pages = 1;
-    }
-
     if (ret == RAM_SAVE_CONTROL_DELAYED) {
+        *pages = 1;
         return true;
     }
+    *pages = ret;
     return true;
 }
 
diff --git a/migration/rdma.c b/migration/rdma.c
index 416dec00a2..12d3c23fdc 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3239,13 +3239,12 @@ qio_channel_rdma_shutdown(QIOChannel *ioc,
  *
  *    @size : Number of bytes to transfer
  *
- *    @bytes_sent : User-specificed pointer to indicate how many bytes were
+ *    @pages_sent : User-specificed pointer to indicate how many pages were
  *                  sent. Usually, this will not be more than a few bytes of
  *                  the protocol because most transfers are sent asynchronously.
  */
-static size_t qemu_rdma_save_page(QEMUFile *f,
-                                  ram_addr_t block_offset, ram_addr_t offset,
-                                  size_t size, uint64_t *bytes_sent)
+static int qemu_rdma_save_page(QEMUFile *f, ram_addr_t block_offset,
+                               ram_addr_t offset, size_t size)
 {
     QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(qemu_file_get_ioc(f));
     RDMAContext *rdma;
@@ -3277,18 +3276,6 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
         goto err;
     }
 
-    /*
-     * We always return 1 bytes because the RDMA
-     * protocol is completely asynchronous. We do not yet know
-     * whether an  identified chunk is zero or not because we're
-     * waiting for other pages to potentially be merged with
-     * the current chunk. So, we have to call qemu_update_position()
-     * later on when the actual write occurs.
-     */
-    if (bytes_sent) {
-        *bytes_sent = 1;
-    }
-
     /*
      * Drain the Completion Queue if possible, but do not block,
      * just poll.
-- 
2.40.1

Re: [PATCH v2 15/16] migration/rdma: Simplify the function that saves a page

[Leonardo Brás]

On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> When we sent a page through QEMUFile hooks (RDMA) there are three
> posiblities:
> - We are not using RDMA. return RAM_SAVE_CONTROL_DELAYED and
>   control_save_page() returns false to let anything else to proceed.
> - There is one error but we are using RDMA.  Then we return a negative
>   value, control_save_page() needs to return true.
> - Everything goes well and RDMA start the sent of the page
>   asynchronously.  It returns RAM_SAVE_CONTROL_DELAYED and we need to
>   return 1 for ram_save_page_legacy.
> 
> Clear?
> 
> I know, I know, the interfaz is as bad as it gets.  I think that now
> it is a bit clearer, but this needs to be done some other way.

interface?

> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/qemu-file.h | 14 ++++++--------
>  migration/qemu-file.c | 12 ++++++------
>  migration/ram.c       | 10 +++-------
>  migration/rdma.c      | 19 +++----------------
>  4 files changed, 18 insertions(+), 37 deletions(-)
> 
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index 37f42315c7..ed77996201 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -49,11 +49,10 @@ typedef int (QEMURamHookFunc)(QEMUFile *f, uint64_t flags, void *data);
>   * This function allows override of where the RAM page
>   * is saved (such as RDMA, for example.)
>   */
> -typedef size_t (QEMURamSaveFunc)(QEMUFile *f,
> -                                 ram_addr_t block_offset,
> -                                 ram_addr_t offset,
> -                                 size_t size,
> -                                 uint64_t *bytes_sent);
> +typedef int (QEMURamSaveFunc)(QEMUFile *f,
> +                              ram_addr_t block_offset,
> +                              ram_addr_t offset,
> +                              size_t size);
>  
>  typedef struct QEMUFileHooks {
>      QEMURamHookFunc *before_ram_iterate;
> @@ -146,9 +145,8 @@ void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data);
>  #define RAM_SAVE_CONTROL_NOT_SUPP -1000
>  #define RAM_SAVE_CONTROL_DELAYED  -2000
>  
> -size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
> -                             ram_addr_t offset, size_t size,
> -                             uint64_t *bytes_sent);
> +int ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
> +                          ram_addr_t offset, size_t size);
>  QIOChannel *qemu_file_get_ioc(QEMUFile *file);
>  
>  #endif
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 72e130631d..32ef5e9651 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -336,14 +336,14 @@ void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data)
>      }
>  }
>  
> -size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
> -                             ram_addr_t offset, size_t size,
> -                             uint64_t *bytes_sent)
> +int ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
> +                          ram_addr_t offset, size_t size)
>  {
>      if (f->hooks && f->hooks->save_page) {
> -        int ret = f->hooks->save_page(f, block_offset,
> -                                      offset, size, bytes_sent);
> -
> +        int ret = f->hooks->save_page(f, block_offset, offset, size);
> +        /*
> +         * RAM_SAVE_CONTROL_* are negative values
> +         */
>          if (ret != RAM_SAVE_CONTROL_DELAYED &&
>              ret != RAM_SAVE_CONTROL_NOT_SUPP) {
>              if (ret < 0) {
> diff --git a/migration/ram.c b/migration/ram.c
> index 2d3927a15f..f9fcbb3bb8 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1173,23 +1173,19 @@ static int save_zero_page(PageSearchStatus *pss, QEMUFile *f, RAMBlock *block,
>  static bool control_save_page(PageSearchStatus *pss, RAMBlock *block,
>                                ram_addr_t offset, int *pages)
>  {
> -    uint64_t bytes_xmit = 0;
>      int ret;
>  
> -    *pages = -1;
>      ret = ram_control_save_page(pss->pss_channel, block->offset, offset,
> -                                TARGET_PAGE_SIZE, &bytes_xmit);
> +                                TARGET_PAGE_SIZE);
>      if (ret == RAM_SAVE_CONTROL_NOT_SUPP) {
>          return false;
>      }
>  
> -    if (bytes_xmit) {
> -        *pages = 1;
> -    }
> -
>      if (ret == RAM_SAVE_CONTROL_DELAYED) {
> +        *pages = 1;
>          return true;
>      }
> +    *pages = ret;
>      return true;
>  }
>  
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 416dec00a2..12d3c23fdc 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -3239,13 +3239,12 @@ qio_channel_rdma_shutdown(QIOChannel *ioc,
>   *
>   *    @size : Number of bytes to transfer
>   *
> - *    @bytes_sent : User-specificed pointer to indicate how many bytes were
> + *    @pages_sent : User-specificed pointer to indicate how many pages were
>   *                  sent. Usually, this will not be more than a few bytes of
>   *                  the protocol because most transfers are sent asynchronously.
>   */

There is new doc to pages_sent but the parameter is not added to the signature
bellow. Am I missing something?

> -static size_t qemu_rdma_save_page(QEMUFile *f,
> -                                  ram_addr_t block_offset, ram_addr_t offset,
> -                                  size_t size, uint64_t *bytes_sent)
> +static int qemu_rdma_save_page(QEMUFile *f, ram_addr_t block_offset,
> +                               ram_addr_t offset, size_t size)
>  {
>      QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(qemu_file_get_ioc(f));
>      RDMAContext *rdma;
> @@ -3277,18 +3276,6 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
>          goto err;
>      }
>  
> -    /*
> -     * We always return 1 bytes because the RDMA
> -     * protocol is completely asynchronous. We do not yet know
> -     * whether an  identified chunk is zero or not because we're
> -     * waiting for other pages to potentially be merged with
> -     * the current chunk. So, we have to call qemu_update_position()
> -     * later on when the actual write occurs.
> -     */
> -    if (bytes_sent) {
> -        *bytes_sent = 1;
> -    }
> -
>      /*
>       * Drain the Completion Queue if possible, but do not block,
>       * just poll.

Oh, so this one complements 13/16. 
Since it doesn't do imaginary transfers anymore, there is no need to use
bytes_sent pointer to keep track of them anymore.

Other than the pages_sent above that I couldn't understand:
Reviewed-by: Leonardo Bras <leobras@redhat.com>

  

Re: [PATCH v2 15/16] migration/rdma: Simplify the function that saves a page

[Juan Quintela]

Leonardo Brás <leobras@redhat.com> wrote:
> On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
>> When we sent a page through QEMUFile hooks (RDMA) there are three
>> posiblities:
>> - We are not using RDMA. return RAM_SAVE_CONTROL_DELAYED and
>>   control_save_page() returns false to let anything else to proceed.
>> - There is one error but we are using RDMA.  Then we return a negative
>>   value, control_save_page() needs to return true.
>> - Everything goes well and RDMA start the sent of the page
>>   asynchronously.  It returns RAM_SAVE_CONTROL_DELAYED and we need to
>>   return 1 for ram_save_page_legacy.
>> 
>> Clear?
>> 
>> I know, I know, the interfaz is as bad as it gets.  I think that now
>> it is a bit clearer, but this needs to be done some other way.
>
> interface?

Yeap.  I used the Spanish spelling, that, you know, in English is wrong O:-)

Thanks.
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index 416dec00a2..12d3c23fdc 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -3239,13 +3239,12 @@ qio_channel_rdma_shutdown(QIOChannel *ioc,
>>   *
>>   *    @size : Number of bytes to transfer
>>   *
>> - *    @bytes_sent : User-specificed pointer to indicate how many bytes were
>> + *    @pages_sent : User-specificed pointer to indicate how many pages were
>>   *                  sent. Usually, this will not be more than a few bytes of
>>   *                  the protocol because most transfers are sent asynchronously.
>>   */
>
> There is new doc to pages_sent but the parameter is not added to the signature
> bellow. Am I missing something?

Good catch.

I redid this patch several times.  And it appears that I forgot some leftovers.

>
>> -static size_t qemu_rdma_save_page(QEMUFile *f,
>> -                                  ram_addr_t block_offset, ram_addr_t offset,
>> -                                  size_t size, uint64_t *bytes_sent)
>> +static int qemu_rdma_save_page(QEMUFile *f, ram_addr_t block_offset,
>> +                               ram_addr_t offset, size_t size)
>>  {
>>      QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(qemu_file_get_ioc(f));
>>      RDMAContext *rdma;
>> @@ -3277,18 +3276,6 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
>>          goto err;
>>      }
>>  
>> -    /*
>> -     * We always return 1 bytes because the RDMA
>> -     * protocol is completely asynchronous. We do not yet know
>> -     * whether an  identified chunk is zero or not because we're
>> -     * waiting for other pages to potentially be merged with
>> -     * the current chunk. So, we have to call qemu_update_position()
>> -     * later on when the actual write occurs.
>> -     */
>> -    if (bytes_sent) {
>> -        *bytes_sent = 1;
>> -    }
>> -
>>      /*
>>       * Drain the Completion Queue if possible, but do not block,
>>       * just poll.
>
> Oh, so this one complements 13/16. 
> Since it doesn't do imaginary transfers anymore, there is no need to use
> bytes_sent pointer to keep track of them anymore.
>
> Other than the pages_sent above that I couldn't understand:
> Reviewed-by: Leonardo Bras <leobras@redhat.com>

Dropping that bit.

Thanks.

Re: [PATCH v2 15/16] migration/rdma: Simplify the function that saves a page

[Leonardo Bras Soares Passos]

On Fri, May 26, 2023 at 5:21 AM Juan Quintela <quintela@redhat.com> wrote:
>
> Leonardo Brás <leobras@redhat.com> wrote:
> > On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> >> When we sent a page through QEMUFile hooks (RDMA) there are three
> >> posiblities:
> >> - We are not using RDMA. return RAM_SAVE_CONTROL_DELAYED and
> >>   control_save_page() returns false to let anything else to proceed.
> >> - There is one error but we are using RDMA.  Then we return a negative
> >>   value, control_save_page() needs to return true.
> >> - Everything goes well and RDMA start the sent of the page
> >>   asynchronously.  It returns RAM_SAVE_CONTROL_DELAYED and we need to
> >>   return 1 for ram_save_page_legacy.
> >>
> >> Clear?
> >>
> >> I know, I know, the interfaz is as bad as it gets.  I think that now
> >> it is a bit clearer, but this needs to be done some other way.
> >
> > interface?
>
> Yeap.  I used the Spanish spelling, that, you know, in English is wrong O:-)

Happens to me all the time :)

>
> Thanks.
> >> diff --git a/migration/rdma.c b/migration/rdma.c
> >> index 416dec00a2..12d3c23fdc 100644
> >> --- a/migration/rdma.c
> >> +++ b/migration/rdma.c
> >> @@ -3239,13 +3239,12 @@ qio_channel_rdma_shutdown(QIOChannel *ioc,
> >>   *
> >>   *    @size : Number of bytes to transfer
> >>   *
> >> - *    @bytes_sent : User-specificed pointer to indicate how many bytes were
> >> + *    @pages_sent : User-specificed pointer to indicate how many pages were
> >>   *                  sent. Usually, this will not be more than a few bytes of
> >>   *                  the protocol because most transfers are sent asynchronously.
> >>   */
> >
> > There is new doc to pages_sent but the parameter is not added to the signature
> > bellow. Am I missing something?
>
> Good catch.

:)

>
> I redid this patch several times.  And it appears that I forgot some leftovers.
>
> >
> >> -static size_t qemu_rdma_save_page(QEMUFile *f,
> >> -                                  ram_addr_t block_offset, ram_addr_t offset,
> >> -                                  size_t size, uint64_t *bytes_sent)
> >> +static int qemu_rdma_save_page(QEMUFile *f, ram_addr_t block_offset,
> >> +                               ram_addr_t offset, size_t size)
> >>  {
> >>      QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(qemu_file_get_ioc(f));
> >>      RDMAContext *rdma;
> >> @@ -3277,18 +3276,6 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
> >>          goto err;
> >>      }
> >>
> >> -    /*
> >> -     * We always return 1 bytes because the RDMA
> >> -     * protocol is completely asynchronous. We do not yet know
> >> -     * whether an  identified chunk is zero or not because we're
> >> -     * waiting for other pages to potentially be merged with
> >> -     * the current chunk. So, we have to call qemu_update_position()
> >> -     * later on when the actual write occurs.
> >> -     */
> >> -    if (bytes_sent) {
> >> -        *bytes_sent = 1;
> >> -    }
> >> -
> >>      /*
> >>       * Drain the Completion Queue if possible, but do not block,
> >>       * just poll.
> >
> > Oh, so this one complements 13/16.
> > Since it doesn't do imaginary transfers anymore, there is no need to use
> > bytes_sent pointer to keep track of them anymore.
> >
> > Other than the pages_sent above that I couldn't understand:
> > Reviewed-by: Leonardo Bras <leobras@redhat.com>
>
> Dropping that bit.
>
> Thanks.
>

[PATCH v2 16/16] migration/multifd: Compute transferred bytes correctly

[Juan Quintela]

In the past, we had to put the in the main thread all the operations
related with sizes due to qemu_file not beeing thread safe.  As now
all counters are atomic, we can update the counters just after the
do the write.  As an aditional bonus, we are able to use the right
value for the compression methods.  Right now we were assuming that
there were no compression at all.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/multifd.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index aabf9b6d98..0bf5958a9c 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -175,6 +175,7 @@ void multifd_register_ops(int method, MultiFDMethods *ops)
 static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
 {
     MultiFDInit_t msg = {};
+    size_t size = sizeof(msg);
     int ret;
 
     msg.magic = cpu_to_be32(MULTIFD_MAGIC);
@@ -182,10 +183,12 @@ static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
     msg.id = p->id;
     memcpy(msg.uuid, &qemu_uuid.data, sizeof(msg.uuid));
 
-    ret = qio_channel_write_all(p->c, (char *)&msg, sizeof(msg), errp);
+    ret = qio_channel_write_all(p->c, (char *)&msg, size, errp);
     if (ret != 0) {
         return -1;
     }
+    stat64_add(&mig_stats.multifd_bytes, size);
+    stat64_add(&mig_stats.transferred, size);
     return 0;
 }
 
@@ -395,7 +398,6 @@ static int multifd_send_pages(QEMUFile *f)
     static int next_channel;
     MultiFDSendParams *p = NULL; /* make happy gcc */
     MultiFDPages_t *pages = multifd_send_state->pages;
-    uint64_t transferred;
 
     if (qatomic_read(&multifd_send_state->exiting)) {
         return -1;
@@ -430,10 +432,7 @@ static int multifd_send_pages(QEMUFile *f)
     p->packet_num = multifd_send_state->packet_num++;
     multifd_send_state->pages = p->pages;
     p->pages = pages;
-    transferred = ((uint64_t) pages->num) * p->page_size + p->packet_len;
     qemu_mutex_unlock(&p->mutex);
-    stat64_add(&mig_stats.transferred, transferred);
-    stat64_add(&mig_stats.multifd_bytes, transferred);
     qemu_sem_post(&p->sem);
 
     return 1;
@@ -715,6 +714,8 @@ static void *multifd_send_thread(void *opaque)
                 if (ret != 0) {
                     break;
                 }
+                stat64_add(&mig_stats.multifd_bytes, p->packet_len);
+                stat64_add(&mig_stats.transferred, p->packet_len);
             } else {
                 /* Send header using the same writev call */
                 p->iov[0].iov_len = p->packet_len;
@@ -727,6 +728,8 @@ static void *multifd_send_thread(void *opaque)
                 break;
             }
 
+            stat64_add(&mig_stats.multifd_bytes, p->next_packet_size);
+            stat64_add(&mig_stats.transferred, p->next_packet_size);
             qemu_mutex_lock(&p->mutex);
             p->pending_job--;
             qemu_mutex_unlock(&p->mutex);
-- 
2.40.1

Re: [PATCH v2 16/16] migration/multifd: Compute transferred bytes correctly

[Leonardo Brás]

On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> In the past, we had to put the in the main thread all the operations
> related with sizes due to qemu_file not beeing thread safe.  As now
> all counters are atomic, we can update the counters just after the
> do the write.  As an aditional bonus, we are able to use the right
> value for the compression methods.  Right now we were assuming that
> there were no compression at all.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/multifd.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/multifd.c b/migration/multifd.c
> index aabf9b6d98..0bf5958a9c 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -175,6 +175,7 @@ void multifd_register_ops(int method, MultiFDMethods *ops)
>  static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
>  {
>      MultiFDInit_t msg = {};
> +    size_t size = sizeof(msg);
>      int ret;
>  
>      msg.magic = cpu_to_be32(MULTIFD_MAGIC);
> @@ -182,10 +183,12 @@ static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
>      msg.id = p->id;
>      memcpy(msg.uuid, &qemu_uuid.data, sizeof(msg.uuid));
>  
> -    ret = qio_channel_write_all(p->c, (char *)&msg, sizeof(msg), errp);
> +    ret = qio_channel_write_all(p->c, (char *)&msg, size, errp);
>      if (ret != 0) {
>          return -1;
>      }
> +    stat64_add(&mig_stats.multifd_bytes, size);
> +    stat64_add(&mig_stats.transferred, size);
>      return 0;
>  }

Humm, those are atomic ops, right?

You think we could have 'multifd_bytes' and 'transferred' in the same cacheline,
to avoid 2 cacheline bounces?

Well, it's unrelated to this patchset, so:

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

>  
> @@ -395,7 +398,6 @@ static int multifd_send_pages(QEMUFile *f)
>      static int next_channel;
>      MultiFDSendParams *p = NULL; /* make happy gcc */
>      MultiFDPages_t *pages = multifd_send_state->pages;
> -    uint64_t transferred;
>  
>      if (qatomic_read(&multifd_send_state->exiting)) {
>          return -1;
> @@ -430,10 +432,7 @@ static int multifd_send_pages(QEMUFile *f)
>      p->packet_num = multifd_send_state->packet_num++;
>      multifd_send_state->pages = p->pages;
>      p->pages = pages;
> -    transferred = ((uint64_t) pages->num) * p->page_size + p->packet_len;
>      qemu_mutex_unlock(&p->mutex);
> -    stat64_add(&mig_stats.transferred, transferred);
> -    stat64_add(&mig_stats.multifd_bytes, transferred);
>      qemu_sem_post(&p->sem);
>  
>      return 1;
> @@ -715,6 +714,8 @@ static void *multifd_send_thread(void *opaque)
>                  if (ret != 0) {
>                      break;
>                  }
> +                stat64_add(&mig_stats.multifd_bytes, p->packet_len);
> +                stat64_add(&mig_stats.transferred, p->packet_len);
>              } else {
>                  /* Send header using the same writev call */
>                  p->iov[0].iov_len = p->packet_len;
> @@ -727,6 +728,8 @@ static void *multifd_send_thread(void *opaque)
>                  break;
>              }
>  
> +            stat64_add(&mig_stats.multifd_bytes, p->next_packet_size);
> +            stat64_add(&mig_stats.transferred, p->next_packet_size);
>              qemu_mutex_lock(&p->mutex);
>              p->pending_job--;
>              qemu_mutex_unlock(&p->mutex);

Re: [PATCH v2 16/16] migration/multifd: Compute transferred bytes correctly

[Juan Quintela]

Leonardo Brás <leobras@redhat.com> wrote:
> On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
>> In the past, we had to put the in the main thread all the operations
>> related with sizes due to qemu_file not beeing thread safe.  As now
>> all counters are atomic, we can update the counters just after the
>> do the write.  As an aditional bonus, we are able to use the right
>> value for the compression methods.  Right now we were assuming that
>> there were no compression at all.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/multifd.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>> 
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index aabf9b6d98..0bf5958a9c 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -175,6 +175,7 @@ void multifd_register_ops(int method, MultiFDMethods *ops)
>>  static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
>>  {
>>      MultiFDInit_t msg = {};
>> +    size_t size = sizeof(msg);
>>      int ret;
>>  
>>      msg.magic = cpu_to_be32(MULTIFD_MAGIC);
>> @@ -182,10 +183,12 @@ static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
>>      msg.id = p->id;
>>      memcpy(msg.uuid, &qemu_uuid.data, sizeof(msg.uuid));
>>  
>> -    ret = qio_channel_write_all(p->c, (char *)&msg, sizeof(msg), errp);
>> +    ret = qio_channel_write_all(p->c, (char *)&msg, size, errp);
>>      if (ret != 0) {
>>          return -1;
>>      }
>> +    stat64_add(&mig_stats.multifd_bytes, size);
>> +    stat64_add(&mig_stats.transferred, size);
>>      return 0;
>>  }
>
> Humm, those are atomic ops, right?
>
> You think we could have 'multifd_bytes' and 'transferred' in the same cacheline,
> to avoid 2 cacheline bounces?

Don't matter on next series.

mig_stats.transferred is dropped.

And transferred becomes:

qemu_file_transferred + multifd_bytes + rdma_bytes.

So everytime that we do a write, we only update one counter.

> Well, it's unrelated to this patchset, so:
>
> Reviewed-by: Leonardo Bras <leobras@redhat.com>

Re: [PATCH v2 16/16] migration/multifd: Compute transferred bytes correctly

[Leonardo Bras Soares Passos]

On Fri, May 26, 2023 at 5:24 AM Juan Quintela <quintela@redhat.com> wrote:
>
> Leonardo Brás <leobras@redhat.com> wrote:
> > On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> >> In the past, we had to put the in the main thread all the operations
> >> related with sizes due to qemu_file not beeing thread safe.  As now
> >> all counters are atomic, we can update the counters just after the
> >> do the write.  As an aditional bonus, we are able to use the right
> >> value for the compression methods.  Right now we were assuming that
> >> there were no compression at all.
> >>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  migration/multifd.c | 13 ++++++++-----
> >>  1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/migration/multifd.c b/migration/multifd.c
> >> index aabf9b6d98..0bf5958a9c 100644
> >> --- a/migration/multifd.c
> >> +++ b/migration/multifd.c
> >> @@ -175,6 +175,7 @@ void multifd_register_ops(int method, MultiFDMethods *ops)
> >>  static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
> >>  {
> >>      MultiFDInit_t msg = {};
> >> +    size_t size = sizeof(msg);
> >>      int ret;
> >>
> >>      msg.magic = cpu_to_be32(MULTIFD_MAGIC);
> >> @@ -182,10 +183,12 @@ static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
> >>      msg.id = p->id;
> >>      memcpy(msg.uuid, &qemu_uuid.data, sizeof(msg.uuid));
> >>
> >> -    ret = qio_channel_write_all(p->c, (char *)&msg, sizeof(msg), errp);
> >> +    ret = qio_channel_write_all(p->c, (char *)&msg, size, errp);
> >>      if (ret != 0) {
> >>          return -1;
> >>      }
> >> +    stat64_add(&mig_stats.multifd_bytes, size);
> >> +    stat64_add(&mig_stats.transferred, size);
> >>      return 0;
> >>  }
> >
> > Humm, those are atomic ops, right?
> >
> > You think we could have 'multifd_bytes' and 'transferred' in the same cacheline,
> > to avoid 2 cacheline bounces?
>
> Don't matter on next series.
>
> mig_stats.transferred is dropped.
>
> And transferred becomes:
>
> qemu_file_transferred + multifd_bytes + rdma_bytes.
>
> So everytime that we do a write, we only update one counter.

That's even better :)

Thanks!

>
> > Well, it's unrelated to this patchset, so:
> >
> > Reviewed-by: Leonardo Bras <leobras@redhat.com>
>