[PATCH 00/21] Migration: More migration atomic counters

[PATCH 00/21] Migration: More migration atomic counters

[Juan Quintela]

Hi

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 (21):
  migration: A rate limit value of 0 is valid
  migration: Don't use INT64_MAX for unlimited rate
  migration: We set the rate_limit by a second
  qemu-file: make qemu_file_[sg]et_rate_limit() use an uint64_t
  qemu-file: Make rate_limit_used an uint64_t
  qemu-file: Remove total from qemu_file_total_transferred_*()
  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

 hw/ppc/spapr.c                      |  5 +-
 hw/s390x/s390-stattrib.c            |  2 +-
 include/migration/qemu-file-types.h |  2 +-
 migration/block-dirty-bitmap.c      |  2 +-
 migration/block.c                   |  9 ++--
 migration/meson.build               |  2 +-
 migration/migration-stats.c         | 56 ++++++++++++++++++++
 migration/migration-stats.h         | 59 +++++++++++++++++++++
 migration/migration.c               | 46 ++++++-----------
 migration/migration.h               |  1 -
 migration/multifd.c                 | 14 ++---
 migration/options.c                 |  7 ++-
 migration/qemu-file.c               | 79 +++++------------------------
 migration/qemu-file.h               | 43 ++++------------
 migration/ram.c                     | 34 +++++++------
 migration/rdma.c                    | 64 ++++++++++++-----------
 migration/savevm.c                  | 27 +++++++---
 migration/trace-events              |  3 ++
 migration/vmstate.c                 |  8 +--
 19 files changed, 259 insertions(+), 204 deletions(-)

-- 
2.40.0

[PATCH 01/21] migration: A rate limit value of 0 is valid

[Juan Quintela]

And it is the best way to not have rate_limit.

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

diff --git a/migration/migration.c b/migration/migration.c
index 232e387109..1192f1ebf1 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2117,12 +2117,7 @@ static int postcopy_start(MigrationState *ms)
      * will notice we're in POSTCOPY_ACTIVE and not actually
      * wrap their state up here
      */
-    /* 0 max-postcopy-bandwidth means unlimited */
-    if (!bandwidth) {
-        qemu_file_set_rate_limit(ms->to_dst_file, INT64_MAX);
-    } else {
-        qemu_file_set_rate_limit(ms->to_dst_file, bandwidth / XFER_LIMIT_RATIO);
-    }
+    qemu_file_set_rate_limit(ms->to_dst_file, bandwidth / XFER_LIMIT_RATIO);
     if (migrate_postcopy_ram()) {
         /* Ping just for debugging, helps line traces up */
         qemu_savevm_send_ping(ms->to_dst_file, 2);
-- 
2.40.0

Re: [PATCH 01/21] migration: A rate limit value of 0 is valid

[Cédric Le Goater]

On 5/8/23 15:08, Juan Quintela wrote:
> And it is the best way to not have rate_limit.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

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

C.

> ---
>   migration/migration.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 232e387109..1192f1ebf1 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2117,12 +2117,7 @@ static int postcopy_start(MigrationState *ms)
>        * will notice we're in POSTCOPY_ACTIVE and not actually
>        * wrap their state up here
>        */
> -    /* 0 max-postcopy-bandwidth means unlimited */
> -    if (!bandwidth) {
> -        qemu_file_set_rate_limit(ms->to_dst_file, INT64_MAX);
> -    } else {
> -        qemu_file_set_rate_limit(ms->to_dst_file, bandwidth / XFER_LIMIT_RATIO);
> -    }
> +    qemu_file_set_rate_limit(ms->to_dst_file, bandwidth / XFER_LIMIT_RATIO);
>       if (migrate_postcopy_ram()) {
>           /* Ping just for debugging, helps line traces up */
>           qemu_savevm_send_ping(ms->to_dst_file, 2);

[PATCH 02/21] migration: Don't use INT64_MAX for unlimited rate

[Juan Quintela]

Use 0 instead.

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

diff --git a/migration/migration.c b/migration/migration.c
index 1192f1ebf1..3979a98949 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2296,7 +2296,7 @@ static void migration_completion(MigrationState *s)
             }
             if (ret >= 0) {
                 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, 0);
                 ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
                                                          s->block_inactive);
             }
@@ -3044,7 +3044,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, 0);
 
     setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
     /*
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index f4cfd05c67..745361d238 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -731,6 +731,9 @@ int qemu_file_rate_limit(QEMUFile *f)
     if (qemu_file_get_error(f)) {
         return 1;
     }
+    /*
+     *  rate_limit_max == 0 means no rate_limit enfoncement.
+     */
     if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) {
         return 1;
     }
-- 
2.40.0

Re: [PATCH 02/21] migration: Don't use INT64_MAX for unlimited rate

[Harsh Prateek Bora]

On 5/8/23 18:38, Juan Quintela wrote:
> Use 0 instead.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>   migration/migration.c | 4 ++--
>   migration/qemu-file.c | 3 +++
>   2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 1192f1ebf1..3979a98949 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2296,7 +2296,7 @@ static void migration_completion(MigrationState *s)
>               }
>               if (ret >= 0) {
>                   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, 0);

#define RATE_LIMIT_MAX 0

How about having a macro and use that which conveys the meaning in all 
call instances wherever it is getting passed ?


>                   ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
>                                                            s->block_inactive);
>               }
> @@ -3044,7 +3044,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, 0);
>   
>       setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
>       /*
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index f4cfd05c67..745361d238 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -731,6 +731,9 @@ int qemu_file_rate_limit(QEMUFile *f)
>       if (qemu_file_get_error(f)) {
>           return 1;
>       }
> +    /*
> +     *  rate_limit_max == 0 means no rate_limit enfoncement.
> +     */
>       if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) {
>           return 1;
>       }

Re: [PATCH 02/21] migration: Don't use INT64_MAX for unlimited rate

[Juan Quintela]

Harsh Prateek Bora <harshpb@linux.ibm.com> wrote:
> On 5/8/23 18:38, Juan Quintela wrote:
>> Use 0 instead.
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>   migration/migration.c | 4 ++--
>>   migration/qemu-file.c | 3 +++
>>   2 files changed, 5 insertions(+), 2 deletions(-)
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 1192f1ebf1..3979a98949 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -2296,7 +2296,7 @@ static void migration_completion(MigrationState *s)
>>               }
>>               if (ret >= 0) {
>>                   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, 0);
>
> #define RATE_LIMIT_MAX 0
>
> How about having a macro and use that which conveys the meaning in all
> call instances wherever it is getting passed ?

I almost preffer the macro.

      qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX);

seems quite explanatory?

Thanks, Juan.

>
>>                   ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
>>                                                            s->block_inactive);
>>               }
>> @@ -3044,7 +3044,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, 0);
>>         setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
>>       /*
>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>> index f4cfd05c67..745361d238 100644
>> --- a/migration/qemu-file.c
>> +++ b/migration/qemu-file.c
>> @@ -731,6 +731,9 @@ int qemu_file_rate_limit(QEMUFile *f)
>>       if (qemu_file_get_error(f)) {
>>           return 1;
>>       }
>> +    /*
>> +     *  rate_limit_max == 0 means no rate_limit enfoncement.
>> +     */
>>       if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) {
>>           return 1;
>>       }

Re: [PATCH 02/21] migration: Don't use INT64_MAX for unlimited rate

[Harsh Prateek Bora]

On 5/9/23 17:21, Juan Quintela wrote:
> Harsh Prateek Bora <harshpb@linux.ibm.com> wrote:
>> On 5/8/23 18:38, Juan Quintela wrote:
>>> Use 0 instead.
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>>    migration/migration.c | 4 ++--
>>>    migration/qemu-file.c | 3 +++
>>>    2 files changed, 5 insertions(+), 2 deletions(-)
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 1192f1ebf1..3979a98949 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -2296,7 +2296,7 @@ static void migration_completion(MigrationState *s)
>>>                }
>>>                if (ret >= 0) {
>>>                    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, 0);
>>
>> #define RATE_LIMIT_MAX 0
>>
>> How about having a macro and use that which conveys the meaning in all
>> call instances wherever it is getting passed ?
> 
> I almost preffer the macro.
> 
>        qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX);
> 
> seems quite explanatory?
> 
Yes, definitely.

Thanks
Harsh
> Thanks, Juan.
> 
>>
>>>                    ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
>>>                                                             s->block_inactive);
>>>                }
>>> @@ -3044,7 +3044,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, 0);
>>>          setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
>>>        /*
>>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>>> index f4cfd05c67..745361d238 100644
>>> --- a/migration/qemu-file.c
>>> +++ b/migration/qemu-file.c
>>> @@ -731,6 +731,9 @@ int qemu_file_rate_limit(QEMUFile *f)
>>>        if (qemu_file_get_error(f)) {
>>>            return 1;
>>>        }
>>> +    /*
>>> +     *  rate_limit_max == 0 means no rate_limit enfoncement.
>>> +     */
>>>        if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) {
>>>            return 1;
>>>        }
> 

Re: [PATCH 02/21] migration: Don't use INT64_MAX for unlimited rate

[Cédric Le Goater]

On 5/9/23 13:51, Juan Quintela wrote:
> Harsh Prateek Bora <harshpb@linux.ibm.com> wrote:
>> On 5/8/23 18:38, Juan Quintela wrote:
>>> Use 0 instead.
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>>    migration/migration.c | 4 ++--
>>>    migration/qemu-file.c | 3 +++
>>>    2 files changed, 5 insertions(+), 2 deletions(-)
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 1192f1ebf1..3979a98949 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -2296,7 +2296,7 @@ static void migration_completion(MigrationState *s)
>>>                }
>>>                if (ret >= 0) {
>>>                    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, 0);
>>
>> #define RATE_LIMIT_MAX 0
>>
>> How about having a macro and use that which conveys the meaning in all
>> call instances wherever it is getting passed ?
> 
> I almost preffer the macro.
> 
>        qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX);
> 
> seems quite explanatory?

yep. and I would drop the comment qemu_file_rate_limit().

Thanks,

C.


> 
> Thanks, Juan.
> 
>>
>>>                    ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
>>>                                                             s->block_inactive);
>>>                }
>>> @@ -3044,7 +3044,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, 0);
>>>          setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
>>>        /*
>>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>>> index f4cfd05c67..745361d238 100644
>>> --- a/migration/qemu-file.c
>>> +++ b/migration/qemu-file.c
>>> @@ -731,6 +731,9 @@ int qemu_file_rate_limit(QEMUFile *f)
>>>        if (qemu_file_get_error(f)) {
>>>            return 1;
>>>        }
>>> +    /*
>>> +     *  rate_limit_max == 0 means no rate_limit enfoncement.
>>> +     */
>>>        if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) {
>>>            return 1;
>>>        }
> 

Re: [PATCH 02/21] migration: Don't use INT64_MAX for unlimited rate

[Juan Quintela]

