Re: [PATCH v2 1/2] exec/ram_addr: return nr of dirty pages in cpu_physical_memory_set_dirty_lebitmap()

[Peter Xu <peterx@redhat.com>]

On Thu, May 25, 2023 at 12:43:20PM +0100, Joao Martins wrote:
> In preparation for including the number of dirty pages in the
> vfio_get_dirty_bitmap() tracepoint, return the number of dirty pages in
> cpu_physical_memory_set_dirty_lebitmap() similar to
> cpu_physical_memory_sync_dirty_bitmap().

The patch itself looks good to me, but it's slightly different from sync
version because that was only for MIGRATION bitmap, meanwhile it counts
newly dirtied ones (so exclude already dirtied ones even if re-dirtied in
the MIGRATION bitmap), while this one counts any dirty bits in *bitmap.

Shall we perhaps state it somewhere explicitly?  A comment for retval might
be suitable above the function?

Thanks,

> 
> To avoid counting twice when GLOBAL_DIRTY_RATE is enabled, stash the
> number of bits set per bitmap quad in a variable (@nbits) and reuse it
> there.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  include/exec/ram_addr.h | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index f4fb6a211175..8b8f271d0731 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -333,14 +333,16 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
>  }
>  
>  #if !defined(_WIN32)
> -static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
> -                                                          ram_addr_t start,
> -                                                          ram_addr_t pages)
> +static inline
> +uint64_t cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
> +                                                ram_addr_t start,
> +                                                ram_addr_t pages)
>  {
>      unsigned long i, j;
> -    unsigned long page_number, c;
> +    unsigned long page_number, c, nbits;
>      hwaddr addr;
>      ram_addr_t ram_addr;
> +    uint64_t num_dirty = 0;
>      unsigned long len = (pages + HOST_LONG_BITS - 1) / HOST_LONG_BITS;
>      unsigned long hpratio = qemu_real_host_page_size() / TARGET_PAGE_SIZE;
>      unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
> @@ -368,6 +370,7 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>                  if (bitmap[k]) {
>                      unsigned long temp = leul_to_cpu(bitmap[k]);
>  
> +                    nbits = ctpopl(temp);
>                      qatomic_or(&blocks[DIRTY_MEMORY_VGA][idx][offset], temp);
>  
>                      if (global_dirty_tracking) {
> @@ -376,10 +379,12 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>                                  temp);
>                          if (unlikely(
>                              global_dirty_tracking & GLOBAL_DIRTY_DIRTY_RATE)) {
> -                            total_dirty_pages += ctpopl(temp);
> +                            total_dirty_pages += nbits;
>                          }
>                      }
>  
> +                    num_dirty += nbits;
> +
>                      if (tcg_enabled()) {
>                          qatomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset],
>                                     temp);
> @@ -408,9 +413,11 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>          for (i = 0; i < len; i++) {
>              if (bitmap[i] != 0) {
>                  c = leul_to_cpu(bitmap[i]);
> +                nbits = ctpopl(c);
>                  if (unlikely(global_dirty_tracking & GLOBAL_DIRTY_DIRTY_RATE)) {
> -                    total_dirty_pages += ctpopl(c);
> +                    total_dirty_pages += nbits;
>                  }
> +                num_dirty += nbits;
>                  do {
>                      j = ctzl(c);
>                      c &= ~(1ul << j);
> @@ -423,6 +430,8 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>              }
>          }
>      }
> +
> +    return num_dirty;
>  }
>  #endif /* not _WIN32 */
>  
> -- 
> 2.31.1
> 

-- 
Peter Xu