Re: [Qemu-devel] [PATCH v2] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp

[Dr. David Alan Gilbert]

* Ashijeet Acharya (address@hidden) wrote:
> Mark old-commands for speed and downtime as deprecated.
> Move max-bandwidth and downtime-limit into migrate-set-parameters for
> setting maximum migration speed and expected downtime limit parameters
> respectively.
> Change downtime units to milliseconds (only for new-command) and update
> the query part in both hmp and qmp qemu control interfaces.
> 
> Signed-off-by: Ashijeet Acharya <address@hidden>
> ---
>  hmp.c                         |  27 ++++++++++
>  include/migration/migration.h |   1 -
>  migration/migration.c         | 120 
> ++++++++++++++++++++++++++++++++----------
>  qapi-schema.json              |  33 ++++++++++--
>  qmp-commands.hx               |  14 +++--
>  5 files changed, 159 insertions(+), 36 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index cc2056e..2559f5e 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 */
>  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,9 @@ 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;
>      long valueint = 0;
> +    char *endp;
>      Error *err = NULL;
>      bool has_compress_level = false;
>      bool has_compress_threads = false;
> @@ -1260,6 +1270,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 +1303,19 @@ 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)) {

There's no requirement for the bandwidth to be a power of 2 - I quite
often set it to 100M for example.

> +                    error_setg(&err, "Invalid size %s", valuestr);
> +                    goto cleanup;
> +                }
> +                break;
> +            case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
> +                has_downtime_limit = true;
> +                use_int_value = true;
> +                break;
>              }
>  
>              if (use_int_value) {
> @@ -1308,6 +1333,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, valueint,
>                                         &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..115a9c7 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. */
 'Time in nanoseconds we are allowed to stop the source for to send last part' 
might
be better?

> +#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 / 1000000;
>  
>      return params;
>  }
> @@ -642,7 +648,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>          info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
>              - s->total_time;
>          info->has_expected_downtime = true;
> -        info->expected_downtime = s->expected_downtime;
> +        info->expected_downtime = s->expected_downtime / 1000;

I don't understand this change here; why are we changing the output
scale here?

>          info->has_setup_time = true;
>          info->setup_time = s->setup_time;
>  
> @@ -670,7 +676,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>          info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
>              - s->total_time;
>          info->has_expected_downtime = true;
> -        info->expected_downtime = s->expected_downtime;
> +        info->expected_downtime = s->expected_downtime / 1000;

and here.

>          info->has_setup_time = true;
>          info->setup_time = s->setup_time;
>  
> @@ -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,
> +                                int64_t downtime_limit,
>                                  Error **errp)
>  {
>      MigrationState *s = migrate_get_current();
> @@ -808,6 +818,12 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>                     "cpu_throttle_increment",
>                     "an integer in the range of 1 to 99");
>      }
> +    if (has_max_bandwidth &&
> +            (max_bandwidth < 0 || max_bandwidth > SIZE_MAX)) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> +                   "max_bandwidth",
> +                   "invalid value");
> +    }
>  
>      if (has_compress_level) {
>          s->parameters.compress_level = compress_level;
> @@ -832,6 +848,22 @@ 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) {
> +        s->parameters.max_bandwidth = max_bandwidth;
> +        if (s->to_dst_file) {
> +            qemu_file_set_rate_limit(s->to_dst_file,
> +                                s->parameters.max_bandwidth / 
> XFER_LIMIT_RATIO);
> +        }
> +    }
> +    if (has_downtime_limit) {
> +        downtime_limit *= 1e6;

Why 1e6?   If this value comes in milliseconds, so we're now in ns?
OK, but comment to say.

> +        downtime_limit = MAX(0, MIN(INT64_MAX, downtime_limit));

There's no point limiting against INT64_MAX, it's an int64_t so it can't
be larger than that anyway.  The reason for the old code in 
qmp_migrate_set_downtime is that it
was using a double and converting to uint64 so it's checking the
conversion.
(Also since we're in integer it's unusual to use 1e6)

> +        s = migrate_get_current();
> +
> +        max_downtime = downtime_limit;
> +        s->parameters.downtime_limit = max_downtime;
> +    }
>  }
>  
>  
> @@ -1163,30 +1195,62 @@ int64_t qmp_query_migrate_cache_size(Error **errp)
>      return migrate_xbzrle_cache_size();
>  }
>  
> -void qmp_migrate_set_speed(int64_t value, Error **errp)
> -{
> -    MigrationState *s;
> -
> -    if (value < 0) {
> -        value = 0;
> -    }
> -    if (value > SIZE_MAX) {
> -        value = SIZE_MAX;
> -    }
> -
> -    s = migrate_get_current();
> -    s->bandwidth_limit = value;
> -    if (s->to_dst_file) {
> -        qemu_file_set_rate_limit(s->to_dst_file,
> -                                 s->bandwidth_limit / XFER_LIMIT_RATIO);
> -    }
> -}
> -
> -void qmp_migrate_set_downtime(double value, Error **errp)
> -{
> -    value *= 1e9;
> -    value = MAX(0, MIN(UINT64_MAX, value));
> -    max_downtime = (uint64_t)value;
> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
> +{
> +    bool has_compress_level = false;
> +    bool has_compress_threads = false;
> +    bool has_decompress_threads = false;
> +    bool has_cpu_throttle_initial = false;
> +    bool has_cpu_throttle_increment = false;
> +    bool has_tls_creds = false;
> +    bool has_tls_hostname = false;
> +    bool has_max_bandwidth = true;
> +    bool has_downtime_limit = false;
> +    const char *valuestr = NULL;
> +    long valueint = 0;
> +    Error *err = NULL;
> +
> +    qmp_migrate_set_parameters(has_compress_level, valueint,
> +                               has_compress_threads, valueint,
> +                               has_decompress_threads, valueint,
> +                               has_cpu_throttle_initial, valueint,
> +                               has_cpu_throttle_increment, valueint,
> +                               has_tls_creds, valuestr,
> +                               has_tls_hostname, valuestr,
> +                               has_max_bandwidth, valuebw,
> +                               has_downtime_limit, valueint,
> +                               &err);

It's a bit long, but I don't think there's a neater way.

> +}
> +
> +void qmp_migrate_set_downtime(double valuedowntime, Error **errp)
> +{
> +    bool has_compress_level = false;
> +    bool has_compress_threads = false;
> +    bool has_decompress_threads = false;
> +    bool has_cpu_throttle_initial = false;
> +    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 = true;
> +    const char *valuestr = NULL;
> +    long valueint = 0;
> +    int64_t valuebw = 0;
> +    valuedowntime *= 1000; /* Convert to milliseconds */
> +    valuedowntime = (int64_t)valuedowntime;

This is where you need the MAX/MIN trick you had above because
this is the double->int64 conversion.

> +    Error *err = NULL;
> +
> +    qmp_migrate_set_parameters(has_compress_level, valueint,
> +                               has_compress_threads, valueint,
> +                               has_decompress_threads, valueint,
> +                               has_cpu_throttle_initial, valueint,
> +                               has_cpu_throttle_increment, valueint,
> +                               has_tls_creds, valuestr,
> +                               has_tls_hostname, valuestr,
> +                               has_max_bandwidth, valuebw,
> +                               has_downtime_limit, valuedowntime,
> +                               &err);
>  }
>  
>  bool migrate_postcopy_ram(void)
> @@ -1858,7 +1922,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..1a7c1cb 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -637,12 +637,19 @@
>  #                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. maximum speed in
> +#                 s/bytes/bytes per second. (Since 2.8)

typo ? just bytes per second  ?

> +#
> +# @downtime-limit: set maximum tolerated downtime for migration. maximum 
> downtime
> +#                  in milliseconds (Since 2.8)
> +#
>  # 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 +685,12 @@
>  #                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. maximum speed in
> +#                 s/bytes/bytes per second. (Since 2.8)
> +#
> +# @downtime-limit: set maximum tolerated downtime for migration. maximum 
> downtime
> +#                  in milliseconds (Since 2.8)
> +#
>  # Since: 2.4
>  ##
>  { 'command': 'migrate-set-parameters',
> @@ -687,7 +700,9 @@
>              '*cpu-throttle-initial': 'int',
>              '*cpu-throttle-increment': 'int',
>              '*tls-creds': 'str',
> -            '*tls-hostname': 'str'} }
> +            '*tls-hostname': 'str',
> +            '*max-bandwidth': 'int',
> +            '*downtime-limit': 'int'} }
>  
>  #
>  # @MigrationParameters
> @@ -721,6 +736,12 @@
>  #                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. maximum speed in
> +#                 s/bytes/bytes per second. (Since 2.8)
> +#
> +# @downtime-limit: set maximum tolerated downtime for migration. maximum 
> downtime
> +#                  in milliseconds (Since 2.8)
> +#
>  # Since: 2.4
>  ##
>  { 'struct': 'MigrationParameters',
> @@ -730,7 +751,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
>  #
> @@ -1812,6 +1835,8 @@
>  #
>  # Returns: nothing on success
>  #
> +# Notes: This command is deprecated in favour of 'migrate-set-parameters'
> +#
>  # Since: 0.14.0
>  ##
>  { 'command': 'migrate_set_downtime', 'data': {'value': 'number'} }
> @@ -1825,7 +1850,7 @@
>  #
>  # Returns: nothing on success
>  #
> -# Notes: A value lesser than zero will be automatically round up to zero.
> +# Notes: This command is deprecated in favour of 'migrate-set-parameters'
>  #
>  # Since: 0.14.0
>  ##
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6866264..9c4504d 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 (in bytes) (json-int)
> +- "downtime-limit": set maximum tolerated downtime (in milliseconds) for
> +                          migrations (json-int)
>  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:i?",
>          .mhandler.cmd_new = qmp_marshal_migrate_set_parameters,
>      },
>  SQMP
> @@ -3820,6 +3822,10 @@ Query current migration parameters
>                                      throttled (json-int)
>           - "cpu-throttle-increment" : throttle increasing percentage for
>                                        auto-converge (json-int)
> +         - "max-bandwidth" : maximium migration speed in s/bytes/bytes per 
> second
> +                             (json-int)
> +         - "downtime-limit" : maximum tolerated downtime of migration in
> +                              milliseconds (json-int)
>  
>  Arguments:
>  
> @@ -3832,7 +3838,9 @@ Example:
>           "cpu-throttle-increment": 10,
>           "compress-threads": 8,
>           "compress-level": 1,
> -         "cpu-throttle-initial": 20
> +         "cpu-throttle-initial": 20,
> +         "max-downtime": 33554432,

That should be max-bandwidth?

> +         "downtime-limit": 300
>        }
>     }
>  
> -- 
> 2.6.2
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK