On 2014/2/28 17:51, Dr. David Alan Gilbert wrote:
> * Gonglei (Arei) (arei.gong...@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 <chenlian...@huawei.com>
>> Signed-off-by: Gonglei <arei.gong...@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 / dgilb...@redhat.com / Manchester, UK
Best regards,
-Gonglei
