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

[Orit Wasserman]

On 06/27/2012 11:04 PM, Eric Blake wrote:
> 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.
> 
I'm sorry , I'm somehow lost all the fixes I did to this patch.
I will be more careful next time.
Thanks a lot for reviewing it.
 
>> +        .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.
> 
Correct I should store the rounded size not user size.I will change
resize to return the new size.

I'm not sure it is possible to know what is the available host memory from QEMU,
it can also reduce with time.
We don't allocate a large size of memory here (only meta data) , the allocation 
is done
when caching a new page. The cache doesn't have to be full.
So checking available memory here doesn't guaranty that we will actually have 
memory later.
 
I agree aborting here is not the best behavior but it can happen with regular 
migration too
sadly. In regular migration the incoming QEMU can abort on out of memory when 
starting the guest.
 
I can switch back to malloc and check the return value, would that be ok ?

Orit
> 
>> +# @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.
>