| Message ID | 1473075436-32489-1-git-send-email-ashijeetacharya@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 05/09/2016 13:37, Ashijeet Acharya wrote: > Include migrate_set_speed and migrate_set_downtime inside migrate_set_parameters for setting maximum migration speed and expected downtime parameters respectively. Also update the query part for both in qmp and hmp qemu control interfaces. > > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> > --- > hmp-commands.hx | 6 +++--- > hmp.c | 30 +++++++++++++++++++++++++++++- > include/migration/migration.h | 1 - > migration/migration.c | 30 ++++++++++++++++++++++++++---- > qapi-schema.json | 26 +++++++++++++++++++++++--- > qmp-commands.hx | 13 ++++++++++--- > 6 files changed, 91 insertions(+), 15 deletions(-) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 848efee..4dc7899 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -1009,15 +1009,15 @@ ETEXI > > { > .name = "migrate_set_parameter", > - .args_type = "parameter:s,value:s", > - .params = "parameter value", > + .args_type = "parameter:s,value:s?,speed:o?", > + .params = "parameter value speed", > .help = "Set the parameter for migration", > .mhandler.cmd = hmp_migrate_set_parameter, > .command_completion = migrate_set_parameter_completion, > }, > > STEXI > -@item migrate_set_parameter @var{parameter} @var{value} > +@item migrate_set_parameter @var{parameter} @var{value} @var{speed} This is wrong, it should not use a third argument. migrate_set_parameter is just receiving a key/value pair. Since value is a string, you can use qemu_strtosz in hmp_migrate_set_parameter to convert it to bytes and print an "invalid size" error if invalid. > @findex migrate_set_parameter > Set the parameter @var{parameter} for migration. > ETEXI > diff --git a/hmp.c b/hmp.c > index cc2056e..fd50e83 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_MIGRATE_SET_SPEED], > + params->migrate_set_speed); > + monitor_printf(mon, " %s: %" PRId64, > + MigrationParameter_lookup[MIGRATION_PARAMETER_MIGRATE_SET_DOWNTIME], > + params->migrate_set_downtime); > 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"); > @@ -1250,8 +1258,11 @@ void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict) > 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"); > + const char *valuestr = qdict_get_try_str(qdict, "value"); > + int64_t valuespeed = 0; > + double valuedowntime = 0; > long valueint = 0; > + char *endp; > Error *err = NULL; > bool has_compress_level = false; > bool has_compress_threads = false; > @@ -1260,6 +1271,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_migrate_set_speed = false; > + bool has_migrate_set_downtime = false; > bool use_int_value = false; > int i; > > @@ -1291,6 +1304,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > case MIGRATION_PARAMETER_TLS_HOSTNAME: > has_tls_hostname = true; > break; > + case MIGRATION_PARAMETER_MIGRATE_SET_SPEED: > + has_migrate_set_speed = true; > + valuespeed = qdict_get_int(qdict, "speed"); > + break; > + case MIGRATION_PARAMETER_MIGRATE_SET_DOWNTIME: > + has_migrate_set_downtime = true; > + valuedowntime = strtod(valuestr, &endp); > + if (valuestr == endp) { > + error_setg(&err, "Unable to parse '%s' as a number", > + valuestr); > + goto cleanup; > + } > + break; > } > > if (use_int_value) { > @@ -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, > + .migrate_set_speed = MAX_THROTTLE, > + .migrate_set_downtime = 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->migrate_set_speed = s->parameters.migrate_set_speed; > + params->migrate_set_downtime = s->parameters.migrate_set_downtime; > > return params; > } > @@ -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_migrate_set_speed, > + int64_t migrate_set_speed, > + bool has_migrate_set_downtime, > + double migrate_set_downtime, > Error **errp) > { > MigrationState *s = migrate_get_current(); > @@ -832,6 +842,12 @@ 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_migrate_set_speed) { > + qmp_migrate_set_speed(migrate_set_speed, NULL); > + } > + if (has_migrate_set_downtime) { > + qmp_migrate_set_downtime(migrate_set_downtime, NULL); > + } This is backwards; qmp_migrate_set_speed and qmp_migrate_set_downtime should be implemented in terms of qmp_migrate_set_parameters, not the other way round. > } > > > @@ -1175,18 +1191,24 @@ void qmp_migrate_set_speed(int64_t value, Error **errp) > } > > s = migrate_get_current(); > - s->bandwidth_limit = value; > + s->parameters.migrate_set_speed = value; > if (s->to_dst_file) { > qemu_file_set_rate_limit(s->to_dst_file, > - s->bandwidth_limit / XFER_LIMIT_RATIO); > + s->parameters.migrate_set_speed / XFER_LIMIT_RATIO); > } > } > > void qmp_migrate_set_downtime(double value, Error **errp) > { > + MigrationState *s; > + > value *= 1e9; > value = MAX(0, MIN(UINT64_MAX, value)); > + > + s = migrate_get_current(); > + > max_downtime = (uint64_t)value; > + s->parameters.migrate_set_downtime = max_downtime; > } > > bool migrate_postcopy_ram(void) > @@ -1858,7 +1880,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.migrate_set_speed / 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..b98be44 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -637,12 +637,18 @@ > # hostname must be provided so that the server's x509 > # certificate identity can be validated. (Since 2.7) > # > +# @migrate-set-speed: to set maximum speed for migration. A value lesser than > +# zero will be automatically round upto zero. > +# > +# @migrate-set-downtime: set maximum tolerated downtime for migration. Please add "Since 2.8" to the documentation for the new members. > +# > # 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', 'migrate-set-speed', > + 'migrate-set-downtime'] } MigrationParameter names are not verbs. It should be "downtime-limit" and "max-bandwidth", or something similar. > > # > # @migrate-set-parameters > @@ -678,6 +684,11 @@ > # hostname must be provided so that the server's x509 > # certificate identity can be validated. (Since 2.7) > # > +# @migrate-set-speed: to set maximum speed for migration. A value lesser than > +# zero will be automatically round upto zero. > +# > +# @migrate-set-downtime: set maximum tolerated downtime for migration. > +# > # Since: 2.4 > ## > { 'command': 'migrate-set-parameters', > @@ -687,7 +698,9 @@ > '*cpu-throttle-initial': 'int', > '*cpu-throttle-increment': 'int', > '*tls-creds': 'str', > - '*tls-hostname': 'str'} } > + '*tls-hostname': 'str', > + '*migrate-set-speed': 'int', > + '*migrate-set-downtime': 'number'} } Same here. > > # > # @MigrationParameters > @@ -721,6 +734,11 @@ > # hostname must be provided so that the server's x509 > # certificate identity can be validated. (Since 2.7) > # > +# @migrate-set-speed: to set maximum speed for migration. A value lesser than > +# zero will be automatically round upto zero. > +# > +# @migrate-set-downtime: set maximum tolerated downtime for migration. Same here, note that these are "Since 2.8". > +# > # Since: 2.4 > ## > { 'struct': 'MigrationParameters', > @@ -730,7 +748,9 @@ > 'cpu-throttle-initial': 'int', > 'cpu-throttle-increment': 'int', > 'tls-creds': 'str', > - 'tls-hostname': 'str'} } > + 'tls-hostname': 'str', > + 'migrate-set-speed': 'int', > + 'migrate-set-downtime': 'int'} } Same here about the names. > ## > # @query-migrate-parameters > # > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 6866264..c4d3809 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) > - > +- "migrate-set-speed": set maximum speed for migrations (json-octets) > +- "migrate-set-downtime": set maximum tolerated downtime (in seconds) for > + migrations (json-number) Same here about the names. > 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?,migrate-set-speed:o?,migrate-set-downtime:T?", Same here about the names. Also use "i" for QMP commands. > .mhandler.cmd_new = qmp_marshal_migrate_set_parameters, > }, > SQMP > @@ -3820,6 +3822,9 @@ Query current migration parameters > throttled (json-int) > - "cpu-throttle-increment" : throttle increasing percentage for > auto-converge (json-int) > + - "migrate-set-speed" : maximium migration speed (json-octets) > + - "migration-set-downtime" : maximum tolerated downtime of migration > + (json-number) Same here about the names. > Arguments: > > @@ -3832,7 +3837,9 @@ Example: > "cpu-throttle-increment": 10, > "compress-threads": 8, > "compress-level": 1, > - "cpu-throttle-initial": 20 > + "cpu-throttle-initial": 20, > + "migration-set-speed": 33554432, > + "migration-set-downtime": 300000000 Same here about the names. > } > } > >
On Mon, Sep 5, 2016 at 5:15 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 05/09/2016 13:37, Ashijeet Acharya wrote: >> Include migrate_set_speed and migrate_set_downtime inside migrate_set_parameters for setting maximum migration speed and expected downtime parameters respectively. Also update the query part for both in qmp and hmp qemu control interfaces. >> >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> >> --- >> hmp-commands.hx | 6 +++--- >> hmp.c | 30 +++++++++++++++++++++++++++++- >> include/migration/migration.h | 1 - >> migration/migration.c | 30 ++++++++++++++++++++++++++---- >> qapi-schema.json | 26 +++++++++++++++++++++++--- >> qmp-commands.hx | 13 ++++++++++--- >> 6 files changed, 91 insertions(+), 15 deletions(-) >> >> diff --git a/hmp-commands.hx b/hmp-commands.hx >> index 848efee..4dc7899 100644 >> --- a/hmp-commands.hx >> +++ b/hmp-commands.hx >> @@ -1009,15 +1009,15 @@ ETEXI >> >> { >> .name = "migrate_set_parameter", >> - .args_type = "parameter:s,value:s", >> - .params = "parameter value", >> + .args_type = "parameter:s,value:s?,speed:o?", >> + .params = "parameter value speed", >> .help = "Set the parameter for migration", >> .mhandler.cmd = hmp_migrate_set_parameter, >> .command_completion = migrate_set_parameter_completion, >> }, >> >> STEXI >> -@item migrate_set_parameter @var{parameter} @var{value} >> +@item migrate_set_parameter @var{parameter} @var{value} @var{speed} > > This is wrong, it should not use a third argument. > migrate_set_parameter is just receiving a key/value pair. > > Since value is a string, you can use qemu_strtosz in > hmp_migrate_set_parameter to convert it to bytes and print an "invalid > size" error if invalid. Yeah. This one was bugging me but wasn't sure what the right logic was. Will make these changes asap. > >> @findex migrate_set_parameter >> Set the parameter @var{parameter} for migration. >> ETEXI >> diff --git a/hmp.c b/hmp.c >> index cc2056e..fd50e83 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_MIGRATE_SET_SPEED], >> + params->migrate_set_speed); >> + monitor_printf(mon, " %s: %" PRId64, >> + MigrationParameter_lookup[MIGRATION_PARAMETER_MIGRATE_SET_DOWNTIME], >> + params->migrate_set_downtime); >> 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"); >> @@ -1250,8 +1258,11 @@ void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict) >> 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"); >> + const char *valuestr = qdict_get_try_str(qdict, "value"); >> + int64_t valuespeed = 0; >> + double valuedowntime = 0; >> long valueint = 0; >> + char *endp; >> Error *err = NULL; >> bool has_compress_level = false; >> bool has_compress_threads = false; >> @@ -1260,6 +1271,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_migrate_set_speed = false; >> + bool has_migrate_set_downtime = false; >> bool use_int_value = false; >> int i; >> >> @@ -1291,6 +1304,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) >> case MIGRATION_PARAMETER_TLS_HOSTNAME: >> has_tls_hostname = true; >> break; >> + case MIGRATION_PARAMETER_MIGRATE_SET_SPEED: >> + has_migrate_set_speed = true; >> + valuespeed = qdict_get_int(qdict, "speed"); >> + break; >> + case MIGRATION_PARAMETER_MIGRATE_SET_DOWNTIME: >> + has_migrate_set_downtime = true; >> + valuedowntime = strtod(valuestr, &endp); >> + if (valuestr == endp) { >> + error_setg(&err, "Unable to parse '%s' as a number", >> + valuestr); >> + goto cleanup; >> + } >> + break; >> } >> >> if (use_int_value) { >> @@ -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, >> + .migrate_set_speed = MAX_THROTTLE, >> + .migrate_set_downtime = 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->migrate_set_speed = s->parameters.migrate_set_speed; >> + params->migrate_set_downtime = s->parameters.migrate_set_downtime; >> >> return params; >> } >> @@ -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_migrate_set_speed, >> + int64_t migrate_set_speed, >> + bool has_migrate_set_downtime, >> + double migrate_set_downtime, >> Error **errp) >> { >> MigrationState *s = migrate_get_current(); >> @@ -832,6 +842,12 @@ 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_migrate_set_speed) { >> + qmp_migrate_set_speed(migrate_set_speed, NULL); >> + } >> + if (has_migrate_set_downtime) { >> + qmp_migrate_set_downtime(migrate_set_downtime, NULL); >> + } > > This is backwards; qmp_migrate_set_speed and qmp_migrate_set_downtime > should be implemented in terms of qmp_migrate_set_parameters, not the > other way round. Okay. Got it! > >> } >> >> >> @@ -1175,18 +1191,24 @@ void qmp_migrate_set_speed(int64_t value, Error **errp) >> } >> >> s = migrate_get_current(); >> - s->bandwidth_limit = value; >> + s->parameters.migrate_set_speed = value; >> if (s->to_dst_file) { >> qemu_file_set_rate_limit(s->to_dst_file, >> - s->bandwidth_limit / XFER_LIMIT_RATIO); >> + s->parameters.migrate_set_speed / XFER_LIMIT_RATIO); >> } >> } >> >> void qmp_migrate_set_downtime(double value, Error **errp) >> { >> + MigrationState *s; >> + >> value *= 1e9; >> value = MAX(0, MIN(UINT64_MAX, value)); >> + >> + s = migrate_get_current(); >> + >> max_downtime = (uint64_t)value; >> + s->parameters.migrate_set_downtime = max_downtime; >> } >> >> bool migrate_postcopy_ram(void) >> @@ -1858,7 +1880,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.migrate_set_speed / 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..b98be44 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -637,12 +637,18 @@ >> # hostname must be provided so that the server's x509 >> # certificate identity can be validated. (Since 2.7) >> # >> +# @migrate-set-speed: to set maximum speed for migration. A value lesser than >> +# zero will be automatically round upto zero. >> +# >> +# @migrate-set-downtime: set maximum tolerated downtime for migration. > > Please add "Since 2.8" to the documentation for the new members. Alright. > >> +# >> # 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', 'migrate-set-speed', >> + 'migrate-set-downtime'] } > > MigrationParameter names are not verbs. It should be "downtime-limit" > and "max-bandwidth", or something similar. > I will change the names then. >> >> # >> # @migrate-set-parameters >> @@ -678,6 +684,11 @@ >> # hostname must be provided so that the server's x509 >> # certificate identity can be validated. (Since 2.7) >> # >> +# @migrate-set-speed: to set maximum speed for migration. A value lesser than >> +# zero will be automatically round upto zero. >> +# >> +# @migrate-set-downtime: set maximum tolerated downtime for migration. >> +# >> # Since: 2.4 >> ## >> { 'command': 'migrate-set-parameters', >> @@ -687,7 +698,9 @@ >> '*cpu-throttle-initial': 'int', >> '*cpu-throttle-increment': 'int', >> '*tls-creds': 'str', >> - '*tls-hostname': 'str'} } >> + '*tls-hostname': 'str', >> + '*migrate-set-speed': 'int', >> + '*migrate-set-downtime': 'number'} } > > Same here. > >> >> # >> # @MigrationParameters >> @@ -721,6 +734,11 @@ >> # hostname must be provided so that the server's x509 >> # certificate identity can be validated. (Since 2.7) >> # >> +# @migrate-set-speed: to set maximum speed for migration. A value lesser than >> +# zero will be automatically round upto zero. >> +# >> +# @migrate-set-downtime: set maximum tolerated downtime for migration. > > Same here, note that these are "Since 2.8". > >> +# >> # Since: 2.4 >> ## >> { 'struct': 'MigrationParameters', >> @@ -730,7 +748,9 @@ >> 'cpu-throttle-initial': 'int', >> 'cpu-throttle-increment': 'int', >> 'tls-creds': 'str', >> - 'tls-hostname': 'str'} } >> + 'tls-hostname': 'str', >> + 'migrate-set-speed': 'int', >> + 'migrate-set-downtime': 'int'} } > > Same here about the names. > >> ## >> # @query-migrate-parameters >> # >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index 6866264..c4d3809 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) >> - >> +- "migrate-set-speed": set maximum speed for migrations (json-octets) >> +- "migrate-set-downtime": set maximum tolerated downtime (in seconds) for >> + migrations (json-number) > > Same here about the names. > >> 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?,migrate-set-speed:o?,migrate-set-downtime:T?", > > Same here about the names. Also use "i" for QMP commands. > >> .mhandler.cmd_new = qmp_marshal_migrate_set_parameters, >> }, >> SQMP >> @@ -3820,6 +3822,9 @@ Query current migration parameters >> throttled (json-int) >> - "cpu-throttle-increment" : throttle increasing percentage for >> auto-converge (json-int) >> + - "migrate-set-speed" : maximium migration speed (json-octets) >> + - "migration-set-downtime" : maximum tolerated downtime of migration >> + (json-number) > > Same here about the names. > >> Arguments: >> >> @@ -3832,7 +3837,9 @@ Example: >> "cpu-throttle-increment": 10, >> "compress-threads": 8, >> "compress-level": 1, >> - "cpu-throttle-initial": 20 >> + "cpu-throttle-initial": 20, >> + "migration-set-speed": 33554432, >> + "migration-set-downtime": 300000000 > > Same here about the names. > >> } >> } >> >>
On 05/09/2016 13:59, Ashijeet Acharya wrote: >>> >> + if (has_migrate_set_speed) { >>> >> + qmp_migrate_set_speed(migrate_set_speed, NULL); >>> >> + } >>> >> + if (has_migrate_set_downtime) { >>> >> + qmp_migrate_set_downtime(migrate_set_downtime, NULL); >>> >> + } >> > >> > This is backwards; qmp_migrate_set_speed and qmp_migrate_set_downtime >> > should be implemented in terms of qmp_migrate_set_parameters, not the >> > other way round. > > Okay. Got it! Thinking more about it, it's even better to have two patches. One doing it this way (qmp_migrate_set_parameters calls the old functions), the second that reverses the direction. Thanks! Paolo
On Mon, Sep 5, 2016 at 5:31 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 05/09/2016 13:59, Ashijeet Acharya wrote: >>>> >> + if (has_migrate_set_speed) { >>>> >> + qmp_migrate_set_speed(migrate_set_speed, NULL); >>>> >> + } >>>> >> + if (has_migrate_set_downtime) { >>>> >> + qmp_migrate_set_downtime(migrate_set_downtime, NULL); >>>> >> + } >>> > >>> > This is backwards; qmp_migrate_set_speed and qmp_migrate_set_downtime >>> > should be implemented in terms of qmp_migrate_set_parameters, not the >>> > other way round. >> >> Okay. Got it! > > Thinking more about it, it's even better to have two patches. One doing > it this way (qmp_migrate_set_parameters calls the old functions), the > second that reverses the direction. > Fine, I will send both the versions soon. Ashijeet > Thanks! > > Paolo
On Mon, Sep 5, 2016 at 5:15 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 05/09/2016 13:37, Ashijeet Acharya wrote: >> Include migrate_set_speed and migrate_set_downtime inside migrate_set_parameters for setting maximum migration speed and expected downtime parameters respectively. Also update the query part for both in qmp and hmp qemu control interfaces. >> >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> >> --- >> hmp-commands.hx | 6 +++--- >> hmp.c | 30 +++++++++++++++++++++++++++++- >> include/migration/migration.h | 1 - >> migration/migration.c | 30 ++++++++++++++++++++++++++---- >> qapi-schema.json | 26 +++++++++++++++++++++++--- >> qmp-commands.hx | 13 ++++++++++--- >> 6 files changed, 91 insertions(+), 15 deletions(-) >> 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?,migrate-set-speed:o?,migrate-set-downtime:T?", > > Same here about the names. Also use "i" for QMP commands. I think we will have to use "T" for downtime at-least otherwise you cant use float values for setting seconds like "0.2" for example. No issues using "i" with bandwidth though. Ashijeet
On 05/09/2016 15:09, Ashijeet Acharya wrote: >>> >> + "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?,migrate-set-speed:o?,migrate-set-downtime:T?", >> > >> > Same here about the names. Also use "i" for QMP commands. > I think we will have to use "T" for downtime at-least otherwise you > cant use float values for setting seconds like "0.2" for example. > No issues using "i" with bandwidth though. Right, I should have mentioned that my remark was about the "o" (it caught my eye because of your change to hmp-commands.hx). Sorry! Paolo
On Mon, Sep 05, 2016 at 03:16:04PM +0200, Paolo Bonzini wrote: > > > On 05/09/2016 15:09, Ashijeet Acharya wrote: > >>> >> + "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?,migrate-set-speed:o?,migrate-set-downtime:T?", > >> > > >> > Same here about the names. Also use "i" for QMP commands. > > I think we will have to use "T" for downtime at-least otherwise you > > cant use float values for setting seconds like "0.2" for example. > > No issues using "i" with bandwidth though. > > Right, I should have mentioned that my remark was about the "o" (it > caught my eye because of your change to hmp-commands.hx). Sorry! I've always thought it a rather wierd to use fractional seconds for downtime. It sort of made sense for HMP, but should really never have been carried over into QMP. IMHO, we should make it just take integer milliseconds for downtime with the new API. Regards, Daniel
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Mon, Sep 05, 2016 at 03:16:04PM +0200, Paolo Bonzini wrote: >> >> >> On 05/09/2016 15:09, Ashijeet Acharya wrote: >> >>> >> + "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?,migrate-set-speed:o?,migrate-set-downtime:T?", >> >> > >> >> > Same here about the names. Also use "i" for QMP commands. >> > I think we will have to use "T" for downtime at-least otherwise you >> > cant use float values for setting seconds like "0.2" for example. >> > No issues using "i" with bandwidth though. >> >> Right, I should have mentioned that my remark was about the "o" (it >> caught my eye because of your change to hmp-commands.hx). Sorry! > > I've always thought it a rather wierd to use fractional seconds for > downtime. It sort of made sense for HMP, but should really never have > been carried over into QMP. IMHO, we should make it just take integer > milliseconds for downtime with the new API. Seconds are a fine unit, and using floating-point numbers isn't weird, especially not in JSON. What's actually weird is our zoo of time units: seconds (migrate_set_downtime), milliseconds (MigrationInfo), microseconds (NetdevTapOptions), nanoseconds (BlockDeviceTimedStats), seconds + fractional nanoseconds (SnapshotInfo), ... No adult supervision. We should've picked *one* time unit for QMP. The obvious time unit is seconds. Nanoseconds would've worked, too. Time units are probably beyond repair now. We can still try to make them at least locally consistent, i.e. stick to what related interfaces use.
diff --git a/hmp-commands.hx b/hmp-commands.hx index 848efee..4dc7899 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1009,15 +1009,15 @@ ETEXI { .name = "migrate_set_parameter", - .args_type = "parameter:s,value:s", - .params = "parameter value", + .args_type = "parameter:s,value:s?,speed:o?", + .params = "parameter value speed", .help = "Set the parameter for migration", .mhandler.cmd = hmp_migrate_set_parameter, .command_completion = migrate_set_parameter_completion, }, STEXI -@item migrate_set_parameter @var{parameter} @var{value} +@item migrate_set_parameter @var{parameter} @var{value} @var{speed} @findex migrate_set_parameter Set the parameter @var{parameter} for migration. ETEXI diff --git a/hmp.c b/hmp.c index cc2056e..fd50e83 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_MIGRATE_SET_SPEED], + params->migrate_set_speed); + monitor_printf(mon, " %s: %" PRId64, + MigrationParameter_lookup[MIGRATION_PARAMETER_MIGRATE_SET_DOWNTIME], + params->migrate_set_downtime); 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"); @@ -1250,8 +1258,11 @@ void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict) 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"); + const char *valuestr = qdict_get_try_str(qdict, "value"); + int64_t valuespeed = 0; + double valuedowntime = 0; long valueint = 0; + char *endp; Error *err = NULL; bool has_compress_level = false; bool has_compress_threads = false; @@ -1260,6 +1271,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_migrate_set_speed = false; + bool has_migrate_set_downtime = false; bool use_int_value = false; int i; @@ -1291,6 +1304,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) case MIGRATION_PARAMETER_TLS_HOSTNAME: has_tls_hostname = true; break; + case MIGRATION_PARAMETER_MIGRATE_SET_SPEED: + has_migrate_set_speed = true; + valuespeed = qdict_get_int(qdict, "speed"); + break; + case MIGRATION_PARAMETER_MIGRATE_SET_DOWNTIME: + has_migrate_set_downtime = true; + valuedowntime = strtod(valuestr, &endp); + if (valuestr == endp) { + error_setg(&err, "Unable to parse '%s' as a number", + valuestr); + goto cleanup; + } + break; } if (use_int_value) { @@ -1308,6 +1334,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_migrate_set_speed, valuespeed, + has_migrate_set_downtime, valuedowntime, &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..3768911 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, + .migrate_set_speed = MAX_THROTTLE, + .migrate_set_downtime = 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->migrate_set_speed = s->parameters.migrate_set_speed; + params->migrate_set_downtime = s->parameters.migrate_set_downtime; return params; } @@ -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_migrate_set_speed, + int64_t migrate_set_speed, + bool has_migrate_set_downtime, + double migrate_set_downtime, Error **errp) { MigrationState *s = migrate_get_current(); @@ -832,6 +842,12 @@ 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_migrate_set_speed) { + qmp_migrate_set_speed(migrate_set_speed, NULL); + } + if (has_migrate_set_downtime) { + qmp_migrate_set_downtime(migrate_set_downtime, NULL); + } } @@ -1175,18 +1191,24 @@ void qmp_migrate_set_speed(int64_t value, Error **errp) } s = migrate_get_current(); - s->bandwidth_limit = value; + s->parameters.migrate_set_speed = value; if (s->to_dst_file) { qemu_file_set_rate_limit(s->to_dst_file, - s->bandwidth_limit / XFER_LIMIT_RATIO); + s->parameters.migrate_set_speed / XFER_LIMIT_RATIO); } } void qmp_migrate_set_downtime(double value, Error **errp) { + MigrationState *s; + value *= 1e9; value = MAX(0, MIN(UINT64_MAX, value)); + + s = migrate_get_current(); + max_downtime = (uint64_t)value; + s->parameters.migrate_set_downtime = max_downtime; } bool migrate_postcopy_ram(void) @@ -1858,7 +1880,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.migrate_set_speed / 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..b98be44 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -637,12 +637,18 @@ # hostname must be provided so that the server's x509 # certificate identity can be validated. (Since 2.7) # +# @migrate-set-speed: to set maximum speed for migration. A value lesser than +# zero will be automatically round upto zero. +# +# @migrate-set-downtime: set maximum tolerated downtime for migration. +# # 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', 'migrate-set-speed', + 'migrate-set-downtime'] } # # @migrate-set-parameters @@ -678,6 +684,11 @@ # hostname must be provided so that the server's x509 # certificate identity can be validated. (Since 2.7) # +# @migrate-set-speed: to set maximum speed for migration. A value lesser than +# zero will be automatically round upto zero. +# +# @migrate-set-downtime: set maximum tolerated downtime for migration. +# # Since: 2.4 ## { 'command': 'migrate-set-parameters', @@ -687,7 +698,9 @@ '*cpu-throttle-initial': 'int', '*cpu-throttle-increment': 'int', '*tls-creds': 'str', - '*tls-hostname': 'str'} } + '*tls-hostname': 'str', + '*migrate-set-speed': 'int', + '*migrate-set-downtime': 'number'} } # # @MigrationParameters @@ -721,6 +734,11 @@ # hostname must be provided so that the server's x509 # certificate identity can be validated. (Since 2.7) # +# @migrate-set-speed: to set maximum speed for migration. A value lesser than +# zero will be automatically round upto zero. +# +# @migrate-set-downtime: set maximum tolerated downtime for migration. +# # Since: 2.4 ## { 'struct': 'MigrationParameters', @@ -730,7 +748,9 @@ 'cpu-throttle-initial': 'int', 'cpu-throttle-increment': 'int', 'tls-creds': 'str', - 'tls-hostname': 'str'} } + 'tls-hostname': 'str', + 'migrate-set-speed': 'int', + 'migrate-set-downtime': 'int'} } ## # @query-migrate-parameters # diff --git a/qmp-commands.hx b/qmp-commands.hx index 6866264..c4d3809 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) - +- "migrate-set-speed": set maximum speed for migrations (json-octets) +- "migrate-set-downtime": set maximum tolerated downtime (in seconds) for + migrations (json-number) 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?,migrate-set-speed:o?,migrate-set-downtime:T?", .mhandler.cmd_new = qmp_marshal_migrate_set_parameters, }, SQMP @@ -3820,6 +3822,9 @@ Query current migration parameters throttled (json-int) - "cpu-throttle-increment" : throttle increasing percentage for auto-converge (json-int) + - "migrate-set-speed" : maximium migration speed (json-octets) + - "migration-set-downtime" : maximum tolerated downtime of migration + (json-number) Arguments: @@ -3832,7 +3837,9 @@ Example: "cpu-throttle-increment": 10, "compress-threads": 8, "compress-level": 1, - "cpu-throttle-initial": 20 + "cpu-throttle-initial": 20, + "migration-set-speed": 33554432, + "migration-set-downtime": 300000000 } }
Include migrate_set_speed and migrate_set_downtime inside migrate_set_parameters for setting maximum migration speed and expected downtime parameters respectively. Also update the query part for both in qmp and hmp qemu control interfaces. Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> --- hmp-commands.hx | 6 +++--- hmp.c | 30 +++++++++++++++++++++++++++++- include/migration/migration.h | 1 - migration/migration.c | 30 ++++++++++++++++++++++++++---- qapi-schema.json | 26 +++++++++++++++++++++++--- qmp-commands.hx | 13 ++++++++++--- 6 files changed, 91 insertions(+), 15 deletions(-)