diff mbox

page_cache: dup memory on insert

Message ID 38D0DA1A-A001-4AD1-9F63-E4CEE4EC4610@dlhnet.de
State New
Headers show

Commit Message

Peter Lieven Feb. 25, 2013, 12:50 p.m. UTC 
Am 25.02.2013 um 13:36 schrieb Peter Maydell <peter.maydell@linaro.org>:

> On 25 February 2013 12:17, Peter Lieven <pl@dlhnet.de> wrote:
>> Am 25.02.2013 um 13:13 schrieb Peter Maydell <peter.maydell@linaro.org>:
>>> 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?




> 
> -- PMM

Comments

Peter Maydell Feb. 25, 2013, 1:08 p.m. UTC | #1 
On 25 February 2013 12:50, Peter Lieven <pl@dlhnet.de> wrote:
> 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;

Yeah, more or less. You could roll the two if () conditions
into "if (new_it->it_data && new_it->it_age >= old_it->it_age)"
but that's just a style thing.

-- PMM
diff mbox

Patch

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;