Re: [Qemu-devel] [PATCH v3 23/27] migration: don't use an array for storing migrate parameters

[Dr. David Alan Gilbert]

* Daniel P. Berrange (address@hidden) 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 <address@hidden>

(You could have done this as a separate patch rather than put
it in this big series)


> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  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 / address@hidden / Manchester, UK