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

[Orit Wasserman]

On 05/16/2012 07:45 PM, Eric Blake wrote:
> 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().
ok
> 
> 
>> +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?
I will update it.
> 
>> +        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.
ok
> 
> 
>>  ##
>> +# @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.
sure
> 
>> +#
>> +# 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?
The input is int64 so I prefer to keep it this way.
The calling function does the check for 0 , negative numbers and overflow
but I can add those checks here too.

> 
> 
>> +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.
Well the user is considering how much memory is going to be used and I though 
that it
is simpler to use 1G than 30.
But I guess the user is libvirt so it can be changed to order.

> 
>> +
>> +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)?
> 
correct 512M not bytes ...

Orit