Cédric Le Goater <clg@kaod.org> wrote:
> On 5/9/23 13:51, Juan Quintela wrote:
>> Harsh Prateek Bora <harshpb@linux.ibm.com> wrote:
>>> On 5/8/23 18:38, Juan Quintela wrote:
>>>> Use 0 instead.
>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>> ---
>>>>    migration/migration.c | 4 ++--
>>>>    migration/qemu-file.c | 3 +++
>>>>    2 files changed, 5 insertions(+), 2 deletions(-)
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index 1192f1ebf1..3979a98949 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -2296,7 +2296,7 @@ static void migration_completion(MigrationState *s)
>>>>                }
>>>>                if (ret >= 0) {
>>>>                    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, 0);
>>>
>>> #define RATE_LIMIT_MAX 0
>>>
>>> How about having a macro and use that which conveys the meaning in all
>>> call instances wherever it is getting passed ?
>> I almost preffer the macro.
>>        qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX);
>> seems quite explanatory?
>
> yep. and I would drop the comment qemu_file_rate_limit().

I dropped it once by error.
And reviewer didn't noticed either.

So ....

[PATCH 03/21] migration: We set the rate_limit by a second

[Juan Quintela]

That the implementation does the check every 100 milliseconds is an
implementation detail that shouldn't be seen on the interfaz.
Notice that all callers of qemu_file_set_rate_limit() used the
division or pass 0, so this change is a NOP.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 7 +++----
 migration/options.c   | 4 ++--
 migration/qemu-file.c | 6 +++++-
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 3979a98949..e17a6538b4 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2117,7 +2117,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 / XFER_LIMIT_RATIO);
+    qemu_file_set_rate_limit(ms->to_dst_file, bandwidth);
     if (migrate_postcopy_ram()) {
         /* Ping just for debugging, helps line traces up */
         qemu_savevm_send_ping(ms->to_dst_file, 2);
@@ -3207,11 +3207,10 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
 
     if (resume) {
         /* This is a resumed migration */
-        rate_limit = migrate_max_postcopy_bandwidth() /
-            XFER_LIMIT_RATIO;
+        rate_limit = migrate_max_postcopy_bandwidth();
     } else {
         /* This is a fresh new migration */
-        rate_limit = migrate_max_bandwidth() / XFER_LIMIT_RATIO;
+        rate_limit = migrate_max_bandwidth();
 
         /* Notify before starting migration thread */
         notifier_list_notify(&migration_state_notifiers, s);
diff --git a/migration/options.c b/migration/options.c
index 2e759cc306..d04b5fbc3a 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1243,7 +1243,7 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
         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 / XFER_LIMIT_RATIO);
+                                s->parameters.max_bandwidth);
         }
     }
 
@@ -1275,7 +1275,7 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
         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 / XFER_LIMIT_RATIO);
+                    s->parameters.max_postcopy_bandwidth);
         }
     }
     if (params->has_max_cpu_throttle) {
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 745361d238..12cf7fb04e 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -29,6 +29,7 @@
 #include "migration.h"
 #include "qemu-file.h"
 #include "trace.h"
+#include "options.h"
 #include "qapi/error.h"
 
 #define IO_BUF_SIZE 32768
@@ -747,7 +748,10 @@ int64_t qemu_file_get_rate_limit(QEMUFile *f)
 
 void qemu_file_set_rate_limit(QEMUFile *f, int64_t limit)
 {
-    f->rate_limit_max = 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)
-- 
2.40.0

Re: [PATCH 03/21] migration: We set the rate_limit by a second

[Cédric Le Goater]

On 5/8/23 15:08, Juan Quintela wrote:
> That the implementation does the check every 100 milliseconds is an
> implementation detail that shouldn't be seen on the interfaz.

Si. Pero, "interface" es mejor aqui.

> Notice that all callers of qemu_file_set_rate_limit() used the
> division or pass 0, so this change is a NOP.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>


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

C.


> ---
>   migration/migration.c | 7 +++----
>   migration/options.c   | 4 ++--
>   migration/qemu-file.c | 6 +++++-
>   3 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 3979a98949..e17a6538b4 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2117,7 +2117,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 / XFER_LIMIT_RATIO);
> +    qemu_file_set_rate_limit(ms->to_dst_file, bandwidth);
>       if (migrate_postcopy_ram()) {
>           /* Ping just for debugging, helps line traces up */
>           qemu_savevm_send_ping(ms->to_dst_file, 2);
> @@ -3207,11 +3207,10 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>   
>       if (resume) {
>           /* This is a resumed migration */
> -        rate_limit = migrate_max_postcopy_bandwidth() /
> -            XFER_LIMIT_RATIO;
> +        rate_limit = migrate_max_postcopy_bandwidth();
>       } else {
>           /* This is a fresh new migration */
> -        rate_limit = migrate_max_bandwidth() / XFER_LIMIT_RATIO;
> +        rate_limit = migrate_max_bandwidth();
>   
>           /* Notify before starting migration thread */
>           notifier_list_notify(&migration_state_notifiers, s);
> diff --git a/migration/options.c b/migration/options.c
> index 2e759cc306..d04b5fbc3a 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -1243,7 +1243,7 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>           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 / XFER_LIMIT_RATIO);
> +                                s->parameters.max_bandwidth);
>           }
>       }
>   
> @@ -1275,7 +1275,7 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>           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 / XFER_LIMIT_RATIO);
> +                    s->parameters.max_postcopy_bandwidth);
>           }
>       }
>       if (params->has_max_cpu_throttle) {
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 745361d238..12cf7fb04e 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -29,6 +29,7 @@
>   #include "migration.h"
>   #include "qemu-file.h"
>   #include "trace.h"
> +#include "options.h"
>   #include "qapi/error.h"
>   
>   #define IO_BUF_SIZE 32768
> @@ -747,7 +748,10 @@ int64_t qemu_file_get_rate_limit(QEMUFile *f)
>   
>   void qemu_file_set_rate_limit(QEMUFile *f, int64_t limit)
>   {
> -    f->rate_limit_max = 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)

Re: [PATCH 03/21] migration: We set the rate_limit by a second

[Juan Quintela]

Cédric Le Goater <clg@kaod.org> wrote:
> On 5/8/23 15:08, Juan Quintela wrote:
>> That the implementation does the check every 100 milliseconds is an
>> implementation detail that shouldn't be seen on the interfaz.
>
> Si. Pero, "interface" es mejor aqui.

Muchas gracias.

>
>> Notice that all callers of qemu_file_set_rate_limit() used the
>> division or pass 0, so this change is a NOP.
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>

Gracias de nuevo.

[PATCH 04/21] qemu-file: make qemu_file_[sg]et_rate_limit() use an uint64_t

[Juan Quintela]

It is really size_t.  Everything else uses uint64_t, so move this to
uint64_t as well.  A size can't be negative anyways.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Message-Id: <20230504113841.23130-4-quintela@redhat.com>

---

Don't drop the check if rate_limit_max is zero.
---
 migration/qemu-file.c | 6 +++---
 migration/qemu-file.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 12cf7fb04e..346b683929 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -44,7 +44,7 @@ struct QEMUFile {
      * Maximum amount of data in bytes to transfer during one
      * rate limiting time window
      */
-    int64_t rate_limit_max;
+    uint64_t rate_limit_max;
     /*
      * Total amount of data in bytes queued for transfer
      * during this rate limiting time window
@@ -741,12 +741,12 @@ int qemu_file_rate_limit(QEMUFile *f)
     return 0;
 }
 
-int64_t qemu_file_get_rate_limit(QEMUFile *f)
+uint64_t qemu_file_get_rate_limit(QEMUFile *f)
 {
     return f->rate_limit_max;
 }
 
-void qemu_file_set_rate_limit(QEMUFile *f, int64_t limit)
+void qemu_file_set_rate_limit(QEMUFile *f, uint64_t limit)
 {
     /*
      * 'limit' is per second.  But we check it each 100 miliseconds.
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 4f26bf6961..04ca48cbef 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -138,8 +138,8 @@ void qemu_file_reset_rate_limit(QEMUFile *f);
  * need to be applied to the rate limiting calcuations
  */
 void qemu_file_acct_rate_limit(QEMUFile *f, int64_t len);
-void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
-int64_t qemu_file_get_rate_limit(QEMUFile *f);
+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);
-- 
2.40.0

Re: [PATCH 04/21] qemu-file: make qemu_file_[sg]et_rate_limit() use an uint64_t

[Cédric Le Goater]

On 5/8/23 15:08, Juan Quintela wrote:
> It is really size_t.  Everything else uses uint64_t, so move this to
> uint64_t as well.  A size can't be negative anyways.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Message-Id: <20230504113841.23130-4-quintela@redhat.com>



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

C. *
> 
> ---
> 
> Don't drop the check if rate_limit_max is zero.
> ---
>   migration/qemu-file.c | 6 +++---
>   migration/qemu-file.h | 4 ++--
>   2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 12cf7fb04e..346b683929 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -44,7 +44,7 @@ struct QEMUFile {
>        * Maximum amount of data in bytes to transfer during one
>        * rate limiting time window
>        */
> -    int64_t rate_limit_max;
> +    uint64_t rate_limit_max;
>       /*
>        * Total amount of data in bytes queued for transfer
>        * during this rate limiting time window
> @@ -741,12 +741,12 @@ int qemu_file_rate_limit(QEMUFile *f)
>       return 0;
>   }
>   
> -int64_t qemu_file_get_rate_limit(QEMUFile *f)
> +uint64_t qemu_file_get_rate_limit(QEMUFile *f)
>   {
>       return f->rate_limit_max;
>   }
>   
> -void qemu_file_set_rate_limit(QEMUFile *f, int64_t limit)
> +void qemu_file_set_rate_limit(QEMUFile *f, uint64_t limit)
>   {
>       /*
>        * 'limit' is per second.  But we check it each 100 miliseconds.
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index 4f26bf6961..04ca48cbef 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -138,8 +138,8 @@ void qemu_file_reset_rate_limit(QEMUFile *f);
>    * need to be applied to the rate limiting calcuations
>    */
>   void qemu_file_acct_rate_limit(QEMUFile *f, int64_t len);
> -void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
> -int64_t qemu_file_get_rate_limit(QEMUFile *f);
> +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);

[PATCH 05/21] qemu-file: Make rate_limit_used an uint64_t

[Juan Quintela]

Change all the functions that use it.  It was already passed as
uint64_t.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20230504113841.23130-5-quintela@redhat.com>
---
 migration/qemu-file.c | 4 ++--
 migration/qemu-file.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 346b683929..f3cb0cd94f 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -49,7 +49,7 @@ struct QEMUFile {
      * Total amount of data in bytes queued for transfer
      * during this rate limiting time window
      */
-    int64_t rate_limit_used;
+    uint64_t rate_limit_used;
 
     /* The sum of bytes transferred on the wire */
     uint64_t total_transferred;
@@ -759,7 +759,7 @@ void qemu_file_reset_rate_limit(QEMUFile *f)
     f->rate_limit_used = 0;
 }
 
-void qemu_file_acct_rate_limit(QEMUFile *f, int64_t len)
+void qemu_file_acct_rate_limit(QEMUFile *f, uint64_t len)
 {
     f->rate_limit_used += len;
 }
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 04ca48cbef..d758e7f10b 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -137,7 +137,7 @@ void qemu_file_reset_rate_limit(QEMUFile *f);
  * 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, int64_t len);
+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);
-- 
2.40.0

Re: [PATCH 05/21] qemu-file: Make rate_limit_used an uint64_t

[Cédric Le Goater]

