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

[Leonardo Brás]

On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
>  migration/migration-stats.h | 8 +++++++-
>  migration/migration-stats.c | 7 +++++--
>  migration/migration.c       | 2 +-
>  3 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index 91fda378d3..f1465c2ebe 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -81,6 +81,10 @@ typedef struct {
>       * Number of bytes sent during precopy stage.
>       */
>      Stat64 precopy_bytes;
> +    /*
> +     * Amount of transferred data at the start of current cycle.
> +     */
> +    Stat64 rate_limit_start;
>      /*
>       * Maximum amount of data we can send in a cycle.
>       */
> @@ -136,8 +140,10 @@ uint64_t migration_rate_get(void);
>   * migration_rate_reset: Reset the rate limit counter.
>   *
>   * This is called when we know we start a new transfer cycle.
> + *
> + * @f: QEMUFile used for main migration channel
>   */
> -void migration_rate_reset(void);
> +void migration_rate_reset(QEMUFile *f);
>  
>  /**
>   * migration_rate_set: Set the maximum amount that can be transferred.
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index 301392d208..da2bb69a15 100644
> --- a/migration/migration-stats.c
> +++ b/migration/migration-stats.c
> @@ -31,7 +31,9 @@ bool migration_rate_exceeded(QEMUFile *f)
>          return true;
>      }
>  
> -    uint64_t rate_limit_used = stat64_get(&mig_stats.rate_limit_used);
> +    uint64_t rate_limit_start = stat64_get(&mig_stats.rate_limit_start);
> +    uint64_t rate_limit_current = migration_transferred_bytes(f);
> +    uint64_t rate_limit_used = rate_limit_current - rate_limit_start;
>      uint64_t rate_limit_max = stat64_get(&mig_stats.rate_limit_max);

So, IIUC, instead of updating mig_stats.rate_limit_used every time data is sent,
the idea is to 'reset' it to migration_transferred_bytes() at the beginning of a
cycle, and read migration_transferred_bytes() again for checking if the limit
was not crossed.

Its a nice change since there is no need to update 2 counters, when 1 is enough.

I think it would look nicer if squashed with 9/16, though. It would make it more
clear this is being added to replace migration_rate_account() strategy.

What do you think?

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

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