Re: [Qemu-devel] [PATCH 3/7] XBZRLE: optimize XBZRLE to decrease the cache missing

[Gonglei]

On 2014/2/28 17:51, Dr. David Alan Gilbert wrote:

> * 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.
> 

Check it.

> 
>> 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.
> 

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 / address@hidden / Manchester, UK


Best regards,
-Gonglei