On 5/8/23 15:08, Juan Quintela wrote:
> Change all the functions that use it.  It was already passed as
> uint64_t.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Message-Id: <20230504113841.23130-5-quintela@redhat.com>


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

C.

> ---
>   migration/qemu-file.c | 4 ++--
>   migration/qemu-file.h | 2 +-
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 346b683929..f3cb0cd94f 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -49,7 +49,7 @@ struct QEMUFile {
>        * Total amount of data in bytes queued for transfer
>        * during this rate limiting time window
>        */
> -    int64_t rate_limit_used;
> +    uint64_t rate_limit_used;
>   
>       /* The sum of bytes transferred on the wire */
>       uint64_t total_transferred;
> @@ -759,7 +759,7 @@ void qemu_file_reset_rate_limit(QEMUFile *f)
>       f->rate_limit_used = 0;
>   }
>   
> -void qemu_file_acct_rate_limit(QEMUFile *f, int64_t len)
> +void qemu_file_acct_rate_limit(QEMUFile *f, uint64_t len)
>   {
>       f->rate_limit_used += len;
>   }
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index 04ca48cbef..d758e7f10b 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -137,7 +137,7 @@ void qemu_file_reset_rate_limit(QEMUFile *f);
>    * 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, int64_t len);
> +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);

[PATCH 06/21] qemu-file: Remove total from qemu_file_total_transferred_*()

[Juan Quintela]

Function is already quite long.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/block.c     |  4 ++--
 migration/migration.c |  2 +-
 migration/qemu-file.c |  4 ++--
 migration/qemu-file.h | 10 +++++-----
 migration/savevm.c    |  6 +++---
 migration/vmstate.c   |  5 ++---
 6 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index a37678ce95..12617b4152 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -747,7 +747,7 @@ static int block_save_setup(QEMUFile *f, void *opaque)
 static int block_save_iterate(QEMUFile *f, void *opaque)
 {
     int ret;
-    uint64_t last_bytes = qemu_file_total_transferred(f);
+    uint64_t last_bytes = qemu_file_transferred(f);
 
     trace_migration_block_save("iterate", block_mig_state.submitted,
                                block_mig_state.transferred);
@@ -799,7 +799,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
     }
 
     qemu_put_be64(f, BLK_MIG_FLAG_EOS);
-    uint64_t delta_bytes = qemu_file_total_transferred(f) - last_bytes;
+    uint64_t delta_bytes = qemu_file_transferred(f) - last_bytes;
     return (delta_bytes > 0);
 }
 
diff --git a/migration/migration.c b/migration/migration.c
index e17a6538b4..b1cfb56523 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2621,7 +2621,7 @@ 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_total_transferred(s->to_dst_file) +
+    return qemu_file_transferred(s->to_dst_file) +
         stat64_get(&mig_stats.multifd_bytes);
 }
 
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index f3cb0cd94f..6ebc2bd3ec 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -709,7 +709,7 @@ int coroutine_mixed_fn qemu_get_byte(QEMUFile *f)
     return result;
 }
 
-uint64_t qemu_file_total_transferred_fast(QEMUFile *f)
+uint64_t qemu_file_transferred_fast(QEMUFile *f)
 {
     uint64_t ret = f->total_transferred;
     int i;
@@ -721,7 +721,7 @@ uint64_t qemu_file_total_transferred_fast(QEMUFile *f)
     return ret;
 }
 
-uint64_t qemu_file_total_transferred(QEMUFile *f)
+uint64_t qemu_file_transferred(QEMUFile *f)
 {
     qemu_fflush(f);
     return f->total_transferred;
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index d758e7f10b..ab164a58d0 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -68,7 +68,7 @@ void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks);
 int qemu_fclose(QEMUFile *f);
 
 /*
- * qemu_file_total_transferred:
+ * qemu_file_transferred:
  *
  * Report the total number of bytes transferred with
  * this file.
@@ -83,19 +83,19 @@ int qemu_fclose(QEMUFile *f);
  *
  * Returns: the total bytes transferred
  */
-uint64_t qemu_file_total_transferred(QEMUFile *f);
+uint64_t qemu_file_transferred(QEMUFile *f);
 
 /*
- * qemu_file_total_transferred_fast:
+ * qemu_file_transferred_fast:
  *
- * As qemu_file_total_transferred except for writable
+ * As qemu_file_transferred except for writable
  * files, where no flush is performed and the reported
  * amount will include the size of any queued buffers,
  * on top of the amount actually transferred.
  *
  * Returns: the total bytes transferred and queued
  */
-uint64_t qemu_file_total_transferred_fast(QEMUFile *f);
+uint64_t qemu_file_transferred_fast(QEMUFile *f);
 
 /*
  * put_buffer without copying the buffer.
diff --git a/migration/savevm.c b/migration/savevm.c
index 032044b1d5..e33788343a 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -927,9 +927,9 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
 static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
                                    JSONWriter *vmdesc)
 {
-    uint64_t old_offset = qemu_file_total_transferred_fast(f);
+    uint64_t old_offset = qemu_file_transferred_fast(f);
     se->ops->save_state(f, se->opaque);
-    uint64_t size = qemu_file_total_transferred_fast(f) - old_offset;
+    uint64_t size = qemu_file_transferred_fast(f) - old_offset;
 
     if (vmdesc) {
         json_writer_int64(vmdesc, "size", size);
@@ -2956,7 +2956,7 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
         goto the_end;
     }
     ret = qemu_savevm_state(f, errp);
-    vm_state_size = qemu_file_total_transferred(f);
+    vm_state_size = qemu_file_transferred(f);
     ret2 = qemu_fclose(f);
     if (ret < 0) {
         goto the_end;
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 351f56104e..af01d54b6f 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -361,7 +361,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                 void *curr_elem = first_elem + size * i;
 
                 vmsd_desc_field_start(vmsd, vmdesc_loop, field, i, n_elems);
-                old_offset = qemu_file_total_transferred_fast(f);
+                old_offset = qemu_file_transferred_fast(f);
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
                     assert(curr_elem);
                     curr_elem = *(void **)curr_elem;
@@ -391,8 +391,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                     return ret;
                 }
 
-                written_bytes = qemu_file_total_transferred_fast(f) -
-                                    old_offset;
+                written_bytes = qemu_file_transferred_fast(f) - old_offset;
                 vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes, i);
 
                 /* Compressed arrays only care about the first element */
-- 
2.40.0

Re: [PATCH 06/21] qemu-file: Remove total from qemu_file_total_transferred_*()

[Cédric Le Goater]

On 5/8/23 15:08, Juan Quintela wrote:
> Function is already quite long.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>



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

C.

> ---
>   migration/block.c     |  4 ++--
>   migration/migration.c |  2 +-
>   migration/qemu-file.c |  4 ++--
>   migration/qemu-file.h | 10 +++++-----
>   migration/savevm.c    |  6 +++---
>   migration/vmstate.c   |  5 ++---
>   6 files changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/migration/block.c b/migration/block.c
> index a37678ce95..12617b4152 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -747,7 +747,7 @@ static int block_save_setup(QEMUFile *f, void *opaque)
>   static int block_save_iterate(QEMUFile *f, void *opaque)
>   {
>       int ret;
> -    uint64_t last_bytes = qemu_file_total_transferred(f);
> +    uint64_t last_bytes = qemu_file_transferred(f);
>   
>       trace_migration_block_save("iterate", block_mig_state.submitted,
>                                  block_mig_state.transferred);
> @@ -799,7 +799,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>       }
>   
>       qemu_put_be64(f, BLK_MIG_FLAG_EOS);
> -    uint64_t delta_bytes = qemu_file_total_transferred(f) - last_bytes;
> +    uint64_t delta_bytes = qemu_file_transferred(f) - last_bytes;
>       return (delta_bytes > 0);
>   }
>   
> diff --git a/migration/migration.c b/migration/migration.c
> index e17a6538b4..b1cfb56523 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2621,7 +2621,7 @@ 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_total_transferred(s->to_dst_file) +
> +    return qemu_file_transferred(s->to_dst_file) +
>           stat64_get(&mig_stats.multifd_bytes);
>   }
>   
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index f3cb0cd94f..6ebc2bd3ec 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -709,7 +709,7 @@ int coroutine_mixed_fn qemu_get_byte(QEMUFile *f)
>       return result;
>   }
>   
> -uint64_t qemu_file_total_transferred_fast(QEMUFile *f)
> +uint64_t qemu_file_transferred_fast(QEMUFile *f)
>   {
>       uint64_t ret = f->total_transferred;
>       int i;
> @@ -721,7 +721,7 @@ uint64_t qemu_file_total_transferred_fast(QEMUFile *f)
>       return ret;
>   }
>   
> -uint64_t qemu_file_total_transferred(QEMUFile *f)
> +uint64_t qemu_file_transferred(QEMUFile *f)
>   {
>       qemu_fflush(f);
>       return f->total_transferred;
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index d758e7f10b..ab164a58d0 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -68,7 +68,7 @@ void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks);
>   int qemu_fclose(QEMUFile *f);
>   
>   /*
> - * qemu_file_total_transferred:
> + * qemu_file_transferred:
>    *
>    * Report the total number of bytes transferred with
>    * this file.
> @@ -83,19 +83,19 @@ int qemu_fclose(QEMUFile *f);
>    *
>    * Returns: the total bytes transferred
>    */
> -uint64_t qemu_file_total_transferred(QEMUFile *f);
> +uint64_t qemu_file_transferred(QEMUFile *f);
>   
>   /*
> - * qemu_file_total_transferred_fast:
> + * qemu_file_transferred_fast:
>    *
> - * As qemu_file_total_transferred except for writable
> + * As qemu_file_transferred except for writable
>    * files, where no flush is performed and the reported
>    * amount will include the size of any queued buffers,
>    * on top of the amount actually transferred.
>    *
>    * Returns: the total bytes transferred and queued
>    */
> -uint64_t qemu_file_total_transferred_fast(QEMUFile *f);
> +uint64_t qemu_file_transferred_fast(QEMUFile *f);
>   
>   /*
>    * put_buffer without copying the buffer.
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 032044b1d5..e33788343a 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -927,9 +927,9 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
>   static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
>                                      JSONWriter *vmdesc)
>   {
> -    uint64_t old_offset = qemu_file_total_transferred_fast(f);
> +    uint64_t old_offset = qemu_file_transferred_fast(f);
>       se->ops->save_state(f, se->opaque);
> -    uint64_t size = qemu_file_total_transferred_fast(f) - old_offset;
> +    uint64_t size = qemu_file_transferred_fast(f) - old_offset;
>   
>       if (vmdesc) {
>           json_writer_int64(vmdesc, "size", size);
> @@ -2956,7 +2956,7 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
>           goto the_end;
>       }
>       ret = qemu_savevm_state(f, errp);
> -    vm_state_size = qemu_file_total_transferred(f);
> +    vm_state_size = qemu_file_transferred(f);
>       ret2 = qemu_fclose(f);
>       if (ret < 0) {
>           goto the_end;
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 351f56104e..af01d54b6f 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -361,7 +361,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                   void *curr_elem = first_elem + size * i;
>   
>                   vmsd_desc_field_start(vmsd, vmdesc_loop, field, i, n_elems);
> -                old_offset = qemu_file_total_transferred_fast(f);
> +                old_offset = qemu_file_transferred_fast(f);
>                   if (field->flags & VMS_ARRAY_OF_POINTER) {
>                       assert(curr_elem);
>                       curr_elem = *(void **)curr_elem;
> @@ -391,8 +391,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                       return ret;
>                   }
>   
> -                written_bytes = qemu_file_total_transferred_fast(f) -
> -                                    old_offset;
> +                written_bytes = qemu_file_transferred_fast(f) - old_offset;
>                   vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes, i);
>   
>                   /* Compressed arrays only care about the first element */

[PATCH 07/21] 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/meson.build |  2 +-
 migration/ram.c       | 14 ++++++++++++++
 migration/savevm.c    | 19 +++++++++++++++++--
 migration/vmstate.c   |  3 +++
 4 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/migration/meson.build b/migration/meson.build
index da1897fadf..b27fc9eeb6 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',
@@ -19,7 +20,6 @@ softmmu_ss.add(files(
   'fd.c',
   'global_state.c',
   'migration-hmp-cmds.c',
-  'migration-stats.c',
   'migration.c',
   'multifd.c',
   'multifd-zlib.c',
diff --git a/migration/ram.c b/migration/ram.c
index 5e7bf20ca5..5ae1fdba45 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);
     }
@@ -1617,6 +1618,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);
             }
             /*
@@ -3245,6 +3247,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;
@@ -3263,16 +3266,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;
             }
         }
     }
@@ -3289,11 +3296,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;
 }
 
@@ -3434,6 +3444,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();
 
@@ -3478,8 +3489,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;
-- 
2.40.0

Re: [PATCH 07/21] migration: Correct transferred bytes value

[Harsh Prateek Bora]

On 5/8/23 18:38, 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

s/whrer/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/meson.build |  2 +-
>   migration/ram.c       | 14 ++++++++++++++
>   migration/savevm.c    | 19 +++++++++++++++++--
>   migration/vmstate.c   |  3 +++
>   4 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/meson.build b/migration/meson.build
> index da1897fadf..b27fc9eeb6 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',
> @@ -19,7 +20,6 @@ softmmu_ss.add(files(
>     'fd.c',
>     'global_state.c',
>     'migration-hmp-cmds.c',
> -  'migration-stats.c',
>     'migration.c',
>     'multifd.c',
>     'multifd-zlib.c',
> diff --git a/migration/ram.c b/migration/ram.c
> index 5e7bf20ca5..5ae1fdba45 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);

I see lot of hard-coded math like above in below code as well.
Although compiler will do its optimization, but we may choose to write 
it like either of below:
  - sizeof(??) + size + sizeof(??)
  - <macro1> + size + <macro2>
  - size + 16 /* explaining what 8 + 8 means here */
I guess first method or usage of macros instead of hard-coded numbers is 
better. Applies to all instances below.

regards,
Harsh

>       if (qemu_file_get_error(file)) {
>           return qemu_file_get_error(file);
>       }
> @@ -1617,6 +1618,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);
>               }
>               /*
> @@ -3245,6 +3247,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;
> @@ -3263,16 +3266,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;
>               }
>           }
>       }
> @@ -3289,11 +3296,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;
>   }
>   
> @@ -3434,6 +3444,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();
>   
> @@ -3478,8 +3489,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;

Re: [PATCH 07/21] migration: Correct transferred bytes value

[Juan Quintela]

Harsh Prateek Bora <harshpb@linux.ibm.com> wrote:
> On 5/8/23 18:38, 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
>
> s/whrer/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/meson.build |  2 +-
>>   migration/ram.c       | 14 ++++++++++++++
>>   migration/savevm.c    | 19 +++++++++++++++++--
>>   migration/vmstate.c   |  3 +++
>>   4 files changed, 35 insertions(+), 3 deletions(-)
>> diff --git a/migration/meson.build b/migration/meson.build
>> index da1897fadf..b27fc9eeb6 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',
>> @@ -19,7 +20,6 @@ softmmu_ss.add(files(
>>     'fd.c',
>>     'global_state.c',
>>     'migration-hmp-cmds.c',
>> -  'migration-stats.c',
>>     'migration.c',
>>     'multifd.c',
>>     'multifd-zlib.c',
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 5e7bf20ca5..5ae1fdba45 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);
>
> I see lot of hard-coded math like above in below code as well.
> Although compiler will do its optimization, but we may choose to write
> it like either of below:
>  - sizeof(??) + size + sizeof(??)
>  - <macro1> + size + <macro2>
>  - size + 16 /* explaining what 8 + 8 means here */
> I guess first method or usage of macros instead of hard-coded numbers
> is better. Applies to all instances below.

It is removed later (on my tree).

The reason this patch is here is that I get the same value with
transferred and with the qemu_file_size and multifd_bytes together.
That makes much easier to verify that I was not doing something wrong.

Later, Juan.

> regards,
> Harsh
>
>>       if (qemu_file_get_error(file)) {
>>           return qemu_file_get_error(file);
>>       }
>> @@ -1617,6 +1618,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);
>>               }
>>               /*
>> @@ -3245,6 +3247,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;
>> @@ -3263,16 +3266,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;
>>               }
>>           }
>>       }
>> @@ -3289,11 +3296,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;
>>   }
>>   @@ -3434,6 +3444,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();
>>   @@ -3478,8 +3489,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;

[PATCH 08/21] migration: Move setup_time to mig_stats

[Juan Quintela]

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

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-stats.c |  7 +++++++
 migration/migration-stats.h | 14 ++++++++++++++
 migration/migration.c       |  9 ++++-----
 migration/migration.h       |  1 -
 4 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index 2f2cea965c..5278c6c821 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 calculate_time_since(Stat64 *val, int64_t since)
+{
+    int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST);
+    stat64_set(val, now - since);
+}
diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index cf8a4f0410..73c73d75b9 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -69,6 +69,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.
      */
@@ -81,4 +85,14 @@ typedef struct {
 
 extern MigrationAtomicStats mig_stats;
 
+/**
+ * calculate_time_since: Calculate how much time has passed
+ *
+ * @val: stat64 where to store the time
+ * @since: reference time since we want to calculate
+ *
+ * Returns: Nothing.  The time is stored in val.
+ */
+
+void calculate_time_since(Stat64 *val, int64_t since);
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index b1cfb56523..72286de969 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -884,7 +884,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;
@@ -1387,7 +1387,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;
@@ -2640,7 +2639,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;
     }
@@ -2965,7 +2964,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;
+    calculate_time_since(&mig_stats.setup_time, setup_start);
 
     trace_migration_thread_setup_complete();
 
@@ -3077,7 +3076,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;
+    calculate_time_since(&mig_stats.setup_time, setup_start);
 
     trace_migration_thread_setup_complete();
     s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
diff --git a/migration/migration.h b/migration/migration.h
index 3a918514e7..7f554455ac 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -311,7 +311,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
-- 
2.40.0

Re: [PATCH 08/21] migration: Move setup_time to mig_stats

[Cédric Le Goater]

On 5/8/23 15:08, Juan Quintela wrote:
> It is a time that needs to be cleaned each time cancel migration.
> Once there ccreate calculate_time_since() to calculate how time since
> a time in the past.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>   migration/migration-stats.c |  7 +++++++
>   migration/migration-stats.h | 14 ++++++++++++++
>   migration/migration.c       |  9 ++++-----
>   migration/migration.h       |  1 -
>   4 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index 2f2cea965c..5278c6c821 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 calculate_time_since(Stat64 *val, int64_t since)
> +{
> +    int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> +    stat64_set(val, now - since);
> +}
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index cf8a4f0410..73c73d75b9 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -69,6 +69,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.
>        */
> @@ -81,4 +85,14 @@ typedef struct {
>   
>   extern MigrationAtomicStats mig_stats;
>   
> +/**
> + * calculate_time_since: Calculate how much time has passed
> + *
> + * @val: stat64 where to store the time
> + * @since: reference time since we want to calculate
> + *
> + * Returns: Nothing.  The time is stored in val.
> + */
> +
> +void calculate_time_since(Stat64 *val, int64_t since);

Since this routine is in the "migration" namespace, I would rename it to

   void migration_time_since(Stat64 *val, int64_t since);

of even

   void migration_time_since(MigrationAtomicStats *stat, int64_t since);

Do you need it elsewhere than in migration.c ?

Thanks,

C.

>   #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index b1cfb56523..72286de969 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -884,7 +884,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;
> @@ -1387,7 +1387,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;
> @@ -2640,7 +2639,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;
>       }
> @@ -2965,7 +2964,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;
> +    calculate_time_since(&mig_stats.setup_time, setup_start);
>   
>       trace_migration_thread_setup_complete();
>   
> @@ -3077,7 +3076,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;
> +    calculate_time_since(&mig_stats.setup_time, setup_start);
>   
>       trace_migration_thread_setup_complete();
>       s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> diff --git a/migration/migration.h b/migration/migration.h
> index 3a918514e7..7f554455ac 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -311,7 +311,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

Re: [PATCH 08/21] migration: Move setup_time to mig_stats

[Juan Quintela]

Cédric Le Goater <clg@kaod.org> wrote:
> On 5/8/23 15:08, Juan Quintela wrote:
>> It is a time that needs to be cleaned each time cancel migration.
>> Once there ccreate calculate_time_since() to calculate how time since
>> a time in the past.
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>   migration/migration-stats.c |  7 +++++++
>>   migration/migration-stats.h | 14 ++++++++++++++
>>   migration/migration.c       |  9 ++++-----
>>   migration/migration.h       |  1 -
>>   4 files changed, 25 insertions(+), 6 deletions(-)
>> diff --git a/migration/migration-stats.c
>> b/migration/migration-stats.c
>> index 2f2cea965c..5278c6c821 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 calculate_time_since(Stat64 *val, int64_t since)
>> +{
>> +    int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST);
>> +    stat64_set(val, now - since);
>> +}
>> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
>> index cf8a4f0410..73c73d75b9 100644
>> --- a/migration/migration-stats.h
>> +++ b/migration/migration-stats.h
>> @@ -69,6 +69,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.
>>        */
>> @@ -81,4 +85,14 @@ typedef struct {
>>     extern MigrationAtomicStats mig_stats;
>>   +/**
>> + * calculate_time_since: Calculate how much time has passed
>> + *
>> + * @val: stat64 where to store the time
>> + * @since: reference time since we want to calculate
>> + *
>> + * Returns: Nothing.  The time is stored in val.
>> + */
>> +
>> +void calculate_time_since(Stat64 *val, int64_t since);
>
> Since this routine is in the "migration" namespace, I would rename it to
>
>   void migration_time_since(Stat64 *val, int64_t since);
>
> of even
>
>   void migration_time_since(MigrationAtomicStats *stat, int64_t since);
>
> Do you need it elsewhere than in migration.c ?

Not yet.

I can change to this and later change if needed.

Thanks, Juan.

[PATCH 09/21] 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>
---
 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 6ebc2bd3ec..8de1ecd082 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -301,7 +301,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);
@@ -518,7 +520,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);
 }
 
@@ -536,7 +537,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;
@@ -553,7 +553,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.0

Re: [PATCH 09/21] qemu-file: Account for rate_limit usage on qemu_fflush()

[Cédric Le Goater]

On 5/8/23 15:08, 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>

Thanks,

C.

> ---
>   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 6ebc2bd3ec..8de1ecd082 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -301,7 +301,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);
> @@ -518,7 +520,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);
>   }
>   
> @@ -536,7 +537,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;
> @@ -553,7 +553,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);
>   }
>   

[PATCH 10/21] migration: Move rate_limit_max and rate_limit_used to migration_stats

[Juan Quintela]

This 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_limit_exceeded
qemu_file_set_rate_limit -> migration_rate_limit_set
qemu_file_get_rate_limit -> migration_rate_limit_get
qemu_file_reset_rate_limit -> migration_rate_limit_reset
qemu_file_acct_rate_limit -> migration_rate_limit_account.

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

---

If you have any good suggestion for better names, I am all ears.
---
 hw/ppc/spapr.c                      |  5 +--
 hw/s390x/s390-stattrib.c            |  2 +-
 include/migration/qemu-file-types.h |  2 +-
 migration/block-dirty-bitmap.c      |  2 +-
 migration/block.c                   |  5 +--
 migration/migration-stats.c         | 41 ++++++++++++++++++++++
 migration/migration-stats.h         | 42 +++++++++++++++++++++++
 migration/migration.c               | 14 ++++----
 migration/multifd.c                 |  2 +-
 migration/options.c                 |  7 ++--
 migration/qemu-file.c               | 53 ++---------------------------
 migration/qemu-file.h               | 11 ------
 migration/ram.c                     |  2 +-
 migration/savevm.c                  |  2 +-
 14 files changed, 108 insertions(+), 82 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ddc9c7b1a1..dbd2753278 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_limit_exceeded(f));
 
     if (index >= htabslots) {
         assert(index == htabslots);
@@ -2237,7 +2237,8 @@ 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_limit_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..fb0a20f2e1 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_limit_exceeded(f) == 0) {
         reallen = sac->get_stattr(sas, &start_gfn, buflen, buf);
         if (reallen < 0) {
             g_free(buf);
diff --git a/include/migration/qemu-file-types.h b/include/migration/qemu-file-types.h
index 1436f9ce92..0354f45198 100644
--- a/include/migration/qemu-file-types.h
+++ b/include/migration/qemu-file-types.h
@@ -165,6 +165,6 @@ 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);
+bool migration_rate_limit_exceeded(QEMUFile *f);
 
 #endif
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 20f36e6bd8..a815678926 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_limit_exceeded(f)) {
                 return;
             }
         }
diff --git a/migration/block.c b/migration/block.c
index 12617b4152..fc1caa9ca6 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_limit_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_limit_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 5278c6c821..e01842cabc 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,43 @@ void calculate_time_since(Stat64 *val, int64_t since)
     int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST);
     stat64_set(val, now - since);
 }
+
+bool migration_rate_limit_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);
+    /*
+     *  rate_limit_max == 0 means no rate_limit enfoncement.
+     */
+    if (rate_limit_max > 0 && rate_limit_used > rate_limit_max) {
+        return true;
+    }
+    return false;
+}
+
+uint64_t migration_rate_limit_get(void)
+{
+    return stat64_get(&mig_stats.rate_limit_max);
+}
+
+void migration_rate_limit_set(uint64_t limit)
+{
+    /*
+     * 'limit' is per second.  But we check it each BUFER_DELAY miliseconds.
+     */
+    stat64_set(&mig_stats.rate_limit_max, limit);
+}
+
+void migration_rate_limit_reset(void)
+{
+    stat64_set(&mig_stats.rate_limit_used, 0);
+}
+
+void migration_rate_limit_account(uint64_t len)
+{
+    stat64_add(&mig_stats.rate_limit_used, len);
+}
diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index 73c73d75b9..65f11ec7d1 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -69,6 +69,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.
      */
@@ -95,4 +103,38 @@ extern MigrationAtomicStats mig_stats;
  */
 
 void calculate_time_since(Stat64 *val, int64_t since);
+
+/**
+ * migration_rate_limit_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_limit_account(uint64_t len);
+
+/**
+ * migration_rate_limit_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_limit_get(void);
+
+/**
+ * migration_rate_limit_reset: Reset the rate limit counter.
+ *
+ * This is called when we know we start a new transfer cycle.
+ */
+void migration_rate_limit_reset(void);
+
+/**
+ * migration_rate_limit_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_limit_set(uint64_t new_rate);
+
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index 72286de969..370998600e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2116,7 +2116,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_limit_set(bandwidth);
     if (migrate_postcopy_ram()) {
         /* Ping just for debugging, helps line traces up */
         qemu_savevm_send_ping(ms->to_dst_file, 2);
@@ -2295,7 +2295,7 @@ static void migration_completion(MigrationState *s)
             }
             if (ret >= 0) {
                 s->block_inactive = !migrate_colo();
-                qemu_file_set_rate_limit(s->to_dst_file, 0);
+                migration_rate_limit_set(0);
                 ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
                                                          s->block_inactive);
             }
@@ -2691,7 +2691,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_limit_reset();
 
     update_iteration_initial_status(s);
 
