Re: [Qemu-devel] [PATCH 06/10] migration: Make sure that we pass the right cache size

[Juan Quintela <quintela@redhat.com>]

Peter Xu <peterx@redhat.com> wrote:
> On Tue, Oct 10, 2017 at 08:15:38PM +0200, Juan Quintela wrote:
>> Instead of passing silently round down the number of pages, make it an
>> error that the cache size is not a multiple of 2.
>
> s/multiple/power/?

Thanks.

> Would this patch break existing users?

I have a problem here. Current code:
- "silently" truncate the value that we pass
- "silently" uses a different value that the one that it shows as used
- "shows" a value that is different of what has been set to

So, I have to break one of them:
- only allow valid values (and then we output the same value that user
  input)
- store "setup" value, and use something different "internaly"

No clear winner.  This way we are at least consistent with everything
else.  Documentation already required a power of 2 value.  And we give a
good error message.

Later, Juan.


>
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/page_cache.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>> 
>> diff --git a/migration/page_cache.c b/migration/page_cache.c
>> index 9a9d13d6a2..96268c3aea 100644
>> --- a/migration/page_cache.c
>> +++ b/migration/page_cache.c
>> @@ -58,6 +58,13 @@ PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
>>          return NULL;
>>      }
>>  
>> +    /* round down to the nearest power of 2 */
>> +    if (!is_power_of_2(num_pages)) {
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> +                   "is not a power of two number of pages");
>> +        return NULL;
>> +    }
>> +
>>      /* We prefer not to abort if there is no memory */
>>      cache = g_try_malloc(sizeof(*cache));
>>      if (!cache) {
>> @@ -65,11 +72,6 @@ PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
>>                     "Failed to allocate cache");
>>          return NULL;
>>      }
>> -    /* round down to the nearest power of 2 */
>> -    if (!is_power_of_2(num_pages)) {
>> -        num_pages = pow2floor(num_pages);
>> -        DPRINTF("rounding down to %" PRId64 "\n", num_pages);
>> -    }
>>      cache->page_size = page_size;
>>      cache->num_items = 0;
>>      cache->max_num_items = num_pages;
>> -- 
>> 2.13.6
>>