Re: [PATCH v3 2/3] memory: introduce DirtyRateDirtyPages and util function

[Peter Xu <peterx@redhat.com>]

On Thu, Jul 15, 2021 at 11:51:32PM +0800, huangy81@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> introduce DirtyRateDirtyPages and use it to the dirty pages
> along with memory_global_dirty_log_sync.
> introduce cpu_physical_memory_dirtyrate_reset_protect to
> clear dirty bitmap within slot in kvm
> 
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> ---
>  include/exec/ram_addr.h | 19 +++++++++++++++++++
>  migration/dirtyrate.c   |  2 ++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 45c9132..dce0f46 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -26,6 +26,8 @@
>  #include "exec/ramlist.h"
>  #include "exec/ramblock.h"
>  
> +extern uint64_t DirtyRateDirtyPages;
> +
>  /**
>   * clear_bmap_size: calculate clear bitmap size
>   *
> @@ -415,6 +417,17 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>              }
>          }
>      }
> +
> +    if (global_dirty_tracking &&

This check can be dropped.

> +        global_dirty_tracking & GLOBAL_DIRTY_DIRTY_RATE) {
> +        long nr = BITS_TO_LONGS(pages);
> +        for (i = 0; i < nr; i++) {
> +            if (bitmap[i]) {
> +                unsigned long temp = leul_to_cpu(bitmap[i]);
> +                DirtyRateDirtyPages += ctpopl(temp);

IMHO we don't need to loop the bitmap twice; we can still do it in above if
blocks when looping.

Note that this variable (DirtyRateDirtyPages) should either be protected by a
lock or updated using qatomic_add().  I think it's easier we just read/write it
with BQL as it should be required for cpu_physical_memory_set_dirty_lebitmap()
callers.  So looks okay.

> +            }
> +        }
> +    }
>  }
>  #endif /* not _WIN32 */
>  
> @@ -510,5 +523,11 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
>  
>      return num_dirty;
>  }
> +
> +static inline
> +void cpu_physical_memory_dirtyrate_reset_protect(RAMBlock *rb)
> +{
> +    memory_region_clear_dirty_bitmap(rb->mr, 0, rb->used_length);
> +}

The name cpu_physical_memory_dirtyrate_reset_protect() didn't really help
anything.. I'd suggest we drop this helper and call it directly in the next
patch.

>  #endif
>  #endif
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 3c8c5e2..c465e62 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -28,6 +28,8 @@
>  #include "sysemu/runstate.h"
>  #include "exec/memory.h"
>  
> +uint64_t DirtyRateDirtyPages;

CamelCase is normally used for type definitions in QEMU.  Maybe define it as
"total_dirty_pages"?  Then we never reset it to zero, but only increase it.

Also let's comment above it: it's protected by BQL.  So far it should be enough.

Thanks,

> +
>  typedef struct DirtyPageRecord {
>      uint64_t start_pages;
>      uint64_t end_pages;
> -- 
> 1.8.3.1
> 

-- 
Peter Xu