* Ashijeet Acharya (ashijeetacha...@gmail.com) 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>
> ---
> 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(-)
>
> diff --git a/hmp.c b/hmp.c
> index cc2056e..2559f5e 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 */
> 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,9 @@ 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;
> long valueint = 0;
> + char *endp;
> Error *err = NULL;
> bool has_compress_level = false;
> bool has_compress_threads = false;
> @@ -1260,6 +1270,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 +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.
> + error_setg(&err, "Invalid size %s", valuestr);
> + goto cleanup;
> + }
> + break;
> + case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
> + has_downtime_limit = true;
> + use_int_value = true;
> + break;
> }
>
> if (use_int_value) {
> @@ -1308,6 +1333,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, valueint,
> &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..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?
> +#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?
> 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.
> + 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.
(Also since we're in integer it's unusual to use 1e6)
> + 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.
> +}
> +
> +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.
> + 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..1a7c1cb 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
> +# s/bytes/bytes per second. (Since 2.8)
typo ? just bytes per second ?
> +#
> +# @downtime-limit: set maximum tolerated downtime for migration. maximum
> downtime
> +# in milliseconds (Since 2.8)
> +#
> # 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 +685,12 @@
> # 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
> +# s/bytes/bytes per second. (Since 2.8)
> +#
> +# @downtime-limit: set maximum tolerated downtime for migration. maximum
> downtime
> +# in milliseconds (Since 2.8)
> +#
> # Since: 2.4
> ##
> { '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'} }
>
> #
> # @MigrationParameters
> @@ -721,6 +736,12 @@
> # 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
> +# s/bytes/bytes per second. (Since 2.8)
> +#
> +# @downtime-limit: set maximum tolerated downtime for migration. maximum
> downtime
> +# in milliseconds (Since 2.8)
> +#
> # Since: 2.4
> ##
> { 'struct': 'MigrationParameters',
> @@ -730,7 +751,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
> #
> @@ -1812,6 +1835,8 @@
> #
> # Returns: nothing on success
> #
> +# Notes: This command is deprecated in favour of 'migrate-set-parameters'
> +#
> # Since: 0.14.0
> ##
> { 'command': 'migrate_set_downtime', 'data': {'value': 'number'} }
> @@ -1825,7 +1850,7 @@
> #
> # Returns: nothing on success
> #
> -# Notes: A value lesser than zero will be automatically round up to zero.
> +# Notes: This command is deprecated in favour of 'migrate-set-parameters'
> #
> # Since: 0.14.0
> ##
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6866264..9c4504d 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 (in bytes) (json-int)
> +- "downtime-limit": set maximum tolerated downtime (in milliseconds) for
> + migrations (json-int)
> 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:i?",
> .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)
> + - "downtime-limit" : maximum tolerated downtime of migration in
> + milliseconds (json-int)
>
> Arguments:
>
> @@ -3832,7 +3838,9 @@ Example:
> "cpu-throttle-increment": 10,
> "compress-threads": 8,
> "compress-level": 1,
> - "cpu-throttle-initial": 20
> + "cpu-throttle-initial": 20,
> + "max-downtime": 33554432,
That should be max-bandwidth?
> + "downtime-limit": 300
> }
> }
>
> --
> 2.6.2
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
