Patchwork page_cache: Fix memory leak

login
register
mail settings
Submitter Peter Lieven
Date Feb. 25, 2013, 11:42 a.m.
Message ID <512B4E09.1040908@dlhnet.de>
Download mbox | patch
Permalink /patch/222911/
State New
Headers show

Comments

Peter Lieven - Feb. 25, 2013, 11:42 a.m.
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 <pl@kamp.de>
---
  page_cache.c |    4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
Orit Wasserman - Feb. 25, 2013, 11:58 a.m.
On 02/25/2013 01:42 PM, Peter Lieven 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 <pl@kamp.de>
> ---
>  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) {
>          cache->num_items++;
> +    } else {
> +        g_free(it->it_data);

Why? we don't allocate here but just store the pointer.
It is the caller responsibility to allocate/free the data,
for example for migration it is the guest memory page.

Orit
>      }
> 
>      it->it_data = pdata;
Peter Lieven - Feb. 25, 2013, 12:08 p.m.
Am 25.02.2013 um 12:58 schrieb Orit Wasserman <owasserm@redhat.com>:

> On 02/25/2013 01:42 PM, Peter Lieven 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 <pl@kamp.de>
>> ---
>> 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) {
>>         cache->num_items++;
>> +    } else {
>> +        g_free(it->it_data);
> 
> Why? we don't allocate here but just store the pointer.
> It is the caller responsibility to allocate/free the data,
> for example for migration it is the guest memory page.

It is being dupped when the page is added to the cache.

            cache_insert(XBZRLE.cache, current_addr,
                         g_memdup(current_data, TARGET_PAGE_SIZE));

This is, of course, necessary because the memory might change
and you need the state of the page that has been transferred to the
destination.

It is also obvious that the cache needs to hold his own copy because
it frees the data on finish and on resize.

Peter


> 
> Orit
>>     }
>> 
>>     it->it_data = pdata;
>
Peter Maydell - Feb. 25, 2013, 12:21 p.m.
On 25 February 2013 11:42, Peter Lieven <pl@dlhnet.de> 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 <pl@kamp.de>
> ---
>  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.

-- PMM
Peter Lieven - Feb. 25, 2013, 12:27 p.m.
Am 25.02.2013 um 13:21 schrieb Peter Maydell <peter.maydell@linaro.org>:

> On 25 February 2013 11:42, Peter Lieven <pl@dlhnet.de> 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 <pl@kamp.de>
>> ---
>> 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

Patch

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) {
          cache->num_items++;
+    } else {
+        g_free(it->it_data);
      }

      it->it_data = pdata;