Re: [Qemu-devel] [PATCH v3] Move migrate_set_speed and migrate_set_downtime into migrate_set_parameter

["Daniel P. Berrange" <berrange@redhat.com>]

On Mon, Sep 05, 2016 at 07:56:26PM +0530, Ashijeet Acharya wrote:
> Include migrate_set_speed and migrate_set_downtime inside migrate_set_parameters for setting maximum migration speed and expected downtime parameters respectively. Also update the query part for both in qmp and hmp qemu control interfaces.

Please put line breaks in your commit message at 72 characters


Also, when sending v2, v3, etc it is preferred to send it as a new
top-level thread. ie, don't set headers to be a reply to your v1.

> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
>  hmp.c                         | 33 +++++++++++++++++++++++++++++++++
>  include/migration/migration.h |  1 -
>  migration/migration.c         | 30 ++++++++++++++++++++++++++----
>  qapi-schema.json              | 26 +++++++++++++++++++++++---
>  qmp-commands.hx               | 13 ++++++++++---
>  5 files changed, 92 insertions(+), 11 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index cc2056e..c92769b 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -304,6 +304,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, " %s: '%s'",
>              MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_HOSTNAME],
>              params->tls_hostname ? : "");
> +        monitor_printf(mon, " %s: %" PRId64,
> +            MigrationParameter_lookup[MIGRATION_PARAMETER_MAX_BANDWIDTH],
> +            params->max_bandwidth);
> +        monitor_printf(mon, " %s: %" PRId64,
> +            MigrationParameter_lookup[MIGRATION_PARAMETER_DOWNTIME_LIMIT],
> +            params->downtime_limit);
>          monitor_printf(mon, "\n");
>      }
>  
> @@ -1193,6 +1199,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &err);
>  }
>  
> +/* Kept for old-commands compatibility */

I don't think this comment really adds any value. Instead, in the qapi
schema, you should mark the legacy methods as deprecated though.

