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

[Ashijeet Acharya]

On Mon, Sep 5, 2016 at 5:15 PM, Paolo Bonzini <address@hidden> wrote:
>
>
> On 05/09/2016 13:37, 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.
>>
>> Signed-off-by: Ashijeet Acharya <address@hidden>
>> ---
>>  hmp-commands.hx               |  6 +++---
>>  hmp.c                         | 30 +++++++++++++++++++++++++++++-
>>  include/migration/migration.h |  1 -
>>  migration/migration.c         | 30 ++++++++++++++++++++++++++----
>>  qapi-schema.json              | 26 +++++++++++++++++++++++---
>>  qmp-commands.hx               | 13 ++++++++++---
>>  6 files changed, 91 insertions(+), 15 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 848efee..4dc7899 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1009,15 +1009,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}
>
> This is wrong, it should not use a third argument.
> migrate_set_parameter is just receiving a key/value pair.
>
> Since value is a string, you can use qemu_strtosz in
> hmp_migrate_set_parameter to convert it to bytes and print an "invalid
> size" error if invalid.

Yeah. This one was bugging me but wasn't sure what the right logic was.
Will make these changes asap.

>
>>  @findex migrate_set_parameter
>>  Set the parameter @var{parameter} for migration.
>>  ETEXI
>> diff --git a/hmp.c b/hmp.c
>> index cc2056e..fd50e83 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,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");
>> @@ -1250,8 +1258,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 +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_migrate_set_speed = false;
>> +    bool has_migrate_set_downtime = false;
>>      bool use_int_value = false;
>>      int i;
>>
>> @@ -1291,6 +1304,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) {
>> @@ -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) {
>> +        qmp_migrate_set_speed(migrate_set_speed, NULL);
>> +    }
>> +    if (has_migrate_set_downtime) {
>> +        qmp_migrate_set_downtime(migrate_set_downtime, NULL);
>> +    }
>
> This is backwards; qmp_migrate_set_speed and qmp_migrate_set_downtime
> should be implemented in terms of qmp_migrate_set_parameters, not the
> other way round.

Okay. Got it!

>
>>  }
>>
>>
>> @@ -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)
>>  {
>> +    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..b98be44 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)
>>  #
>> +# @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.
>
> Please add "Since 2.8" to the documentation for the new members.

Alright.

>
>> +#
>>  # 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'] }
>
> MigrationParameter names are not verbs.  It should be "downtime-limit"
> and "max-bandwidth", or something similar.
>

I will change the names then.

>>
>>  #
>>  # @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)
>>  #
>> +# @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 +698,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'} }
>
> Same here.
>
>>
>>  #
>>  # @MigrationParameters
>> @@ -721,6 +734,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.
>
> Same here, note that these are "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',
>> +            'migrate-set-speed': 'int',
>> +            'migrate-set-downtime': 'int'} }
>
> Same here about the names.
>
>>  ##
>>  # @query-migrate-parameters
>>  #
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 6866264..c4d3809 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)
>> -
>> +- "migrate-set-speed": set maximum speed for migrations (json-octets)
>> +- "migrate-set-downtime": set maximum tolerated downtime (in seconds) for
>> +                          migrations (json-number)
>
> Same here about the names.
>
>>  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?,migrate-set-speed:o?,migrate-set-downtime:T?",
>
> Same here about the names.  Also use "i" for QMP commands.
>
>>          .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)
>> +         - "migrate-set-speed" : maximium migration speed (json-octets)
>> +         - "migration-set-downtime" : maximum tolerated downtime of 
>> migration
>> +                                      (json-number)
>
> Same here about the names.
>
>>  Arguments:
>>
>> @@ -3832,7 +3837,9 @@ Example:
>>           "cpu-throttle-increment": 10,
>>           "compress-threads": 8,
>>           "compress-level": 1,
>> -         "cpu-throttle-initial": 20
>> +         "cpu-throttle-initial": 20,
>> +         "migration-set-speed": 33554432,
>> +         "migration-set-downtime": 300000000
>
> Same here about the names.
>
>>        }
>>     }
>>
>>