| Message ID | 20230515195709.63843-9-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Migration: More migration atomic counters | expand |
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); >
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. 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. 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. Regards, Juan.
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. >
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); 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);