On 06/27/2012 07:55 PM, Eric Blake wrote:
> On 06/27/2012 04:34 AM, Orit Wasserman wrote:
>> Add LRU page cache mechanism.
>> The page are accessed by their address.
>>
>> Signed-off-by: Benoit Hudzia <benoit.hud...@sap.com>
>> Signed-off-by: Petter Svard <pett...@cs.umu.se>
>> Signed-off-by: Aidan Shribman <aidan.shrib...@sap.com>
>> Signed-off-by: Orit Wasserman <owass...@redhat.com>
>
>> +++ b/cache.c
>
> cache.c is a rather generic name; should it be page-cache.c instead to
> reflect that it only caches memory pages?
>
>> @@ -0,0 +1,217 @@
>> +/*
>> + * Page cache for qemu
>> + * The cache is base on a hash on the page address
>
> s/base on a hash on/based on a hash of/
>
>> +
>> +Cache *cache_init(int64_t num_pages, unsigned int page_size)
>> +{
>
>> +
>> + /* round down to the nearest power of 2 */
>> + if (!is_power_of_2(num_pages)) {
>> + num_pages = 1 << ffs(num_pages);
>
> That's not how you round down. For example, if I passed in 0x5, you end
> up giving me 1 << ffs(5) == 1 << 1 == 2, but the correct answer should be 4.
>
> http://graphics.stanford.edu/~seander/bithacks.html#IntegerLogObvious
> and http://aggregate.org/MAGIC/#Leading%20Zero%20Count give some hints
> about what you really want to be doing; offhand, I came up with this (it
> works because you already rejected negative num_pages):
>
> if (!is_power_of_2(num_pages)) {
> num_pages |= num_pages >> 1;
> num_pages |= num_pages >> 2;
> num_pages |= num_pages >> 4;
> num_pages |= num_pages >> 8;
> num_pages |= num_pages >> 16;
> num_pages |= num_pages >> 32;
> num_pages -= num_pages / 2;
> }
>
>> + cache->page_cache = g_malloc((cache->max_num_items) *
>> + sizeof(CacheItem));
>> + if (!cache->page_cache) {
>> + DPRINTF("could not allocate cache\n");
>> + g_free(cache);
>> + return NULL;
>> + }
>> +
>> + for (i = 0; i < cache->max_num_items; i++) {
>> + cache->page_cache[i].it_data = NULL;
>> + cache->page_cache[i].it_age = 0;
>
> Does g_malloc leave memory uninitialized, or is it like calloc where it
> zeros out the memory making these two assignments redundant?
g_malloc doesn't initialize memory, g_malloc0 does.
>
>> +
>> +int cache_resize(Cache *cache, int64_t new_num_pages)
>> +{
>> + Cache *new_cache;
>> + int i;
>> +
>> + CacheItem *old_it, *new_it;
>> +
>> + g_assert(cache);
>> +
>> + /* same size */
>> + if (new_num_pages == cache->max_num_items) {
>> + return 0;
>> + }
>> +
>> + /* cache was not inited */
>> + if (cache->page_cache == NULL) {
>> + return -1;
>> + }
>
> Shouldn't these two conditions be swapped? Non-init failure should take
> precedence over no size change.
>
> If new_num_pages is not a power of 2, but rounds down to the same as the
> existing size, the size won't compare equal and you end up wasting a lot
> of effort moving pages between the resulting identically sized caches.
> I'd factor out your rounding-down code, and call it from multiple places
> prior to checking for size equality.
>
>> + /* 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/collision , if there is/collision, if there is,/
>
>> +++ b/include/qemu/cache.h
>> @@ -0,0 +1,79 @@
>> +/*
>> + * Page cache for qemu
>> + * The cache is base on a hash on the page address
>
> Same comments as for cache.c.
>
