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 <ashijeetacha...@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
signature.asc
Description: OpenPGP digital signature
