qemu-devel
[Top][All Lists]
Advanced

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

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


From: Orit Wasserman
Subject: Re: [Qemu-devel] [PATCH v9 01/10] Add cache handling functions
Date: Wed, 18 Apr 2012 19:23:16 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120329 Thunderbird/11.0.1

On 04/18/2012 05:34 PM, Juan Quintela wrote:
> 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.

OK.

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

Will be removed ...

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

I will add a check .

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

I guess we can fail the migration in case of an 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.
Will be fixed
> 
>> +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.
> 
sure

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

The assert should be first .

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

The checks are done in the calling function (patch 9) . Do you think I should 
move them here ?

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




reply via email to

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