Re: [Qemu-devel] [PATCH v9 01/10] Add cache handling functions

[Juan Quintela]

Orit Wasserman <address@hidden> wrote:
> Add LRU page cache mechanism.
> The page are accessed by their address.
>
> +
> +typedef struct CacheItem {
> +    ram_addr_t it_addr;
> +    unsigned long it_age;
> +    uint8_t *it_data;
> +} CacheItem;
> +
> +typedef struct CacheBucket {
> +    CacheItem bkt_item[CACHE_N_WAY];
> +} CacheBucket;
> +
> +static CacheBucket *page_cache;
> +static int64_t cache_num_buckets;
> +static uint64_t cache_max_item_age;
> +static int64_t cache_num_items;

I will put this 3 variables inside CacheBucket, just to make clear that
they are related.

> +
> +static void cache_init(int64_t num_buckets);
> +static void cache_fini(void);
> +static int cache_is_cached(ram_addr_t addr);
> +static int cache_get_oldest(CacheBucket *buck);
> +static int cache_get_newest(CacheBucket *buck, ram_addr_t addr);
> +static void cache_insert(ram_addr_t id, uint8_t *pdata, int use_buffer);
> +static unsigned long cache_get_cache_pos(ram_addr_t address);
> +static CacheItem *cache_item_get(unsigned long pos, int item);
> +static void cache_resize(int64_t new_size);

None of this are needed.  Notice that cache_resize is not marked static later.


> +
> +/***********************************************************/

We normally don't use this to split code O:-)

> +static void cache_init(int64_t num_bytes)
> +{
> +    int i;
> +
> +    cache_num_items = 0;
> +    cache_max_item_age = 0;
> +    cache_num_buckets = num_bytes / (TARGET_PAGE_SIZE * CACHE_N_WAY);

Do we check that num_bytes is not negative?  I can't see.

> +    assert(cache_num_buckets);
> +    DPRINTF("Setting cache buckets to %lu\n", cache_num_buckets);
> +
> +    assert(!page_cache);

Only user of this function make page_cache = NULL before calling.
Returning an error looks more sensible, but haven't yet looked at the
next patechs to see if we have a way to do anything good with the error.

> +    page_cache = (CacheBucket *)g_malloc((cache_num_buckets) *
> +            sizeof(CacheBucket));

g_malloc() returns void * (well gpointer, but that is void*).  We don't
need to cast its return.

> +static int cache_is_cached(ram_addr_t addr)
> +{
> +    unsigned long pos = cache_get_cache_pos(addr);
> +
> +    assert(page_cache);
> +    CacheBucket *bucket = &page_cache[pos];
> +    return cache_get_newest(bucket, addr);
> +}

Can we make this function return a bool?  Basically we put that anything
!= -1 means cached, -1 means non-cached.

> +
> +static void cache_insert(unsigned long addr, uint8_t *pdata, int
> use_buffer)

Change use_buffer to a bool?

or even better, just remove the parameter altogether and change inteface
to:

cache_insert(addr, foo, 1) -> cache_insert(addr, foo)
cache_insert(addr, foo, 0) -> cache_insert(addr, g_memdup(foo,TARGET_PAGE_SIZE))

and remove all the use_buffer() code?

> +{
> +    unsigned long pos;
> +    int slot = -1;
> +    CacheBucket *bucket;
> +
> +    pos = cache_get_cache_pos(addr);
> +    assert(page_cache);

Function call in previous line already use page_cache, switch lines or just 
remove
the assert?


> +void cache_resize(int64_t new_size)
> +{
> +    int64_t new_num_buckets = new_size/(TARGET_PAGE_SIZE * CACHE_N_WAY);
> +    CacheBucket *old_page_cache = page_cache;
> +    int i;
> +    int64_t old_num_buckets = cache_num_buckets;
> +
> +    /* same size */
> +    if (new_num_buckets == cache_num_buckets) {
> +        return;
> +    }

Shouldn't we check that new_num_buckets makes sense?

> +    /* cache was not inited */
> +    if (page_cache == NULL) {
> +        return;
> +    }
> +
> +    /* create a new cache */
> +    page_cache = NULL;
> +    cache_init(new_size);

Later, Juan.