| Message ID | 512B5096.7030901@dlhnet.de |
|---|---|
| State | New |
| Headers | show |
On 25 February 2013 11:52, Peter Lieven <pl@dlhnet.de> 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. > 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; > } Doesn't this introduce a leak on cache resize in the case where the element being moved from the old cache to the new does not collide with any element we've already moved? [ie the code path where we just cache_insert() the old item's data]. -- PMM
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. 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 <pl@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; > }
Am 25.02.2013 um 13:15 schrieb Orit Wasserman <owasserm@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. 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 <pl@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; >> } >
Am 25.02.2013 um 13:13 schrieb Peter Maydell <peter.maydell@linaro.org>: > On 25 February 2013 11:52, Peter Lieven <pl@dlhnet.de> 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. > >> 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; >> } > > Doesn't this introduce a leak on cache resize in the case where > the element being moved from the old cache to the new does not > collide with any element we've already moved? [ie the code > path where we just cache_insert() the old item's data]. you are right. maybe we need different functions for inserts made during resize and inserts from outside. Peter > > -- PMM
On 02/25/2013 02:17 PM, Peter Lieven wrote: > > Am 25.02.2013 um 13:15 schrieb Orit Wasserman <owasserm@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 <pl@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; >>> } >> >
On 25 February 2013 12:17, Peter Lieven <pl@dlhnet.de> wrote: > Am 25.02.2013 um 13:13 schrieb Peter Maydell <peter.maydell@linaro.org>: >> Doesn't this introduce a leak on cache resize in the case where >> the element being moved from the old cache to the new does not >> collide with any element we've already moved? [ie the code >> path where we just cache_insert() the old item's data]. > > you are right. maybe we need different functions for inserts made > during resize and inserts from outside. Looking a little more closely, I think you need that anyway, because cache_insert() updates the it_age field, when I would have expected that in the "just moving everything over to the resized cache" case we would want to retain the existing age. Since cache_resize() already has code in it that does a direct access and copy of CacheItem* fields [for the "copy old_it over new_it" case] it might be cleaner to either (a) have all the cases open-coded in cache_resize() rather than calling cache_insert(), or (b) abstract out the whole of the resize inner loop into something like cache_insert_item(PageCache *cache, CacheItem *olditem) { /* Insert olditem (an item from a different page cache) into this one. * If there is a collision then we keep the data from whichever of * olditem and the existing entry is newer. In either case, olditem's * data pointer is either copied into the new cache or freed, so * the caller must do nothing further with it. */ } Extra bonus leak: I think cache_resize() is leaking cache->page_cache. -- PMM
Am 25.02.2013 um 13:33 schrieb Orit Wasserman <owasserm@redhat.com>: > On 02/25/2013 02:17 PM, Peter Lieven wrote: >> >> Am 25.02.2013 um 13:15 schrieb Orit Wasserman <owasserm@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. 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 > 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 <pl@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; >>>> } >>> >> >
On 02/25/2013 02:33 PM, Orit Wasserman wrote: > On 02/25/2013 02:17 PM, Peter Lieven wrote: >> >> Am 25.02.2013 um 13:15 schrieb Orit Wasserman <owasserm@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 Fixing myself :), There is a leak in case of a collision, so no need to abort in the external function but free the memory. 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 <pl@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; >>>> } >>> >> > >
On 02/25/2013 02:37 PM, Peter Lieven wrote: > > Am 25.02.2013 um 13:33 schrieb Orit Wasserman <owasserm@redhat.com>: > >> On 02/25/2013 02:17 PM, Peter Lieven wrote: >>> >>> Am 25.02.2013 um 13:15 schrieb Orit Wasserman <owasserm@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. > > 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 <pl@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; >>>>> } >>>> >>> >> > >
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; }
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 <pl@kamp.de> --- arch_init.c | 3 +-- include/migration/page_cache.h | 3 ++- page_cache.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)