| Message ID | 20230508130909.65420-3-quintela@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Migration: More migration atomic counters | expand |
On 5/8/23 18:38, Juan Quintela wrote: > Use 0 instead. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/migration.c | 4 ++-- > migration/qemu-file.c | 3 +++ > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 1192f1ebf1..3979a98949 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -2296,7 +2296,7 @@ static void migration_completion(MigrationState *s) > } > if (ret >= 0) { > 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, 0); #define RATE_LIMIT_MAX 0 How about having a macro and use that which conveys the meaning in all call instances wherever it is getting passed ? > ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false, > s->block_inactive); > } > @@ -3044,7 +3044,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, 0); > > setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST); > /* > diff --git a/migration/qemu-file.c b/migration/qemu-file.c > index f4cfd05c67..745361d238 100644 > --- a/migration/qemu-file.c > +++ b/migration/qemu-file.c > @@ -731,6 +731,9 @@ int qemu_file_rate_limit(QEMUFile *f) > if (qemu_file_get_error(f)) { > return 1; > } > + /* > + * rate_limit_max == 0 means no rate_limit enfoncement. > + */ > if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) { > return 1; > }
Harsh Prateek Bora <harshpb@linux.ibm.com> wrote: > On 5/8/23 18:38, Juan Quintela wrote: >> Use 0 instead. >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> migration/migration.c | 4 ++-- >> migration/qemu-file.c | 3 +++ >> 2 files changed, 5 insertions(+), 2 deletions(-) >> diff --git a/migration/migration.c b/migration/migration.c >> index 1192f1ebf1..3979a98949 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -2296,7 +2296,7 @@ static void migration_completion(MigrationState *s) >> } >> if (ret >= 0) { >> 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, 0); > > #define RATE_LIMIT_MAX 0 > > How about having a macro and use that which conveys the meaning in all > call instances wherever it is getting passed ? I almost preffer the macro. qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX); seems quite explanatory? Thanks, Juan. > >> ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false, >> s->block_inactive); >> } >> @@ -3044,7 +3044,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, 0); >> setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST); >> /* >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c >> index f4cfd05c67..745361d238 100644 >> --- a/migration/qemu-file.c >> +++ b/migration/qemu-file.c >> @@ -731,6 +731,9 @@ int qemu_file_rate_limit(QEMUFile *f) >> if (qemu_file_get_error(f)) { >> return 1; >> } >> + /* >> + * rate_limit_max == 0 means no rate_limit enfoncement. >> + */ >> if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) { >> return 1; >> }
On 5/9/23 17:21, Juan Quintela wrote: > Harsh Prateek Bora <harshpb@linux.ibm.com> wrote: >> On 5/8/23 18:38, Juan Quintela wrote: >>> Use 0 instead. >>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>> --- >>> migration/migration.c | 4 ++-- >>> migration/qemu-file.c | 3 +++ >>> 2 files changed, 5 insertions(+), 2 deletions(-) >>> diff --git a/migration/migration.c b/migration/migration.c >>> index 1192f1ebf1..3979a98949 100644 >>> --- a/migration/migration.c >>> +++ b/migration/migration.c >>> @@ -2296,7 +2296,7 @@ static void migration_completion(MigrationState *s) >>> } >>> if (ret >= 0) { >>> 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, 0); >> >> #define RATE_LIMIT_MAX 0 >> >> How about having a macro and use that which conveys the meaning in all >> call instances wherever it is getting passed ? > > I almost preffer the macro. > > qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX); > > seems quite explanatory? > Yes, definitely. Thanks Harsh > Thanks, Juan. > >> >>> ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false, >>> s->block_inactive); >>> } >>> @@ -3044,7 +3044,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, 0); >>> setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST); >>> /* >>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c >>> index f4cfd05c67..745361d238 100644 >>> --- a/migration/qemu-file.c >>> +++ b/migration/qemu-file.c >>> @@ -731,6 +731,9 @@ int qemu_file_rate_limit(QEMUFile *f) >>> if (qemu_file_get_error(f)) { >>> return 1; >>> } >>> + /* >>> + * rate_limit_max == 0 means no rate_limit enfoncement. >>> + */ >>> if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) { >>> return 1; >>> } >
On 5/9/23 13:51, Juan Quintela wrote: > Harsh Prateek Bora <harshpb@linux.ibm.com> wrote: >> On 5/8/23 18:38, Juan Quintela wrote: >>> Use 0 instead. >>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>> --- >>> migration/migration.c | 4 ++-- >>> migration/qemu-file.c | 3 +++ >>> 2 files changed, 5 insertions(+), 2 deletions(-) >>> diff --git a/migration/migration.c b/migration/migration.c >>> index 1192f1ebf1..3979a98949 100644 >>> --- a/migration/migration.c >>> +++ b/migration/migration.c >>> @@ -2296,7 +2296,7 @@ static void migration_completion(MigrationState *s) >>> } >>> if (ret >= 0) { >>> 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, 0); >> >> #define RATE_LIMIT_MAX 0 >> >> How about having a macro and use that which conveys the meaning in all >> call instances wherever it is getting passed ? > > I almost preffer the macro. > > qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX); > > seems quite explanatory? yep. and I would drop the comment qemu_file_rate_limit(). Thanks, C. > > Thanks, Juan. > >> >>> ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false, >>> s->block_inactive); >>> } >>> @@ -3044,7 +3044,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, 0); >>> setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST); >>> /* >>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c >>> index f4cfd05c67..745361d238 100644 >>> --- a/migration/qemu-file.c >>> +++ b/migration/qemu-file.c >>> @@ -731,6 +731,9 @@ int qemu_file_rate_limit(QEMUFile *f) >>> if (qemu_file_get_error(f)) { >>> return 1; >>> } >>> + /* >>> + * rate_limit_max == 0 means no rate_limit enfoncement. >>> + */ >>> if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) { >>> return 1; >>> } >
Cédric Le Goater <clg@kaod.org> wrote: > On 5/9/23 13:51, Juan Quintela wrote: >> Harsh Prateek Bora <harshpb@linux.ibm.com> wrote: >>> On 5/8/23 18:38, Juan Quintela wrote: >>>> Use 0 instead. >>>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>>> --- >>>> migration/migration.c | 4 ++-- >>>> migration/qemu-file.c | 3 +++ >>>> 2 files changed, 5 insertions(+), 2 deletions(-) >>>> diff --git a/migration/migration.c b/migration/migration.c >>>> index 1192f1ebf1..3979a98949 100644 >>>> --- a/migration/migration.c >>>> +++ b/migration/migration.c >>>> @@ -2296,7 +2296,7 @@ static void migration_completion(MigrationState *s) >>>> } >>>> if (ret >= 0) { >>>> 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, 0); >>> >>> #define RATE_LIMIT_MAX 0 >>> >>> How about having a macro and use that which conveys the meaning in all >>> call instances wherever it is getting passed ? >> I almost preffer the macro. >> qemu_file_set_rate_limit(s->to_dst_file, RATE_LIMIT_MAX); >> seems quite explanatory? > > yep. and I would drop the comment qemu_file_rate_limit(). I dropped it once by error. And reviewer didn't noticed either. So ....
diff --git a/migration/migration.c b/migration/migration.c index 1192f1ebf1..3979a98949 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2296,7 +2296,7 @@ static void migration_completion(MigrationState *s) } if (ret >= 0) { 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, 0); ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false, s->block_inactive); } @@ -3044,7 +3044,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, 0); setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST); /* diff --git a/migration/qemu-file.c b/migration/qemu-file.c index f4cfd05c67..745361d238 100644 --- a/migration/qemu-file.c +++ b/migration/qemu-file.c @@ -731,6 +731,9 @@ int qemu_file_rate_limit(QEMUFile *f) if (qemu_file_get_error(f)) { return 1; } + /* + * rate_limit_max == 0 means no rate_limit enfoncement. + */ if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) { return 1; }
Use 0 instead. Signed-off-by: Juan Quintela <quintela@redhat.com> --- migration/migration.c | 4 ++-- migration/qemu-file.c | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-)