Re: [PATCH v1 1/2] migration/xbzrle: replace transferred xbzrle bytes with encoded bytes

[Daniel P . Berrangé]

On Mon, Apr 20, 2020 at 11:06:42AM +0800, Wei Wang wrote:
> Like compressed_size which indicates how many bytes are compressed, we
> need encoded_size to understand how many bytes are encoded with xbzrle
> during migration.
> 
> Replace the old xbzrle_counter.bytes, instead of adding a new counter,
> because we don't find a usage of xbzrle_counter.bytes currently, which
> includes 3 more bytes of the migration transfer protocol header (in
> addition to the encoding header). The encoded_size will further be used
> to calculate the encoding rate.
> 
> Signed-off-by: Yi Sun <address@hidden>
> Signed-off-by: Wei Wang <address@hidden>
> ---
>  migration/migration.c |  2 +-
>  migration/ram.c       | 18 +++++++++---------
>  monitor/hmp-cmds.c    |  4 ++--
>  qapi/migration.json   |  6 +++---
>  4 files changed, 15 insertions(+), 15 deletions(-)


> diff --git a/qapi/migration.json b/qapi/migration.json
> index eca2981d0a..bf195ff6ac 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -62,7 +62,7 @@
>  #
>  # @cache-size: XBZRLE cache size
>  #
> -# @bytes: amount of bytes already transferred to the target VM
> +# @encoded_size: amount of bytes encoded

Woah, this is part of QEMU's public API, so it isn't permissible to just
arbitrarily remove a field with no warning, and replace it with a new
field reporting different data. Adding a new field is allowed, but any
existing field should be deprecated first, if there is a genuine need
to remove it. If it isn't costly though, just leave the existing field
unchanged.

I would also note that the other fields in this struct use a hyphen, not
an underscore.

>  #
>  # @pages: amount of pages transferred to the target VM
>  #
> @@ -75,7 +75,7 @@
>  # Since: 1.2
>  ##
>  { 'struct': 'XBZRLECacheStats',
> -  'data': {'cache-size': 'int', 'bytes': 'int', 'pages': 'int',
> +  'data': {'cache-size': 'int', 'encoded_size': 'int', 'pages': 'int',
>             'cache-miss': 'int', 'cache-miss-rate': 'number',
>             'overflow': 'int' } }
>  
> @@ -333,7 +333,7 @@
>  #          },
>  #          "xbzrle-cache":{
>  #             "cache-size":67108864,
> -#             "bytes":20971520,
> +#             "encoded_size":20971520,
>  #             "pages":2444343,
>  #             "cache-miss":2244,
>  #             "cache-miss-rate":0.123,
> -- 
> 2.20.1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|