Re: [Qemu-devel] [PATCH v5 1/9] Add cache handling functions

[Orit Wasserman <owasserm@redhat.com>]

On 01/04/2012 01:46 PM, Stefan Hajnoczi wrote:
> On Tue, Jan 3, 2012 at 3:34 PM, Orit Wasserman <owasserm@redhat.com> wrote:
>> +static unsigned long cache_get_cache_pos(ram_addr_t address)
>> +{
>> +    unsigned long pos;
>> +
>> +    assert(cache_num_buckets);
>> +    pos = (address/TARGET_PAGE_SIZE) & (cache_num_buckets - 1);
> 
> Where do we guarantee that cache_num_buckets is a power of 2?

We should do it  to when setting cache size , I will add the check.
> 
>> +static void cache_insert(unsigned long addr, uint8_t *pdata)
>> +{
>> +    unsigned long pos;
>> +    int slot = -1;
>> +    CacheBucket *bucket;
>> +
>> +    pos = cache_get_cache_pos(addr);
>> +    assert(page_cache);
>> +    bucket = &page_cache[pos];
>> +    slot = cache_get_oldest(bucket); /* evict LRU */
>> +
>> +    /* actual update of entry */
>> +    CacheItem *it = cache_item_get(pos, slot);
>> +    if (!it->it_data) {
>> +        cache_num_items++;
>> +    }
>> +    g_free(it->it_data);
>> +
>> +    it->it_data = g_malloc0(TARGET_PAGE_SIZE);
>> +    memcpy(it->it_data, pdata, TARGET_PAGE_SIZE);
> 
> If we're evicting an entry:
> 1. Free existing data.
> 2. Allocate new data.
> 3. Zero new data.
> 4. Memcpy pdata into new data.
> 
> That does a bunch of extra work.  How about:
> 1. Memcpy pdata over old data.

I noticed it too , i will fix it in next version.
> 
> So:
> if (!it->it_data) {
>     cache_num_items++;
>     it->it_data = g_malloc(TARGET_PAGE_SIZE);
> }
> memcpy(it->it_data, pdata, TARGET_PAGE_SIZE);