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

[Ashijeet Acharya]

On Thu, Sep 8, 2016 at 3:33 AM, Eric Blake <address@hidden> wrote:
> On 09/06/2016 08:39 AM, Ashijeet Acharya 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>
>> ---
>
>> +++ b/migration/migration.c
>> @@ -44,6 +44,9 @@
>>  #define BUFFER_DELAY     100
>>  #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
>>
>> +/* Time in nanoseconds we are allowed to stop the source for to send last 
>> part */
>
> Long line.  Also, a grammar issue:
>
> s/source for to send/source, for sending the/

Okay, I will change it.

>
>> @@ -832,6 +848,21 @@ 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 *= 1000000; /* convert to nanoseconds */
>
> Are you sure this won't overflow?

Should I explicitly cast it? Like;
downtime_limit *= (int64_t)1000000;
This should work right?

>> +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,
>
> Ugg. This looks gross.  No need to name a bunch of variables set to
> false, when you can instead use QAPI's boxed conventions to pass a
> pointer to a struct, and rely on 0-initialization of the struct to leave
> all the parameters that you don't care about unmentioned.
>

I know. But I was not sure if I can do something else.
'hmp.c' does it in a similar way. I am not aware of the method you described.
Maybe you can direct me to some place where I can code by example.
(Sorry! I am a newbie)

>> +                               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);
>> +
>> +}
>> +
>> +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;
>
> Again, gross.
>
>> +    const char *valuestr = NULL;
>> +    long valueint = 0;
>> +    int64_t valuebw = 0;
>> +    valuedowntime *= 1000; /* Convert to milliseconds */
>> +    valuedowntime = MAX(0, MIN(INT64_MAX, valuedowntime));
>> +    valuedowntime = (int64_t)valuedowntime;
>
> Useless statement; the cast would already implicitly happen when passing
> it to the function call.

Okay, I will remove that.

>
>> +    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..a8ee2d4 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
>> +#                 bytes per second. (Since 2.8)
>> +#
>> +# @downtime-limit: set maximum tolerated downtime for migration. maximum 
>> downtime
>
> Long line. Please wrap to stay under 80 columns.

Okay.

>
>> +#                  in milliseconds (Since 2.8)
>
> Are we sure milliseconds is the desired scale?

I am really confused regarding the scale for downtime now.
There seems to be a divided opinion on this one among developers with
everyone having their valid points.
I will stall the updated patch until we have a common ground on this one.

>
>>  { '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'} }
>
> For that matter, would a floating point downtime-limit make any more
> sense (in which case, I'd argue that having it be in seconds rather than
> milliseconds may be nicer)?

Yeah! Floating point seems useful only if we are talking in seconds.
A floating point value in milliseconds seems impractical as far as I understand.

>
>
>> @@ -1812,6 +1835,8 @@
>>  #
>>  # Returns: nothing on success
>>  #
>> +# Notes: This command is deprecated in favour of 'migrate-set-parameters'
>
> I don't know if we have a strong preference for US vs. UK spelling in
> documentation.

I can change it to 'favor' if you like.

>
>> @@ -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?",
>
> Long line; you can break it up (but then again, we hope to get rid of
> all .args_type lines once Marc-Andre's qapi work lands)

Breaking it rejects the arguments after the break I guess and I get an
error if I use them in qmp later.
The line was long even before including 'max-bandwidth' and
'downtime-limit' anyway.

Ashijeet

> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>