* [Qemu-devel] [PATCH 3/7] XBZRLE: optimize XBZRLE to decrease the cache missing
@ 2014-02-28 4:09 Gonglei (Arei)
2014-02-28 9:51 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 3+ messages in thread
From: Gonglei (Arei) @ 2014-02-28 4:09 UTC (permalink / raw)
To: Gonglei (Arei), qemu-devel
Cc: Peter Maydell, Juan Quintela, pl, owasserm, aliguori,
chenliang (T),
pbonzini
Avoid the hot pages cache replacing by others to remarkable decrease cache
missing. The counter of updating dirty bitmap is used to indicate the cached
page age.
Signed-off-by: ChenLiang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
arch_init.c | 8 +++++---
include/migration/page_cache.h | 8 ++++++--
page_cache.c | 19 +++++++++++++++----
3 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 6823c5a..fc71331 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -315,7 +315,8 @@ static void xbzrle_cache_zero_page(ram_addr_t current_addr)
/* We don't care if this fails to allocate a new cache page
* as long as it updated an old one */
- cache_insert(XBZRLE.cache, current_addr, ZERO_TARGET_PAGE);
+ cache_insert(XBZRLE.cache, current_addr, ZERO_TARGET_PAGE,
+ get_bitmap_sync_cnt());
}
#define ENCODING_FLAG_XBZRLE 0x1
@@ -327,10 +328,11 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t **current_data,
int encoded_len = 0, bytes_sent = -1;
uint8_t *prev_cached_page;
- if (!cache_is_cached(XBZRLE.cache, current_addr)) {
+ if (!cache_is_cached(XBZRLE.cache, current_addr, get_bitmap_sync_cnt())) {
acct_info.xbzrle_cache_miss++;
if (!last_stage) {
- if (cache_insert(XBZRLE.cache, current_addr, *current_data) == -1) {
+ if (cache_insert(XBZRLE.cache, current_addr, *current_data,
+ get_bitmap_sync_cnt()) == -1) {
return -1;
} else {
/* update *current_data when the page has been
diff --git a/include/migration/page_cache.h b/include/migration/page_cache.h
index 2d5ce2d..cadce55 100644
--- a/include/migration/page_cache.h
+++ b/include/migration/page_cache.h
@@ -43,8 +43,10 @@ void cache_fini(PageCache *cache);
*
* @cache pointer to the PageCache struct
* @addr: page addr
+ * @current_age indicate age of the page if cache hit
*/
-bool cache_is_cached(const PageCache *cache, uint64_t addr);
+bool cache_is_cached(const PageCache *cache, uint64_t addr,
+ uint64_t current_age);
/**
* get_cached_data: Get the data cached for an addr
@@ -65,8 +67,10 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr);
* @cache pointer to the PageCache struct
* @addr: page address
* @pdata: pointer to the page
+ * @current_age indicate age of the page if the page is inserted into cache
*/
-int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata);
+int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata,
+ uint64_t current_age);
/**
* cache_resize: resize the page cache. In case of size reduction the extra
diff --git a/page_cache.c b/page_cache.c
index b033681..fa58ab2 100644
--- a/page_cache.c
+++ b/page_cache.c
@@ -121,7 +121,8 @@ static size_t cache_get_cache_pos(const PageCache *cache,
return pos;
}
-bool cache_is_cached(const PageCache *cache, uint64_t addr)
+bool cache_is_cached(const PageCache *cache, uint64_t addr,
+ uint64_t current_age)
{
size_t pos;
@@ -130,7 +131,12 @@ bool cache_is_cached(const PageCache *cache, uint64_t addr)
pos = cache_get_cache_pos(cache, addr);
- return (cache->page_cache[pos].it_addr == addr);
+ if (cache->page_cache[pos].it_addr == addr) {
+ /* updata the it_age when the cache hit */
+ cache->page_cache[pos].it_age = current_age;
+ return true;
+ }
+ return false;
}
static CacheItem *cache_get_by_addr(const PageCache *cache, uint64_t addr)
@@ -150,7 +156,8 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr)
return cache_get_by_addr(cache, addr)->it_data;
}
-int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata)
+int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata,
+ uint64_t current_age)
{
CacheItem *it = NULL;
@@ -161,6 +168,10 @@ int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata)
/* actual update of entry */
it = cache_get_by_addr(cache, addr);
+ if ((it->it_data != NULL) && (it->it_age + 2 > current_age)) {
+ /* the cache page is fresh, don't replace it */
+ return -1;
+ }
/* allocate page */
if (!it->it_data) {
it->it_data = g_try_malloc(cache->page_size);
@@ -173,7 +184,7 @@ int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata)
memcpy(it->it_data, pdata, cache->page_size);
- it->it_age = ++cache->max_item_age;
+ it->it_age = current_age;
it->it_addr = addr;
return 0;
--
1.7.12.4
Best regards,
-Gonglei
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] XBZRLE: optimize XBZRLE to decrease the cache missing
2014-02-28 4:09 [Qemu-devel] [PATCH 3/7] XBZRLE: optimize XBZRLE to decrease the cache missing Gonglei (Arei)
@ 2014-02-28 9:51 ` Dr. David Alan Gilbert
2014-02-28 11:00 ` Gonglei
0 siblings, 1 reply; 3+ messages in thread
From: Dr. David Alan Gilbert @ 2014-02-28 9:51 UTC (permalink / raw)
To: Gonglei (Arei)
Cc: chenliang (T),
Peter Maydell, Juan Quintela, pl, qemu-devel, aliguori, pbonzini
* Gonglei (Arei) (arei.gonglei@huawei.com) wrote:
> Avoid the hot pages cache replacing by others to remarkable decrease cache
> missing. The counter of updating dirty bitmap is used to indicate the cached
> page age.
It seems worth noting that you've changed 'it_age's use - that's ok, but
perhaps not obvious.
Also, the change in the behaviour of cache_insert to 'fail' when it decides
not to insert a page is not obvious.
> Signed-off-by: ChenLiang <chenliang88@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
> arch_init.c | 8 +++++---
> include/migration/page_cache.h | 8 ++++++--
> page_cache.c | 19 +++++++++++++++----
> 3 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 6823c5a..fc71331 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -315,7 +315,8 @@ static void xbzrle_cache_zero_page(ram_addr_t current_addr)
>
> /* We don't care if this fails to allocate a new cache page
> * as long as it updated an old one */
> - cache_insert(XBZRLE.cache, current_addr, ZERO_TARGET_PAGE);
> + cache_insert(XBZRLE.cache, current_addr, ZERO_TARGET_PAGE,
> + get_bitmap_sync_cnt());
> }
>
> #define ENCODING_FLAG_XBZRLE 0x1
> @@ -327,10 +328,11 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t **current_data,
> int encoded_len = 0, bytes_sent = -1;
> uint8_t *prev_cached_page;
>
> - if (!cache_is_cached(XBZRLE.cache, current_addr)) {
> + if (!cache_is_cached(XBZRLE.cache, current_addr, get_bitmap_sync_cnt())) {
> acct_info.xbzrle_cache_miss++;
> if (!last_stage) {
> - if (cache_insert(XBZRLE.cache, current_addr, *current_data) == -1) {
> + if (cache_insert(XBZRLE.cache, current_addr, *current_data,
> + get_bitmap_sync_cnt()) == -1) {
> return -1;
> } else {
> /* update *current_data when the page has been
> diff --git a/include/migration/page_cache.h b/include/migration/page_cache.h
> index 2d5ce2d..cadce55 100644
> --- a/include/migration/page_cache.h
> +++ b/include/migration/page_cache.h
> @@ -43,8 +43,10 @@ void cache_fini(PageCache *cache);
> *
> * @cache pointer to the PageCache struct
> * @addr: page addr
> + * @current_age indicate age of the page if cache hit
> */
> -bool cache_is_cached(const PageCache *cache, uint64_t addr);
> +bool cache_is_cached(const PageCache *cache, uint64_t addr,
> + uint64_t current_age);
>
> /**
> * get_cached_data: Get the data cached for an addr
> @@ -65,8 +67,10 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr);
> * @cache pointer to the PageCache struct
> * @addr: page address
> * @pdata: pointer to the page
> + * @current_age indicate age of the page if the page is inserted into cache
> */
> -int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata);
> +int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata,
> + uint64_t current_age);
The comment on cache_insert needs changing, it currently says:
* Returns -1 on error
that's not true any more, it'll return -1 on error or if it just decided
that it's better not to cache it.
> /**
> * cache_resize: resize the page cache. In case of size reduction the extra
> diff --git a/page_cache.c b/page_cache.c
> index b033681..fa58ab2 100644
> --- a/page_cache.c
> +++ b/page_cache.c
> @@ -121,7 +121,8 @@ static size_t cache_get_cache_pos(const PageCache *cache,
> return pos;
> }
>
> -bool cache_is_cached(const PageCache *cache, uint64_t addr)
> +bool cache_is_cached(const PageCache *cache, uint64_t addr,
> + uint64_t current_age)
> {
> size_t pos;
>
> @@ -130,7 +131,12 @@ bool cache_is_cached(const PageCache *cache, uint64_t addr)
>
> pos = cache_get_cache_pos(cache, addr);
>
> - return (cache->page_cache[pos].it_addr == addr);
> + if (cache->page_cache[pos].it_addr == addr) {
> + /* updata the it_age when the cache hit */
Typo: updata -> update
> + cache->page_cache[pos].it_age = current_age;
> + return true;
> + }
> + return false;
> }
>
> static CacheItem *cache_get_by_addr(const PageCache *cache, uint64_t addr)
> @@ -150,7 +156,8 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr)
> return cache_get_by_addr(cache, addr)->it_data;
> }
>
> -int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata)
> +int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata,
> + uint64_t current_age)
> {
>
> CacheItem *it = NULL;
> @@ -161,6 +168,10 @@ int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata)
> /* actual update of entry */
> it = cache_get_by_addr(cache, addr);
>
> + if ((it->it_data != NULL) && (it->it_age + 2 > current_age)) {
> + /* the cache page is fresh, don't replace it */
> + return -1;
> + }
A magical constant '2' - should probably a define/const somewhere.
> /* allocate page */
> if (!it->it_data) {
> it->it_data = g_try_malloc(cache->page_size);
> @@ -173,7 +184,7 @@ int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata)
>
> memcpy(it->it_data, pdata, cache->page_size);
>
> - it->it_age = ++cache->max_item_age;
> + it->it_age = current_age;
> it->it_addr = addr;
>
> return 0;
Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] XBZRLE: optimize XBZRLE to decrease the cache missing
2014-02-28 9:51 ` Dr. David Alan Gilbert
@ 2014-02-28 11:00 ` Gonglei
0 siblings, 0 replies; 3+ messages in thread
From: Gonglei @ 2014-02-28 11:00 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: chenliang (T),
Peter Maydell, Juan Quintela, pl, qemu-devel, aliguori, pbonzini
On 2014/2/28 17:51, Dr. David Alan Gilbert wrote:
> * Gonglei (Arei) (arei.gonglei@huawei.com) wrote:
>> Avoid the hot pages cache replacing by others to remarkable decrease cache
>> missing. The counter of updating dirty bitmap is used to indicate the cached
>> page age.
>
> It seems worth noting that you've changed 'it_age's use - that's ok, but
> perhaps not obvious.
>
> Also, the change in the behaviour of cache_insert to 'fail' when it decides
> not to insert a page is not obvious.
>
Check it.
>
>> Signed-off-by: ChenLiang <chenliang88@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>> arch_init.c | 8 +++++---
>> include/migration/page_cache.h | 8 ++++++--
>> page_cache.c | 19 +++++++++++++++----
>> 3 files changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index 6823c5a..fc71331 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -315,7 +315,8 @@ static void xbzrle_cache_zero_page(ram_addr_t current_addr)
>>
>> /* We don't care if this fails to allocate a new cache page
>> * as long as it updated an old one */
>> - cache_insert(XBZRLE.cache, current_addr, ZERO_TARGET_PAGE);
>> + cache_insert(XBZRLE.cache, current_addr, ZERO_TARGET_PAGE,
>> + get_bitmap_sync_cnt());
>> }
>>
>> #define ENCODING_FLAG_XBZRLE 0x1
>> @@ -327,10 +328,11 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t **current_data,
>> int encoded_len = 0, bytes_sent = -1;
>> uint8_t *prev_cached_page;
>>
>> - if (!cache_is_cached(XBZRLE.cache, current_addr)) {
>> + if (!cache_is_cached(XBZRLE.cache, current_addr, get_bitmap_sync_cnt())) {
>> acct_info.xbzrle_cache_miss++;
>> if (!last_stage) {
>> - if (cache_insert(XBZRLE.cache, current_addr, *current_data) == -1) {
>> + if (cache_insert(XBZRLE.cache, current_addr, *current_data,
>> + get_bitmap_sync_cnt()) == -1) {
>> return -1;
>> } else {
>> /* update *current_data when the page has been
>> diff --git a/include/migration/page_cache.h b/include/migration/page_cache.h
>> index 2d5ce2d..cadce55 100644
>> --- a/include/migration/page_cache.h
>> +++ b/include/migration/page_cache.h
>> @@ -43,8 +43,10 @@ void cache_fini(PageCache *cache);
>> *
>> * @cache pointer to the PageCache struct
>> * @addr: page addr
>> + * @current_age indicate age of the page if cache hit
>> */
>> -bool cache_is_cached(const PageCache *cache, uint64_t addr);
>> +bool cache_is_cached(const PageCache *cache, uint64_t addr,
>> + uint64_t current_age);
>>
>> /**
>> * get_cached_data: Get the data cached for an addr
>> @@ -65,8 +67,10 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr);
>> * @cache pointer to the PageCache struct
>> * @addr: page address
>> * @pdata: pointer to the page
>> + * @current_age indicate age of the page if the page is inserted into cache
>> */
>> -int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata);
>> +int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata,
>> + uint64_t current_age);
>
> The comment on cache_insert needs changing, it currently says:
> * Returns -1 on error
>
> that's not true any more, it'll return -1 on error or if it just decided
> that it's better not to cache it.
>
Check.
>> /**
>> * cache_resize: resize the page cache. In case of size reduction the extra
>> diff --git a/page_cache.c b/page_cache.c
>> index b033681..fa58ab2 100644
>> --- a/page_cache.c
>> +++ b/page_cache.c
>> @@ -121,7 +121,8 @@ static size_t cache_get_cache_pos(const PageCache *cache,
>> return pos;
>> }
>>
>> -bool cache_is_cached(const PageCache *cache, uint64_t addr)
>> +bool cache_is_cached(const PageCache *cache, uint64_t addr,
>> + uint64_t current_age)
>> {
>> size_t pos;
>>
>> @@ -130,7 +131,12 @@ bool cache_is_cached(const PageCache *cache, uint64_t addr)
>>
>> pos = cache_get_cache_pos(cache, addr);
>>
>> - return (cache->page_cache[pos].it_addr == addr);
>> + if (cache->page_cache[pos].it_addr == addr) {
>> + /* updata the it_age when the cache hit */
>
> Typo: updata -> update
>
>> + cache->page_cache[pos].it_age = current_age;
>> + return true;
>> + }
>> + return false;
>> }
>>
>> static CacheItem *cache_get_by_addr(const PageCache *cache, uint64_t addr)
>> @@ -150,7 +156,8 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr)
>> return cache_get_by_addr(cache, addr)->it_data;
>> }
>>
>> -int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata)
>> +int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata,
>> + uint64_t current_age)
>> {
>>
>> CacheItem *it = NULL;
>> @@ -161,6 +168,10 @@ int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata)
>> /* actual update of entry */
>> it = cache_get_by_addr(cache, addr);
>>
>> + if ((it->it_data != NULL) && (it->it_age + 2 > current_age)) {
>> + /* the cache page is fresh, don't replace it */
>> + return -1;
>> + }
>
> A magical constant '2' - should probably a define/const somewhere.
Check.
>
>> /* allocate page */
>> if (!it->it_data) {
>> it->it_data = g_try_malloc(cache->page_size);
>> @@ -173,7 +184,7 @@ int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata)
>>
>> memcpy(it->it_data, pdata, cache->page_size);
>>
>> - it->it_age = ++cache->max_item_age;
>> + it->it_age = current_age;
>> it->it_addr = addr;
>>
>> return 0;
>
> Dave
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Best regards,
-Gonglei
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-02-28 11:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-28 4:09 [Qemu-devel] [PATCH 3/7] XBZRLE: optimize XBZRLE to decrease the cache missing Gonglei (Arei)
2014-02-28 9:51 ` Dr. David Alan Gilbert
2014-02-28 11:00 ` Gonglei
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.