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

[Peter Maydell]

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.

-- PMM