>  void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)
>  {
>      double value = qdict_get_double(qdict, "value");
> @@ -1211,6 +1218,7 @@ void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +/* Kept for old-commands compatibility */
>  void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict)
>  {
>      int64_t value = qdict_get_int(qdict, "value");
> @@ -1251,7 +1259,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>  {
>      const char *param = qdict_get_str(qdict, "parameter");
>      const char *valuestr = qdict_get_str(qdict, "value");
> +    int64_t valuebw = 0;
> +    double valuedowntime = 0;
>      long valueint = 0;
> +    char *endp;
>      Error *err = NULL;
>      bool has_compress_level = false;
>      bool has_compress_threads = false;
> @@ -1260,6 +1271,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>      bool has_cpu_throttle_increment = false;
>      bool has_tls_creds = false;
>      bool has_tls_hostname = false;
> +    bool has_max_bandwidth = false;
> +    bool has_downtime_limit = false;
>      bool use_int_value = false;
>      int i;
>  
> @@ -1291,6 +1304,24 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>              case MIGRATION_PARAMETER_TLS_HOSTNAME:
>                  has_tls_hostname = true;
>                  break;
> +            case MIGRATION_PARAMETER_MAX_BANDWIDTH:
> +                has_max_bandwidth = true;
> +                valuebw = qemu_strtosz(valuestr, &endp);
> +                if (valuebw < 0 || (size_t)valuebw != valuebw || *endp != '\0'
> +                    || !is_power_of_2(valuebw)) {
> +                    error_setg(&err, "Invalid size %s", valuestr);
> +                    goto cleanup;
> +                }
> +                break;
> +            case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
> +                has_downtime_limit = true;
> +                valuedowntime = strtod(valuestr, &endp);
> +                if (valuestr == endp) {
> +                    error_setg(&err, "Unable to parse '%s' as a number",
> +                               valuestr);
> +                    goto cleanup;
> +                }
> +                break;
>              }
>  
>              if (use_int_value) {
> @@ -1308,6 +1339,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>                                         has_cpu_throttle_increment, valueint,
>                                         has_tls_creds, valuestr,
>                                         has_tls_hostname, valuestr,
> +                                       has_max_bandwidth, valuebw,
> +                                       has_downtime_limit, valuedowntime,
>                                         &err);
>              break;
>          }
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 3c96623..a5429ee 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -129,7 +129,6 @@ struct MigrationSrcPageRequest {
>  
>  struct MigrationState
>  {
> -    int64_t bandwidth_limit;
>      size_t bytes_xfer;
>      size_t xfer_limit;
>      QemuThread thread;
> diff --git a/migration/migration.c b/migration/migration.c
> index 955d5ee..4b54b58 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -44,6 +44,9 @@
>  #define BUFFER_DELAY     100
>  #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
>  
> +/* Amount of nanoseconds we are willing to wait for migration to be down. */
> +#define DEFAULT_MIGRATE_SET_DOWNTIME 300000000
> +
>  /* Default compression thread count */
>  #define DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT 8
>  /* Default decompression thread count, usually decompression is at
> @@ -80,7 +83,6 @@ MigrationState *migrate_get_current(void)
>      static bool once;
>      static MigrationState current_migration = {
>          .state = MIGRATION_STATUS_NONE,
> -        .bandwidth_limit = MAX_THROTTLE,
>          .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
>          .mbps = -1,
>          .parameters = {
> @@ -89,6 +91,8 @@ MigrationState *migrate_get_current(void)
>              .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
>              .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
>              .cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
> +            .max_bandwidth = MAX_THROTTLE,
> +            .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
>          },
>      };
>  
> @@ -566,6 +570,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
>      params->tls_creds = g_strdup(s->parameters.tls_creds);
>      params->tls_hostname = g_strdup(s->parameters.tls_hostname);
> +    params->max_bandwidth = s->parameters.max_bandwidth;
> +    params->downtime_limit = s->parameters.downtime_limit;
>  
>      return params;
>  }
> @@ -773,6 +779,10 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>                                  const char *tls_creds,
>                                  bool has_tls_hostname,
>                                  const char *tls_hostname,
> +                                bool has_max_bandwidth,
> +                                int64_t max_bandwidth,
> +                                bool has_downtime_limit,
> +                                double downtime_limit,
>                                  Error **errp)
>  {
>      MigrationState *s = migrate_get_current();
> @@ -832,6 +842,12 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>          g_free(s->parameters.tls_hostname);
>          s->parameters.tls_hostname = g_strdup(tls_hostname);
>      }
> +    if (has_max_bandwidth) {
> +        qmp_migrate_set_speed(max_bandwidth, NULL);
> +    }
> +    if (has_downtime_limit) {
> +        qmp_migrate_set_downtime(downtime_limit, NULL);
> +    }
>  }
>  
>  
> @@ -1175,18 +1191,24 @@ void qmp_migrate_set_speed(int64_t value, Error **errp)
>      }
>  
>      s = migrate_get_current();
> -    s->bandwidth_limit = value;
> +    s->parameters.max_bandwidth = value;
>      if (s->to_dst_file) {
>          qemu_file_set_rate_limit(s->to_dst_file,
> -                                 s->bandwidth_limit / XFER_LIMIT_RATIO);
> +                            s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
>      }
>  }
>  
>  void qmp_migrate_set_downtime(double value, Error **errp)
>  {
> +    MigrationState *s;
> +
>      value *= 1e9;
>      value = MAX(0, MIN(UINT64_MAX, value));
> +
> +    s = migrate_get_current();
> +
>      max_downtime = (uint64_t)value;
> +    s->parameters.downtime_limit = max_downtime;
>  }
>  
>  bool migrate_postcopy_ram(void)
> @@ -1858,7 +1880,7 @@ void migrate_fd_connect(MigrationState *s)
>  
>      qemu_file_set_blocking(s->to_dst_file, true);
>      qemu_file_set_rate_limit(s->to_dst_file,
> -                             s->bandwidth_limit / XFER_LIMIT_RATIO);
> +                             s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
>  
>      /* Notify before starting migration thread */
>      notifier_list_notify(&migration_state_notifiers, s);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5658723..250eac5 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -637,12 +637,18 @@
>  #                hostname must be provided so that the server's x509
>  #                certificate identity can be validated. (Since 2.7)
>  #
> +# @max-bandwidth: to set maximum speed for migration. A value lesser than
> +#                     zero will be automatically round upto zero. Since 2.8)

Document the units for this ? eg is it bits-per-second, kb-per-second,
mb-per-second, etc

> +#
> +# @downtime-limit: set maximum tolerated downtime for migration. Since 2.8)

Again, units ? nanoseconds ? milliseconds ?

> +#
>  # Since: 2.4
>  ##
>  { 'enum': 'MigrationParameter',
>    'data': ['compress-level', 'compress-threads', 'decompress-threads',
>             'cpu-throttle-initial', 'cpu-throttle-increment',
> -           'tls-creds', 'tls-hostname'] }
> +           'tls-creds', 'tls-hostname', 'max-bandwidth',
> +           'downtime-limit'] }
>  
>  #
>  # @migrate-set-parameters
> @@ -678,6 +684,11 @@
>  #                hostname must be provided so that the server's x509
>  #                certificate identity can be validated. (Since 2.7)
>  #
> +# @max-bandwidth: to set maximum speed for migration. A value lesser than
> +#                     zero will be automatically round upto zero. Since 2.8)

