RE: [PATCH v4 09/10] Add the function of colo_bitmap_clear_diry.

[Rao, Lei]

-----Original Message-----
From: Dr. David Alan Gilbert <dgilbert@redhat.com> 
Sent: Friday, March 26, 2021 2:08 AM
To: Rao, Lei <lei.rao@intel.com>
Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com; 
jasowang@redhat.com; quintela@redhat.com; pbonzini@redhat.com; 
lukasstraub2@web.de; qemu-devel@nongnu.org
Subject: Re: [PATCH v4 09/10] Add the function of colo_bitmap_clear_diry.

* leirao (lei.rao@intel.com) wrote:
> From: "Rao, Lei" <lei.rao@intel.com>
> 
> When we use continuous dirty memory copy for flushing ram cache on 
> secondary VM, we can also clean up the bitmap of contiguous dirty page 
> memory. This also can reduce the VM stop time during checkpoint.
> 
> Signed-off-by: Lei Rao <lei.rao@intel.com>
> ---
>  migration/ram.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c index a258466..ae1e659 
> 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -855,6 +855,30 @@ unsigned long colo_bitmap_find_dirty(RAMState *rs, 
> RAMBlock *rb,
>      return first;
>  }
>  
> +/**
> + * colo_bitmap_clear_dirty:when we flush ram cache to ram, we will 
> +use
> + * continuous memory copy, so we can also clean up the bitmap of 
> +contiguous
> + * dirty memory.
> + */
> +static inline bool colo_bitmap_clear_dirty(RAMState *rs,
> +                                           RAMBlock *rb,
> +                                           unsigned long start,
> +                                           unsigned long num) {
> +    bool ret;
> +    unsigned long i = 0;
> +
> +    qemu_mutex_lock(&rs->bitmap_mutex);

Please use QEMU_LOCK_GUARD(&rs->bitmap_mutex);

Will be changed in V5. Thanks.

> +    for (i = 0; i < num; i++) {
> +        ret = test_and_clear_bit(start + i, rb->bmap);
> +        if (ret) {
> +            rs->migration_dirty_pages--;
> +        }
> +    }
> +    qemu_mutex_unlock(&rs->bitmap_mutex);
> +    return ret;

This implementation is missing the clear_bmap code that 
migration_bitmap_clear_dirty has.
I think that's necessary now.

Are we sure there's any benefit in this?

Dave

There is such a note about clear_bmap in struct RAMBlock:
"On destination side, this should always be NULL, and the variable 
`clear_bmap_shift' is meaningless."
This means that clear_bmap is always NULL on secondary VM. And for the behavior 
of flush ram cache to ram, we will always only happen on secondary VM.
So, I think the clear_bmap code is unnecessary for COLO.
As for the benefits, When the number of dirty pages from flush ram cache to ram 
is too much. it will reduce the number of locks acquired.

Lei

> +}
> +
>  static inline bool migration_bitmap_clear_dirty(RAMState *rs,
>                                                  RAMBlock *rb,
>                                                  unsigned long page) 
> @@ -3700,7 +3724,6 @@ void colo_flush_ram_cache(void)
>      void *src_host;
>      unsigned long offset = 0;
>      unsigned long num = 0;
> -    unsigned long i = 0;
>  
>      memory_global_dirty_log_sync();
>      WITH_RCU_READ_LOCK_GUARD() {
> @@ -3722,9 +3745,7 @@ void colo_flush_ram_cache(void)
>                  num = 0;
>                  block = QLIST_NEXT_RCU(block, next);
>              } else {
> -                for (i = 0; i < num; i++) {
> -                    migration_bitmap_clear_dirty(ram_state, block, offset + 
> i);
> -                }
> +                colo_bitmap_clear_dirty(ram_state, block, offset, 
> + num);
>                  dst_host = block->host
>                           + (((ram_addr_t)offset) << TARGET_PAGE_BITS);
>                  src_host = block->colo_cache
> --
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK