| Message ID | 1473147386-29021-1-git-send-email-ashijeetacharya@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* 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
On Tue, Sep 6, 2016 at 4:32 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > * 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(-) >> >> @@ -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. Oh, i wasn't aware of that. I will make the correction. >> 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? > Okay; I can change the comment. I took that because 'max_downtime' had a similar comment. >> +#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? If i don't do that and the > >> 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. I converted it to nano so that I don't have to change the calculations later which currently are based on nano as the unit. I will include the comment though. > >> + 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. Yeah, I missed on that. I had it there because earlier I was having both new/old-commands taking value in double. I will move it to qmp_migrate_set_downtime() now. Sorry! > (Also since we're in integer it's unusual to use 1e6) I changed it to '1000000' now. > >> + 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. I am not sure what I can do about that. Its true the arguments are too many. But hmp_migrate_set_parameter() does it the same way, and I followed that. >> +} >> + >> +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. Yeah moved it over here. Also, there seems to be an issue with changing the units to milliseconds as Juan and Markus stressed upon it in the other thread. I am not sure what to do now. Ashijeet > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Tue, Sep 6, 2016 at 6:52 PM, Ashijeet Acharya <ashijeetacharya@gmail.com> wrote: > On Tue, Sep 6, 2016 at 4:32 PM, Dr. David Alan Gilbert > <dgilbert@redhat.com> wrote: >> * 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(-) >>> >>> @@ -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? > > If i don't do that and the Sorry, I forgot to complete that. I got my mistake and I will revert it to original. Thanks Ashijeet >> -- >> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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)) { + 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. */ +#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; 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; 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; + downtime_limit = MAX(0, MIN(INT64_MAX, downtime_limit)); + + 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); + +} + +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; + 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) +# +# @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, + "downtime-limit": 300 } }
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(-)