Re: [PATCH 2/4] migration: check for rate_limit_max for RATE_LIMIT_DISABLED

[Peter Xu]

On Thu, Sep 21, 2023 at 11:56:23PM -0700, Elena Ufimtseva wrote:
> In migration rate limiting atomic operations are used
> to read the rate limit variables and transferred bytes and
> they are expensive. Check first if rate_limit_max is equal
> to RATE_LIMIT_DISABLED and return false immediately if so.
> 
> Signed-off-by: Elena Ufimtseva 

Reviewed-by: Peter Xu 

One trivial comment:

> ---
>  migration/migration-stats.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index 095d6d75bb..abc31483d5 100644
> --- a/migration/migration-stats.c
> +++ b/migration/migration-stats.c
> @@ -24,14 +24,14 @@ bool migration_rate_exceeded(QEMUFile *f)
>  return true;
>  }
>  
> -uint64_t rate_limit_start = stat64_get(_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(_stats.rate_limit_max);

Side note: since we have a helper, this can be migration_rate_get() too.

> -
>  if (rate_limit_max == RATE_LIMIT_DISABLED) {
>  return false;
>  }

empty line would be nice.

> +uint64_t rate_limit_start = stat64_get(_stats.rate_limit_start);
> +uint64_t rate_limit_current = migration_transferred_bytes(f);
> +uint64_t rate_limit_used = rate_limit_current - rate_limit_start;
> +
>  if (rate_limit_max > 0 && rate_limit_used > rate_limit_max) {
>  return true;
>  }
> -- 
> 2.34.1
> 
> 

-- 
Peter Xu

Re: [PATCH 2/4] migration: check for rate_limit_max for RATE_LIMIT_DISABLED

[Fabiano Rosas]

Elena Ufimtseva  writes:

> In migration rate limiting atomic operations are used
> to read the rate limit variables and transferred bytes and
> they are expensive. Check first if rate_limit_max is equal
> to RATE_LIMIT_DISABLED and return false immediately if so.
>
> Signed-off-by: Elena Ufimtseva 
> ---
>  migration/migration-stats.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index 095d6d75bb..abc31483d5 100644
> --- a/migration/migration-stats.c
> +++ b/migration/migration-stats.c
> @@ -24,14 +24,14 @@ bool migration_rate_exceeded(QEMUFile *f)
>  return true;
>  }
>  
> -uint64_t rate_limit_start = stat64_get(_stats.rate_limit_start);
> -uint64_t rate_limit_current = migration_transferred_bytes(f);

There's a qemu_fflush() hiding inside migration_transferred_bytes(). It
currently always flushes if we haven't detected an error in the
file. After this patch we will stop flushing at this point if
ratelimiting is disabled.

You might want to add that information to the commit message to make it
easier to track if this ends up causing a regression.

Reviewed-by: Fabiano Rosas