[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v14 04/13] Add cache handling functions
Date: Tue, 03 Jul 2012 14:24:49 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120615 Thunderbird/13.0.1

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 <address@hidden>
> Signed-off-by: Petter Svard <address@hidden>
> Signed-off-by: Aidan Shribman <address@hidden>
> Signed-off-by: Orit Wasserman <address@hidden>

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

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

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

> +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   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]