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

[Eric Blake]

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?

> +
> +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.

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

signature.asc
Description: OpenPGP digital signature