[PATCH v3 0/3] support dirtyrate measurement with dirty bitmap

[PATCH v3 0/3] support dirtyrate measurement with dirty bitmap

[huangy81]

From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>

v3:
- do not touch cpu_physical_memory_sync_dirty_bitmap

- rename global var DirtyRateIncreasedPages to DirtyRateDirtyPages 
  
- stat dirty pages in cpu_physical_memory_set_dirty_lebitmap, which
  is on the execution path of memory_global_dirty_log_sync and can
  be used for dirty rate measuring.

the v3 implemention runs well, we could get accurate dirtyrate
as v1 and simplify the implementation heavyly. it do not touch
the any ram_list.dirty_memory[*], so do has no conflict with migraion
at all. 
if migration is running at the same time with dirtyrate measuring,
measuring may reset protection of pages after
memory_global_dirty_log_sync before migration iteration, but it has
no side affect because kvm_log_clear_one_slot can guarantee that
same dirty_bmap in a slot shouldn't be cleared twice.

v2:
- drop the DIRTY_MEMORY_DIRTY_RATE dirty bits

- reuse the DIRTY_MEMORY_MIGRATION dirty bits to stat the dirty
  pages.

- introduce global var DirtyRateIncreasedPages to stat the
  increased dirty pages

- simplify the logic of calculation. skip the 1'round of
  log sync unconditionally

changes of this version are based on Peter's advice,
like the version 1, it is posted for the sake of RFC.
ideally, this patshset may be merged after the commit:
"migration/dirtyrate: implement dirty-ring dirtyrate calculation"

v1:
the dirtyrate measurement implemented by page-sampling originally, it
is not accurate in some scenarios, so we have introduced dirty-ring
based dirtyrate measurement(maybe it will be merged soon), it fix the
accuracy of page-sampling, and more importantly, it is at the
granualrity of vcpu.

dirty-ring method can be used when dirty-ring enable, as supplementary,
we introduce dirty-bitmap method to calculating dirtyrate when dirty log
enable, so that we can also get the accurate dirtyrate if needed in the
absence of dirty-ring.

three things has done to implement the measurement:
- introduce a fresh new dirty bits named DIRTY_MEMORY_DIRTY_RATE, which
  is used to store dirty bitmap after fetching it from kvm. why we do
  not reuse the existing DIRTY_MEMORY_MIGRATION dirty bits is we do not
  want to interfere with migration of and let implementation clear, this 
  is also the reason why dirty_memory be split.

  DIRTY_MEMORY_DIRTY_RATE dirty bits will be filled when
  memory_global_dirty_log_sync executed if GLOBAL_DIRTY_DIRTY_RATE bit
  be set in the global_dirty_tracking flag.

- introduce kvm_get_manual_dirty_log_protect function so that we can
  probe the protect caps of kvm when calculating.

- implement dirtyrate measurement with dirty bitmap with following step:
  1. start the dirty log. 

  2. probe the protect cap, if KVM_DIRTY_LOG_INITIALLY_SET enable, skip
     skip the 1'R and do the reset page protection manually, since kvm
     file bitmap with 1 bits if this cap is enabled. 

  3. clear the DIRTY_MEMORY_DIRTY_RATE dirty bits, prepare to store 
     the dirty bitmap.

  4. start memory_global_dirty_log_sync and fetch dirty bitmap from kvm

  5. reap the DIRTY_MEMORY_DIRTY_RATE dirty bits and do the calculation.

this patchset rebases on the commit 
"migration/dirtyrate: implement dirty-ring dirtyrate calculation",
since the above feature has not been merged, so we post this patch
for the sake of RFC. ideally, this patshset may be merged after it.

Please, review, thanks !

Best Regards !

Hyman Huang(黄勇) (3):
  KVM: introduce kvm_get_manual_dirty_log_protect
  memory: introduce DirtyRateDirtyPages and util function
  migration/dirtyrate: implement dirty-bitmap dirtyrate calculation

 accel/kvm/kvm-all.c     |   6 +++
 hmp-commands.hx         |   9 ++--
 include/exec/ram_addr.h |  19 ++++++++
 include/sysemu/kvm.h    |   1 +
 migration/dirtyrate.c   | 118 ++++++++++++++++++++++++++++++++++++++++++++----
 migration/trace-events  |   1 +
 qapi/migration.json     |   6 ++-
 7 files changed, 145 insertions(+), 15 deletions(-)

-- 
1.8.3.1

[PATCH v3 1/3] KVM: introduce kvm_get_manual_dirty_log_protect

[huangy81]

From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>