@@ -2847,7 +2847,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_limit_exceeded(s->to_dst_file)) {
 
         if (qemu_file_get_error(s->to_dst_file)) {
             return false;
@@ -2969,7 +2969,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_limit_exceeded(s->to_dst_file)) {
             MigIterateState iter_state = migration_iteration_run(s);
             if (iter_state == MIG_ITERATE_SKIP) {
                 continue;
@@ -3043,7 +3043,7 @@ static void *bg_migration_thread(void *opaque)
     rcu_register_thread();
     object_ref(OBJECT(s));
 
-    qemu_file_set_rate_limit(s->to_dst_file, 0);
+    migration_rate_limit_set(0);
 
     setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
     /*
@@ -3215,7 +3215,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_limit_set(rate_limit);
     qemu_file_set_blocking(s->to_dst_file, true);
 
     /*
diff --git a/migration/multifd.c b/migration/multifd.c
index 4e71c19292..2efb313be4 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -432,7 +432,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_limit_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 d04b5fbc3a..a024fa3ce6 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_limit_set(s->parameters.max_bandwidth);
         }
     }
 
@@ -1274,8 +1274,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_limit_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 8de1ecd082..3f993e24af 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"
@@ -40,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;
 
@@ -302,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_limit_account(size);
             f->total_transferred += size;
         }
 
@@ -355,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_limit_account(size);
         }
 
         if (ret != RAM_SAVE_CONTROL_DELAYED &&
@@ -726,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;
-    }
-    /*
-     *  rate_limit_max == 0 means no rate_limit enfoncement.
-     */
-    if (f->rate_limit_max > 0 && 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/qemu-file.h b/migration/qemu-file.h
index ab164a58d0..46029b951c 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -129,17 +129,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/migration/ram.c b/migration/ram.c
index 5ae1fdba45..2339a99932 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3351,7 +3351,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_limit_exceeded(f)) == 0 ||
                postcopy_has_request(rs)) {
             int pages;
 
diff --git a/migration/savevm.c b/migration/savevm.c
index c7af9050c2..376118bc98 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_limit_exceeded(f)) {
             return 0;
         }
         trace_savevm_section_start(se->idstr, se->section_id);
-- 
2.40.0

Re: [PATCH 10/21] migration: Move rate_limit_max and rate_limit_used to migration_stats

[Harsh Prateek Bora]

On 5/8/23 18:38, Juan Quintela wrote:
> This way we can make them atomic and use this functions from any

s/this/these

> 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_limit_exceeded
> qemu_file_set_rate_limit -> migration_rate_limit_set
> qemu_file_get_rate_limit -> migration_rate_limit_get
> qemu_file_reset_rate_limit -> migration_rate_limit_reset
> qemu_file_acct_rate_limit -> migration_rate_limit_account.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> ---
> 
> If you have any good suggestion for better names, I am all ears.
> ---
>   hw/ppc/spapr.c                      |  5 +--
>   hw/s390x/s390-stattrib.c            |  2 +-
>   include/migration/qemu-file-types.h |  2 +-
>   migration/block-dirty-bitmap.c      |  2 +-
>   migration/block.c                   |  5 +--
>   migration/migration-stats.c         | 41 ++++++++++++++++++++++
>   migration/migration-stats.h         | 42 +++++++++++++++++++++++
>   migration/migration.c               | 14 ++++----
>   migration/multifd.c                 |  2 +-
>   migration/options.c                 |  7 ++--
>   migration/qemu-file.c               | 53 ++---------------------------
>   migration/qemu-file.h               | 11 ------
>   migration/ram.c                     |  2 +-
>   migration/savevm.c                  |  2 +-
>   14 files changed, 108 insertions(+), 82 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ddc9c7b1a1..dbd2753278 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_limit_exceeded(f));
>   
>       if (index >= htabslots) {
>           assert(index == htabslots);
> @@ -2237,7 +2237,8 @@ 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_limit_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..fb0a20f2e1 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_limit_exceeded(f) == 0) {
        while (final ? 1 : !migration_rate_limit_exceeded(f)) {
>           reallen = sac->get_stattr(sas, &start_gfn, buflen, buf);
>           if (reallen < 0) {
>               g_free(buf);
> diff --git a/include/migration/qemu-file-types.h b/include/migration/qemu-file-types.h
> index 1436f9ce92..0354f45198 100644
> --- a/include/migration/qemu-file-types.h
> +++ b/include/migration/qemu-file-types.h
> @@ -165,6 +165,6 @@ 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);
> +bool migration_rate_limit_exceeded(QEMUFile *f);
> return type is also getting changed, could be mentioned in commit log.

>   #endif
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 20f36e6bd8..a815678926 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_limit_exceeded(f)) {
>                   return;
>               }
>           }
> diff --git a/migration/block.c b/migration/block.c
> index 12617b4152..fc1caa9ca6 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_limit_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_limit_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 5278c6c821..e01842cabc 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,43 @@ void calculate_time_since(Stat64 *val, int64_t since)
>       int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST);
>       stat64_set(val, now - since);
>   }
> +
> +bool migration_rate_limit_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);
> +    /*
> +     *  rate_limit_max == 0 means no rate_limit enfoncement.
> +     */
> +    if (rate_limit_max > 0 && rate_limit_used > rate_limit_max) {
> +        return true;
> +    }
> +    return false;
> +}
> +
> +uint64_t migration_rate_limit_get(void)
> +{
> +    return stat64_get(&mig_stats.rate_limit_max);
> +}
> +
> +void migration_rate_limit_set(uint64_t limit)
> +{
> +    /*
> +     * 'limit' is per second.  But we check it each BUFER_DELAY miliseconds.
> +     */
> +    stat64_set(&mig_stats.rate_limit_max, limit);
> +}
> +
> +void migration_rate_limit_reset(void)
> +{
> +    stat64_set(&mig_stats.rate_limit_used, 0);
> +}
> +
> +void migration_rate_limit_account(uint64_t len)
> +{
> +    stat64_add(&mig_stats.rate_limit_used, len);
> +}
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index 73c73d75b9..65f11ec7d1 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -69,6 +69,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.
>        */
> @@ -95,4 +103,38 @@ extern MigrationAtomicStats mig_stats;
>    */
>   
>   void calculate_time_since(Stat64 *val, int64_t since);
> +
> +/**
> + * migration_rate_limit_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_limit_account(uint64_t len);
> +
> +/**
> + * migration_rate_limit_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_limit_get(void);
> +
> +/**
> + * migration_rate_limit_reset: Reset the rate limit counter.
> + *
> + * This is called when we know we start a new transfer cycle.
> + */
> +void migration_rate_limit_reset(void);
> +
> +/**
> + * migration_rate_limit_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_limit_set(uint64_t new_rate);
> +
>   #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index 72286de969..370998600e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2116,7 +2116,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_limit_set(bandwidth);
>       if (migrate_postcopy_ram()) {
>           /* Ping just for debugging, helps line traces up */
>           qemu_savevm_send_ping(ms->to_dst_file, 2);
> @@ -2295,7 +2295,7 @@ static void migration_completion(MigrationState *s)
>               }
>               if (ret >= 0) {
>                   s->block_inactive = !migrate_colo();
> -                qemu_file_set_rate_limit(s->to_dst_file, 0);
> +                migration_rate_limit_set(0);
>                   ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
>                                                            s->block_inactive);
>               }
> @@ -2691,7 +2691,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_limit_reset();
>   
>       update_iteration_initial_status(s);
>   
> @@ -2847,7 +2847,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_limit_exceeded(s->to_dst_file)) {
>   
>           if (qemu_file_get_error(s->to_dst_file)) {
>               return false;
> @@ -2969,7 +2969,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_limit_exceeded(s->to_dst_file)) {
>               MigIterateState iter_state = migration_iteration_run(s);
>               if (iter_state == MIG_ITERATE_SKIP) {
>                   continue;
> @@ -3043,7 +3043,7 @@ static void *bg_migration_thread(void *opaque)
>       rcu_register_thread();
>       object_ref(OBJECT(s));
>   
> -    qemu_file_set_rate_limit(s->to_dst_file, 0);
> +    migration_rate_limit_set(0);
>   
>       setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
>       /*
> @@ -3215,7 +3215,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_limit_set(rate_limit);
>       qemu_file_set_blocking(s->to_dst_file, true);
>   
>       /*
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 4e71c19292..2efb313be4 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -432,7 +432,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_limit_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 d04b5fbc3a..a024fa3ce6 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_limit_set(s->parameters.max_bandwidth);
>           }
>       }
>   
> @@ -1274,8 +1274,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_limit_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 8de1ecd082..3f993e24af 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"
> @@ -40,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;
>   
> @@ -302,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_limit_account(size);
>               f->total_transferred += size;
>           }
>   
> @@ -355,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_limit_account(size);
>           }
>   
>           if (ret != RAM_SAVE_CONTROL_DELAYED &&
> @@ -726,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;
> -    }
> -    /*
> -     *  rate_limit_max == 0 means no rate_limit enfoncement.
> -     */
> -    if (f->rate_limit_max > 0 && 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/qemu-file.h b/migration/qemu-file.h
> index ab164a58d0..46029b951c 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -129,17 +129,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/migration/ram.c b/migration/ram.c
> index 5ae1fdba45..2339a99932 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3351,7 +3351,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_limit_exceeded(f)) == 0 ||
            while (!(ret = migration_rate_limit_exceeded(f))) ||

Otherwise, looks fine.

regards,
Harsh
>                  postcopy_has_request(rs)) {
>               int pages;
>   
> diff --git a/migration/savevm.c b/migration/savevm.c
> index c7af9050c2..376118bc98 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_limit_exceeded(f)) {
>               return 0;
>           }
>           trace_savevm_section_start(se->idstr, se->section_id);

Re: [PATCH 10/21] migration: Move rate_limit_max and rate_limit_used to migration_stats

[Juan Quintela]

Harsh Prateek Bora <harshpb@linux.ibm.com> wrote:
> On 5/8/23 18:38, Juan Quintela wrote:
>> This way we can make them atomic and use this functions from any
>
> s/this/these
>

Fixed.

Thanks.

Re: [PATCH 10/21] migration: Move rate_limit_max and rate_limit_used to migration_stats

[Harsh Prateek Bora]

On 5/9/23 16:40, Juan Quintela wrote:
> Harsh Prateek Bora <harshpb@linux.ibm.com> wrote:
>> On 5/8/23 18:38, Juan Quintela wrote:
>>> This way we can make them atomic and use this functions from any
>>
>> s/this/these
>>
> 
> Fixed.
> 
Sure, providing ack from ppc/spapr perspective.

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> Thanks.
> 
> 

Re: [PATCH 10/21] migration: Move rate_limit_max and rate_limit_used to migration_stats

[Cédric Le Goater]

On 5/8/23 15:08, Juan Quintela wrote:
> This 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_limit_exceeded
> qemu_file_set_rate_limit -> migration_rate_limit_set
> qemu_file_get_rate_limit -> migration_rate_limit_get
> qemu_file_reset_rate_limit -> migration_rate_limit_reset
> qemu_file_acct_rate_limit -> migration_rate_limit_account.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> ---
> 
> If you have any good suggestion for better names, I am all ears.

May be :

  qemu_file_rate_limit -> migration_rate_limit_is_exceeded
  qemu_file_acct_rate_limit -> migration_rate_limit_inc


Also, migration_rate_limit() would need some prefix to understand what is
its purpose.

Do we really need "_limit" in the names ?

Thanks,

C.

> ---
>   hw/ppc/spapr.c                      |  5 +--
>   hw/s390x/s390-stattrib.c            |  2 +-
>   include/migration/qemu-file-types.h |  2 +-
>   migration/block-dirty-bitmap.c      |  2 +-
>   migration/block.c                   |  5 +--
>   migration/migration-stats.c         | 41 ++++++++++++++++++++++
>   migration/migration-stats.h         | 42 +++++++++++++++++++++++
>   migration/migration.c               | 14 ++++----
>   migration/multifd.c                 |  2 +-
>   migration/options.c                 |  7 ++--
>   migration/qemu-file.c               | 53 ++---------------------------
>   migration/qemu-file.h               | 11 ------
>   migration/ram.c                     |  2 +-
>   migration/savevm.c                  |  2 +-
>   14 files changed, 108 insertions(+), 82 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ddc9c7b1a1..dbd2753278 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_limit_exceeded(f));
>   
>       if (index >= htabslots) {
>           assert(index == htabslots);
> @@ -2237,7 +2237,8 @@ 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_limit_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..fb0a20f2e1 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_limit_exceeded(f) == 0) {
>           reallen = sac->get_stattr(sas, &start_gfn, buflen, buf);
>           if (reallen < 0) {
>               g_free(buf);
> diff --git a/include/migration/qemu-file-types.h b/include/migration/qemu-file-types.h
> index 1436f9ce92..0354f45198 100644
> --- a/include/migration/qemu-file-types.h
> +++ b/include/migration/qemu-file-types.h
> @@ -165,6 +165,6 @@ 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);
> +bool migration_rate_limit_exceeded(QEMUFile *f);
>   
>   #endif
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 20f36e6bd8..a815678926 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_limit_exceeded(f)) {
>                   return;
>               }
>           }
> diff --git a/migration/block.c b/migration/block.c
> index 12617b4152..fc1caa9ca6 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_limit_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_limit_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 5278c6c821..e01842cabc 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,43 @@ void calculate_time_since(Stat64 *val, int64_t since)
>       int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST);
>       stat64_set(val, now - since);
>   }
> +
> +bool migration_rate_limit_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);
> +    /*
> +     *  rate_limit_max == 0 means no rate_limit enfoncement.
> +     */
> +    if (rate_limit_max > 0 && rate_limit_used > rate_limit_max) {
> +        return true;
> +    }
> +    return false;
> +}
> +
> +uint64_t migration_rate_limit_get(void)
> +{
> +    return stat64_get(&mig_stats.rate_limit_max);
> +}
> +
> +void migration_rate_limit_set(uint64_t limit)
> +{
> +    /*
> +     * 'limit' is per second.  But we check it each BUFER_DELAY miliseconds.
> +     */
> +    stat64_set(&mig_stats.rate_limit_max, limit);
> +}
> +
> +void migration_rate_limit_reset(void)
> +{
> +    stat64_set(&mig_stats.rate_limit_used, 0);
> +}
> +
> +void migration_rate_limit_account(uint64_t len)
> +{
> +    stat64_add(&mig_stats.rate_limit_used, len);
> +}
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index 73c73d75b9..65f11ec7d1 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -69,6 +69,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.
>        */
> @@ -95,4 +103,38 @@ extern MigrationAtomicStats mig_stats;
>    */
>   
>   void calculate_time_since(Stat64 *val, int64_t since);
> +
> +/**
> + * migration_rate_limit_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_limit_account(uint64_t len);
> +
> +/**
> + * migration_rate_limit_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_limit_get(void);
> +
> +/**
> + * migration_rate_limit_reset: Reset the rate limit counter.
> + *
> + * This is called when we know we start a new transfer cycle.
> + */
> +void migration_rate_limit_reset(void);
> +
> +/**
> + * migration_rate_limit_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_limit_set(uint64_t new_rate);
> +
>   #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index 72286de969..370998600e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2116,7 +2116,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_limit_set(bandwidth);
>       if (migrate_postcopy_ram()) {
>           /* Ping just for debugging, helps line traces up */
>           qemu_savevm_send_ping(ms->to_dst_file, 2);
> @@ -2295,7 +2295,7 @@ static void migration_completion(MigrationState *s)
>               }
>               if (ret >= 0) {
>                   s->block_inactive = !migrate_colo();
> -                qemu_file_set_rate_limit(s->to_dst_file, 0);
> +                migration_rate_limit_set(0);
>                   ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
>                                                            s->block_inactive);
>               }
> @@ -2691,7 +2691,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_limit_reset();
>   
>       update_iteration_initial_status(s);
>   
> @@ -2847,7 +2847,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_limit_exceeded(s->to_dst_file)) {
>   
>           if (qemu_file_get_error(s->to_dst_file)) {
>               return false;
> @@ -2969,7 +2969,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_limit_exceeded(s->to_dst_file)) {
>               MigIterateState iter_state = migration_iteration_run(s);
>               if (iter_state == MIG_ITERATE_SKIP) {
>                   continue;
> @@ -3043,7 +3043,7 @@ static void *bg_migration_thread(void *opaque)
>       rcu_register_thread();
>       object_ref(OBJECT(s));
>   
> -    qemu_file_set_rate_limit(s->to_dst_file, 0);
> +    migration_rate_limit_set(0);
>   
>       setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
>       /*
> @@ -3215,7 +3215,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_limit_set(rate_limit);
>       qemu_file_set_blocking(s->to_dst_file, true);
>   
>       /*
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 4e71c19292..2efb313be4 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -432,7 +432,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_limit_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 d04b5fbc3a..a024fa3ce6 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_limit_set(s->parameters.max_bandwidth);
>           }
>       }
>   
> @@ -1274,8 +1274,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_limit_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 8de1ecd082..3f993e24af 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"
> @@ -40,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;
>   
> @@ -302,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_limit_account(size);
>               f->total_transferred += size;
>           }
>   
> @@ -355,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_limit_account(size);
>           }
>   
>           if (ret != RAM_SAVE_CONTROL_DELAYED &&
> @@ -726,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;
> -    }
> -    /*
> -     *  rate_limit_max == 0 means no rate_limit enfoncement.
> -     */
> -    if (f->rate_limit_max > 0 && 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/qemu-file.h b/migration/qemu-file.h
> index ab164a58d0..46029b951c 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -129,17 +129,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/migration/ram.c b/migration/ram.c
> index 5ae1fdba45..2339a99932 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3351,7 +3351,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_limit_exceeded(f)) == 0 ||
>                  postcopy_has_request(rs)) {
>               int pages;
>   
> diff --git a/migration/savevm.c b/migration/savevm.c
> index c7af9050c2..376118bc98 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_limit_exceeded(f)) {
>               return 0;
>           }
>           trace_savevm_section_start(se->idstr, se->section_id);

Re: [PATCH 10/21] migration: Move rate_limit_max and rate_limit_used to migration_stats

[Juan Quintela]

Cédric Le Goater <clg@kaod.org> wrote:
> On 5/8/23 15:08, Juan Quintela wrote:
>> This 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_limit_exceeded
>> qemu_file_set_rate_limit -> migration_rate_limit_set
>> qemu_file_get_rate_limit -> migration_rate_limit_get
>> qemu_file_reset_rate_limit -> migration_rate_limit_reset
>> qemu_file_acct_rate_limit -> migration_rate_limit_account.
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>> If you have any good suggestion for better names, I am all ears.
>
> May be :
>
>  qemu_file_rate_limit -> migration_rate_limit_is_exceeded

I try not to put _is_ in function names.  If it needs to be there, I
think that I need to rename the functino.

migration_rate_limit_exceeded()

seems clear to me.


>  qemu_file_acct_rate_limit -> migration_rate_limit_inc

My problem for this one is that we are not increasing the rate_limit, we
are "decreasing" the amount of data we have for this period.  That is
why I thought about _account(), but who knows.


> Also, migration_rate_limit() would need some prefix to understand what is
> its purpose.

What do you mean here?
This is the only rate_limit that I can think in migration.

> Do we really need "_limit" in the names ?

You have a point here.

If nobody complains/suggest anything else, I will drop the _limit for
the next submission.

Thanks very much.

Re: [PATCH 10/21] migration: Move rate_limit_max and rate_limit_used to migration_stats

[Cédric Le Goater]

On 5/15/23 15:09, Juan Quintela wrote:
> Cédric Le Goater <clg@kaod.org> wrote:
>> On 5/8/23 15:08, Juan Quintela wrote:
>>> This 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_limit_exceeded
>>> qemu_file_set_rate_limit -> migration_rate_limit_set
>>> qemu_file_get_rate_limit -> migration_rate_limit_get
>>> qemu_file_reset_rate_limit -> migration_rate_limit_reset
>>> qemu_file_acct_rate_limit -> migration_rate_limit_account.
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>> If you have any good suggestion for better names, I am all ears.
>>
>> May be :
>>
>>   qemu_file_rate_limit -> migration_rate_limit_is_exceeded
> 
> I try not to put _is_ in function names.  If it needs to be there, I
> think that I need to rename the functino.

It is common practice for functions doing a simple test and returning a bool.
No big deal anyway.
  
> migration_rate_limit_exceeded()
> 
> seems clear to me.
> 
>>   qemu_file_acct_rate_limit -> migration_rate_limit_inc
> 
> My problem for this one is that we are not increasing the rate_limit, we
> are "decreasing" the amount of data we have for this period.  That is
> why I thought about _account(), but who knows.
> 
> 
>> Also, migration_rate_limit() would need some prefix to understand what is
>> its purpose.
> 
> What do you mean here?

I am referring to :

   /* Returns true if the rate limiting was broken by an urgent request */
   bool migration_rate_limit(void)
   {
       ...
       return urgent;
   }

which existed prior to the name changes and I thought migration_rate_limit()
would suffer the same fate. May be keep the '_limit' suffix for this one if
you remove it for the others ?

Thanks,

C.


> This is the only rate_limit that I can think in migration.
> 
>> Do we really need "_limit" in the names ?
> 
> You have a point here.
> 
> If nobody complains/suggest anything else, I will drop the _limit for
> the next submission.
> 
> Thanks very much.
> 

Re: [PATCH 10/21] migration: Move rate_limit_max and rate_limit_used to migration_stats

[Juan Quintela]

Cédric Le Goater <clg@kaod.org> wrote:
> On 5/15/23 15:09, Juan Quintela wrote:
>> Cédric Le Goater <clg@kaod.org> wrote:
>>> On 5/8/23 15:08, Juan Quintela wrote:
>>>> This 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_limit_exceeded
>>>> qemu_file_set_rate_limit -> migration_rate_limit_set
>>>> qemu_file_get_rate_limit -> migration_rate_limit_get
>>>> qemu_file_reset_rate_limit -> migration_rate_limit_reset
>>>> qemu_file_acct_rate_limit -> migration_rate_limit_account.
>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>> ---
>>>> If you have any good suggestion for better names, I am all ears.
>>>
>>> May be :
>>>
>>>   qemu_file_rate_limit -> migration_rate_limit_is_exceeded
>> I try not to put _is_ in function names.  If it needs to be there, I
>> think that I need to rename the functino.
>
> It is common practice for functions doing a simple test and returning a bool.
> No big deal anyway.
>  > migration_rate_limit_exceeded()
>> seems clear to me.
>> 
>>>   qemu_file_acct_rate_limit -> migration_rate_limit_inc
>> My problem for this one is that we are not increasing the
>> rate_limit, we
>> are "decreasing" the amount of data we have for this period.  That is
>> why I thought about _account(), but who knows.
>> 
>>> Also, migration_rate_limit() would need some prefix to understand what is
>>> its purpose.
>> What do you mean here?
>
> I am referring to :
>
>   /* Returns true if the rate limiting was broken by an urgent request */
>   bool migration_rate_limit(void)
>   {
>       ...
>       return urgent;
>   }
>
> which existed prior to the name changes and I thought migration_rate_limit()
> would suffer the same fate. May be keep the '_limit' suffix for this one if
> you remove it for the others ?

ok, will think about this one.

Later, Juan.

Re: [PATCH 10/21] migration: Move rate_limit_max and rate_limit_used to migration_stats

[Juan Quintela]

Cédric Le Goater <clg@kaod.org> wrote:
> On 5/15/23 15:09, Juan Quintela wrote:
>> Cédric Le Goater <clg@kaod.org> wrote:
>>> On 5/8/23 15:08, Juan Quintela wrote:
>>>> This 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_limit_exceeded
>>>> qemu_file_set_rate_limit -> migration_rate_limit_set
>>>> qemu_file_get_rate_limit -> migration_rate_limit_get
>>>> qemu_file_reset_rate_limit -> migration_rate_limit_reset
>>>> qemu_file_acct_rate_limit -> migration_rate_limit_account.
>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>> ---
>>>> If you have any good suggestion for better names, I am all ears.
>>>
>>> May be :
>>>
>>>   qemu_file_rate_limit -> migration_rate_limit_is_exceeded
>> I try not to put _is_ in function names.  If it needs to be there, I
>> think that I need to rename the functino.
>
> It is common practice for functions doing a simple test and returning a bool.
> No big deal anyway.
>  > migration_rate_limit_exceeded()
>> seems clear to me.
>> 
>>>   qemu_file_acct_rate_limit -> migration_rate_limit_inc
>> My problem for this one is that we are not increasing the
>> rate_limit, we
>> are "decreasing" the amount of data we have for this period.  That is
>> why I thought about _account(), but who knows.
>> 
>>> Also, migration_rate_limit() would need some prefix to understand what is
>>> its purpose.
>> What do you mean here?
>
> I am referring to :
>
>   /* Returns true if the rate limiting was broken by an urgent request */
>   bool migration_rate_limit(void)
>   {
>       ...
>       return urgent;
>   }

out of ideas:

migration_rate_wait()
- the good
  *we wait if we have to
- the bad
  we can be interrupted if there is anything urgent
  we only wait if counters says that we have to

migration_rate_check()
* we always check
* we return a value consistent with checking
* but we check if we have to wait, not if there is anythying urgent

I am leaving it with migration_rate_limit() name until someone cames
with a better one.  It is not worse than what we have in.


>
> which existed prior to the name changes and I thought migration_rate_limit()
> would suffer the same fate. May be keep the '_limit' suffix for this one if
> you remove it for the others ?

I am no sure if migration_rate() is better than migration_rate_limit().

Later, Juan.

[PATCH 11/21] 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>
---
 migration/migration-stats.c |  6 ++++++
 migration/migration-stats.h |  9 +++++++++
 migration/migration.c       | 13 +++----------
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index e01842cabc..fba66c4577 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -63,3 +63,9 @@ void migration_rate_limit_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-stats.h b/migration/migration-stats.h
index 65f11ec7d1..c82fce9608 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -137,4 +137,13 @@ void migration_rate_limit_reset(void);
  */
 void migration_rate_limit_set(uint64_t new_rate);
 
+/**
+ * migration_transferred_bytes: Return number of bytes transferred
+ *
+ * Returtns 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.c b/migration/migration.c
index 370998600e..e6d262ffe1 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2617,16 +2617,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;
 
@@ -2652,7 +2645,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();
 }
 
@@ -2667,7 +2660,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.0

Re: [PATCH 11/21] migration: Move migration_total_bytes() to migration-stats.c

[Cédric Le Goater]

On 5/8/23 15:08, 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>

C.


> ---
>   migration/migration-stats.c |  6 ++++++
>   migration/migration-stats.h |  9 +++++++++
>   migration/migration.c       | 13 +++----------
>   3 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index e01842cabc..fba66c4577 100644
> --- a/migration/migration-stats.c
> +++ b/migration/migration-stats.c
> @@ -63,3 +63,9 @@ void migration_rate_limit_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-stats.h b/migration/migration-stats.h
> index 65f11ec7d1..c82fce9608 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -137,4 +137,13 @@ void migration_rate_limit_reset(void);
>    */
>   void migration_rate_limit_set(uint64_t new_rate);
>   
> +/**
> + * migration_transferred_bytes: Return number of bytes transferred
> + *
> + * Returtns 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.c b/migration/migration.c
> index 370998600e..e6d262ffe1 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2617,16 +2617,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;
>   
> @@ -2652,7 +2645,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();
>   }
>   
> @@ -2667,7 +2660,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;

[PATCH 12/21] migration: Add a trace for migration_transferred_bytes

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 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 fba66c4577..46b2b0d06e 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;
@@ -66,6 +67,9 @@ void migration_rate_limit_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 92161eeac5..4b6e802833 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.0

Re: [PATCH 12/21] migration: Add a trace for migration_transferred_bytes

[Cédric Le Goater]

On 5/8/23 15:09, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>

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

Thanks,

C.


> ---
>   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 fba66c4577..46b2b0d06e 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;
> @@ -66,6 +67,9 @@ void migration_rate_limit_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 92161eeac5..4b6e802833 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"

[PATCH 13/21] migration: Use migration_transferred_bytes() to calculate rate_limit

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-stats.c | 7 +++++--
 migration/migration-stats.h | 6 +++++-
 migration/migration.c       | 2 +-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index 46b2b0d06e..eb1a2c1ad4 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -31,7 +31,9 @@ bool migration_rate_limit_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);
     /*
      *  rate_limit_max == 0 means no rate_limit enfoncement.
@@ -55,9 +57,10 @@ void migration_rate_limit_set(uint64_t limit)
     stat64_set(&mig_stats.rate_limit_max, limit);
 }
 
-void migration_rate_limit_reset(void)
+void migration_rate_limit_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_limit_account(uint64_t len)
diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index c82fce9608..4029f1deab 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -69,6 +69,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.
      */
@@ -126,7 +130,7 @@ uint64_t migration_rate_limit_get(void);
  *
  * This is called when we know we start a new transfer cycle.
  */
-void migration_rate_limit_reset(void);
+void migration_rate_limit_reset(QEMUFile *f);
 
 /**
  * migration_rate_limit_set: Set the maximum amount that can be transferred.
diff --git a/migration/migration.c b/migration/migration.c
index e6d262ffe1..6922c612e4 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2684,7 +2684,7 @@ static void migration_update_counters(MigrationState *s,
             stat64_get(&mig_stats.dirty_bytes_last_sync) / bandwidth;
     }
 
-    migration_rate_limit_reset();
+    migration_rate_limit_reset(s->to_dst_file);
 
     update_iteration_initial_status(s);
 
-- 
2.40.0

Re: [PATCH 13/21] migration: Use migration_transferred_bytes() to calculate rate_limit

[Cédric Le Goater]

On 5/8/23 15:09, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>

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

C.

> ---
>   migration/migration-stats.c | 7 +++++--
>   migration/migration-stats.h | 6 +++++-
>   migration/migration.c       | 2 +-
>   3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index 46b2b0d06e..eb1a2c1ad4 100644
> --- a/migration/migration-stats.c
> +++ b/migration/migration-stats.c
> @@ -31,7 +31,9 @@ bool migration_rate_limit_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);
>       /*
>        *  rate_limit_max == 0 means no rate_limit enfoncement.
> @@ -55,9 +57,10 @@ void migration_rate_limit_set(uint64_t limit)
>       stat64_set(&mig_stats.rate_limit_max, limit);
>   }
>   
> -void migration_rate_limit_reset(void)
> +void migration_rate_limit_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_limit_account(uint64_t len)
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index c82fce9608..4029f1deab 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -69,6 +69,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.
>        */
> @@ -126,7 +130,7 @@ uint64_t migration_rate_limit_get(void);
>    *
>    * This is called when we know we start a new transfer cycle.
>    */
> -void migration_rate_limit_reset(void);
> +void migration_rate_limit_reset(QEMUFile *f);
>   
>   /**
>    * migration_rate_limit_set: Set the maximum amount that can be transferred.
> diff --git a/migration/migration.c b/migration/migration.c
> index e6d262ffe1..6922c612e4 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2684,7 +2684,7 @@ static void migration_update_counters(MigrationState *s,
>               stat64_get(&mig_stats.dirty_bytes_last_sync) / bandwidth;
>       }
>   
> -    migration_rate_limit_reset();
> +    migration_rate_limit_reset(s->to_dst_file);
>   
>       update_iteration_initial_status(s);
>   

[PATCH 14/21] 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>
---
 migration/migration-stats.c |  6 ------
 migration/migration-stats.h | 14 --------------
 migration/multifd.c         |  1 -
 migration/qemu-file.c       |  4 ----
 4 files changed, 25 deletions(-)

diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index eb1a2c1ad4..a42b5d953e 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -59,15 +59,9 @@ void migration_rate_limit_set(uint64_t limit)
 
 void migration_rate_limit_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_limit_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/migration-stats.h b/migration/migration-stats.h
index 4029f1deab..ab4cc15a74 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -77,10 +77,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.
      */
@@ -108,16 +104,6 @@ extern MigrationAtomicStats mig_stats;
 
 void calculate_time_since(Stat64 *val, int64_t since);
 
-/**
- * migration_rate_limit_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_limit_account(uint64_t len);
-
 /**
  * migration_rate_limit_get: Get the maximum amount that can be transferred.
  *
diff --git a/migration/multifd.c b/migration/multifd.c
index 2efb313be4..9d2ade7abc 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -432,7 +432,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_limit_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 3f993e24af..0086d67d83 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_limit_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_limit_account(size);
-        }
 
         if (ret != RAM_SAVE_CONTROL_DELAYED &&
             ret != RAM_SAVE_CONTROL_NOT_SUPP) {
-- 
2.40.0

Re: [PATCH 14/21] migration: We don't need the field rate_limit_used anymore

[Cédric Le Goater]

On 5/8/23 15:09, 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>

Thanks,

C.


> ---
>   migration/migration-stats.c |  6 ------
>   migration/migration-stats.h | 14 --------------
>   migration/multifd.c         |  1 -
>   migration/qemu-file.c       |  4 ----
>   4 files changed, 25 deletions(-)
> 
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index eb1a2c1ad4..a42b5d953e 100644
> --- a/migration/migration-stats.c
> +++ b/migration/migration-stats.c
> @@ -59,15 +59,9 @@ void migration_rate_limit_set(uint64_t limit)
>   
>   void migration_rate_limit_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_limit_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/migration-stats.h b/migration/migration-stats.h
> index 4029f1deab..ab4cc15a74 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -77,10 +77,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.
>        */
> @@ -108,16 +104,6 @@ extern MigrationAtomicStats mig_stats;
>   
>   void calculate_time_since(Stat64 *val, int64_t since);
>   
> -/**
> - * migration_rate_limit_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_limit_account(uint64_t len);
> -
>   /**
>    * migration_rate_limit_get: Get the maximum amount that can be transferred.
>    *
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 2efb313be4..9d2ade7abc 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -432,7 +432,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_limit_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 3f993e24af..0086d67d83 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_limit_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_limit_account(size);
> -        }
>   
>           if (ret != RAM_SAVE_CONTROL_DELAYED &&
>               ret != RAM_SAVE_CONTROL_NOT_SUPP) {

[PATCH 15/21] 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.c |  5 +++--
 migration/migration-stats.h |  4 ++++
 migration/rdma.c            | 22 ++++++++++++++++++++--
 migration/trace-events      |  2 +-
 4 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index a42b5d953e..1c2c0b3077 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -65,8 +65,9 @@ void migration_rate_limit_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/migration-stats.h b/migration/migration-stats.h
index ab4cc15a74..cbab99cfdc 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -77,6 +77,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/rdma.c b/migration/rdma.c
index 2cd8f1cc66..941797506a 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 4b6e802833..800cfce547 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.0

[PATCH 16/21] 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 2339a99932..558f2ed3b1 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1356,13 +1356,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.0

[PATCH 17/21] 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 941797506a..dac3d91e16 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;
@@ -3928,7 +3927,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.0

[PATCH 18/21] 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 0086d67d83..951f046c39 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 558f2ed3b1..f889e39a20 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1349,7 +1349,6 @@ static bool control_save_page(PageSearchStatus *pss, RAMBlock *block,
     }
 
     if (bytes_xmit) {
-        ram_transferred_add(bytes_xmit);
         *pages = 1;
     }
 
-- 
2.40.0

[PATCH 19/21] 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.c | 5 -----
 migration/qemu-file.h | 8 --------
 2 files changed, 13 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 951f046c39..bfaba840ca 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
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 46029b951c..9feac5edbc 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -121,14 +121,6 @@ int qemu_put_qemu_file(QEMUFile *f_des, QEMUFile *f_src);
  */
 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);
-- 
2.40.0

[PATCH 20/21] 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.c | 12 ++++++------
 migration/qemu-file.h | 14 ++++++--------
 migration/ram.c       | 10 +++-------
 migration/rdma.c      | 19 +++----------------
 4 files changed, 18 insertions(+), 37 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index bfaba840ca..58ae7a40c9 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/qemu-file.h b/migration/qemu-file.h
index 9feac5edbc..6573e2b62d 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;
@@ -145,9 +144,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/ram.c b/migration/ram.c
index f889e39a20..ec3bf831fb 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1338,23 +1338,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 dac3d91e16..afdd359878 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.0

[PATCH 21/21] 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 9d2ade7abc..3a19d8e304 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;
 }
 
