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

[Eric Blake]

On 03/11/2014 06:53 AM, address@hidden wrote:
> From: ChenLiang <address@hidden>
> 
> Avoid the hot pages being replaced by others to remarkable decrease cache

s/the hot/hot/
s/remarkable/remarkably/

> missing. The counter of updating dirty bitmap is used to indicate the cached

s/missing/misses/

> page age.

Please give more rationale - when claiming that something gave a speed
improvement, it helps to back up that claim with the sample program you
ran in the guest along with before and after numbers to back up your
point.  A "remarkable decrease" is subjective, but pre- and post-patch
numbers is objective.

This may mean rebasing your series to re-order patches, and FIRST export
the counters that you then use to justify this patch (the current order
of applying the optimization, and only THEN exposing the numbers through
QMP, makes it harder to prove that this patch helped).

> 
> Signed-off-by: ChenLiang <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> ---
>  arch_init.c                    |  8 +++++---
>  include/migration/page_cache.h | 10 +++++++---
>  page_cache.c                   | 23 +++++++++++++++++++----
>  3 files changed, 31 insertions(+), 10 deletions(-)
> 

>          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) {

Indentation - I'd write:

           if (cache_insert(XBZRLE.cache, current_addr, *current_data,
                            get_bitmap_sync_cnt()) == -1) {

This patch needs to be rebased once you fix 2/10 to just directly use
the variable instead of wrapping it inside pointless static inline
accessor functions.


> @@ -161,6 +171,11 @@ 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 +
> +        CACHED_PAGE_LIFETIME > current_age)) {

Unnecessary (), and odd choice of line break location coupled with poor
indentation.  It fits fine on one 80-column line (hmm, my email may wrap
because I write email in fewer columns than 80):

    if (it->it_data && it->it_age + CACHED_PAGE_LIFETIME > current_age) {

and if you MUST break lines, breaking on the lower-priority operators
makes it easier to read the grouping:

    if (it->it_data &&
        it->it_age + CACHED_PAGE_LIFETIME > current_age) {

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature