[PATCH v6 0/2] support dirtyrate measurement with dirty bitmap

[PATCH v6 0/2] support dirtyrate measurement with dirty bitmap

[huangy81]

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

v6:
- pre-check if dirty tracking for dirtyrate is running
  before stating the dirty pages

v5:
- let recording dirty pages after memory_global_dirty_log_sync
  make dirtyrate result more accurate

v4:
- drop the first commit:
  "KVM: introduce kvm_get_manual_dirty_log_protect"

- clear dirty log unconditionally so that the first commit
  can be dropped.

- rename global var DirtyRateDirtyPages to total_dirty_pages

- stat the dirty pages along with the existing loop in
  cpu_physical_memory_set_dirty_lebitmap

- stat the increased dirty pages like the way of dirty-ring

- add BQL when fetch dirty log and clear dirty log

- do not clear dirty log after measuring.

v4 implementation clear log unconditionally so that the
kvm_get_manual_dirty_log_protect can be dropped and so do the
first commit.

other main modification is add the BQL when fetch and clear
dirty log.

last modification is do not clear dirty log after measuring.
if dirty tracking be stopped after measuring, clear dirty
log make no sense.if dirty tracking is running after mesauring,
clear dirty log can be handled by the caller who is interested in it.

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(黄勇) (2):
  memory: introduce total_dirty_pages to stat dirty pages
  migration/dirtyrate: implement dirty-bitmap dirtyrate calculation

 hmp-commands.hx         |   9 ++--
 include/exec/ram_addr.h |   9 ++++
 migration/dirtyrate.c   | 122 ++++++++++++++++++++++++++++++++++++++++++++----
 qapi/migration.json     |   6 ++-
 4 files changed, 131 insertions(+), 15 deletions(-)

-- 
1.8.3.1

[PATCH v6 1/2] memory: introduce total_dirty_pages to stat dirty pages

[huangy81]

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

introduce global var total_dirty_pages to stat dirty pages
along with memory_global_dirty_log_sync.

Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
---
 include/exec/ram_addr.h | 9 +++++++++
 migration/dirtyrate.c   | 7 +++++++
 2 files changed, 16 insertions(+)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 45c9132..64fb936 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 total_dirty_pages;
+
 /**
  * clear_bmap_size: calculate clear bitmap size
  *
@@ -373,6 +375,10 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
                         qatomic_or(
                                 &blocks[DIRTY_MEMORY_MIGRATION][idx][offset],
                                 temp);
+                        if (unlikely(
+                            global_dirty_tracking & GLOBAL_DIRTY_DIRTY_RATE)) {
+                            total_dirty_pages += ctpopl(temp);
+                        }
                     }
 
                     if (tcg_enabled()) {
@@ -403,6 +409,9 @@ 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]);
+                if (unlikely(global_dirty_tracking & GLOBAL_DIRTY_DIRTY_RATE)) {
+                    total_dirty_pages += ctpopl(c);
+                }
                 do {
                     j = ctzl(c);
                     c &= ~(1ul << j);
diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index f92c4b4..17b3d2c 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -28,6 +28,13 @@
 #include "sysemu/runstate.h"
 #include "exec/memory.h"
 
+/*
+ * total_dirty_pages is procted by BQL and is used
+ * to stat dirty pages during the period of two
+ * memory_global_dirty_log_sync
+ */
+uint64_t total_dirty_pages;
+
 typedef struct DirtyPageRecord {
     uint64_t start_pages;
     uint64_t end_pages;
-- 
1.8.3.1

[PATCH v6 2/2] 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 | 115 ++++++++++++++++++++++++++++++++++++++++++++++----
 qapi/migration.json   |   6 ++-
 3 files changed, 115 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 17b3d2c..f9e4c03 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"
@@ -118,6 +119,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));
@@ -416,6 +421,13 @@ static void dirtyrate_global_dirty_log_stop(void)
     qemu_mutex_unlock_iothread();
 }
 
+static void dirtyrate_global_dirty_log_sync(void)
+{
+    qemu_mutex_lock_iothread();
+    memory_global_dirty_log_sync();
+    qemu_mutex_unlock_iothread();
+}
+
 static int64_t do_calculate_dirtyrate_vcpu(DirtyPageRecord dirty_pages)
 {
     uint64_t memory_size_MB;
@@ -429,6 +441,75 @@ static int64_t do_calculate_dirtyrate_vcpu(DirtyPageRecord dirty_pages)
     return memory_size_MB / time_s;
 }
 
