From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48595) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ae4Kj-0000fq-PB for qemu-devel@nongnu.org; Thu, 10 Mar 2016 12:25:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ae4Ke-0000E9-Oh for qemu-devel@nongnu.org; Thu, 10 Mar 2016 12:25:09 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38439) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ae4Ke-0000E1-Ea for qemu-devel@nongnu.org; Thu, 10 Mar 2016 12:25:04 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id D39A67F0A2 for ; Thu, 10 Mar 2016 17:25:03 +0000 (UTC) Date: Thu, 10 Mar 2016 17:25:00 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20160310172459.GA16887@work-vm> References: <1456499430-8558-1-git-send-email-berrange@redhat.com> <1456499430-8558-24-git-send-email-berrange@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1456499430-8558-24-git-send-email-berrange@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 23/27] migration: don't use an array for storing migrate parameters List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: Amit Shah , qemu-devel@nongnu.org, Juan Quintela * 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 (You could have done this as a separate patch rather than put it in this big series) > Signed-off-by: Daniel P. Berrange > --- > 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