[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/7] XBZRLE: optimize XBZRLE to decrease the cac
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH 3/7] XBZRLE: optimize XBZRLE to decrease the cache missing |
Date: |
Fri, 28 Feb 2014 09:51:26 +0000 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
* Gonglei (Arei) (address@hidden) 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 <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> ---
> 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 / address@hidden / Manchester, UK