introduce kvm_get_manual_dirty_log_protect for measureing
dirtyrate via dirty bitmap. calculation of dirtyrate need
to sync dirty log and depends on the features of dirty log.

Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
---
 accel/kvm/kvm-all.c  | 6 ++++++
 include/sysemu/kvm.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index e0e88a2..f7d9ae0 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -245,6 +245,12 @@ int kvm_get_max_memslots(void)
     return s->nr_slots;
 }
 
+uint64_t kvm_get_manual_dirty_log_protect(void)
+{
+    KVMState *s = KVM_STATE(current_accel());
+    return s->manual_dirty_log_protect;
+}
+
 /* Called with KVMMemoryListener.slots_lock held */
 static KVMSlot *kvm_get_free_slot(KVMMemoryListener *kml)
 {
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 7b22aeb..b668d49 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -533,6 +533,7 @@ int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source);
 int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target);
 struct ppc_radix_page_info *kvm_get_radix_page_info(void);
 int kvm_get_max_memslots(void);
+uint64_t kvm_get_manual_dirty_log_protect(void);
 
 /* Notify resamplefd for EOI of specific interrupts. */
 void kvm_resample_fd_notify(int gsi);
-- 
1.8.3.1

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

[huangy81]

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 &&
+        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);
+            }
+        }
+    }
 }
 #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);
+}
 #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;
+
 typedef struct DirtyPageRecord {
     uint64_t start_pages;
     uint64_t end_pages;
-- 
1.8.3.1

[PATCH v3 3/3] migration/dirtyrate: implement dirty-bitmap dirtyrate calculation

[huangy81]

From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>

introduce dirty-bitmap mode as the third method of calc-dirty-rate.
implement dirty-bitmap dirtyrate calculation, which can be used
to measuring dirtyrate in the absence of dirty-ring.

introduce "dirty_bitmap:-b" option in hmp calc_dirty_rate to
indicate dirty bitmap method should be used for calculation.

Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
---
 hmp-commands.hx        |   9 ++--
 migration/dirtyrate.c  | 116 +++++++++++++++++++++++++++++++++++++++++++++----
 migration/trace-events |   1 +
 qapi/migration.json    |   6 ++-
 4 files changed, 117 insertions(+), 15 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index f7fc9d7..605973c 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1738,9 +1738,10 @@ ERST
 
     {
         .name       = "calc_dirty_rate",
-        .args_type  = "dirty_ring:-r,second:l,sample_pages_per_GB:l?",
-        .params     = "[-r] second [sample_pages_per_GB]",
-        .help       = "start a round of guest dirty rate measurement (using -d to"
-                      "\n\t\t\t specify dirty ring as the method of calculation)",
+        .args_type  = "dirty_ring:-r,dirty_bitmap:-b,second:l,sample_pages_per_GB:l?",
+        .params     = "[-r] [-b] second [sample_pages_per_GB]",
+        .help       = "start a round of guest dirty rate measurement (using -r to"
+                      "\n\t\t\t specify dirty ring as the method of calculation and"
+                      "\n\t\t\t -b to specify dirty bitmap as method of calculation)",
         .cmd        = hmp_calc_dirty_rate,
     },
diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index c465e62..8006a0d 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -15,6 +15,7 @@
 #include "qapi/error.h"
 #include "cpu.h"
 #include "exec/ramblock.h"
+#include "exec/ram_addr.h"
 #include "qemu/rcu_queue.h"
 #include "qemu/main-loop.h"
 #include "qapi/qapi-commands-migration.h"
@@ -113,6 +114,10 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
             }
             info->vcpu_dirty_rate = head;
         }
+
+        if (dirtyrate_mode == DIRTY_RATE_MEASURE_MODE_DIRTY_BITMAP) {
+            info->sample_pages = 0;
+        }
     }
 
     trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
@@ -410,6 +415,83 @@ static void dirtyrate_global_dirty_log_stop(void)
     qemu_mutex_unlock_iothread();
 }
 
