From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60453) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XmMdG-0006ur-K2 for qemu-devel@nongnu.org; Thu, 06 Nov 2014 07:57:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XmMdA-0006QB-3I for qemu-devel@nongnu.org; Thu, 06 Nov 2014 07:57:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51338) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XmMd9-0006Pz-SS for qemu-devel@nongnu.org; Thu, 06 Nov 2014 07:57:40 -0500 Message-ID: <545B703E.7030907@redhat.com> Date: Thu, 06 Nov 2014 13:57:34 +0100 From: Eric Blake MIME-Version: 1.0 References: <1415272128-8273-1-git-send-email-liang.z.li@intel.com> <1415272128-8273-3-git-send-email-liang.z.li@intel.com> In-Reply-To: <1415272128-8273-3-git-send-email-liang.z.li@intel.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="AMppmjud7GLk3uuhbDEEaafk4HuvKNhUG" Subject: Re: [Qemu-devel] [v2 2/2] migration: Implement multiple compression threads List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Li Liang , qemu-devel@nongnu.org Cc: yang.z.zhang@intel.com, armbru@redhat.com, lcapitulino@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --AMppmjud7GLk3uuhbDEEaafk4HuvKNhUG Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/06/2014 12:08 PM, Li Liang wrote: > Instead of sending the guest memory directly, this solution compress > the ram page before sending, after receiving, the data will be > decompressed. > This feature can help to reduce the data transferred about > 60%, this is very useful when the network bandwidth is limited, > and the migration time can also be reduced about 70%. The > feature is off by default, following the document > docs/multiple-compression-threads.txt for information to use it. >=20 > Reviewed-by: Eric Blake Please DON'T add this line unless the author spelled it out (or if they mentioned that it would be okay if you fix minor issues). I intentionally omitted a reviewed-by on v1: https://lists.gnu.org/archive/html/qemu-devel/2014-11/msg00672.html because I was not happy with the patch as it was presented and did not think the work to fix it was trivial. Furthermore, my review of v1 was just over the interface, and not the entire patch; there are very likely still bugs lurking in the .c files. Once again, I'm going to limit my review of v2 to the interface (at least in this email): > Signed-off-by: Li Liang > --- > +++ b/qapi-schema.json > @@ -491,13 +491,17 @@ > # to enable the capability on the source VM. The feature is d= isabled by > # default. (since 1.6) > # > +# @compress: Using the multiple compression threads to accelerate live= migration. > +# This feature can help to reduce the migration traffic, by s= ending > +# compressed pages. The feature is disabled by default. (sinc= e 2.3) > +# > # @auto-converge: If enabled, QEMU will automatically throttle down th= e guest > # to speed up convergence of RAM migration. (since 1.6) > # > # Since: 1.2 > ## > { 'enum': 'MigrationCapability', > - 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks'] }= > + 'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks', '= compress'] } > =20 I'll repeat what I said on v1 (but this time, with some links to back it up :) We really need to avoid a proliferation of new commands, two per tunable does not scale well. I think now is the time to implement my earlier suggestion at making MigrationCapability become THE resource for tunables= : https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02274.html > +++ b/qmp-commands.hx > @@ -705,7 +705,138 @@ Example: > <- { "return": 67108864 } > =20 > EQMP > +{ > + .name =3D "migrate-set-compress-level", > + .args_type =3D "value:i", > + .mhandler.cmd_new =3D qmp_marshal_input_migrate_set_compress_l= evel, > + }, > + > +SQMP > +migrate-set-compress-level > +---------------------- Convention in this file is to have the --- line extended out to the length of the text it is tied to (you are missing four bytes, corresponding to the tail "evel") > + > +Set compress level to be used by compress migration, the compress leve= l is an integer s/compress level/the compression level/ (twice) > +between 0 and 9 s/9/9, where 9 means try harder for smaller compression at the expense of more CPU time/ > + > +Arguments: > + > +- "value": compress level (json-int) > + > +Example: > + > +-> { "execute": "migrate-set-compress-level", "arguments": { "value": = 536870912 } } Umm, 536870912 is not an integer between 0 and 9. > +SQMP > +query-migrate-compress-level > +------------------------ --- length > + > +Show compress level to be used by compress migration > + > +returns a json-object with the following information: > +- "size" : json-int > + > +Example: > + > +-> { "execute": "query-migrate-compress-level" } > +<- { "return": 67108864 } Ewww. Please no new interfaces that return raw ints. Rather, return a dictionary with one key/value pair holding the int. Raw ints are not as extensible as dictionaries. Also, make the example realistic - 67108864 is not a valid compression level. { "return": { "level": 9 } } > +migrate-set-compress-threads > +---------------------- --- length > + > +Set compress thread count to be used by compress migration, the compre= ss thread count is an integer > +between 1 and 255 > + > +Arguments: > + > +- "value": compress threads (json-int) > + > +Example: > + > +-> { "execute": "migrate-set-compress-threads", "arguments": { "value"= : 536870912 } } Value out of range 1-255 > +<- { "return": {} } > + > +EQMP > + { > + .name =3D "query-migrate-compress-threads", > + .args_type =3D "", > + .mhandler.cmd_new =3D qmp_marshal_input_query_migrate_compress= _threads, > + }, > + > +SQMP > +query-migrate-compress-threads > +------------------------ --- length > + > +Show compress thread count to be used by compress migration > + > +returns a json-object with the following information: > +- "size" : json-int > + > +Example: > + > +-> { "execute": "query-migrate-compress-threads" } > +<- { "return": 67108864 } out of range, raw int return and so on in the rest of the patch (I'll quit calling it out, especially if we switch over to my enhanced set-capabilities proposal) --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --AMppmjud7GLk3uuhbDEEaafk4HuvKNhUG Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg iQEcBAEBCAAGBQJUW3A+AAoJEKeha0olJ0NqxiYH/jVuuWMtVdzL5elIxAGnR85w KDKEFY6Jvx8h1N8qoLvBi6vkTL+chdD+UE2oONI/faeUicLqfHekAOYtFphl2UXt XjU9ELYSSwy1+SUGMPKq3F8+oNjq2Rm8HWlfC9zyCR1VhN1F0v02hh2+nnPN9VaM HmNuEIHnQwbfIEhx4YgK8AxSDQ6vBX3jyyPmL3/6OXCbpVo3QGoiRHKgNqOfWryL +kZZy7h8D8MRxoTCotdxdOOAMsrDPgTZcrgSoREs/QYp8OkJjAEtX34iGhUme0yk ooxsPEjH6+6HDVT7FISA79HbyNyR5BMR4dPBrrxojmjNelMPSGdoRcvIaVl9DMk= =/zXx -----END PGP SIGNATURE----- --AMppmjud7GLk3uuhbDEEaafk4HuvKNhUG--