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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3122 bytes --]

On 07/03/2012 07:52 AM, Orit Wasserman wrote:
> Add LRU page cache mechanism.
> The page are accessed by their address.
> 
> Signed-off-by: Benoit Hudzia <benoit.hudzia@sap.com>
> Signed-off-by: Petter Svard <petters@cs.umu.se>
> Signed-off-by: Aidan Shribman <aidan.shribman@sap.com>
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>

> +PageCache *cache_init(int64_t num_pages, unsigned int page_size)
> +{
> +    int i;

Type mismatch.  Either 'i' needs to be int64_t, or you don't need
num_pages to be 64-bit.  Although I think it will be unlikely to
encounter someone with a desire to use 2**32 pages of 4096 bytes each as
their cache, I can't rule it out, so I think 'i' needs to be bigger
rather than changing your API to be smaller.

> +
> +    PageCache *cache = g_malloc(sizeof(PageCache));

Personally, I like the style g_malloc(sizeof(*cache)), as it is immune
to type changes on cache.  If you later change the type of 'cache', your
style has to make two edits to this line, while my style only makes one
edit.

> +
> +    if (num_pages <= 0) {
> +        DPRINTF("invalid number pages\n");

s/number pages/number of pages/

> +    cache->page_cache = g_malloc((cache->max_num_items) *
> +                                 sizeof(CacheItem));

Here my style is even more useful.  With your style, I have to search
elsewhere in the code to validate that cache->page_cache really does
have the type CacheItem; but my style means I can see the result at once
without having to care about typing:

cache->page_cache = g_malloc(cache->max_num_items *
                             sizeof(*cache->page_cache));

> +void cache_fini(PageCache *cache)
> +{
> +    int i;

Type mismatch again; max_num_items can be larger than INT_MAX, so you
need a bigger type for 'i'.

> +
> +    g_assert(cache);
> +    g_assert(cache->page_cache);
> +
> +    for (i = 0; i < cache->max_num_items; i++) {
> +        g_free(cache->page_cache[i].it_data);
> +        cache->page_cache[i].it_data = 0;

Wasted assignment, since you are just about to free cache->page_cache
anyways.

> +uint8_t *get_cached_data(const PageCache *cache, uint64_t addr)
> +{
> +    return cache_get_by_addr(cache, addr)->it_data;
> +}
> +
> +void cache_insert(PageCache *cache, unsigned long addr, uint8_t *pdata)

Why are you using 'uint64_t addr' in some places, and 'unsigned long
addr' in others?

> +    /* 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/ ,/,/ (no space before comma in English)

Shouldn't we instead prefer to keep the page with larger age, regardless
of whether we found it first or second?  That is, a cache is more likely
to be useful on most-recently-used pages, rather than on which address
happened to map to the collision first.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]