Patchwork page_cache: dup memory on insert

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

Comments

Peter Lieven - Feb. 25, 2013, 11:52 a.m.
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(-)
Peter Maydell - Feb. 25, 2013, 12:13 p.m.
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
Orit Wasserman - Feb. 25, 2013, 12:15 p.m.
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;
>  }
Peter Lieven - Feb. 25, 2013, 12:17 p.m.
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;
>> }
>
Peter Lieven - Feb. 25, 2013, 12:17 p.m.
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
Orit Wasserman - Feb. 25, 2013, 12:33 p.m.
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;
>>> }
>>
>
Peter Maydell - Feb. 25, 2013, 12:36 p.m.
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
Peter Lieven - Feb. 25, 2013, 12:37 p.m.
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;
>>>> }
>>> 
>> 
>
Orit Wasserman - Feb. 25, 2013, 12:40 p.m.
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;
>>>> }
>>>
>>
> 
>
Orit Wasserman - Feb. 25, 2013, 12:41 p.m.
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;
>>>>> }
>>>>
>>>
>>
> 
>

Patch

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;
  }