Re: [Qemu-devel] [PATCH v13 04/13] Add cache handling functions

[Orit Wasserman]

On 06/27/2012 07:55 PM, Eric Blake wrote:
> On 06/27/2012 04:34 AM, Orit Wasserman wrote:
>> Add LRU page cache mechanism.
>> The page are accessed by their address.
>>
>> 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>
> 
>> +++ b/cache.c
> 
> cache.c is a rather generic name; should it be page-cache.c instead to
> reflect that it only caches memory pages?
> 
>> @@ -0,0 +1,217 @@
>> +/*
>> + * Page cache for qemu
>> + * The cache is base on a hash on the page address
> 
> s/base on a hash on/based on a hash of/
> 
>> +
>> +Cache *cache_init(int64_t num_pages, unsigned int page_size)
>> +{
> 
>> +
>> +    /* round down to the nearest power of 2 */
>> +    if (!is_power_of_2(num_pages)) {
>> +        num_pages = 1 << ffs(num_pages);
> 
> That's not how you round down.  For example, if I passed in 0x5, you end
> up giving me 1 << ffs(5) == 1 << 1 == 2, but the correct answer should be 4.
> 
> http://graphics.stanford.edu/~seander/bithacks.html#IntegerLogObvious
> and http://aggregate.org/MAGIC/#Leading%20Zero%20Count give some hints
> about what you really want to be doing; offhand, I came up with this (it
> works because you already rejected negative num_pages):
> 
> if (!is_power_of_2(num_pages)) {
>     num_pages |= num_pages >> 1;
>     num_pages |= num_pages >> 2;
>     num_pages |= num_pages >> 4;
>     num_pages |= num_pages >> 8;
>     num_pages |= num_pages >> 16;
>     num_pages |= num_pages >> 32;
>     num_pages -= num_pages / 2;
> }
> 
>> +    cache->page_cache = g_malloc((cache->max_num_items) *
>> +                                 sizeof(CacheItem));
>> +    if (!cache->page_cache) {
>> +        DPRINTF("could not allocate cache\n");
>> +        g_free(cache);
>> +        return NULL;
>> +    }
>> +
>> +    for (i = 0; i < cache->max_num_items; i++) {
>> +        cache->page_cache[i].it_data = NULL;
>> +        cache->page_cache[i].it_age = 0;
> 
> Does g_malloc leave memory uninitialized, or is it like calloc where it
> zeros out the memory making these two assignments redundant?

g_malloc doesn't initialize memory, g_malloc0 does.

> 
>> +
>> +int cache_resize(Cache *cache, int64_t new_num_pages)
>> +{
>> +    Cache *new_cache;
>> +    int i;
>> +
>> +    CacheItem *old_it, *new_it;
>> +
>> +    g_assert(cache);
>> +
>> +    /* same size */
>> +    if (new_num_pages == cache->max_num_items) {
>> +        return 0;
>> +    }
>> +
>> +    /* cache was not inited */
>> +    if (cache->page_cache == NULL) {
>> +        return -1;
>> +    }
> 
> Shouldn't these two conditions be swapped?  Non-init failure should take
> precedence over no size change.
> 
> If new_num_pages is not a power of 2, but rounds down to the same as the
> existing size, the size won't compare equal and you end up wasting a lot
> of effort moving pages between the resulting identically sized caches.
> I'd factor out your rounding-down code, and call it from multiple places
> prior to checking for size equality.
> 
>> +    /* move all data from old cache */
>> +    for (i = 0; i < cache->max_num_items; i++) {
>> +        old_it = &cache->page_cache[i];
>> +        if (old_it->it_addr != -1) {
>> +            /* check for collision , if there  is keep the first value */
> 
> s/collision , if there  is/collision, if there is,/
> 
>> +++ b/include/qemu/cache.h
>> @@ -0,0 +1,79 @@
>> +/*
>> + * Page cache for qemu
>> + * The cache is base on a hash on the page address
> 
> Same comments as for cache.c.
>