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?
> +
> +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.
--
Eric Blake ebl...@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
