From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34738) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhkwb-00079C-WD for qemu-devel@nongnu.org; Wed, 07 Sep 2016 18:03:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bhkwW-0004Tj-QP for qemu-devel@nongnu.org; Wed, 07 Sep 2016 18:03:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59650) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhkwW-0004Td-I8 for qemu-devel@nongnu.org; Wed, 07 Sep 2016 18:03:40 -0400 References: <1473169187-22750-1-git-send-email-ashijeetacharya@gmail.com> From: Eric Blake Message-ID: <01e7066e-b807-95a6-6a5e-3435651a949e@redhat.com> Date: Wed, 7 Sep 2016 17:03:37 -0500 MIME-Version: 1.0 In-Reply-To: <1473169187-22750-1-git-send-email-ashijeetacharya@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="NWFwJE7XhojLHbpJoUkJs8iheoXiw0FSs" Subject: Re: [Qemu-devel] [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp 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) --NWFwJE7XhojLHbpJoUkJs8iheoXiw0FSs 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: <01e7066e-b807-95a6-6a5e-3435651a949e@redhat.com> Subject: Re: [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp References: <1473169187-22750-1-git-send-email-ashijeetacharya@gmail.com> In-Reply-To: <1473169187-22750-1-git-send-email-ashijeetacharya@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > Signed-off-by: Ashijeet Acharya > --- > +++ b/migration/migration.c > @@ -44,6 +44,9 @@ > #define BUFFER_DELAY 100 > #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY) > =20 > +/* Time in nanoseconds we are allowed to stop the source for to send l= ast 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 =3D g_strdup(tls_hostname); > } > + if (has_max_bandwidth) { > + s->parameters.max_bandwidth =3D 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 (has_downtime_limit) { > + downtime_limit *=3D 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 =3D false; > + bool has_compress_threads =3D false; > + bool has_decompress_threads =3D false; > + bool has_cpu_throttle_initial =3D false; > + bool has_cpu_throttle_increment =3D false; > + bool has_tls_creds =3D false; > + bool has_tls_hostname =3D false; > + bool has_max_bandwidth =3D true; > + bool has_downtime_limit =3D false; > + const char *valuestr =3D NULL; > + long valueint =3D 0; > + Error *err =3D 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 =3D false; > + bool has_compress_threads =3D false; > + bool has_decompress_threads =3D false; > + bool has_cpu_throttle_initial =3D false; > + bool has_cpu_throttle_increment =3D false; > + bool has_tls_creds =3D false; > + bool has_tls_hostname =3D false; > + bool has_max_bandwidth =3D false; > + bool has_downtime_limit =3D true; Again, gross. > + const char *valuestr =3D NULL; > + long valueint =3D 0; > + int64_t valuebw =3D 0; > + valuedowntime *=3D 1000; /* Convert to milliseconds */ > + valuedowntime =3D MAX(0, MIN(INT64_MAX, valuedowntime)); > + valuedowntime =3D (int64_t)valuedowntime; Useless statement; the cast would already implicitly happen when passing it to the function call. > + Error *err =3D 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); > } > =20 > bool migrate_postcopy_ram(void) > @@ -1858,7 +1922,7 @@ void migrate_fd_connect(MigrationState *s) > =20 > 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); > =20 > /* 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. maxim= um 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-paramete= rs' I don't know if we have a strong preference for US vs. UK spelling in documentation. > @@ -3803,7 +3805,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?", 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 =3D qmp_marshal_migrate_set_parameters, > }, > SQMP > @@ -3820,6 +3822,10 @@ Query current migration parameters > throttled (json-int) > - "cpu-throttle-increment" : throttle increasing percentage f= or > 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/,, --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --NWFwJE7XhojLHbpJoUkJs8iheoXiw0FSs 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/ iQEcBAEBCAAGBQJX0I66AAoJEKeha0olJ0NqxC4IAKWhKuagzvA9MgLVIhotB4Xy RERE5VnggJ7Y6pUC6DZTa6ICxSX1pDwkYdPMLXH8u/l3qtZosBZ3OEI6IpRRJXWm fGL1T+JTVIyaLG1LAQ1F3QE7NgtrzjxGd3J+oUWdil4NgCBVr8uuHiJwYiaAwknY WWTmU1aFu9eFMhmKiayAlR68028ois6/3tN0gOk0uC6kkGa4xllFsGdz6asJLJCb O76ZqvFDOuliaJHNxcJmkJWfxhAnx2uU4V7jqi7/W/sY65hcCXuW0zMGEQLffnnx EMgw/baP3X/OiZ8s2keX6KacKEwC63s0uUG4rLW58bol/G1gLD/xacJF0oaDpXM= =FdKF -----END PGP SIGNATURE----- --NWFwJE7XhojLHbpJoUkJs8iheoXiw0FSs--