| Message ID | 1473169187-22750-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> Thanks, applied for 2.8. Later, Juan.
On Wed, Sep 7, 2016 at 2:11 PM, Juan Quintela <quintela@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> > > > Thanks, applied for 2.8. Great! Thanks a lot. Ashijeet > > Later, Juan.
On 09/06/2016 08:39 AM, Ashijeet Acharya wrote: > Mark old-commands for speed and downtime as deprecated. > Move max-bandwidth and downtime-limit into migrate-set-parameters for > setting maximum migration speed and expected downtime limit parameters > respectively. > Change downtime units to milliseconds (only for new-command) and update > the query part in both hmp and qmp qemu control interfaces. > > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> > --- > +++ b/migration/migration.c > @@ -44,6 +44,9 @@ > #define BUFFER_DELAY 100 > #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY) > > +/* Time in nanoseconds we are allowed to stop the source for to send last part */ Long line. Also, a grammar issue: s/source for to send/source, for sending the/ > @@ -832,6 +848,21 @@ void qmp_migrate_set_parameters(bool has_compress_level, > g_free(s->parameters.tls_hostname); > s->parameters.tls_hostname = g_strdup(tls_hostname); > } > + if (has_max_bandwidth) { > + s->parameters.max_bandwidth = max_bandwidth; > + if (s->to_dst_file) { > + qemu_file_set_rate_limit(s->to_dst_file, > + s->parameters.max_bandwidth / XFER_LIMIT_RATIO); > + } > + } > + if (has_downtime_limit) { > + downtime_limit *= 1000000; /* convert to nanoseconds */ Are you sure this won't overflow? > +void qmp_migrate_set_speed(int64_t valuebw, Error **errp) > +{ > + bool has_compress_level = false; > + bool has_compress_threads = false; > + bool has_decompress_threads = false; > + bool has_cpu_throttle_initial = false; > + bool has_cpu_throttle_increment = false; > + bool has_tls_creds = false; > + bool has_tls_hostname = false; > + bool has_max_bandwidth = true; > + bool has_downtime_limit = false; > + const char *valuestr = NULL; > + long valueint = 0; > + Error *err = NULL; > + > + qmp_migrate_set_parameters(has_compress_level, valueint, > + has_compress_threads, valueint, Ugg. This looks gross. No need to name a bunch of variables set to false, when you can instead use QAPI's boxed conventions to pass a pointer to a struct, and rely on 0-initialization of the struct to leave all the parameters that you don't care about unmentioned. > + has_decompress_threads, valueint, > + has_cpu_throttle_initial, valueint, > + has_cpu_throttle_increment, valueint, > + has_tls_creds, valuestr, > + has_tls_hostname, valuestr, > + has_max_bandwidth, valuebw, > + has_downtime_limit, valueint, > + &err); > + > +} > + > +void qmp_migrate_set_downtime(double valuedowntime, Error **errp) > +{ > + bool has_compress_level = false; > + bool has_compress_threads = false; > + bool has_decompress_threads = false; > + bool has_cpu_throttle_initial = false; > + bool has_cpu_throttle_increment = false; > + bool has_tls_creds = false; > + bool has_tls_hostname = false; > + bool has_max_bandwidth = false; > + bool has_downtime_limit = true; Again, gross. > + const char *valuestr = NULL; > + long valueint = 0; > + int64_t valuebw = 0; > + valuedowntime *= 1000; /* Convert to milliseconds */ > + valuedowntime = MAX(0, MIN(INT64_MAX, valuedowntime)); > + valuedowntime = (int64_t)valuedowntime; Useless statement; the cast would already implicitly happen when passing it to the function call. > + Error *err = NULL; > + > + qmp_migrate_set_parameters(has_compress_level, valueint, > + has_compress_threads, valueint, > + has_decompress_threads, valueint, > + has_cpu_throttle_initial, valueint, > + has_cpu_throttle_increment, valueint, > + has_tls_creds, valuestr, > + has_tls_hostname, valuestr, > + has_max_bandwidth, valuebw, > + has_downtime_limit, valuedowntime, > + &err); > } > > bool migrate_postcopy_ram(void) > @@ -1858,7 +1922,7 @@ void migrate_fd_connect(MigrationState *s) > > qemu_file_set_blocking(s->to_dst_file, true); > qemu_file_set_rate_limit(s->to_dst_file, > - s->bandwidth_limit / XFER_LIMIT_RATIO); > + s->parameters.max_bandwidth / XFER_LIMIT_RATIO); > > /* Notify before starting migration thread */ > notifier_list_notify(&migration_state_notifiers, s); > diff --git a/qapi-schema.json b/qapi-schema.json > index 5658723..a8ee2d4 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -637,12 +637,19 @@ > # hostname must be provided so that the server's x509 > # certificate identity can be validated. (Since 2.7) > # > +# @max-bandwidth: to set maximum speed for migration. maximum speed in > +# bytes per second. (Since 2.8) > +# > +# @downtime-limit: set maximum tolerated downtime for migration. maximum downtime Long line. Please wrap to stay under 80 columns. > +# in milliseconds (Since 2.8) Are we sure milliseconds is the desired scale? > { 'command': 'migrate-set-parameters', > @@ -687,7 +700,9 @@ > '*cpu-throttle-initial': 'int', > '*cpu-throttle-increment': 'int', > '*tls-creds': 'str', > - '*tls-hostname': 'str'} } > + '*tls-hostname': 'str', > + '*max-bandwidth': 'int', > + '*downtime-limit': 'int'} } For that matter, would a floating point downtime-limit make any more sense (in which case, I'd argue that having it be in seconds rather than milliseconds may be nicer)? > @@ -1812,6 +1835,8 @@ > # > # Returns: nothing on success > # > +# Notes: This command is deprecated in favour of 'migrate-set-parameters' I don't know if we have a strong preference for US vs. UK spelling in documentation. > @@ -3803,7 +3805,7 @@ EQMP > { > .name = "migrate-set-parameters", > .args_type = > - "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?", > + "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?,max-bandwidth:i?,downtime-limit:i?", Long line; you can break it up (but then again, we hope to get rid of all .args_type lines once Marc-Andre's qapi work lands) > .mhandler.cmd_new = qmp_marshal_migrate_set_parameters, > }, > SQMP > @@ -3820,6 +3822,10 @@ Query current migration parameters > throttled (json-int) > - "cpu-throttle-increment" : throttle increasing percentage for > auto-converge (json-int) > + - "max-bandwidth" : maximium migration speed in s/bytes/bytes per second > + (json-int) Umm, that's not what you meant to type. s,s/bytes/,,
On Thu, Sep 8, 2016 at 3:33 AM, Eric Blake <eblake@redhat.com> wrote: > On 09/06/2016 08:39 AM, Ashijeet Acharya wrote: >> Mark old-commands for speed and downtime as deprecated. >> Move max-bandwidth and downtime-limit into migrate-set-parameters for >> setting maximum migration speed and expected downtime limit parameters >> respectively. >> Change downtime units to milliseconds (only for new-command) and update >> the query part in both hmp and qmp qemu control interfaces. >> >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> >> --- > >> +++ b/migration/migration.c >> @@ -44,6 +44,9 @@ >> #define BUFFER_DELAY 100 >> #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY) >> >> +/* Time in nanoseconds we are allowed to stop the source for to send last part */ > > Long line. Also, a grammar issue: > > s/source for to send/source, for sending the/ Okay, I will change it. > >> @@ -832,6 +848,21 @@ void qmp_migrate_set_parameters(bool has_compress_level, >> g_free(s->parameters.tls_hostname); >> s->parameters.tls_hostname = g_strdup(tls_hostname); >> } >> + if (has_max_bandwidth) { >> + s->parameters.max_bandwidth = max_bandwidth; >> + if (s->to_dst_file) { >> + qemu_file_set_rate_limit(s->to_dst_file, >> + s->parameters.max_bandwidth / XFER_LIMIT_RATIO); >> + } >> + } >> + if (has_downtime_limit) { >> + downtime_limit *= 1000000; /* convert to nanoseconds */ > > Are you sure this won't overflow? Should I explicitly cast it? Like; downtime_limit *= (int64_t)1000000; This should work right? >> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp) >> +{ >> + bool has_compress_level = false; >> + bool has_compress_threads = false; >> + bool has_decompress_threads = false; >> + bool has_cpu_throttle_initial = false; >> + bool has_cpu_throttle_increment = false; >> + bool has_tls_creds = false; >> + bool has_tls_hostname = false; >> + bool has_max_bandwidth = true; >> + bool has_downtime_limit = false; >> + const char *valuestr = NULL; >> + long valueint = 0; >> + Error *err = NULL; >> + >> + qmp_migrate_set_parameters(has_compress_level, valueint, >> + has_compress_threads, valueint, > > Ugg. This looks gross. No need to name a bunch of variables set to > false, when you can instead use QAPI's boxed conventions to pass a > pointer to a struct, and rely on 0-initialization of the struct to leave > all the parameters that you don't care about unmentioned. > I know. But I was not sure if I can do something else. 'hmp.c' does it in a similar way. I am not aware of the method you described. Maybe you can direct me to some place where I can code by example. (Sorry! I am a newbie) >> + has_decompress_threads, valueint, >> + has_cpu_throttle_initial, valueint, >> + has_cpu_throttle_increment, valueint, >> + has_tls_creds, valuestr, >> + has_tls_hostname, valuestr, >> + has_max_bandwidth, valuebw, >> + has_downtime_limit, valueint, >> + &err); >> + >> +} >> + >> +void qmp_migrate_set_downtime(double valuedowntime, Error **errp) >> +{ >> + bool has_compress_level = false; >> + bool has_compress_threads = false; >> + bool has_decompress_threads = false; >> + bool has_cpu_throttle_initial = false; >> + bool has_cpu_throttle_increment = false; >> + bool has_tls_creds = false; >> + bool has_tls_hostname = false; >> + bool has_max_bandwidth = false; >> + bool has_downtime_limit = true; > > Again, gross. > >> + const char *valuestr = NULL; >> + long valueint = 0; >> + int64_t valuebw = 0; >> + valuedowntime *= 1000; /* Convert to milliseconds */ >> + valuedowntime = MAX(0, MIN(INT64_MAX, valuedowntime)); >> + valuedowntime = (int64_t)valuedowntime; > > Useless statement; the cast would already implicitly happen when passing > it to the function call. Okay, I will remove that. > >> + Error *err = NULL; >> + >> + qmp_migrate_set_parameters(has_compress_level, valueint, >> + has_compress_threads, valueint, >> + has_decompress_threads, valueint, >> + has_cpu_throttle_initial, valueint, >> + has_cpu_throttle_increment, valueint, >> + has_tls_creds, valuestr, >> + has_tls_hostname, valuestr, >> + has_max_bandwidth, valuebw, >> + has_downtime_limit, valuedowntime, >> + &err); >> } >> >> bool migrate_postcopy_ram(void) >> @@ -1858,7 +1922,7 @@ void migrate_fd_connect(MigrationState *s) >> >> qemu_file_set_blocking(s->to_dst_file, true); >> qemu_file_set_rate_limit(s->to_dst_file, >> - s->bandwidth_limit / XFER_LIMIT_RATIO); >> + s->parameters.max_bandwidth / XFER_LIMIT_RATIO); >> >> /* Notify before starting migration thread */ >> notifier_list_notify(&migration_state_notifiers, s); >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 5658723..a8ee2d4 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -637,12 +637,19 @@ >> # hostname must be provided so that the server's x509 >> # certificate identity can be validated. (Since 2.7) >> # >> +# @max-bandwidth: to set maximum speed for migration. maximum speed in >> +# bytes per second. (Since 2.8) >> +# >> +# @downtime-limit: set maximum tolerated downtime for migration. maximum downtime > > Long line. Please wrap to stay under 80 columns. Okay. > >> +# in milliseconds (Since 2.8) > > Are we sure milliseconds is the desired scale? I am really confused regarding the scale for downtime now. There seems to be a divided opinion on this one among developers with everyone having their valid points. I will stall the updated patch until we have a common ground on this one. > >> { 'command': 'migrate-set-parameters', >> @@ -687,7 +700,9 @@ >> '*cpu-throttle-initial': 'int', >> '*cpu-throttle-increment': 'int', >> '*tls-creds': 'str', >> - '*tls-hostname': 'str'} } >> + '*tls-hostname': 'str', >> + '*max-bandwidth': 'int', >> + '*downtime-limit': 'int'} } > > For that matter, would a floating point downtime-limit make any more > sense (in which case, I'd argue that having it be in seconds rather than > milliseconds may be nicer)? Yeah! Floating point seems useful only if we are talking in seconds. A floating point value in milliseconds seems impractical as far as I understand. > > >> @@ -1812,6 +1835,8 @@ >> # >> # Returns: nothing on success >> # >> +# Notes: This command is deprecated in favour of 'migrate-set-parameters' > > I don't know if we have a strong preference for US vs. UK spelling in > documentation. I can change it to 'favor' if you like. > >> @@ -3803,7 +3805,7 @@ EQMP >> { >> .name = "migrate-set-parameters", >> .args_type = >> - "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?", >> + "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?,max-bandwidth:i?,downtime-limit:i?", > > Long line; you can break it up (but then again, we hope to get rid of > all .args_type lines once Marc-Andre's qapi work lands) Breaking it rejects the arguments after the break I guess and I get an error if I use them in qmp later. The line was long even before including 'max-bandwidth' and 'downtime-limit' anyway. Ashijeet > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
Eric Blake <eblake@redhat.com> wrote: >> + if (has_max_bandwidth) { >> + s->parameters.max_bandwidth = max_bandwidth; >> + if (s->to_dst_file) { >> + qemu_file_set_rate_limit(s->to_dst_file, >> + s->parameters.max_bandwidth / XFER_LIMIT_RATIO); >> + } >> + } >> + if (has_downtime_limit) { >> + downtime_limit *= 1000000; /* convert to nanoseconds */ > > Are you sure this won't overflow? Ashijeet, Eric mere means that if downtime_limit is bigger that INT64_MAX/1000000, then you get an overflow with the multiplication. Notice that if it overflows, the value is already quite big O:-) 2^63/1000/1000/1000/3600/24/365 292.47 Allowing "only" 292 years of downtime should be enough for the time being (famous last words) O:-) > >> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp) >> +{ >> + bool has_compress_level = false; >> + bool has_compress_threads = false; >> + bool has_decompress_threads = false; >> + bool has_cpu_throttle_initial = false; >> + bool has_cpu_throttle_increment = false; >> + bool has_tls_creds = false; >> + bool has_tls_hostname = false; >> + bool has_max_bandwidth = true; >> + bool has_downtime_limit = false; >> + const char *valuestr = NULL; >> + long valueint = 0; >> + Error *err = NULL; >> + >> + qmp_migrate_set_parameters(has_compress_level, valueint, >> + has_compress_threads, valueint, > > Ugg. This looks gross. No need to name a bunch of variables set to > false, when you can instead use QAPI's boxed conventions to pass a > pointer to a struct, and rely on 0-initialization of the struct to leave > all the parameters that you don't care about unmentioned. We should change qmp_migrate_set_parameters to your new api. I fully agree that this is gross, but it is the way it was written, nothing to do with this patch, really. Ashijeet, if you want to do this change in a different patch before this change, I am all for it, but that is independent of your change. With all the other suggestions of Eric, I agree, or I don't care. (If time is an int in milliseconds or a float, I don't really care one way or another. Whatever libvirt preffers). Thanks, Juan.
On Thu, Sep 8, 2016 at 8:11 PM, Juan Quintela <quintela@redhat.com> wrote: > Eric Blake <eblake@redhat.com> wrote: > >>> + if (has_max_bandwidth) { >>> + s->parameters.max_bandwidth = max_bandwidth; >>> + if (s->to_dst_file) { >>> + qemu_file_set_rate_limit(s->to_dst_file, >>> + s->parameters.max_bandwidth / XFER_LIMIT_RATIO); >>> + } >>> + } >>> + if (has_downtime_limit) { >>> + downtime_limit *= 1000000; /* convert to nanoseconds */ >> >> Are you sure this won't overflow? > > Ashijeet, Eric mere means that if downtime_limit is bigger that > INT64_MAX/1000000, then you get an overflow with the multiplication. Oh I get it now. > Notice that if it overflows, the value is already quite big O:-) > > 2^63/1000/1000/1000/3600/24/365 > 292.47 > > Allowing "only" 292 years of downtime should be enough for the time > being (famous last words) O:-) > Haha! 292 years seems sufficient. :-) > >> >>> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp) >>> +{ >>> + bool has_compress_level = false; >>> + bool has_compress_threads = false; >>> + bool has_decompress_threads = false; >>> + bool has_cpu_throttle_initial = false; >>> + bool has_cpu_throttle_increment = false; >>> + bool has_tls_creds = false; >>> + bool has_tls_hostname = false; >>> + bool has_max_bandwidth = true; >>> + bool has_downtime_limit = false; >>> + const char *valuestr = NULL; >>> + long valueint = 0; >>> + Error *err = NULL; >>> + >>> + qmp_migrate_set_parameters(has_compress_level, valueint, >>> + has_compress_threads, valueint, >> >> Ugg. This looks gross. No need to name a bunch of variables set to >> false, when you can instead use QAPI's boxed conventions to pass a >> pointer to a struct, and rely on 0-initialization of the struct to leave >> all the parameters that you don't care about unmentioned. > > We should change qmp_migrate_set_parameters to your new api. I fully > agree that this is gross, but it is the way it was written, nothing to > do with this patch, really. Yeah, you expressed your disgust about this in your 'to-do list of migration mail' too I remember. > > Ashijeet, if you want to do this change in a different patch before this > change, I am all for it, but that is independent of your change. I can try, but I am not sure about it as I am very new to QAPI's methods. Its better if I embarked upon such a task later with more knowledge of QAPI. > With all the other suggestions of Eric, I agree, or I don't care. > (If time is an int in milliseconds or a float, I don't really care one > way or another. Whatever libvirt preffers). I am keeping it to milliseconds for now then! Juan, do not apply the patch just yet. Let me rectify the small mistakes regarding grammatical errors in comments and send the updated patch in a few. Accept it after that. Thanks Ashijeet > > Thanks, Juan.
On 09/08/2016 10:30 AM, Ashijeet Acharya wrote: >>>> + if (has_downtime_limit) { >>>> + downtime_limit *= 1000000; /* convert to nanoseconds */ >>> >>> Are you sure this won't overflow? >> >> Ashijeet, Eric mere means that if downtime_limit is bigger that >> INT64_MAX/1000000, then you get an overflow with the multiplication. > > Oh I get it now. > >> Notice that if it overflows, the value is already quite big O:-) >> >> 2^63/1000/1000/1000/3600/24/365 >> 292.47 >> >> Allowing "only" 292 years of downtime should be enough for the time >> being (famous last words) O:-) >> > > Haha! 292 years seems sufficient. :-) But it still means we should explicitly check that whatever number the user inputs is not wrapping around to a smaller actual downtime - especially since wraparound to a small number CAN be observed within a human lifespan, for some values of wraparound. Either give the user an error (their input was too big; preferred) or silently convert the user's too-large input to the maximum possible (they won't live long enough to notice the difference, not ideal, but if reporting an error is too hard, it is workable). >>>> + >>>> + qmp_migrate_set_parameters(has_compress_level, valueint, >>>> + has_compress_threads, valueint, >>> >>> Ugg. This looks gross. No need to name a bunch of variables set to >>> false, when you can instead use QAPI's boxed conventions to pass a >>> pointer to a struct, and rely on 0-initialization of the struct to leave >>> all the parameters that you don't care about unmentioned. >> >> We should change qmp_migrate_set_parameters to your new api. I fully >> agree that this is gross, but it is the way it was written, nothing to >> do with this patch, really. > > Yeah, you expressed your disgust about this in your 'to-do list of > migration mail' too I remember. Okay, I'll take the hint and submit a patch to convert migration to use the new boxed parameters from QAPI. It's an independent change, but you would then rebase your series on top of my patch (or if yours lands first, my work would remove the additions in yours, for some churn in the code base, but no real harm done).
diff --git a/hmp.c b/hmp.c index cc2056e..639c3ad 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,18 @@ 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') { + 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 +1332,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..8f620ee 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -44,6 +44,9 @@ #define BUFFER_DELAY 100 #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY) +/* Time in nanoseconds we are allowed to stop the source for to send last part */ +#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; } @@ -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,21 @@ void qmp_migrate_set_parameters(bool has_compress_level, g_free(s->parameters.tls_hostname); s->parameters.tls_hostname = g_strdup(tls_hostname); } + if (has_max_bandwidth) { + s->parameters.max_bandwidth = max_bandwidth; + if (s->to_dst_file) { + qemu_file_set_rate_limit(s->to_dst_file, + s->parameters.max_bandwidth / XFER_LIMIT_RATIO); + } + } + if (has_downtime_limit) { + downtime_limit *= 1000000; /* convert to nanoseconds */ + + s = migrate_get_current(); + + max_downtime = downtime_limit; + s->parameters.downtime_limit = max_downtime; + } } @@ -1163,30 +1194,63 @@ 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 = MAX(0, MIN(INT64_MAX, valuedowntime)); + 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..a8ee2d4 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -637,12 +637,19 @@ # hostname must be provided so that the server's x509 # certificate identity can be validated. (Since 2.7) # +# @max-bandwidth: to set maximum speed for migration. maximum speed in +# bytes per second. (Since 2.8) +# +# @downtime-limit: set maximum tolerated downtime for migration. maximum downtime +# 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 +# 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 +# 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..8b050d0 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-bandwidth": 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 | 26 ++++++++++ include/migration/migration.h | 1 - migration/migration.c | 116 ++++++++++++++++++++++++++++++++---------- qapi-schema.json | 33 ++++++++++-- qmp-commands.hx | 14 +++-- 5 files changed, 156 insertions(+), 34 deletions(-)