+static inline void record_dirtypages_bitmap(DirtyPageRecord *dirty_pages,
+                                            bool start)
+{
+    if (start) {
+        dirty_pages->start_pages = total_dirty_pages;
+    } else {
+        dirty_pages->end_pages = total_dirty_pages;
+    }
+}
+
+static void do_calculate_dirtyrate_bitmap(DirtyPageRecord dirty_pages)
+{
+    DirtyStat.dirty_rate = do_calculate_dirtyrate_vcpu(dirty_pages);
+}
+
+static inline void dirtyrate_manual_reset_protect(void)
+{
+    RAMBlock *block = NULL;
+
+    qemu_mutex_lock_iothread();
+    WITH_RCU_READ_LOCK_GUARD() {
+        RAMBLOCK_FOREACH_MIGRATABLE(block) {
+            memory_region_clear_dirty_bitmap(block->mr, 0,
+                                             block->used_length);
+        }
+    }
+    qemu_mutex_unlock_iothread();
+}
+
+static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
+{
+    int64_t msec = 0;
+    int64_t start_time;
+    DirtyPageRecord dirty_pages;
+
+    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
+     */
+    dirtyrate_global_dirty_log_sync();
+
+    /*
+     * reset page protect manually and unconditionally.
+     * this make sure kvm dirty log be cleared if
+     * KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE cap is enabled.
+     */
+    dirtyrate_manual_reset_protect();
+
+    record_dirtypages_bitmap(&dirty_pages, true);
+
+    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 and stop dirty tracking */
+    dirtyrate_global_dirty_log_stop();
+
+    record_dirtypages_bitmap(&dirty_pages, false);
+
+    do_calculate_dirtyrate_bitmap(dirty_pages);
+}
+
 static void calculate_dirtyrate_dirty_ring(struct DirtyRateConfig config)
 {
     CPUState *cpu;
@@ -514,7 +595,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);
@@ -597,12 +680,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;
     }
 
     /*
@@ -678,9 +764,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) {
@@ -688,6 +773,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/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 v6 2/2] migration/dirtyrate: implement dirty-bitmap dirtyrate calculation

[Peter Xu]

On Fri, Jul 16, 2021 at 07:13:47PM +0800, huangy81@chinatelecom.cn wrote:
> +static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
> +{
> +    int64_t msec = 0;
> +    int64_t start_time;
> +    DirtyPageRecord dirty_pages;

[1]

> +
> +    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
> +     */
> +    dirtyrate_global_dirty_log_sync();
> +
> +    /*
> +     * reset page protect manually and unconditionally.
> +     * this make sure kvm dirty log be cleared if
> +     * KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE cap is enabled.
> +     */
> +    dirtyrate_manual_reset_protect();
> +

[2]

> +    record_dirtypages_bitmap(&dirty_pages, true);

[3]

> +
> +    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 and stop dirty tracking */

I don't think it really fetched anything..  So I think we need:

       dirtyrate_global_dirty_log_sync();

It seems to be there in older versions but not in the latest two versions.

Please still remember to smoke the patches before posting, because without the
log sync we'll read nothing.

> +    dirtyrate_global_dirty_log_stop();
> +
> +    record_dirtypages_bitmap(&dirty_pages, false);

[4]

I think it's easier we take bql at [1]/[3] and release at [2]/[4], rather than
taking it for every function.  Then we can move the bql operations out of
dirtyrate_global_dirty_log_stop() in this patch.

Thanks,

> +
> +    do_calculate_dirtyrate_bitmap(dirty_pages);
> +}

-- 
Peter Xu

Re: [PATCH v6 2/2] migration/dirtyrate: implement dirty-bitmap dirtyrate calculation

[Hyman]

