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

[Ashijeet Acharya]

On Tue, Sep 6, 2016 at 4:32 PM, Dr. David Alan Gilbert
<address@hidden> wrote:
> * 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(-)
>>
>> @@ -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.

Oh, i wasn't aware of that. I will make the correction.

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

Okay; I can change the comment. I took that because 'max_downtime' had
a similar comment.

>> +#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?

If i don't do that and the
>
>>          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.

I converted it to nano so that I don't have to change the calculations
later which currently are based on nano as the unit.
I will include the comment though.

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

Yeah, I missed on that. I had it there because earlier I was having
both new/old-commands taking value in double.
I will move it to qmp_migrate_set_downtime() now. Sorry!

> (Also since we're in integer it's unusual to use 1e6)

I changed it to '1000000' now.

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

I am not sure what I can do about that. Its true the arguments are too many.
But hmp_migrate_set_parameter() does it the same way, and I followed that.

>> +}
>> +
>> +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.

Yeah moved it over here.

Also, there seems to be an issue with changing the units to
milliseconds as Juan and Markus stressed upon it in the other thread.
I am not sure what to do now.

Ashijeet
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK