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

[Leonardo Bras Soares Passos <leobras@redhat.com>]

On Fri, May 26, 2023 at 5:17 AM Juan Quintela <quintela@redhat.com> wrote:
>
> Leonardo Brás <leobras@redhat.com> wrote:
> > On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  migration/migration-stats.h | 8 +++++++-
> >>  migration/migration-stats.c | 7 +++++--
> >>  migration/migration.c       | 2 +-
> >>  3 files changed, 13 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> >> index 91fda378d3..f1465c2ebe 100644
> >> --- a/migration/migration-stats.h
> >> +++ b/migration/migration-stats.h
> >> @@ -81,6 +81,10 @@ typedef struct {
> >>       * Number of bytes sent during precopy stage.
> >>       */
> >>      Stat64 precopy_bytes;
> >> +    /*
> >> +     * Amount of transferred data at the start of current cycle.
> >> +     */
> >> +    Stat64 rate_limit_start;
> >>      /*
> >>       * Maximum amount of data we can send in a cycle.
> >>       */
> >> @@ -136,8 +140,10 @@ uint64_t migration_rate_get(void);
> >>   * migration_rate_reset: Reset the rate limit counter.
> >>   *
> >>   * This is called when we know we start a new transfer cycle.
> >> + *
> >> + * @f: QEMUFile used for main migration channel
> >>   */
> >> -void migration_rate_reset(void);
> >> +void migration_rate_reset(QEMUFile *f);
> >>
> >>  /**
> >>   * migration_rate_set: Set the maximum amount that can be transferred.
> >> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> >> index 301392d208..da2bb69a15 100644
> >> --- a/migration/migration-stats.c
> >> +++ b/migration/migration-stats.c
> >> @@ -31,7 +31,9 @@ bool migration_rate_exceeded(QEMUFile *f)
> >>          return true;
> >>      }
> >>
> >> -    uint64_t rate_limit_used = stat64_get(&mig_stats.rate_limit_used);
> >> +    uint64_t rate_limit_start = stat64_get(&mig_stats.rate_limit_start);
> >> +    uint64_t rate_limit_current = migration_transferred_bytes(f);
> >> +    uint64_t rate_limit_used = rate_limit_current - rate_limit_start;
> >>      uint64_t rate_limit_max = stat64_get(&mig_stats.rate_limit_max);
> >
> > So, IIUC, instead of updating mig_stats.rate_limit_used every time data is sent,
> > the idea is to 'reset' it to migration_transferred_bytes() at the beginning of a
> > cycle, and read migration_transferred_bytes() again for checking if the limit
> > was not crossed.
> >
> > Its a nice change since there is no need to update 2 counters, when 1 is enough.
> >
> > I think it would look nicer if squashed with 9/16, though. It would make it more
> > clear this is being added to replace migration_rate_account() strategy.
> >
> > What do you think?
>
> Already in tree.

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

>
> Done this way because on my tree there was an intermediate patch that
> did something like:
>
>
>     uint64_t rate_limit_used = stat64_get(&mig_stats.rate_limit_used);
>     uint64_t rate_limit_start = stat64_get(&mig_stats.rate_limit_start);
>     uint64_t rate_limit_current = migration_transferred_bytes(f);
>     uint64_t rate_limit_used_new = rate_limit_current - rate_limit_start;
>
>     if (rate_limit_used_new != rate_limit_used) {
>         printf("rate_limit old %lu new %lu\n", ...);
>     }
>
> So I was sure that the counter that I was replacing had the same value
> that the new one.

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

>
> This is the reason why I fixed transferred atomic in the previous patch,
> not because it mattered on the big scheme of things (migration_test was
> missing something like 100KB for the normal stage when I started, that
> for calculations don't matter).  But to check if I was doing the things
> right it mattered.  With that patch my replacement counter was exact,
> and none of the if's triggered.
>
> Except for the device transffer stages, there I missed something like
> 900KB, but it made no sense to go all over the tree to fix a counter
> that I was going to remove later.

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

Thanks for helping me understand this :)

>
> Regards, Juan.
>