From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
To: Ashijeet Acharya <ashijeetacharya@gmail.com>
Cc: lcapitulino@redhat.com, quintela@redhat.com,
amit.shah@redhat.com, eblake@redhat.com, armbru@redhat.com,
pbonzini@redhat.com, berrange@redhat.com, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v2] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp
Date: Tue, 6 Sep 2016 12:02:40 +0100 [thread overview]
Message-ID: <20160906110240.GE2041@work-vm> (raw)
In-Reply-To: <1473147386-29021-1-git-send-email-ashijeetacharya@gmail.com>
* Ashijeet Acharya (ashijeetacharya@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 <ashijeetacharya@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 / dgilbert@redhat.com / Manchester, UK
next prev parent reply other threads:[~2016-09-06 11:02 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-06 7:36 [Qemu-devel] [PATCH v2] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp Ashijeet Acharya
2016-09-06 11:02 ` Dr. David Alan Gilbert [this message]
2016-09-06 13:22 ` Ashijeet Acharya
2016-09-06 13:26 ` Ashijeet Acharya
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160906110240.GE2041@work-vm \
--to=dgilbert@redhat.com \
--cc=amit.shah@redhat.com \
--cc=armbru@redhat.com \
--cc=ashijeetacharya@gmail.com \
--cc=berrange@redhat.com \
--cc=eblake@redhat.com \
--cc=lcapitulino@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=quintela@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.