qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH] page_cache: dup memory on insert


From: Peter Lieven
Subject: Re: [Qemu-devel] [PATCH] page_cache: dup memory on insert
Date: Mon, 25 Feb 2013 13:50:14 +0100

Am 25.02.2013 um 13:36 schrieb Peter Maydell <address@hidden>:

> On 25 February 2013 12:17, Peter Lieven <address@hidden> wrote:
>> Am 25.02.2013 um 13:13 schrieb Peter Maydell <address@hidden>:
>>> Doesn't this introduce a leak on cache resize in the case where
>>> the element being moved from the old cache to the new does not
>>> collide with any element we've already moved? [ie the code
>>> path where we just cache_insert() the old item's data].
>> 
>> you are right. maybe we need different functions for inserts made
>> during resize and inserts from outside.
> 
> Looking a little more closely, I think you need that anyway,
> because cache_insert() updates the it_age field, when I would
> have expected that in the "just moving everything over to the
> resized cache" case we would want to retain the existing age.
> 
> Since cache_resize() already has code in it that does a
> direct access and copy of CacheItem* fields [for the "copy
> old_it over new_it" case] it might be cleaner to either
> (a) have all the cases open-coded in cache_resize() rather
> than calling cache_insert(), or (b) abstract out the
> whole of the resize inner loop into something like
> 
> cache_insert_item(PageCache *cache, CacheItem *olditem)
> {
>    /* Insert olditem (an item from a different page cache) into this one.
>     * If there is a collision then we keep the data from whichever of
>     * olditem and the existing entry is newer. In either case, olditem's
>     * data pointer is either copied into the new cache or freed, so
>     * the caller must do nothing further with it.
>     */
> }
> 
> Extra bonus leak: I think cache_resize() is leaking
> cache->page_cache.

sth like this?

diff --git a/page_cache.c b/page_cache.c
index 376f1db..04205ee 100644
--- a/page_cache.c
+++ b/page_cache.c
@@ -196,21 +196,19 @@ int64_t cache_resize(PageCache *cache, int64_t 
new_num_pages)
             /* check for collision, if there is, keep MRU page */
             new_it = cache_get_by_addr(new_cache, old_it->it_addr);
             if (new_it->it_data) {
-                /* keep the MRU page */
                 if (new_it->it_age >= old_it->it_age) {
                     g_free(old_it->it_data);
-                } else {
-                    g_free(new_it->it_data);
-                    new_it->it_data = old_it->it_data;
-                    new_it->it_age = old_it->it_age;
-                    new_it->it_addr = old_it->it_addr;
+                    continue;
                 }
-            } else {
-                cache_insert(new_cache, old_it->it_addr, old_it->it_data);
             }
+            g_free(new_it->it_data);
+            new_it->it_data = old_it->it_data;
+            new_it->it_age = old_it->it_age;
+            new_it->it_addr = old_it->it_addr;
         }
     }
 
+    g_free(cache->page_cache);
     cache->page_cache = new_cache->page_cache;
     cache->max_num_items = new_cache->max_num_items;
     cache->num_items = new_cache->num_items;



> 
> -- PMM




reply via email to

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