Re: [Qemu-devel] [PATCH v10 8/9] Add set_cachesize command

[Eric Blake]

On 05/16/2012 05:59 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>

>  
> +
> +void xbzrle_cache_resize(int64_t order)
> +{
> +    cache_resize(XBZRLE.cache, pow(2, order));

'1 << order' is much more efficient than a call to pow().


> +void qmp_migrate_set_cachesize(int64_t value, Error **errp)

> +
> +    /* power of 2 */
> +    if (value != 1 && !is_power_of_2(value)) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> +                  "needs to be power of 2");

We already have QERR_PROPERTY_VALUE_NOT_POWER_OF_2, why aren't you using
that here?

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

log2() is rather expensive, ffs() from <strings.h> is more efficient at
converting a single bit into the appropriate order.


>  ##
> +# @migrate_set_cachesize
> +#
> +# Set XBZRLE cache size
> +#
> +# @value: cache size in bytes
> +#
> +# Returns: nothing on success

Document the error for a non-power-of-2 or for overflow.

Document whether this command is safe for an ongoing migration, or
whether it must be called in advance of a migration.

> +#
> +# Since: 1.1

1.2.


> +static inline bool is_power_of_2(int64_t value)
> +{
> +    return !(value & (value - 1));
> +}

This says '0' is a power of 2, which is not true.  Either fix the logic
to exclude 0, or fix the function name to state that you are really
checking that at most one bit is set.

Also, if value is 0x8000000000000000, you are triggering unspecified
behavior per C99.  Is it worth using uint64_t for defined behavior, or
do you need to take precautions regarding negative values?


> +SQMP
> +migrate_set_cachesize
> +---------------------
> +
> +Set cache size to be used by XBZRLE migration
> +
> +Arguments:
> +
> +- "value": cache size in bytes (json-int)

Would it be any easier to take 'order' (log2 of the size) instead of the
actual cache size?  That is, instead of calling "value":1048576, I would
rather type "value":20.

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

Isn't 512 bytes rather small?  And given my argument about taking order
rather than bytes as being easier to use, don't you really mean 512
megabytes (order 29) rather than 512 bytes (order 9)?

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

signature.asc
Description: OpenPGP digital signature