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

[Ashijeet Acharya]

On Mon, Sep 5, 2016 at 1:31 PM, Paolo Bonzini <address@hidden> wrote:
>
>
> On 05/09/2016 09:45, Ashijeet Acharya wrote:
>> Include migrate_set_speed and migrate_set_downtime inside
>> migrate_set_parameters respectively for setting maximum migration
>> speed and expected downtime parameters. Also add the query part for
>> both in qmp and hmp qemu control interfaces.
>>
>> Signed-off-by: Ashijeet Acharya <address@hidden>
>
> You cannot break backwards compatibility for everyone that is using
> those commands, sorry.
>
> Paolo

So should I keep the old commands too and add the new ones anyway for
the query part?
The old ones will also be modified for query. So we will have
compatibility as well as the
query.

Ashijeet
>
>> ---
>>  hmp-commands.hx               | 35 +++-------------------------
>>  hmp.c                         | 40 +++++++++++++++++++++-----------
>>  include/migration/migration.h |  5 +++-
>>  migration/migration.c         | 34 ++++++++++++++++++++++-----
>>  qapi-schema.json              | 53 
>> ++++++++++++++++++-------------------------
>>  qmp-commands.hx               | 51 ++++-------------------------------------
>>  6 files changed, 88 insertions(+), 130 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 848efee..656fbe8 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -964,35 +964,6 @@ Set cache size to @var{value} (in bytes) for xbzrle 
>> migrations.
>>  ETEXI
>>
>>      {
>> -        .name       = "migrate_set_speed",
>> -        .args_type  = "value:o",
>> -        .params     = "value",
>> -        .help       = "set maximum speed (in bytes) for migrations. "
>> -     "Defaults to MB if no size suffix is specified, ie. B/K/M/G/T",
>> -        .mhandler.cmd = hmp_migrate_set_speed,
>> -    },
>> -
>> -STEXI
>> address@hidden migrate_set_speed @var{value}
>> address@hidden migrate_set_speed
>> -Set maximum speed to @var{value} (in bytes) for migrations.
>> -ETEXI
>> -
>> -    {
>> -        .name       = "migrate_set_downtime",
>> -        .args_type  = "value:T",
>> -        .params     = "value",
>> -        .help       = "set maximum tolerated downtime (in seconds) for 
>> migrations",
>> -        .mhandler.cmd = hmp_migrate_set_downtime,
>> -    },
>> -
>> -STEXI
>> address@hidden migrate_set_downtime @var{second}
>> address@hidden migrate_set_downtime
>> -Set maximum tolerated downtime (in seconds) for migration.
>> -ETEXI
>> -
>> -    {
>>          .name       = "migrate_set_capability",
>>          .args_type  = "capability:s,state:b",
>>          .params     = "capability state",
>> @@ -1009,15 +980,15 @@ ETEXI
>>
>>      {
>>          .name       = "migrate_set_parameter",
>> -        .args_type  = "parameter:s,value:s",
>> -        .params     = "parameter value",
>> +        .args_type  = "parameter:s,value:s?,speed:o?",
>> +        .params     = "parameter value speed",
>>          .help       = "Set the parameter for migration",
>>          .mhandler.cmd = hmp_migrate_set_parameter,
>>          .command_completion = migrate_set_parameter_completion,
>>      },
>>
>>  STEXI
>> address@hidden migrate_set_parameter @var{parameter} @var{value}
>> address@hidden migrate_set_parameter @var{parameter} @var{value} @var{speed}
>>  @findex migrate_set_parameter
>>  Set the parameter @var{parameter} for migration.
>>  ETEXI
>> diff --git a/hmp.c b/hmp.c
>> index cc2056e..f61140b 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_MIGRATE_SET_SPEED],
>> +            params->migrate_set_speed);
>> +        monitor_printf(mon, " %s: %" PRId64,
>> +            
>> MigrationParameter_lookup[MIGRATION_PARAMETER_MIGRATE_SET_DOWNTIME],
>> +            params->migrate_set_downtime);
>>          monitor_printf(mon, "\n");
>>      }
>>
>> @@ -1193,12 +1199,6 @@ void hmp_migrate_incoming(Monitor *mon, const QDict 
>> *qdict)
>>      hmp_handle_error(mon, &err);
>>  }
>>
>> -void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)
>> -{
>> -    double value = qdict_get_double(qdict, "value");
>> -    qmp_migrate_set_downtime(value, NULL);
>> -}
>> -
>>  void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict)
>>  {
>>      int64_t value = qdict_get_int(qdict, "value");
>> @@ -1211,12 +1211,6 @@ void hmp_migrate_set_cache_size(Monitor *mon, const 
>> QDict *qdict)
>>      }
>>  }
>>
>> -void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict)
>> -{
>> -    int64_t value = qdict_get_int(qdict, "value");
>> -    qmp_migrate_set_speed(value, NULL);
>> -}
>> -
>>  void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict)
>>  {
>>      const char *cap = qdict_get_str(qdict, "capability");
>> @@ -1250,8 +1244,11 @@ void hmp_migrate_set_capability(Monitor *mon, const 
>> QDict *qdict)
>>  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");
>> +    const char *valuestr = qdict_get_try_str(qdict, "value");
>> +    int64_t valuespeed = 0;
>> +    double valuedowntime = 0;
>>      long valueint = 0;
>> +    char *endp;
>>      Error *err = NULL;
>>      bool has_compress_level = false;
>>      bool has_compress_threads = false;
>> @@ -1260,6 +1257,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_migrate_set_speed = false;
>> +    bool has_migrate_set_downtime = false;
>>      bool use_int_value = false;
>>      int i;
>>
>> @@ -1291,6 +1290,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const 
>> QDict *qdict)
>>              case MIGRATION_PARAMETER_TLS_HOSTNAME:
>>                  has_tls_hostname = true;
>>                  break;
>> +            case MIGRATION_PARAMETER_MIGRATE_SET_SPEED:
>> +                has_migrate_set_speed = true;
>> +                valuespeed = qdict_get_int(qdict, "speed");
>> +                break;
>> +            case MIGRATION_PARAMETER_MIGRATE_SET_DOWNTIME:
>> +                has_migrate_set_downtime = 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 +1320,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_migrate_set_speed, valuespeed,
>> +                                       has_migrate_set_downtime, 
>> valuedowntime,
>>                                         &err);
>>              break;
>>          }
>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>> index 3c96623..65cd2d7 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;
>> @@ -205,6 +204,10 @@ void migration_tls_channel_connect(MigrationState *s,
>>
>>  uint64_t migrate_max_downtime(void);
>>
>> +void migrate_set_preferred_speed(int64_t value, Error **errp);
>> +
>> +void migrate_set_expected_downtime(double value, Error **errp);
>> +
>>  void exec_start_incoming_migration(const char *host_port, Error **errp);
>>
>>  void exec_start_outgoing_migration(MigrationState *s, const char 
>> *host_port, Error **errp);
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 955d5ee..a0385ce 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,
>> +            .migrate_set_speed = MAX_THROTTLE,
>> +            .migrate_set_downtime = 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->migrate_set_speed = s->parameters.migrate_set_speed;
>> +    params->migrate_set_downtime = s->parameters.migrate_set_downtime;
>>
>>      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_migrate_set_speed,
>> +                                int64_t migrate_set_speed,
>> +                                bool has_migrate_set_downtime,
>> +                                double migrate_set_downtime,
>>                                  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_migrate_set_speed) {
>> +        migrate_set_preferred_speed(migrate_set_speed, NULL);
>> +    }
>> +    if (has_migrate_set_downtime) {
>> +        migrate_set_expected_downtime(migrate_set_downtime, NULL);
>> +    }
>>  }
>>
>>
>> @@ -1163,7 +1179,7 @@ int64_t qmp_query_migrate_cache_size(Error **errp)
>>      return migrate_xbzrle_cache_size();
>>  }
>>
>> -void qmp_migrate_set_speed(int64_t value, Error **errp)
>> +void migrate_set_preferred_speed(int64_t value, Error **errp)
>>  {
>>      MigrationState *s;
>>
>> @@ -1175,18 +1191,24 @@ void qmp_migrate_set_speed(int64_t value, Error 
>> **errp)
>>      }
>>
>>      s = migrate_get_current();
>> -    s->bandwidth_limit = value;
>> +    s->parameters.migrate_set_speed = value;
>>      if (s->to_dst_file) {
>>          qemu_file_set_rate_limit(s->to_dst_file,
>> -                                 s->bandwidth_limit / XFER_LIMIT_RATIO);
>> +                                 s->parameters.migrate_set_speed / 
>> XFER_LIMIT_RATIO);
>>      }
>>  }
>>
>> -void qmp_migrate_set_downtime(double value, Error **errp)
>> +void migrate_set_expected_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.migrate_set_downtime = 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.migrate_set_speed / 
>> 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..36b89d9 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -637,12 +637,17 @@
>>  #                hostname must be provided so that the server's x509
>>  #                certificate identity can be validated. (Since 2.7)
>>  #
>> +# @migrate-set-speed: to set maximum speed for migration. A value lesser 
>> than
>> +#                     zero will be automatically round upto zero.
>> +#
>> +# @migrate-set-downtime: set maximum tolerated downtime for migration.
>>  # 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', 'migrate-set-speed',
>> +           'migrate-set-downtime'] }
>>
>>  #
>>  # @migrate-set-parameters
>> @@ -678,6 +683,11 @@
>>  #                hostname must be provided so that the server's x509
>>  #                certificate identity can be validated. (Since 2.7)
>>  #
>> +# @migrate-set-speed: to set maximum speed for migration. A value lesser 
>> than
>> +#                     zero will be automatically round upto zero.
>> +#
>> +# @migrate-set-downtime: set maximum tolerated downtime for migration.
>> +#
>>  # Since: 2.4
>>  ##
>>  { 'command': 'migrate-set-parameters',
>> @@ -687,7 +697,9 @@
>>              '*cpu-throttle-initial': 'int',
>>              '*cpu-throttle-increment': 'int',
>>              '*tls-creds': 'str',
>> -            '*tls-hostname': 'str'} }
>> +            '*tls-hostname': 'str',
>> +            '*migrate-set-speed': 'int',
>> +            '*migrate-set-downtime': 'number'} }
>>
>>  #
>>  # @MigrationParameters
>> @@ -721,6 +733,11 @@
>>  #                hostname must be provided so that the server's x509
>>  #                certificate identity can be validated. (Since 2.7)
>>  #
>> +# @migrate-set-speed: to set maximum speed for migration. A value lesser 
>> than
>> +#                     zero will be automatically round upto zero.
>> +#
>> +# @migrate-set-downtime: set maximum tolerated downtime for migration.
>> +#
>>  # Since: 2.4
>>  ##
>>  { 'struct': 'MigrationParameters',
>> @@ -730,7 +747,9 @@
>>              'cpu-throttle-initial': 'int',
>>              'cpu-throttle-increment': 'int',
>>              'tls-creds': 'str',
>> -            'tls-hostname': 'str'} }
>> +            'tls-hostname': 'str',
>> +            'migrate-set-speed': 'int',
>> +            'migrate-set-downtime': 'int'} }
>>  ##
>>  # @query-migrate-parameters
>>  #
>> @@ -1804,34 +1823,6 @@
>>  { 'command': 'migrate_cancel' }
>>
>>  ##
>> -# @migrate_set_downtime
>> -#
>> -# Set maximum tolerated downtime for migration.
>> -#
>> -# @value: maximum downtime in seconds
>> -#
>> -# Returns: nothing on success
>> -#
>> -# Since: 0.14.0
>> -##
>> -{ 'command': 'migrate_set_downtime', 'data': {'value': 'number'} }
>> -
>> -##
>> -# @migrate_set_speed
>> -#
>> -# Set maximum speed for migration.
>> -#
>> -# @value: maximum speed in bytes.
>> -#
>> -# Returns: nothing on success
>> -#
>> -# Notes: A value lesser than zero will be automatically round up to zero.
>> -#
>> -# Since: 0.14.0
>> -##
>> -{ 'command': 'migrate_set_speed', 'data': {'value': 'int'} }
>> -
>> -##
>>  # @migrate-set-cache-size
>>  #
>>  # Set XBZRLE cache size
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 6866264..ce740e4 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -784,52 +784,6 @@ Example:
>>  EQMP
>>
>>      {
>> -        .name       = "migrate_set_speed",
>> -        .args_type  = "value:o",
>> -        .mhandler.cmd_new = qmp_marshal_migrate_set_speed,
>> -    },
>> -
>> -SQMP
>> -migrate_set_speed
>> ------------------
>> -
>> -Set maximum speed for migrations.
>> -
>> -Arguments:
>> -
>> -- "value": maximum speed, in bytes per second (json-int)
>> -
>> -Example:
>> -
>> --> { "execute": "migrate_set_speed", "arguments": { "value": 1024 } }
>> -<- { "return": {} }
>> -
>> -EQMP
>> -
>> -    {
>> -        .name       = "migrate_set_downtime",
>> -        .args_type  = "value:T",
>> -        .mhandler.cmd_new = qmp_marshal_migrate_set_downtime,
>> -    },
>> -
>> -SQMP
>> -migrate_set_downtime
>> ---------------------
>> -
>> -Set maximum tolerated downtime (in seconds) for migrations.
>> -
>> -Arguments:
>> -
>> -- "value": maximum downtime (json-number)
>> -
>> -Example:
>> -
>> --> { "execute": "migrate_set_downtime", "arguments": { "value": 0.1 } }
>> -<- { "return": {} }
>> -
>> -EQMP
>> -
>> -    {
>>          .name       = "client_migrate_info",
>>          .args_type  = 
>> "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
>>          .params     = "protocol hostname port tls-port cert-subject",
>> @@ -3790,6 +3744,9 @@ Set migration parameters
>>                            throttled for auto-converge (json-int)
>>  - "cpu-throttle-increment": set throttle increasing percentage for
>>                              auto-converge (json-int)
>> +- "migrate-set-speed": set maximum speed for migrations (json-int)
>> +- "migrate-set-downtime": set maximum tolerated downtime (in seconds) for
>> +                          migrations (json-number)
>>
>>  Arguments:
>>
>> @@ -3803,7 +3760,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?,migrate-set-speed:i?,migrate-set-downtime:T?",
>>          .mhandler.cmd_new = qmp_marshal_migrate_set_parameters,
>>      },
>>  SQMP
>>