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

[Ashijeet Acharya]

On Mon, Sep 5, 2016 at 8:06 PM, Daniel P. Berrange <address@hidden> wrote:
> 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.

Alright i will keep that in mind from now onwards.

>
>> Signed-off-by: Ashijeet Acharya <address@hidden>
>> ---
>>  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.

Hmm, that would make more sense.

>
>>  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
>

Should I document it the way it is for old-commands? Like this;

# @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
##

>> +#
>> +# @downtime-limit: set maximum tolerated downtime for migration. Since 2.8)
>
> Again, units ? nanoseconds ? milliseconds ?

Like this;

# @migrate_set_downtime
#
# Set maximum tolerated downtime for migration.
#
# @value: maximum downtime in seconds
#
# Returns: nothing on success
#
# Since: 0.14.0
>
>> +#
>>  # 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.

I dont know, that is exactly what was documented for the old-command
migrate_set_downtime.

This is what i found;

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": {} }

>
>>  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 :|