From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34232) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bk5CM-0002Ou-3a for qemu-devel@nongnu.org; Wed, 14 Sep 2016 04:05:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bk5CJ-0006Xw-CF for qemu-devel@nongnu.org; Wed, 14 Sep 2016 04:05:37 -0400 Received: from mail-oi0-x244.google.com ([2607:f8b0:4003:c06::244]:35682) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bk5CJ-0006Pq-4l for qemu-devel@nongnu.org; Wed, 14 Sep 2016 04:05:35 -0400 Received: by mail-oi0-x244.google.com with SMTP id 2so658017oif.2 for ; Wed, 14 Sep 2016 01:05:13 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <27b83d5e-c371-d67f-7abb-6fcce6e01f79@redhat.com> References: <1473447778-29339-1-git-send-email-ashijeetacharya@gmail.com> <27b83d5e-c371-d67f-7abb-6fcce6e01f79@redhat.com> From: Ashijeet Acharya Date: Wed, 14 Sep 2016 13:35:12 +0530 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v5] migrate: Move max-bandwidth and downtime-limit to migrate_set_parameter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: lcapitulino@redhat.com, quintela@redhat.com, amit.shah@redhat.com, armbru@redhat.com, "dgilbert@redhat.com" , Paolo Bonzini , "Daniel P. Berrange" , QEMU Developers On Wed, Sep 14, 2016 at 12:52 AM, Eric Blake wrote: > On 09/09/2016 02:02 PM, Ashijeet Acharya wrote: >> Mark old-commands for speed and downtime as deprecated. > > Maybe s/old-commands for speed and downtime/the old commands > 'migrate_set_speed' and 'migrate_set_downtime'/ > >> 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. > > This part is fine. > >> NOTE: This patch is solely based on Eric's new boxed parameters from QAPI >> patch series. >> > > This paragraph is useful to reviewers, but won't make sense in the long > run (as my patch series will presumably already be in git first, since > yours does indeed depend on mine), so it can go better... > Okay, I will improve the commit message. >> Signed-off-by: Ashijeet Acharya >> --- > > ...here, along with your changelog. > >> @@ -1295,6 +1307,20 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) >> p.has_tls_hostname = true; >> p.tls_hostname = (char *) valuestr; >> break; >> + case MIGRATION_PARAMETER_MAX_BANDWIDTH: >> + p.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; > > Indentation is off for the goto. > >> >> +++ b/migration/migration.c >> @@ -44,6 +44,10 @@ >> #define BUFFER_DELAY 100 >> #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY) >> >> +/* Time in nanoseconds we are allowed to stop the source, >> + * for sending the last part */ >> +#define DEFAULT_MIGRATE_SET_DOWNTIME 300000000 >> + > > I would have expected that the 'static uint64_t max_downtime' > declaration would completely disappear, now that you are moving all the > data to live inside the rest of the migration parameters. More on that > below [1] > >> @@ -804,6 +813,20 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp) >> "cpu_throttle_increment", >> "an integer in the range of 1 to 99"); >> } >> + if (params->has_max_bandwidth && >> + (params->max_bandwidth < 0 || params->max_bandwidth > SIZE_MAX)) { >> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, >> + "max_bandwidth", >> + "an integer in the range of 0 to SIZE_MAX"); > > Might be nicer to give a numeric value instead of assuming the reader > knows what value SIZE_MAX has, but not the end of the world. I used SIZE_MAX because substituting it with its value looked a bit messy. Should I use its actual value? > >> + return; >> + } >> + if (params->has_downtime_limit && >> + (params->downtime_limit < 0 || params->downtime_limit > 2000000)) { > > You are setting a new maximum limit smaller than what the code > previously allowed; while I think that 2000 seconds as a maximum > downtime is indeed more generous than anyone will ever use in real life, > you should at least document in the commit message that your new smaller > upper limit is intentional. Yeah, I had a discussion with Dave about this one on the IRC. Although 2000 seconds is a sufficiently large value for downtime as we practically only use values in range of milliseconds but I will include it in the commit message. > >> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, >> + "downtime_limit", >> + "an integer in the range of 0 to 2000000 milliseconds"); >> + return; >> + } >> >> if (params->has_compress_level) { >> s->parameters.compress_level = params->compress_level; >> @@ -828,6 +851,20 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp) >> g_free(s->parameters.tls_hostname); >> s->parameters.tls_hostname = g_strdup(params->tls_hostname); >> } >> + if (params->has_max_bandwidth) { >> + s->parameters.max_bandwidth = params->max_bandwidth; >> + if (s->to_dst_file) { >> + qemu_file_set_rate_limit(s->to_dst_file, >> + s->parameters.max_bandwidth / XFER_LIMIT_RATIO); >> + } >> + } >> + if (params->has_downtime_limit) { >> + params->downtime_limit *= 1000000; /* convert to nanoseconds */ > > I'd update the comment to include an additional phrase: "safe from > overflow, since we capped the upper limit above" > Okay. >> + >> + s = migrate_get_current(); >> + max_downtime = params->downtime_limit; >> + s->parameters.downtime_limit = max_downtime; > > [1] Uggh. s->parameters shares the same type as params > (MigrationParameters), but we are using it to hold a limit in > nanoseconds in one instance and a limit in milliseconds in the other. > Which means we have to scale any time we assign from one field to the > other, and cannot use simple copies between the two locations. What's > more, you are then STILL using the file-level variable 'max_downtime' > (in nanoseconds) rather than the new s->parameters.downtime_limit. If > s->parameters is good enough, then we don't need the file-scope > 'max_downtime'. I understand, but I left 'max_downtime' as it is so that I don't have to substitute it everywhere in the file and add unnecessary additions and deletions in the patch but I will remove it now as you recommend. > > I would MUCH rather see us consistently use the SAME scale (milliseconds > is fine), where a single point of documentation in the .json file > correctly describes ALL uses of the downtime_limit member of > MigrationParameters. Even if that means splitting this into a > multi-patch series (one to convert all existing uses of max_downtime to > a scale of milliseconds, the second to then convert from max_downtime > over to the new s->parameters.downtime_limit). > >> >> @@ -1159,30 +1196,27 @@ int64_t qmp_query_migrate_cache_size(Error **errp) >> return migrate_xbzrle_cache_size(); >> } >> >> -void qmp_migrate_set_speed(int64_t value, Error **errp) >> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp) > > Why did we need to rename value to valuebw? > >> { >> - MigrationState *s; >> - >> - if (value < 0) { >> - value = 0; >> - } >> - if (value > SIZE_MAX) { >> - value = SIZE_MAX; >> - } >> + MigrationParameters p = { >> + .has_max_bandwidth = true, >> + .max_bandwidth = valuebw, >> + }; >> >> - 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); >> - } >> + qmp_migrate_set_parameters(&p, errp); >> } >> >> -void qmp_migrate_set_downtime(double value, Error **errp) >> +void qmp_migrate_set_downtime(double valuedowntime, Error **errp) > > Again, why rename value? > >> +++ b/qapi-schema.json > >> @@ -1782,6 +1797,8 @@ >> # >> # Returns: nothing on success >> # >> +# Notes: This command is deprecated in favor of 'migrate-set-parameters' >> +# >> # Since: 0.14.0 >> ## >> { 'command': 'migrate_set_downtime', 'data': {'value': 'number'} } >> @@ -1795,7 +1812,7 @@ >> # >> # Returns: nothing on success >> # >> -# Notes: A value lesser than zero will be automatically round up to zero. >> +# Notes: This command is deprecated in favor of 'migrate-set-parameters' > > Do we need to drop the old note about behavior on negative inputs? On > the one hand, the new interface doesn't suffer from that interface wart, > so the old interface is the only place where the note is even useful; on > the other hand, since we don't want people to use the old interface, not > documenting the wart seems fine to me. Dropping the old note seemed reasonable to me as the old-commands are now only a wrapper around the new ones. So the bounds check error applies on both. > >> @@ -3813,7 +3815,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?", > > Trivial conflict with Marc-Andre's series on qapi-next that removes > .args_type altogether. > > Getting closer, but I still think re-scaling max_downtime should be done > separately from moving it into MigrationParameters, and that we > absolutely do not want to use two different scales for various > MigrationParameters->downtime_limit uses. Okay, I will send the updated patch soon. Ashijeet. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >