| Message ID | 20230515195709.63843-2-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Migration: More migration atomic counters | expand |
On 5/16/23 01:26, Juan Quintela wrote: > Define and use RATE_LIMIT_MAX instead. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/migration-stats.h | 6 ++++++ > migration/migration.c | 4 ++-- > migration/qemu-file.c | 6 +++++- > 3 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/migration/migration-stats.h b/migration/migration-stats.h > index cf8a4f0410..e782f1b0df 100644 > --- a/migration/migration-stats.h > +++ b/migration/migration-stats.h > @@ -15,6 +15,12 @@ > > #include "qemu/stats64.h" > > +/* > + * If rate_limit_max is 0, there is special code to remove the rate .. there is a check in qemu_file_rate_limit() .. > + * limit. > + */ > +#define RATE_LIMIT_MAX 0 > + Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com> > /* > * These are the ram migration statistic counters. It is loosely > * based on MigrationStats. We change to Stat64 any counter that > diff --git a/migration/migration.c b/migration/migration.c > index 039bba4804..c41c7491bb 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -2304,7 +2304,7 @@ static void migration_completion(MigrationState *s) > * them if migration fails or is cancelled. > */ > s->block_inactive = !migrate_colo(); > - qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX); > + qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX); > ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false, > s->block_inactive); > } > @@ -3048,7 +3048,7 @@ static void *bg_migration_thread(void *opaque) > rcu_register_thread(); > object_ref(OBJECT(s)); > > - qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX); > + qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX); > > setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST); > /* > diff --git a/migration/qemu-file.c b/migration/qemu-file.c > index 597054759d..4bc875b452 100644 > --- a/migration/qemu-file.c > +++ b/migration/qemu-file.c > @@ -27,6 +27,7 @@ > #include "qemu/error-report.h" > #include "qemu/iov.h" > #include "migration.h" > +#include "migration-stats.h" > #include "qemu-file.h" > #include "trace.h" > #include "options.h" > @@ -732,7 +733,10 @@ int qemu_file_rate_limit(QEMUFile *f) > if (qemu_file_get_error(f)) { > return 1; > } > - if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) { > + if (f->rate_limit_max == RATE_LIMIT_MAX) { > + return 0; > + } > + if (f->rate_limit_used > f->rate_limit_max) { > return 1; > } > return 0;
Juan Quintela <quintela@redhat.com> writes: > Define and use RATE_LIMIT_MAX instead. Suggest "RATE_LIMIT_MAX_NONE". > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/migration-stats.h | 6 ++++++ > migration/migration.c | 4 ++-- > migration/qemu-file.c | 6 +++++- > 3 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/migration/migration-stats.h b/migration/migration-stats.h > index cf8a4f0410..e782f1b0df 100644 > --- a/migration/migration-stats.h > +++ b/migration/migration-stats.h > @@ -15,6 +15,12 @@ > > #include "qemu/stats64.h" > > +/* > + * If rate_limit_max is 0, there is special code to remove the rate > + * limit. > + */ > +#define RATE_LIMIT_MAX 0 > + > /* > * These are the ram migration statistic counters. It is loosely > * based on MigrationStats. We change to Stat64 any counter that > diff --git a/migration/migration.c b/migration/migration.c > index 039bba4804..c41c7491bb 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -2304,7 +2304,7 @@ static void migration_completion(MigrationState *s) > * them if migration fails or is cancelled. > */ > s->block_inactive = !migrate_colo(); > - qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX); > + qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX); > ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false, > s->block_inactive); > } > @@ -3048,7 +3048,7 @@ static void *bg_migration_thread(void *opaque) > rcu_register_thread(); > object_ref(OBJECT(s)); > > - qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX); > + qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX); > > setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST); > /* > diff --git a/migration/qemu-file.c b/migration/qemu-file.c > index 597054759d..4bc875b452 100644 > --- a/migration/qemu-file.c > +++ b/migration/qemu-file.c > @@ -27,6 +27,7 @@ > #include "qemu/error-report.h" > #include "qemu/iov.h" > #include "migration.h" > +#include "migration-stats.h" > #include "qemu-file.h" > #include "trace.h" > #include "options.h" > @@ -732,7 +733,10 @@ int qemu_file_rate_limit(QEMUFile *f) Given that qemu_file_rate_limit() is really a boolean, could it be declared as such? > if (qemu_file_get_error(f)) { > return 1; > } > - if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) { > + if (f->rate_limit_max == RATE_LIMIT_MAX) { > + return 0; > + } > + if (f->rate_limit_used > f->rate_limit_max) { > return 1; > } > return 0; > -- > 2.40.1
David Edmondson <david.edmondson@oracle.com> wrote: > Juan Quintela <quintela@redhat.com> writes: > >> Define and use RATE_LIMIT_MAX instead. > > Suggest "RATE_LIMIT_MAX_NONE". Then even better RATE_LIMIT_DISABLED? RATE_LIMIT_NONE? Using MAX and NONE at the same time looks strange. >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> migration/migration-stats.h | 6 ++++++ >> migration/migration.c | 4 ++-- >> migration/qemu-file.c | 6 +++++- >> 3 files changed, 13 insertions(+), 3 deletions(-) >> >> diff --git a/migration/migration-stats.h b/migration/migration-stats.h >> index cf8a4f0410..e782f1b0df 100644 >> --- a/migration/migration-stats.h >> +++ b/migration/migration-stats.h >> @@ -15,6 +15,12 @@ >> >> #include "qemu/stats64.h" >> >> +/* >> + * If rate_limit_max is 0, there is special code to remove the rate >> + * limit. >> + */ >> +#define RATE_LIMIT_MAX 0 >> + >> /* >> * These are the ram migration statistic counters. It is loosely >> * based on MigrationStats. We change to Stat64 any counter that >> diff --git a/migration/migration.c b/migration/migration.c >> index 039bba4804..c41c7491bb 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -2304,7 +2304,7 @@ static void migration_completion(MigrationState *s) >> * them if migration fails or is cancelled. >> */ >> s->block_inactive = !migrate_colo(); >> - qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX); >> + qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX); >> ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false, >> s->block_inactive); >> } >> @@ -3048,7 +3048,7 @@ static void *bg_migration_thread(void *opaque) >> rcu_register_thread(); >> object_ref(OBJECT(s)); >> >> - qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX); >> + qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX); >> >> setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST); >> /* >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c >> index 597054759d..4bc875b452 100644 >> --- a/migration/qemu-file.c >> +++ b/migration/qemu-file.c >> @@ -27,6 +27,7 @@ >> #include "qemu/error-report.h" >> #include "qemu/iov.h" >> #include "migration.h" >> +#include "migration-stats.h" >> #include "qemu-file.h" >> #include "trace.h" >> #include "options.h" >> @@ -732,7 +733,10 @@ int qemu_file_rate_limit(QEMUFile *f) > > Given that qemu_file_rate_limit() is really a boolean, could it be > declared as such? I wanted to do on this patch justn $Subject. You can see that when I move this function to migration/migration-stats.c already do the type change. That is patch: [PATCH v2 05/16] migration: Move rate_limit_max and rate_limit_used to migration_stats diff --git a/migration/migration-stats.c b/migration/migration-stats.c index 3431453c90..1b16edae7d 100644 --- a/migration/migration-stats.c +++ b/migration/migration-stats.c @@ -22,3 +23,46 @@ void migration_time_since(MigrationAtomicStats *stats, int64_t since) int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST); stat64_set(&stats->setup_time, now - since); } + +bool migration_rate_exceeded(QEMUFile *f) +{ + if (qemu_file_get_error(f)) { + return true; + } + + uint64_t rate_limit_used = stat64_get(&mig_stats.rate_limit_used); + uint64_t rate_limit_max = stat64_get(&mig_stats.rate_limit_max); + + if (rate_limit_max == RATE_LIMIT_MAX) { + return false; + } + if (rate_limit_max > 0 && rate_limit_used > rate_limit_max) { + return true; + } + return false; +} Thanks, Juan.
Juan Quintela <quintela@redhat.com> writes: > David Edmondson <david.edmondson@oracle.com> wrote: >> Juan Quintela <quintela@redhat.com> writes: >> >>> Define and use RATE_LIMIT_MAX instead. >> >> Suggest "RATE_LIMIT_MAX_NONE". > > Then even better > > RATE_LIMIT_DISABLED? > RATE_LIMIT_NONE? RATE_LIMIT_NONE sounds good to me. > > Using MAX and NONE at the same time looks strange. > >>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>> --- >>> migration/migration-stats.h | 6 ++++++ >>> migration/migration.c | 4 ++-- >>> migration/qemu-file.c | 6 +++++- >>> 3 files changed, 13 insertions(+), 3 deletions(-) >>> >>> diff --git a/migration/migration-stats.h b/migration/migration-stats.h >>> index cf8a4f0410..e782f1b0df 100644 >>> --- a/migration/migration-stats.h >>> +++ b/migration/migration-stats.h >>> @@ -15,6 +15,12 @@ >>> >>> #include "qemu/stats64.h" >>> >>> +/* >>> + * If rate_limit_max is 0, there is special code to remove the rate >>> + * limit. >>> + */ >>> +#define RATE_LIMIT_MAX 0 >>> + >>> /* >>> * These are the ram migration statistic counters. It is loosely >>> * based on MigrationStats. We change to Stat64 any counter that >>> diff --git a/migration/migration.c b/migration/migration.c >>> index 039bba4804..c41c7491bb 100644 >>> --- a/migration/migration.c >>> +++ b/migration/migration.c >>> @@ -2304,7 +2304,7 @@ static void migration_completion(MigrationState *s) >>> * them if migration fails or is cancelled. >>> */ >>> s->block_inactive = !migrate_colo(); >>> - qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX); >>> + qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX); >>> ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false, >>> s->block_inactive); >>> } >>> @@ -3048,7 +3048,7 @@ static void *bg_migration_thread(void *opaque) >>> rcu_register_thread(); >>> object_ref(OBJECT(s)); >>> >>> - qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX); >>> + qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX); >>> >>> setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST); >>> /* >>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c >>> index 597054759d..4bc875b452 100644 >>> --- a/migration/qemu-file.c >>> +++ b/migration/qemu-file.c >>> @@ -27,6 +27,7 @@ >>> #include "qemu/error-report.h" >>> #include "qemu/iov.h" >>> #include "migration.h" >>> +#include "migration-stats.h" >>> #include "qemu-file.h" >>> #include "trace.h" >>> #include "options.h" >>> @@ -732,7 +733,10 @@ int qemu_file_rate_limit(QEMUFile *f) >> >> Given that qemu_file_rate_limit() is really a boolean, could it be >> declared as such? > > I wanted to do on this patch justn $Subject. > > You can see that when I move this function to > migration/migration-stats.c already do the type change. Thank you 👍 > That is patch: > > [PATCH v2 05/16] migration: Move rate_limit_max and rate_limit_used to > migration_stats > > diff --git a/migration/migration-stats.c b/migration/migration-stats.c > index 3431453c90..1b16edae7d 100644 > --- a/migration/migration-stats.c > +++ b/migration/migration-stats.c > @@ -22,3 +23,46 @@ void migration_time_since(MigrationAtomicStats *stats, int64_t since) > int64_t now = qemu_clock_get_ms(QEMU_CLOCK_HOST); > stat64_set(&stats->setup_time, now - since); > } > + > +bool migration_rate_exceeded(QEMUFile *f) > +{ > + if (qemu_file_get_error(f)) { > + return true; > + } > + > + uint64_t rate_limit_used = stat64_get(&mig_stats.rate_limit_used); > + uint64_t rate_limit_max = stat64_get(&mig_stats.rate_limit_max); > + > + if (rate_limit_max == RATE_LIMIT_MAX) { > + return false; > + } > + if (rate_limit_max > 0 && rate_limit_used > rate_limit_max) { > + return true; > + } > + return false; > +} > > Thanks, Juan.
On 5/16/23 11:24, Juan Quintela wrote: > David Edmondson <david.edmondson@oracle.com> wrote: >> Juan Quintela <quintela@redhat.com> writes: >> >>> Define and use RATE_LIMIT_MAX instead. >> >> Suggest "RATE_LIMIT_MAX_NONE". > > Then even better > > RATE_LIMIT_DISABLED? I'd vote for RATE_LIMIT_DISABLED. > RATE_LIMIT_NONE? > > Using MAX and NONE at the same time looks strange. Cheers, C.
On Tue, 2023-05-16 at 14:47 +0200, Cédric Le Goater wrote: > On 5/16/23 11:24, Juan Quintela wrote: > > David Edmondson <david.edmondson@oracle.com> wrote: > > > Juan Quintela <quintela@redhat.com> writes: > > > > > > > Define and use RATE_LIMIT_MAX instead. > > > > > > Suggest "RATE_LIMIT_MAX_NONE". > > > > Then even better > > > > RATE_LIMIT_DISABLED? > > I'd vote for RATE_LIMIT_DISABLED. Me too. > > > RATE_LIMIT_NONE? > > > > Using MAX and NONE at the same time looks strange. > > Cheers, > > C. > Reviewed-by: Leonardo Bras <leobras@redhat.com>
diff --git a/migration/migration-stats.h b/migration/migration-stats.h index cf8a4f0410..e782f1b0df 100644 --- a/migration/migration-stats.h +++ b/migration/migration-stats.h @@ -15,6 +15,12 @@ #include "qemu/stats64.h" +/* + * If rate_limit_max is 0, there is special code to remove the rate + * limit. + */ +#define RATE_LIMIT_MAX 0 + /* * These are the ram migration statistic counters. It is loosely * based on MigrationStats. We change to Stat64 any counter that diff --git a/migration/migration.c b/migration/migration.c index 039bba4804..c41c7491bb 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2304,7 +2304,7 @@ static void migration_completion(MigrationState *s) * them if migration fails or is cancelled. */ s->block_inactive = !migrate_colo(); - qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX); + qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX); ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false, s->block_inactive); } @@ -3048,7 +3048,7 @@ static void *bg_migration_thread(void *opaque) rcu_register_thread(); object_ref(OBJECT(s)); - qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX); + qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX); setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST); /* diff --git a/migration/qemu-file.c b/migration/qemu-file.c index 597054759d..4bc875b452 100644 --- a/migration/qemu-file.c +++ b/migration/qemu-file.c @@ -27,6 +27,7 @@ #include "qemu/error-report.h" #include "qemu/iov.h" #include "migration.h" +#include "migration-stats.h" #include "qemu-file.h" #include "trace.h" #include "options.h" @@ -732,7 +733,10 @@ int qemu_file_rate_limit(QEMUFile *f) if (qemu_file_get_error(f)) { return 1; } - if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) { + if (f->rate_limit_max == RATE_LIMIT_MAX) { + return 0; + } + if (f->rate_limit_used > f->rate_limit_max) { return 1; } return 0;
Define and use RATE_LIMIT_MAX instead. Signed-off-by: Juan Quintela <quintela@redhat.com> --- migration/migration-stats.h | 6 ++++++ migration/migration.c | 4 ++-- migration/qemu-file.c | 6 +++++- 3 files changed, 13 insertions(+), 3 deletions(-)