On Thu, Jul 15, 2021 at 11:51:32PM +0800, huang...@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huang...@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(黄勇) <huang...@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
