Re: [Qemu-devel] [v2 2/2] migration: Implement multiple compression threads

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5370 bytes --]

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.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

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 <liang.z.li@intel.com>
> ---

> +++ b/qapi-schema.json
> @@ -491,13 +491,17 @@
>  #          to enable the capability on the source VM. The feature is disabled 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 sending
> +#          compressed pages. The feature is disabled by default. (since 2.3)
> +#
>  # @auto-converge: If enabled, QEMU will automatically throttle down the 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'] }
>  

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 }
>  
>  EQMP
> +{
> +        .name       = "migrate-set-compress-level",
> +        .args_type  = "value:i",
> +        .mhandler.cmd_new = qmp_marshal_input_migrate_set_compress_level,
> +    },
> +
> +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 level 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 compress 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       = "query-migrate-compress-threads",
> +        .args_type  = "",
> +        .mhandler.cmd_new = 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)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]