From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43058) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjtHZ-0006DT-RW for qemu-devel@nongnu.org; Tue, 13 Sep 2016 15:22:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjtHY-0006Ct-4U for qemu-devel@nongnu.org; Tue, 13 Sep 2016 15:22:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43976) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjtHX-0006CZ-Rc for qemu-devel@nongnu.org; Tue, 13 Sep 2016 15:22:12 -0400 References: <1473447778-29339-1-git-send-email-ashijeetacharya@gmail.com> From: Eric Blake Message-ID: <27b83d5e-c371-d67f-7abb-6fcce6e01f79@redhat.com> Date: Tue, 13 Sep 2016 14:22:09 -0500 MIME-Version: 1.0 In-Reply-To: <1473447778-29339-1-git-send-email-ashijeetacharya@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="aPBgEtCVxuOghKsLpuSbdT0b6weOV7oi7" 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: Ashijeet Acharya , lcapitulino@redhat.com Cc: quintela@redhat.com, amit.shah@redhat.com, armbru@redhat.com, dgilbert@redhat.com, pbonzini@redhat.com, berrange@redhat.com, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --aPBgEtCVxuOghKsLpuSbdT0b6weOV7oi7 From: Eric Blake To: Ashijeet Acharya , lcapitulino@redhat.com Cc: quintela@redhat.com, amit.shah@redhat.com, armbru@redhat.com, dgilbert@redhat.com, pbonzini@redhat.com, berrange@redhat.com, qemu-devel@nongnu.org Message-ID: <27b83d5e-c371-d67f-7abb-6fcce6e01f79@redhat.com> Subject: Re: [PATCH v5] migrate: Move max-bandwidth and downtime-limit to migrate_set_parameter References: <1473447778-29339-1-git-send-email-ashijeetacharya@gmail.com> In-Reply-To: <1473447778-29339-1-git-send-email-ashijeetacharya@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 QA= PI > patch series. >=20 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... > Signed-off-by: Ashijeet Acharya > --- =2E..here, along with your changelog. > @@ -1295,6 +1307,20 @@ void hmp_migrate_set_parameter(Monitor *mon, con= st QDict *qdict) > p.has_tls_hostname =3D true; > p.tls_hostname =3D (char *) valuestr; > break; > + case MIGRATION_PARAMETER_MAX_BANDWIDTH: > + p.has_max_bandwidth =3D true; > + valuebw =3D qemu_strtosz(valuestr, &endp); > + if (valuebw < 0 || (size_t)valuebw !=3D valuebw > + || *endp !=3D '\0') { > + error_setg(&err, "Invalid size %s", valuestr); > + goto cleanup; Indentation is off for the goto. >=20 > +++ b/migration/migration.c > @@ -44,6 +44,10 @@ > #define BUFFER_DELAY 100 > #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY) > =20 > +/* 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(MigrationParameter= s *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. > + return; > + } > + if (params->has_downtime_limit && > + (params->downtime_limit < 0 || params->downtime_limit > 200000= 0)) { 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. > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, > + "downtime_limit", > + "an integer in the range of 0 to 2000000 millisecon= ds"); > + return; > + } > =20 > if (params->has_compress_level) { > s->parameters.compress_level =3D params->compress_level; > @@ -828,6 +851,20 @@ void qmp_migrate_set_parameters(MigrationParameter= s *params, Error **errp) > g_free(s->parameters.tls_hostname); > s->parameters.tls_hostname =3D g_strdup(params->tls_hostname);= > } > + if (params->has_max_bandwidth) { > + s->parameters.max_bandwidth =3D params->max_bandwidth; > + if (s->to_dst_file) { > + qemu_file_set_rate_limit(s->to_dst_file, > + s->parameters.max_bandwidth / XFER_LIM= IT_RATIO); > + } > + } > + if (params->has_downtime_limit) { > + params->downtime_limit *=3D 1000000; /* convert to nanoseconds= */ I'd update the comment to include an additional phrase: "safe from overflow, since we capped the upper limit above" > + > + s =3D migrate_get_current(); > + max_downtime =3D params->downtime_limit; > + s->parameters.downtime_limit =3D 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 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). > =20 > @@ -1159,30 +1196,27 @@ int64_t qmp_query_migrate_cache_size(Error **er= rp) > return migrate_xbzrle_cache_size(); > } > =20 > -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 =3D 0; > - } > - if (value > SIZE_MAX) { > - value =3D SIZE_MAX; > - } > + MigrationParameters p =3D { > + .has_max_bandwidth =3D true, > + .max_bandwidth =3D valuebw, > + }; > =20 > - s =3D migrate_get_current(); > - s->bandwidth_limit =3D 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); > } > =20 > -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-parameter= s' > +# > # 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 ze= ro. > +# Notes: This command is deprecated in favor of 'migrate-set-parameter= s' 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. > @@ -3813,7 +3815,7 @@ EQMP > { > .name =3D "migrate-set-parameters", > .args_type =3D > - "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?,dow= ntime-limit:i?", Trivial conflict with Marc-Andre's series on qapi-next that removes =2Eargs_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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --aPBgEtCVxuOghKsLpuSbdT0b6weOV7oi7 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJX2FHhAAoJEKeha0olJ0Nqj9MH/2anIsjlypGJh4f+g/pwBUbl qD4lgJ9/KF88h+2sAXHYCj5ZY5aFT/q9xVpg1WHVes11V2tFqOrpo74yMh+11f7v /xh2+UU9U3YWeyPI6sDWrCbjis1qZQcv0D32+BvB5iiOnRTKmTnmIyURNyMJwejS AsVNtC+IkJ+yjnwAlOedRn9PyMrYAz9WGID6Rta2uNCzJotbY4L06PilpFFI0JHO I3MNPkG2BuS6DmxTLjvayqjOoekxL+i/Qfxf7kD2n0JlriWsE2cMXPA2K37LQL1s AGbeAVFMzAMOV4Jj9J9Jehl0Z6SI6ZheA2jEUit83v/tXi9Iw5LbdhwJI5/Yj30= =Ly7h -----END PGP SIGNATURE----- --aPBgEtCVxuOghKsLpuSbdT0b6weOV7oi7--