| Message ID | 1456499430-8558-24-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
* Daniel P. Berrange (berrange@redhat.com) wrote: > The MigrateState struct uses an array for storing migration > parameters. This presumes that all future parameters will > be integers too, which is not going to be the case. There > is no functional reason why an array is used, if anything > it makes the code less clear. The QAPI schema already > defines a struct - MigrationParameters - capable of storing > all the individual parameters, so just use that instead of > an array. Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> (You could have done this as a separate patch rather than put it in this big series) > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > include/migration/migration.h | 5 +++- > migration/migration.c | 56 +++++++++++++++++++------------------------ > migration/ram.c | 6 ++--- > 3 files changed, 30 insertions(+), 37 deletions(-) > > diff --git a/include/migration/migration.h b/include/migration/migration.h > index 5d44b07..999a5ee 100644 > --- a/include/migration/migration.h > +++ b/include/migration/migration.h > @@ -133,9 +133,12 @@ struct MigrationState > QemuThread thread; > QEMUBH *cleanup_bh; > QEMUFile *to_dst_file; > - int parameters[MIGRATION_PARAMETER__MAX]; > + > + /* New style params from 'migrate-set-parameters' */ > + MigrationParameters parameters; > > int state; > + /* Old style params from 'migrate' command */ > MigrationParams params; > > /* State related to return path */ > diff --git a/migration/migration.c b/migration/migration.c > index e90a14f..b3bdc31 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -82,16 +82,14 @@ MigrationState *migrate_get_current(void) > .bandwidth_limit = MAX_THROTTLE, > .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE, > .mbps = -1, > - .parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL] = > - DEFAULT_MIGRATE_COMPRESS_LEVEL, > - .parameters[MIGRATION_PARAMETER_COMPRESS_THREADS] = > - DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT, > - .parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] = > - DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT, > - .parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL] = > - DEFAULT_MIGRATE_X_CPU_THROTTLE_INITIAL, > - .parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT] = > - DEFAULT_MIGRATE_X_CPU_THROTTLE_INCREMENT, > + .parameters = { > + .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL, > + .compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT, > + .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT, > + .x_cpu_throttle_initial = DEFAULT_MIGRATE_X_CPU_THROTTLE_INITIAL, > + .x_cpu_throttle_increment = > + DEFAULT_MIGRATE_X_CPU_THROTTLE_INCREMENT, > + }, > }; > > if (!once) { > @@ -525,15 +523,11 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) > MigrationState *s = migrate_get_current(); > > params = g_malloc0(sizeof(*params)); > - params->compress_level = s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL]; > - params->compress_threads = > - s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS]; > - params->decompress_threads = > - s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS]; > - params->x_cpu_throttle_initial = > - s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL]; > - params->x_cpu_throttle_increment = > - s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT]; > + params->compress_level = s->parameters.compress_level; > + params->compress_threads = s->parameters.compress_threads; > + params->decompress_threads = s->parameters.decompress_threads; > + params->x_cpu_throttle_initial = s->parameters.x_cpu_throttle_initial; > + params->x_cpu_throttle_increment = s->parameters.x_cpu_throttle_increment; > > return params; > } > @@ -733,7 +727,8 @@ void qmp_migrate_set_parameters(bool has_compress_level, > bool has_x_cpu_throttle_initial, > int64_t x_cpu_throttle_initial, > bool has_x_cpu_throttle_increment, > - int64_t x_cpu_throttle_increment, Error **errp) > + int64_t x_cpu_throttle_increment, > + Error **errp) > { > MigrationState *s = migrate_get_current(); > > @@ -770,26 +765,23 @@ void qmp_migrate_set_parameters(bool has_compress_level, > } > > if (has_compress_level) { > - s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL] = compress_level; > + s->parameters.compress_level = compress_level; > } > if (has_compress_threads) { > - s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS] = compress_threads; > + s->parameters.compress_threads = compress_threads; > } > if (has_decompress_threads) { > - s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] = > - decompress_threads; > + s->parameters.decompress_threads = decompress_threads; > } > if (has_x_cpu_throttle_initial) { > - s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL] = > - x_cpu_throttle_initial; > + s->parameters.x_cpu_throttle_initial = x_cpu_throttle_initial; > } > - > if (has_x_cpu_throttle_increment) { > - s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT] = > - x_cpu_throttle_increment; > + s->parameters.x_cpu_throttle_increment = x_cpu_throttle_increment; > } > } > > + > void qmp_migrate_start_postcopy(Error **errp) > { > MigrationState *s = migrate_get_current(); > @@ -1173,7 +1165,7 @@ int migrate_compress_level(void) > > s = migrate_get_current(); > > - return s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL]; > + return s->parameters.compress_level; > } > > int migrate_compress_threads(void) > @@ -1182,7 +1174,7 @@ int migrate_compress_threads(void) > > s = migrate_get_current(); > > - return s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS]; > + return s->parameters.compress_threads; > } > > int migrate_decompress_threads(void) > @@ -1191,7 +1183,7 @@ int migrate_decompress_threads(void) > > s = migrate_get_current(); > > - return s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS]; > + return s->parameters.decompress_threads; > } > > bool migrate_use_events(void) > diff --git a/migration/ram.c b/migration/ram.c > index 704f6a9..42957bd 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -426,10 +426,8 @@ static size_t save_page_header(QEMUFile *f, RAMBlock *block, ram_addr_t offset) > static void mig_throttle_guest_down(void) > { > MigrationState *s = migrate_get_current(); > - uint64_t pct_initial = > - s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL]; > - uint64_t pct_icrement = > - s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT]; > + uint64_t pct_initial = s->parameters.x_cpu_throttle_initial; > + uint64_t pct_icrement = s->parameters.x_cpu_throttle_increment; > > /* We have not started throttling yet. Let's start it. */ > if (!cpu_throttle_active()) { > -- > 2.5.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/include/migration/migration.h b/include/migration/migration.h index 5d44b07..999a5ee 100644 --- a/include/migration/migration.h +++ b/include/migration/migration.h @@ -133,9 +133,12 @@ struct MigrationState QemuThread thread; QEMUBH *cleanup_bh; QEMUFile *to_dst_file; - int parameters[MIGRATION_PARAMETER__MAX]; + + /* New style params from 'migrate-set-parameters' */ + MigrationParameters parameters; int state; + /* Old style params from 'migrate' command */ MigrationParams params; /* State related to return path */ diff --git a/migration/migration.c b/migration/migration.c index e90a14f..b3bdc31 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -82,16 +82,14 @@ MigrationState *migrate_get_current(void) .bandwidth_limit = MAX_THROTTLE, .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE, .mbps = -1, - .parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL] = - DEFAULT_MIGRATE_COMPRESS_LEVEL, - .parameters[MIGRATION_PARAMETER_COMPRESS_THREADS] = - DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT, - .parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] = - DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT, - .parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL] = - DEFAULT_MIGRATE_X_CPU_THROTTLE_INITIAL, - .parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT] = - DEFAULT_MIGRATE_X_CPU_THROTTLE_INCREMENT, + .parameters = { + .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL, + .compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT, + .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT, + .x_cpu_throttle_initial = DEFAULT_MIGRATE_X_CPU_THROTTLE_INITIAL, + .x_cpu_throttle_increment = + DEFAULT_MIGRATE_X_CPU_THROTTLE_INCREMENT, + }, }; if (!once) { @@ -525,15 +523,11 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) MigrationState *s = migrate_get_current(); params = g_malloc0(sizeof(*params)); - params->compress_level = s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL]; - params->compress_threads = - s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS]; - params->decompress_threads = - s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS]; - params->x_cpu_throttle_initial = - s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL]; - params->x_cpu_throttle_increment = - s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT]; + params->compress_level = s->parameters.compress_level; + params->compress_threads = s->parameters.compress_threads; + params->decompress_threads = s->parameters.decompress_threads; + params->x_cpu_throttle_initial = s->parameters.x_cpu_throttle_initial; + params->x_cpu_throttle_increment = s->parameters.x_cpu_throttle_increment; return params; } @@ -733,7 +727,8 @@ void qmp_migrate_set_parameters(bool has_compress_level, bool has_x_cpu_throttle_initial, int64_t x_cpu_throttle_initial, bool has_x_cpu_throttle_increment, - int64_t x_cpu_throttle_increment, Error **errp) + int64_t x_cpu_throttle_increment, + Error **errp) { MigrationState *s = migrate_get_current(); @@ -770,26 +765,23 @@ void qmp_migrate_set_parameters(bool has_compress_level, } if (has_compress_level) { - s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL] = compress_level; + s->parameters.compress_level = compress_level; } if (has_compress_threads) { - s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS] = compress_threads; + s->parameters.compress_threads = compress_threads; } if (has_decompress_threads) { - s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] = - decompress_threads; + s->parameters.decompress_threads = decompress_threads; } if (has_x_cpu_throttle_initial) { - s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL] = - x_cpu_throttle_initial; + s->parameters.x_cpu_throttle_initial = x_cpu_throttle_initial; } - if (has_x_cpu_throttle_increment) { - s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT] = - x_cpu_throttle_increment; + s->parameters.x_cpu_throttle_increment = x_cpu_throttle_increment; } } + void qmp_migrate_start_postcopy(Error **errp) { MigrationState *s = migrate_get_current(); @@ -1173,7 +1165,7 @@ int migrate_compress_level(void) s = migrate_get_current(); - return s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL]; + return s->parameters.compress_level; } int migrate_compress_threads(void) @@ -1182,7 +1174,7 @@ int migrate_compress_threads(void) s = migrate_get_current(); - return s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS]; + return s->parameters.compress_threads; } int migrate_decompress_threads(void) @@ -1191,7 +1183,7 @@ int migrate_decompress_threads(void) s = migrate_get_current(); - return s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS]; + return s->parameters.decompress_threads; } bool migrate_use_events(void) diff --git a/migration/ram.c b/migration/ram.c index 704f6a9..42957bd 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -426,10 +426,8 @@ static size_t save_page_header(QEMUFile *f, RAMBlock *block, ram_addr_t offset) static void mig_throttle_guest_down(void) { MigrationState *s = migrate_get_current(); - uint64_t pct_initial = - s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL]; - uint64_t pct_icrement = - s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT]; + uint64_t pct_initial = s->parameters.x_cpu_throttle_initial; + uint64_t pct_icrement = s->parameters.x_cpu_throttle_increment; /* We have not started throttling yet. Let's start it. */ if (!cpu_throttle_active()) {
The MigrateState struct uses an array for storing migration parameters. This presumes that all future parameters will be integers too, which is not going to be the case. There is no functional reason why an array is used, if anything it makes the code less clear. The QAPI schema already defines a struct - MigrationParameters - capable of storing all the individual parameters, so just use that instead of an array. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- include/migration/migration.h | 5 +++- migration/migration.c | 56 +++++++++++++++++++------------------------ migration/ram.c | 6 ++--- 3 files changed, 30 insertions(+), 37 deletions(-)