qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] page_cache: Fix memory leak


From: Peter Lieven
Subject: Re: [Qemu-devel] [PATCH] page_cache: Fix memory leak
Date: Mon, 25 Feb 2013 13:27:02 +0100

Am 25.02.2013 um 13:21 schrieb Peter Maydell <address@hidden>:

> On 25 February 2013 11:42, Peter Lieven <address@hidden> wrote:
>> XBZRLE encoded migration introduced a MRU page cache meachnism.
>> Unfortunately, cached items where never freed on a collision.
>> 
>> This lead to out of memory conditions during XBZRLE migration
>> if the page cache was small and there where a lot of collisions.
>> 
>> Signed-off-by: Peter Lieven <address@hidden>
>> ---
>> page_cache.c |    4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/page_cache.c b/page_cache.c
>> index ba5640b..a6c3a15 100644
>> --- a/page_cache.c
>> +++ b/page_cache.c
>> @@ -152,8 +152,10 @@ void cache_insert(PageCache *cache, uint64_t addr,
>> uint8_t *pdata)
>>     /* actual update of entry */
>>     it = cache_get_by_addr(cache, addr);
>> 
>> -    if (!it->it_data) {
>> +    if (it->it_data == NULL) {
> 
> Please don't make minor syntactic tweaks in patches like this,
> it makes them harder to review.
> 
>>         cache->num_items++;
>> +    } else {
>> +        g_free(it->it_data);
>>     }
> 
> g_free(NULL) is OK so you could just make it unconditional.
> 
> Looking at the code in general I think it is probably the
> right thing to move it to "cache owns (ie duplicates
> and frees) the data", since the code is already most of
> the way there and just has some bits which aren't.

ok, i just had a look at the call to cache_insert from cache_resize.
there cache_insert is only called if it_data == NULL so it doesn`t
do any harm.

May intention was to fix the memory leak. I have noticed that XBZRLE
migration was broken somewhen last year and just haven`t had the time
to look into this. This leak is definitely critical as it kills the source VM
easily if there is sufficient activity.

We can postpone the discussion to change the semantics or do not
enter any discussion as week. I don`t mind.

Peter

> 
> -- PMM




reply via email to

[Prev in Thread] Current Thread [Next in Thread]