| Message ID | 512B4E09.1040908@dlhnet.de |
|---|---|
| State | New |
| Headers | show |
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;
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; >
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
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
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;
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(-)