在 2021/7/17 3:36, Peter Xu 写道:
> On Fri, Jul 16, 2021 at 07:13:47PM +0800, huangy81@chinatelecom.cn wrote:
>> +static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
>> +{
>> +    int64_t msec = 0;
>> +    int64_t start_time;
>> +    DirtyPageRecord dirty_pages;
> 
> [1]
> 
>> +
>> +    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
>> +     */
>> +    dirtyrate_global_dirty_log_sync();
>> +
>> +    /*
>> +     * reset page protect manually and unconditionally.
>> +     * this make sure kvm dirty log be cleared if
>> +     * KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE cap is enabled.
>> +     */
>> +    dirtyrate_manual_reset_protect();
>> +
> 
> [2]
> 
>> +    record_dirtypages_bitmap(&dirty_pages, true);
> 
> [3]
> 
>> +
>> +    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 and stop dirty tracking */
> 
> I don't think it really fetched anything..  So I think we need:
> 
>         dirtyrate_global_dirty_log_sync();
> 
> It seems to be there in older versions but not in the latest two versions.
yes, latest version dropped this because dirtyrate_global_dirty_log_stop 
below already do the sync before stop dirty log, which is recommended in 
patchset of "support dirtyrate at the granualrity of vcpu" and make 
dirtyrate more accurate. the older version do not consider this. :)
> 
> Please still remember to smoke the patches before posting, because without the
> log sync we'll read nothing.
> 
>> +    dirtyrate_global_dirty_log_stop();
>> +
>> +    record_dirtypages_bitmap(&dirty_pages, false);
> 
> [4]
> 
> I think it's easier we take bql at [1]/[3] and release at [2]/[4], rather than
> taking it for every function.  Then we can move the bql operations out of
> dirtyrate_global_dirty_log_stop() in this patch.
yeah, take bql at [1] and release at [2] is reasonable.
but if we try to take bql at [3], it will sleep for calculation time in 
set_sample_page_period which is configured by user, which may be a heavy 
overhead.
how about we take bql at [1] and release at [2], ingore bql at [3]/[4] 
and let it be the same as older versoin. since we only copy 
total_dirty_pages to local var in "get_dirtyrate" thread and maybe we 
don't need bql.
> 
> Thanks,
> 
>> +
>> +    do_calculate_dirtyrate_bitmap(dirty_pages);
>> +}
> 

Re: [PATCH v6 2/2] migration/dirtyrate: implement dirty-bitmap dirtyrate calculation

[Peter Xu]

On Sat, Jul 17, 2021 at 10:57:50AM +0800, Hyman wrote:
> 
> 
> 在 2021/7/17 3:36, Peter Xu 写道:
> > On Fri, Jul 16, 2021 at 07:13:47PM +0800, huangy81@chinatelecom.cn wrote:
> > > +static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
> > > +{
> > > +    int64_t msec = 0;
> > > +    int64_t start_time;
> > > +    DirtyPageRecord dirty_pages;
> > 
> > [1]
> > 
> > > +
> > > +    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
> > > +     */
> > > +    dirtyrate_global_dirty_log_sync();
> > > +
> > > +    /*
> > > +     * reset page protect manually and unconditionally.
> > > +     * this make sure kvm dirty log be cleared if
> > > +     * KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE cap is enabled.
> > > +     */
> > > +    dirtyrate_manual_reset_protect();
> > > +
> > 
> > [2]
> > 
> > > +    record_dirtypages_bitmap(&dirty_pages, true);
> > 
> > [3]
> > 
> > > +
> > > +    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 and stop dirty tracking */
> > 
> > I don't think it really fetched anything..  So I think we need:
> > 
> >         dirtyrate_global_dirty_log_sync();
> > 
> > It seems to be there in older versions but not in the latest two versions.
> yes, latest version dropped this because dirtyrate_global_dirty_log_stop
> below already do the sync before stop dirty log, which is recommended in
> patchset of "support dirtyrate at the granualrity of vcpu" and make
> dirtyrate more accurate. the older version do not consider this. :)

Oh I see.  I was still using your old code so it does not have that bit.  It's okay.

> > 
> > Please still remember to smoke the patches before posting, because without the
> > log sync we'll read nothing.
> > 
> > > +    dirtyrate_global_dirty_log_stop();
> > > +
> > > +    record_dirtypages_bitmap(&dirty_pages, false);
> > 
> > [4]
> > 
> > I think it's easier we take bql at [1]/[3] and release at [2]/[4], rather than
> > taking it for every function.  Then we can move the bql operations out of
> > dirtyrate_global_dirty_log_stop() in this patch.
> yeah, take bql at [1] and release at [2] is reasonable.
> but if we try to take bql at [3], it will sleep for calculation time in
> set_sample_page_period which is configured by user, which may be a heavy
> overhead.
> how about we take bql at [1] and release at [2], ingore bql at [3]/[4] and
> let it be the same as older versoin. since we only copy total_dirty_pages to
> local var in "get_dirtyrate" thread and maybe we don't need bql.

Sounds good, thanks.

-- 
Peter Xu