Units

> +#
> +# @downtime-limit: set maximum tolerated downtime for migration. Since 2.8)

Units

> +#
>  # Since: 2.4
>  ##
>  { 'command': 'migrate-set-parameters',
> @@ -687,7 +698,9 @@
>              '*cpu-throttle-initial': 'int',
>              '*cpu-throttle-increment': 'int',
>              '*tls-creds': 'str',
> -            '*tls-hostname': 'str'} }
> +            '*tls-hostname': 'str',
> +            '*max-bandwidth': 'int',
> +            '*downtime-limit': 'number'} }
>  
>  #
>  # @MigrationParameters
> @@ -721,6 +734,11 @@
>  #                hostname must be provided so that the server's x509
>  #                certificate identity can be validated. (Since 2.7)
>  #
> +# @max-bandwidth: to set maximum speed for migration. A value lesser than
> +#                     zero will be automatically round upto zero. (Since 2.8)
> +#
> +# @downtime-limit: set maximum tolerated downtime for migration. Since 2.8)
> +#
>  # Since: 2.4
>  ##
>  { 'struct': 'MigrationParameters',
> @@ -730,7 +748,9 @@
>              'cpu-throttle-initial': 'int',
>              'cpu-throttle-increment': 'int',
>              'tls-creds': 'str',
> -            'tls-hostname': 'str'} }
> +            'tls-hostname': 'str',
> +            'max-bandwidth': 'int',
> +            'downtime-limit': 'int'} }
>  ##
>  # @query-migrate-parameters
>  #
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6866264..0418cab 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3790,7 +3790,9 @@ Set migration parameters
>                            throttled for auto-converge (json-int)
>  - "cpu-throttle-increment": set throttle increasing percentage for
>                              auto-converge (json-int)
> -
> +- "max-bandwidth": set maximum speed for migrations (json-int)

Document units

> +- "downtime-limit": set maximum tolerated downtime (in seconds) for
> +                          migrations (json-number)

I'm sure units for this are not "seconds" as that is way too coarse.

>  Arguments:
>  
>  Example:
> @@ -3803,7 +3805,7 @@ EQMP
>      {
>          .name       = "migrate-set-parameters",
>          .args_type  =
> -            "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?",
> +            "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?,max-bandwidth:i?,downtime-limit:T?",
>          .mhandler.cmd_new = qmp_marshal_migrate_set_parameters,
>      },
>  SQMP
> @@ -3820,6 +3822,9 @@ Query current migration parameters
>                                      throttled (json-int)
>           - "cpu-throttle-increment" : throttle increasing percentage for
>                                        auto-converge (json-int)
> +         - "max-bandwidth" : maximium migration speed (json-int)
> +         - "downtime-limit" : maximum tolerated downtime of migration
> +                                      (json-int)

Document units


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|