Re: [Qemu-devel] [PATCH v13 12/13] Add set_cachesize command

[Eric Blake]

On 06/27/2012 04:34 AM, Orit Wasserman wrote:
> Change XBZRLE cache size in bytes (the size should be a power of 2).
> If XBZRLE cache size is too small there will be many cache miss.
> 
> Signed-off-by: Benoit Hudzia <address@hidden>
> Signed-off-by: Petter Svard <address@hidden>
> Signed-off-by: Aidan Shribman <address@hidden>
> Signed-off-by: Orit Wasserman <address@hidden>
> ---
>  arch_init.c      |    9 +++++++++
>  hmp-commands.hx  |   18 ++++++++++++++++++
>  hmp.c            |   13 +++++++++++++
>  hmp.h            |    1 +
>  migration.c      |   23 ++++++++++++++++++++++-
>  migration.h      |    2 ++
>  qapi-schema.json |   16 ++++++++++++++++
>  qmp-commands.hx  |   23 +++++++++++++++++++++++
>  8 files changed, 104 insertions(+), 1 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index bfc9f27..ae7c8b1 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -24,6 +24,7 @@
>  #include <stdint.h>
>  #include <stdarg.h>
>  #include <stdlib.h>
> +#include <math.h>

Why?

Furthermore, I think I asked the same question about v12; it's rather
depressing to review a patch when not all the review comments from the
previous revision were addressed.

> +        .help       = "set cache size (in bytes) for XBZRLE migrations,"
> +                   "the cache size will be round down to the nearest power 
> of 2.\n"

s/round/rounded/


> +void qmp_migrate_set_cachesize(int64_t value, Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +
> +    /* Check for truncation */
> +    if (value != (size_t)value) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> +                  "exceeding address space");
> +        return;
> +    }
> +
> +    /* no change */
> +    if (value == s->xbzrle_cache_size) {
> +        return;

Another place where factoring the rounding-down will make it more
efficient.  On the other hand, why do you even worry about it, since...

> +    }
> +
> +    s->xbzrle_cache_size = value;
> +    xbzrle_cache_resize(value);

...xbzrle_cache_resize should be a relatively fast no-op if the
rounded-down size is equal?  Also, aren't you better off storing the
rounded value and not the user's value (in which case, should
xbzrle_cache_resize return a value)?  For that matter, what if the user
requests a resize to a larger value that exceeds available host memory?
 Normally, out-of-memory makes qemu exit, but in this particular case,
we are better off leaving qemu running but just failing the monitor command.


> +# @migrate_set_cachesize
> +#
> +# Set XBZRLE cache size
> +#
> +# @value: cache size in bytes
> +#
> +# The size will be round down to the nearest power of 2.

s/round/rounded/

> +Example:
> +
> +-> { "execute": "migrate_set_cachesize", "arguments": { "value": 512M } }

512M is not a json-int.  I really don't think you even saw my v12
comments on this patch.

-- 
Eric Blake   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature