On Fri, May 26, 2023 at 5:17 AM Juan Quintela <quint...@redhat.com> wrote:
>
> Leonardo Brás <leob...@redhat.com> wrote:
> > On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
> >> Signed-off-by: Juan Quintela <quint...@redhat.com>
> >> Reviewed-by: Cédric Le Goater <c...@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.
>
