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

[Orit Wasserman]

On 02/25/2013 02:37 PM, Peter Lieven wrote:
> 
> Am 25.02.2013 um 13:33 schrieb Orit Wasserman <address@hidden>:
> 
>> 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.
> 
> This is not true. cache_insert is actually called in 2 cases:
> 
> a) there is no entry in the cache at the position calculated for current_addr 
> (it_addr = -1)
> b) there is an entry, but the address of the cached entry does not match 
> current_addr (collision)
> 
> Peter
You are right, good catch.
I will send a fixed patch
Orit
> 
> 
>> 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
>>>
>>> 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;
>>>>> }
>>>>
>>>
>>
> 
>