On 02/25/2013 02:17 PM, Peter Lieven wrote:
>
> Am 25.02.2013 um 13:15 schrieb Orit Wasserman <owass...@redhat.com>:
>
>> 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
>
> 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 <p...@kamp.de>
>>> ---
>>> 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;
>>> }
>>
>
