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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7601 bytes --]

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 <ashijeetacharya@gmail.com>
> ---

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

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

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

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

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

> +#                  in milliseconds (Since 2.8)

Are we sure milliseconds is the desired scale?

>  { '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)?


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

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

>          .mhandler.cmd_new = qmp_marshal_migrate_set_parameters,
>      },
>  SQMP
> @@ -3820,6 +3822,10 @@ Query current migration parameters
>                                      throttled (json-int)
>           - "cpu-throttle-increment" : throttle increasing percentage for
>                                        auto-converge (json-int)
> +         - "max-bandwidth" : maximium migration speed in s/bytes/bytes per second
> +                             (json-int)

Umm, that's not what you meant to type.

s,s/bytes/,,


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]