@@ -396,7 +399,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;
@@ -431,10 +433,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;
@@ -716,6 +715,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;
@@ -728,6 +729,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.0

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

[Peter Xu]

On Mon, May 08, 2023 at 03:09:09PM +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.

Maybe worth mention that initial packet is also accounted after this.

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

Two more trivial nits:

> ---
>  migration/multifd.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 9d2ade7abc..3a19d8e304 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;
>  }
>  
> @@ -396,7 +399,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;
> @@ -431,10 +433,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;
> @@ -716,6 +715,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;
> @@ -728,6 +729,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);

Two nits:

Maybe merge the two so half atomic operations?

Also maybe also worth having a inline helper for adding both multifd_bytes
and transferred?

With/without that, all look good:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

>              qemu_mutex_lock(&p->mutex);
>              p->pending_job--;
>              qemu_mutex_unlock(&p->mutex);
> -- 
> 2.40.0
> 

-- 
Peter Xu

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

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Mon, May 08, 2023 at 03:09:09PM +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.
>
> Maybe worth mention that initial packet is also accounted after this.

Ok.
>>  
>> +            stat64_add(&mig_stats.multifd_bytes, p->next_packet_size);
>> +            stat64_add(&mig_stats.transferred, p->next_packet_size);
>
> Two nits:
>
> Maybe merge the two so half atomic operations?

On my tree, to send after this got in:

77fdd3475c migration: Remove transferred atomic counter

O:-)

> Also maybe also worth having a inline helper for adding both multifd_bytes
> and transferred?

I am removing it.

After next set of packates:

rate limit is calulated as:

begining_period = migration_transferred_bytes();
...

bytes_this_period = migration_transferred_bytes() - begining_period;

transferred is calculated as:
- multifd_bytes + qemu_file_bytes;

So things get really simple.  As all counters are atomic, you do a
write and after the write to increse the write size to the qemu_file or
to the multifd_bytes.  And that is it.


> With/without that, all look good:
>
> Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks, Juan.

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

[Peter Xu]

On Thu, May 18, 2023 at 06:40:18PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Mon, May 08, 2023 at 03:09:09PM +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.
> >
> > Maybe worth mention that initial packet is also accounted after this.
> 
> Ok.
> >>  
> >> +            stat64_add(&mig_stats.multifd_bytes, p->next_packet_size);
> >> +            stat64_add(&mig_stats.transferred, p->next_packet_size);
> >
> > Two nits:
> >
> > Maybe merge the two so half atomic operations?
> 
> On my tree, to send after this got in:
> 
> 77fdd3475c migration: Remove transferred atomic counter
> 
> O:-)

Ah this looks even better, indeed. :)

What I meant was we can also do atomic update in one shot for both
next_packet_size + packet_len.

> 
> > Also maybe also worth having a inline helper for adding both multifd_bytes
> > and transferred?
> 
> I am removing it.
> 
> After next set of packates:
> 
> rate limit is calulated as:
> 
> begining_period = migration_transferred_bytes();
> ...
> 
> bytes_this_period = migration_transferred_bytes() - begining_period;
> 
> transferred is calculated as:
> - multifd_bytes + qemu_file_bytes;
> 
> So things get really simple.  As all counters are atomic, you do a
> write and after the write to increse the write size to the qemu_file or
> to the multifd_bytes.  And that is it.

Agreed.

Thanks,

-- 
Peter Xu