+static inline void dirtyrate_dirtypages_clear(void)
+{
+    DirtyRateDirtyPages = 0;
+}
+
+static inline void dirtyrate_manual_reset_protect(void)
+{
+    RAMBlock *block = NULL;
+
+    WITH_RCU_READ_LOCK_GUARD() {
+        RAMBLOCK_FOREACH_MIGRATABLE(block) {
+            cpu_physical_memory_dirtyrate_reset_protect(block);
+        }
+    }
+}
+
+static void do_calculate_dirtyrate_bitmap(void)
+{
+    uint64_t memory_size_MB;
+    int64_t time_s;
+    uint64_t dirtyrate = 0;
+
+    memory_size_MB = (DirtyRateDirtyPages *TARGET_PAGE_SIZE) >> 20;
+    time_s = DirtyStat.calc_time;
+
+    dirtyrate = memory_size_MB / time_s;
+    DirtyStat.dirty_rate = dirtyrate;
+
+    trace_dirtyrate_do_calculate_bitmap(DirtyRateDirtyPages,
+                                        time_s, dirtyrate);
+}
+
+static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
+{
+    int64_t msec = 0;
+    int64_t start_time;
+    uint64_t protect_flags = kvm_get_manual_dirty_log_protect();
+
+    dirtyrate_global_dirty_log_start();
+
+    /*
+     * 1'round of log sync may return all 1 bits with
+     * KVM_DIRTY_LOG_INITIALLY_SET enable
+     * skip it unconditionally and start dirty tracking
+     * from 2'round of log sync
+     */
+    memory_global_dirty_log_sync();
+
+    /*
+     * reset page protect manually and
+     * start dirty tracking from now on
+     */
+    if (protect_flags & KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE) {
+        dirtyrate_manual_reset_protect();
+    }
+
+    dirtyrate_dirtypages_clear();
+
+    start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+    DirtyStat.start_time = start_time / 1000;
+
+    msec = config.sample_period_seconds * 1000;
+    msec = set_sample_page_period(msec, start_time);
+    DirtyStat.calc_time = msec / 1000;
+
+    /* fetch dirty bitmap from kvm */
+    memory_global_dirty_log_sync();
+
+    do_calculate_dirtyrate_bitmap();
+
+    if (protect_flags & KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE) {
+        dirtyrate_manual_reset_protect();
+    }
+
+    dirtyrate_global_dirty_log_stop();
+}
+
 static int64_t do_calculate_dirtyrate_vcpu(DirtyPageRecord dirty_pages)
 {
     uint64_t memory_size_MB;
@@ -508,7 +590,9 @@ out:
 
 static void calculate_dirtyrate(struct DirtyRateConfig config)
 {
-    if (config.mode == DIRTY_RATE_MEASURE_MODE_DIRTY_RING) {
+    if (config.mode == DIRTY_RATE_MEASURE_MODE_DIRTY_BITMAP) {
+        calculate_dirtyrate_dirty_bitmap(config);
+    } else if (config.mode == DIRTY_RATE_MEASURE_MODE_DIRTY_RING) {
         calculate_dirtyrate_dirty_ring(config);
     } else {
         calculate_dirtyrate_sample_vm(config);
@@ -591,12 +675,15 @@ void qmp_calc_dirty_rate(int64_t calc_time,
 
     /*
      * dirty ring mode only works when kvm dirty ring is enabled.
+     * on the contrary, dirty bitmap mode is not.
      */
-    if ((mode == DIRTY_RATE_MEASURE_MODE_DIRTY_RING) &&
-        !kvm_dirty_ring_enabled()) {
-        error_setg(errp, "dirty ring is disabled, use sample-pages method "
-                         "or remeasure later.");
-        return;
+    if (((mode == DIRTY_RATE_MEASURE_MODE_DIRTY_RING) &&
+        !kvm_dirty_ring_enabled()) ||
+        ((mode == DIRTY_RATE_MEASURE_MODE_DIRTY_BITMAP) &&
+         kvm_dirty_ring_enabled())) {
+        error_setg(errp, "mode %s is not enabled, use other method instead.",
+                         DirtyRateMeasureMode_str(mode));
+         return;
     }
 
     /*
@@ -672,9 +759,8 @@ void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict)
     int64_t sample_pages = qdict_get_try_int(qdict, "sample_pages_per_GB", -1);
     bool has_sample_pages = (sample_pages != -1);
     bool dirty_ring = qdict_get_try_bool(qdict, "dirty_ring", false);
-    DirtyRateMeasureMode mode =
-        (dirty_ring ? DIRTY_RATE_MEASURE_MODE_DIRTY_RING :
-         DIRTY_RATE_MEASURE_MODE_PAGE_SAMPLING);
+    bool dirty_bitmap = qdict_get_try_bool(qdict, "dirty_bitmap", false);
+    DirtyRateMeasureMode mode = DIRTY_RATE_MEASURE_MODE_PAGE_SAMPLING;
     Error *err = NULL;
 
     if (!sec) {
@@ -682,6 +768,18 @@ void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict)
         return;
     }
 
+    if (dirty_ring && dirty_bitmap) {
+        monitor_printf(mon, "Either dirty ring or dirty bitmap "
+                       "can be specified!\n");
+        return;
+    }
+
+    if (dirty_bitmap) {
+        mode = DIRTY_RATE_MEASURE_MODE_DIRTY_BITMAP;
+    } else if (dirty_ring) {
+        mode = DIRTY_RATE_MEASURE_MODE_DIRTY_RING;
+    }
+
     qmp_calc_dirty_rate(sec, has_sample_pages, sample_pages, true,
                         mode, &err);
     if (err) {
diff --git a/migration/trace-events b/migration/trace-events
index ce0b5e6..921f7a7 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -333,6 +333,7 @@ skip_sample_ramblock(const char *idstr, uint64_t ramblock_size) "ramblock name:
 find_page_matched(const char *idstr) "ramblock %s addr or size changed"
 dirtyrate_calculate(int64_t dirtyrate) "dirty rate: %" PRIi64 " MB/s"
 dirtyrate_do_calculate_vcpu(int idx, uint64_t rate) "vcpu[%d]: %"PRIu64 " MB/s"
+dirtyrate_do_calculate_bitmap(uint64_t pages, int64_t time, uint64_t rate) "%"PRIu64 " pages has increased in %"PRIi64 " s, rate %"PRIu64 "MB/s"
 
 # block.c
 migration_block_init_shared(const char *blk_device_name) "Start migration for %s with shared base image"
diff --git a/qapi/migration.json b/qapi/migration.json
index de35528..0b00976 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1747,13 +1747,15 @@
 #
 # @page-sampling: calculate dirtyrate by sampling pages.
 #
-# @dirty-ring: calculate dirtyrate by via dirty ring.
+# @dirty-ring: calculate dirtyrate by dirty ring.
+#
+# @dirty-bitmap: calculate dirtyrate by dirty bitmap.
 #
 # Since: 6.1
 #
 ##
 { 'enum': 'DirtyRateMeasureMode',
-  'data': ['page-sampling', 'dirty-ring'] }
+  'data': ['page-sampling', 'dirty-ring', 'dirty-bitmap'] }
 
 ##
 # @DirtyRateInfo:
-- 
1.8.3.1

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

[Peter Xu]

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

Re: [PATCH v3 3/3] migration/dirtyrate: implement dirty-bitmap dirtyrate calculation

[Peter Xu]

On Thu, Jul 15, 2021 at 11:51:33PM +0800, huangy81@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> introduce dirty-bitmap mode as the third method of calc-dirty-rate.
> implement dirty-bitmap dirtyrate calculation, which can be used
> to measuring dirtyrate in the absence of dirty-ring.
> 
> introduce "dirty_bitmap:-b" option in hmp calc_dirty_rate to
> indicate dirty bitmap method should be used for calculation.
> 
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> ---
>  hmp-commands.hx        |   9 ++--
>  migration/dirtyrate.c  | 116 +++++++++++++++++++++++++++++++++++++++++++++----
>  migration/trace-events |   1 +
>  qapi/migration.json    |   6 ++-
>  4 files changed, 117 insertions(+), 15 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index f7fc9d7..605973c 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1738,9 +1738,10 @@ ERST
>  
>      {
>          .name       = "calc_dirty_rate",
> -        .args_type  = "dirty_ring:-r,second:l,sample_pages_per_GB:l?",
> -        .params     = "[-r] second [sample_pages_per_GB]",
> -        .help       = "start a round of guest dirty rate measurement (using -d to"
> -                      "\n\t\t\t specify dirty ring as the method of calculation)",
> +        .args_type  = "dirty_ring:-r,dirty_bitmap:-b,second:l,sample_pages_per_GB:l?",
> +        .params     = "[-r] [-b] second [sample_pages_per_GB]",
> +        .help       = "start a round of guest dirty rate measurement (using -r to"
> +                      "\n\t\t\t specify dirty ring as the method of calculation and"
> +                      "\n\t\t\t -b to specify dirty bitmap as method of calculation)",
>          .cmd        = hmp_calc_dirty_rate,
>      },
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index c465e62..8006a0d 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -15,6 +15,7 @@
>  #include "qapi/error.h"
>  #include "cpu.h"
>  #include "exec/ramblock.h"
> +#include "exec/ram_addr.h"
>  #include "qemu/rcu_queue.h"
>  #include "qemu/main-loop.h"
>  #include "qapi/qapi-commands-migration.h"
> @@ -113,6 +114,10 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
>              }
>              info->vcpu_dirty_rate = head;
>          }
> +
> +        if (dirtyrate_mode == DIRTY_RATE_MEASURE_MODE_DIRTY_BITMAP) {
> +            info->sample_pages = 0;
> +        }
>      }
>  
>      trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
> @@ -410,6 +415,83 @@ static void dirtyrate_global_dirty_log_stop(void)
>      qemu_mutex_unlock_iothread();
>  }
>  
> +static inline void dirtyrate_dirtypages_clear(void)
> +{
> +    DirtyRateDirtyPages = 0;

I think this is okay; but I think it's easier we keep it increase-only, so we
do things similar to dirty ring - we record the diff before/after.

> +}
> +
> +static inline void dirtyrate_manual_reset_protect(void)
> +{
> +    RAMBlock *block = NULL;
> +
> +    WITH_RCU_READ_LOCK_GUARD() {
> +        RAMBLOCK_FOREACH_MIGRATABLE(block) {
> +            cpu_physical_memory_dirtyrate_reset_protect(block);
> +        }
> +    }
> +}
> +
> +static void do_calculate_dirtyrate_bitmap(void)
> +{
> +    uint64_t memory_size_MB;
> +    int64_t time_s;
> +    uint64_t dirtyrate = 0;
> +
> +    memory_size_MB = (DirtyRateDirtyPages *TARGET_PAGE_SIZE) >> 20;
> +    time_s = DirtyStat.calc_time;
> +
> +    dirtyrate = memory_size_MB / time_s;
> +    DirtyStat.dirty_rate = dirtyrate;
> +
> +    trace_dirtyrate_do_calculate_bitmap(DirtyRateDirtyPages,
> +                                        time_s, dirtyrate);
> +}
> +
> +static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
> +{
> +    int64_t msec = 0;
> +    int64_t start_time;
> +    uint64_t protect_flags = kvm_get_manual_dirty_log_protect();
> +
> +    dirtyrate_global_dirty_log_start();
> +
> +    /*
> +     * 1'round of log sync may return all 1 bits with
> +     * KVM_DIRTY_LOG_INITIALLY_SET enable
> +     * skip it unconditionally and start dirty tracking
> +     * from 2'round of log sync
> +     */
> +    memory_global_dirty_log_sync();

I think we need BQL for calling this.  E.g., memory_listeners can grow as we
call, also I'm not sure all ->log_sync() hooks are thread-safe.

> +
> +    /*
> +     * reset page protect manually and
> +     * start dirty tracking from now on
> +     */
> +    if (protect_flags & KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE) {
> +        dirtyrate_manual_reset_protect();

I think this needs BQL too.

Meanwhile after a second thought I think it's easier to drop patch 1 and call
dirtyrate_manual_reset_protect() unconditionally.  Say, kvm is not the only one
that may need a log clear (or say, re-protect), so checking kvm-only is not
actually a "complete" solution, because when there're more log_clear() hooks we
may want to call them too.

All modules providing log_clear() hook should be able to handle this correctly,
e.g., kvm_log_clear() can be called even without manual protect enabled, see
entry of kvm_physical_log_clear() which checks manual_dirty_log_protect.

> +    }
> +
> +    dirtyrate_dirtypages_clear();
> +
> +    start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    DirtyStat.start_time = start_time / 1000;
> +
> +    msec = config.sample_period_seconds * 1000;
> +    msec = set_sample_page_period(msec, start_time);
> +    DirtyStat.calc_time = msec / 1000;
> +
> +    /* fetch dirty bitmap from kvm */
> +    memory_global_dirty_log_sync();
> +
> +    do_calculate_dirtyrate_bitmap();
> +
> +    if (protect_flags & KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE) {
> +        dirtyrate_manual_reset_protect();
> +    }

I feel like this chunk can be dropped.

> +
> +    dirtyrate_global_dirty_log_stop();
> +}

Thanks,

-- 
Peter Xu

Re: [PATCH v3 0/3] support dirtyrate measurement with dirty bitmap

[Peter Xu]

On Thu, Jul 15, 2021 at 11:51:30PM +0800, huangy81@chinatelecom.cn wrote:
> Hyman Huang(黄勇) (3):
>   KVM: introduce kvm_get_manual_dirty_log_protect
>   memory: introduce DirtyRateDirtyPages and util function
>   migration/dirtyrate: implement dirty-bitmap dirtyrate calculation
> 
>  accel/kvm/kvm-all.c     |   6 +++
>  hmp-commands.hx         |   9 ++--
>  include/exec/ram_addr.h |  19 ++++++++
>  include/sysemu/kvm.h    |   1 +
>  migration/dirtyrate.c   | 118 ++++++++++++++++++++++++++++++++++++++++++++----
>  migration/trace-events  |   1 +
>  qapi/migration.json     |   6 ++-
>  7 files changed, 145 insertions(+), 15 deletions(-)

Yeah this diff looks much nicer. :)

I still have a few last comments here and there, please see each patch.  Thanks.

-- 
Peter Xu