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

[Orit Wasserman]

On 02/25/2013 02:33 PM, Orit Wasserman wrote:
> On 02/25/2013 02:17 PM, Peter Lieven wrote:
>>
>> Am 25.02.2013 um 13:15 schrieb Orit Wasserman <address@hidden>:
>>
>>> Hi Peter,
>>> Now I get the previous patch, it should have been a patch series :).
>>> The reason we allocate from outside of the page cache is because of 
>>> cache_resize
>>> that also uses cache_insert but doesn't duplicate the buffer.
>>> There is no memory leak because if the page is cached we don't call 
>>> cache_insert
>>> but do memcpy instead.
>>
>> Ah ok, haven't noticed the resize case. But there is still a men leak on a 
>> simple collision (first patch) - nothing
>> to do with resize.
> There is no memory leak because the migration code only call the cache_insert 
> if the page is not cached
> (i.e no collision) and the cache_resize calls it when there a collision but 
> doesn't allocate.
> We can split the code to two, on internal insert function that doesn't 
> allocate for cache_resize to call
> and one external that duplicate the buffer (and calls the internal function).
> The external function should abort if there is a collision.
> 
> Orit
Fixing myself :),
There is a leak in case of a collision, so no need to abort in the external 
function but free the memory.

Orit
>>
>> We should discuss if the page cache frees data it should also allocate the 
>> data. Maybe we need to different functions.
>>
>> Peter
>>
>>
>>>
>>> Regards,
>>> Orit
>>> On 02/25/2013 01:52 PM, Peter Lieven wrote:
>>>> The page cache frees all data on finish, on resize and
>>>> if there is collision on insert. So it should be the caches
>>>> responsibility to dup the data that is stored in the cache.
>>>>
>>>> Signed-off-by: Peter Lieven <address@hidden>
>>>> ---
>>>> arch_init.c                    |    3 +--
>>>> include/migration/page_cache.h |    3 ++-
>>>> page_cache.c                   |    2 +-
>>>> 3 files changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch_init.c b/arch_init.c
>>>> index 8da868b..97bcc29 100644
>>>> --- a/arch_init.c
>>>> +++ b/arch_init.c
>>>> @@ -293,8 +293,7 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t 
>>>> *current_data,
>>>>
>>>>     if (!cache_is_cached(XBZRLE.cache, current_addr)) {
>>>>         if (!last_stage) {
>>>> -            cache_insert(XBZRLE.cache, current_addr,
>>>> -                         g_memdup(current_data, TARGET_PAGE_SIZE));
>>>> +            cache_insert(XBZRLE.cache, current_addr, current_data);
>>>>         }
>>>>         acct_info.xbzrle_cache_miss++;
>>>>         return -1;
>>>> diff --git a/include/migration/page_cache.h 
>>>> b/include/migration/page_cache.h
>>>> index 3839ac7..87894fe 100644
>>>> --- a/include/migration/page_cache.h
>>>> +++ b/include/migration/page_cache.h
>>>> @@ -57,7 +57,8 @@ bool cache_is_cached(const PageCache *cache, uint64_t 
>>>> addr);
>>>> uint8_t *get_cached_data(const PageCache *cache, uint64_t addr);
>>>>
>>>> /**
>>>> - * cache_insert: insert the page into the cache. the previous value will 
>>>> be overwritten
>>>> + * cache_insert: insert the page into the cache. the page cache
>>>> + * will dup the data on insert. the previous value will be overwritten
>>>>  *
>>>>  * @cache pointer to the PageCache struct
>>>>  * @addr: page address
>>>> diff --git a/page_cache.c b/page_cache.c
>>>> index a6c3a15..e670d91 100644
>>>> --- a/page_cache.c
>>>> +++ b/page_cache.c
>>>> @@ -158,7 +158,7 @@ void cache_insert(PageCache *cache, uint64_t addr, 
>>>> uint8_t *pdata)
>>>>         g_free(it->it_data);
>>>>     }
>>>>
>>>> -    it->it_data = pdata;
>>>> +    it->it_data = g_memdup(pdata, cache->page_size);
>>>>     it->it_age = ++cache->max_item_age;
>>>>     it->it_addr = addr;
>>>> }
>>>
>>
> 
>