[PATCH v6 00/12] *** A Method for evaluating dirty page rate ***

[PATCH v6 00/12] *** A Method for evaluating dirty page rate ***

[Chuan Zheng]

v5 -> v6:
    fix coding style according to review
    use TARGET_PAGE_SIZE and TARGET_PAGE_BITS instead of self-defined macros
    return start-time and calc-time by qmp command

v4 -> v5:
    fix git apply failed due to meson-build
    add review-by for patches in v3

v3 -> v4:
    use crc32 to get hash result instead of md5
    add DirtyRateStatus to denote calculation status
    add some trace_calls to make it easier to debug
    fix some comments accroding to review

v2 -> v3:
    fix size_t compile warning
    fix codestyle checked by checkpatch.pl

v1 -> v2:
    use g_rand_new() to generate rand_buf
    move RAMBLOCK_FOREACH_MIGRATABLE into migration/ram.h
    add skip_sample_ramblock to filter sampled ramblock
    fix multi-numa vm coredump when query dirtyrate
    rename qapi interface and rename some structures and functions
    succeed to compile by appling each patch
    add test for migrating vm

Sometimes it is neccessary to evaluate dirty page rate before migration.
Users could decide whether to proceed migration based on the evaluation
in case of vm performance loss due to heavy workload.
Unlikey simulating dirtylog sync which could do harm on runnning vm,
we provide a sample-hash method to compare hash results for samping page.
In this way, it would have hardly no impact on vm performance.

Evaluate the dirtypage rate both on running and migration vm.
The VM specifications for migration are as follows:
- VM use 4-K page;
- the number of VCPU is 32;
- the total memory is 32Gigabit;
- use 'mempress' tool to pressurize VM(mempress 4096 1024);
- migration bandwidth is 1GB/s

+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
|                      |  running  |                  migrating                           |
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
| no mempress          |   4MB/s   |          8MB/s      (migrated success)               |
-------------------------------------------------------------------------------------------
| mempress 4096 1024   |  1060MB/s |     456MB/s ~ 1142MB/s (cpu throttle triggered)      |
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
| mempress 4096 4096   |  4114MB/s |     688MB/s ~ 4132MB/s (cpu throttle triggered)      |
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

Test dirtyrate by qmp command like this:
1.  virsh qemu-monitor-command [vmname] '{"execute":"calc-dirty-rate", "arguments": {"calc-time": [sleep-time]}}'; 
2.  sleep specific time which is a bit larger than sleep-time
3.  virsh qemu-monitor-command [vmname] '{"execute":"query-dirty-rate"}'

The qmp command returns like this:
{"return":{"status":"measured","dirty-rate":374,"start-time":3718293,"calc-time":1},"id":"libvirt-15"}

Further test dirtyrate by libvirt api like this:
virsh getdirtyrate [vmname] [sleep-time]

Chuan Zheng (12):
  migration/dirtyrate: setup up query-dirtyrate framwork
  migration/dirtyrate: add DirtyRateStatus to denote calculation status
  migration/dirtyrate: Add RamblockDirtyInfo to store sampled page info
  migration/dirtyrate: Add dirtyrate statistics series functions
  migration/dirtyrate: move RAMBLOCK_FOREACH_MIGRATABLE into ram.h
  migration/dirtyrate: Record hash results for each sampled page
  migration/dirtyrate: Compare page hash results for recorded sampled
    page
  migration/dirtyrate: skip sampling ramblock with size below
    MIN_RAMBLOCK_SIZE
  migration/dirtyrate: Implement set_sample_page_period() and
    get_sample_page_period()
  migration/dirtyrate: Implement calculate_dirtyrate() function
  migration/dirtyrate: Implement
    qmp_cal_dirty_rate()/qmp_get_dirty_rate() function
  migration/dirtyrate: Add trace_calls to make it easier to debug

 migration/dirtyrate.c  | 436 +++++++++++++++++++++++++++++++++++++++++++++++++
 migration/dirtyrate.h  |  70 ++++++++
 migration/meson.build  |   2 +-
 migration/ram.c        |  11 +-
 migration/ram.h        |  10 ++
 migration/trace-events |   8 +
 qapi/migration.json    |  67 ++++++++
 7 files changed, 593 insertions(+), 11 deletions(-)
 create mode 100644 migration/dirtyrate.c
 create mode 100644 migration/dirtyrate.h

-- 
1.8.3.1

[PATCH v6 01/12] migration/dirtyrate: setup up query-dirtyrate framwork

[Chuan Zheng]

Add get_dirtyrate_thread() functions to setup query-dirtyrate
framework.

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/dirtyrate.c | 38 ++++++++++++++++++++++++++++++++++++++
 migration/dirtyrate.h | 29 +++++++++++++++++++++++++++++
 migration/meson.build |  2 +-
 3 files changed, 68 insertions(+), 1 deletion(-)
 create mode 100644 migration/dirtyrate.c
 create mode 100644 migration/dirtyrate.h

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
new file mode 100644
index 0000000..44d673a
--- /dev/null
+++ b/migration/dirtyrate.c
@@ -0,0 +1,38 @@
+/*
+ * Dirtyrate implement code
+ *
+ * Copyright (c) 2017-2020 HUAWEI TECHNOLOGIES CO.,LTD.
+ *
+ * Authors:
+ *  Chuan Zheng <zhengchuan@huawei.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "cpu.h"
+#include "qemu/config-file.h"
+#include "exec/memory.h"
+#include "exec/ramblock.h"
+#include "exec/target_page.h"
+#include "qemu/rcu_queue.h"
+#include "qapi/qapi-commands-migration.h"
+#include "migration.h"
+#include "dirtyrate.h"
+
+static void calculate_dirtyrate(struct DirtyRateConfig config)
+{
+    /* todo */
+    return;
+}
+
+void *get_dirtyrate_thread(void *arg)
+{
+    struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
+
+    calculate_dirtyrate(config);
+
+    return NULL;
+}
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
new file mode 100644
index 0000000..5be9714
--- /dev/null
+++ b/migration/dirtyrate.h
@@ -0,0 +1,29 @@
+/*
+ *  Dirtyrate common functions
+ *
+ *  Copyright (c) 2020 HUAWEI TECHNOLOGIES CO., LTD.
+ *
+ *  Authors:
+ *  Chuan Zheng <zhengchuan@huawei.com>
+ *
+ *  This work is licensed under the terms of the GNU GPL, version 2 or later.
+ *  See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_MIGRATION_DIRTYRATE_H
+#define QEMU_MIGRATION_DIRTYRATE_H
+
+/*
+ * Sample 512 pages per GB as default.
+ * TODO: Make it configurable.
+ */
+#define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
+
+struct DirtyRateConfig {
+    uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
+    int64_t sample_period_seconds; /* time duration between two sampling */
+};
+
+void *get_dirtyrate_thread(void *arg);
+#endif
+
diff --git a/migration/meson.build b/migration/meson.build
index ac8ff14..b5b71c8 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -37,4 +37,4 @@ softmmu_ss.add(when: ['CONFIG_RDMA', rdma], if_true: files('rdma.c'))
 softmmu_ss.add(when: 'CONFIG_LIVE_BLOCK_MIGRATION', if_true: files('block.c'))
 softmmu_ss.add(when: 'CONFIG_ZSTD', if_true: [files('multifd-zstd.c'), zstd])
 
-specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: files('ram.c'))
+specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: files('dirtyrate.c', 'ram.c'))
-- 
1.8.3.1

[PATCH v6 02/12] migration/dirtyrate: add DirtyRateStatus to denote calculation status

[Chuan Zheng]

add DirtyRateStatus to denote calculating status.

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/dirtyrate.c | 22 ++++++++++++++++++++++
 qapi/migration.json   | 17 +++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 44d673a..035ccd1 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -22,6 +22,19 @@
 #include "migration.h"
 #include "dirtyrate.h"
 
+static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
+
+static int dirtyrate_set_state(int *state, int old_state, int new_state)
+{
+    assert(new_state < DIRTY_RATE_STATUS__MAX);
+    if (atomic_cmpxchg(state, old_state, new_state) == old_state) {
+        return 0;
+    } else {
+        return -1;
+    }
+}
+
+
 static void calculate_dirtyrate(struct DirtyRateConfig config)
 {
     /* todo */
@@ -31,8 +44,17 @@ static void calculate_dirtyrate(struct DirtyRateConfig config)
 void *get_dirtyrate_thread(void *arg)
 {
     struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
+    int ret;
+
+    ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_UNSTARTED,
+                              DIRTY_RATE_STATUS_MEASURING);
+    if (ret == -1) {
+        return NULL;
+    }
 
     calculate_dirtyrate(config);
 
+    ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_MEASURING,
+                              DIRTY_RATE_STATUS_MEASURED);
     return NULL;
 }
diff --git a/qapi/migration.json b/qapi/migration.json
index 5f6b061..061ff25 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1720,3 +1720,20 @@
 ##
 { 'event': 'UNPLUG_PRIMARY',
   'data': { 'device-id': 'str' } }
+
+##
+# @DirtyRateStatus:
+#
+# An enumeration of dirtyrate status.
+#
+# @unstarted: the dirtyrate thread has not been started.
+#
+# @measuring: the dirtyrate thread is measuring.
+#
+# @measured: the dirtyrate thread has measured and results are available.
+#
+# Since: 5.2
+#
+##
+{ 'enum': 'DirtyRateStatus',
+  'data': [ 'unstarted', 'measuring', 'measured'] }
-- 
1.8.3.1

[PATCH v6 03/12] migration/dirtyrate: Add RamblockDirtyInfo to store sampled page info

[Chuan Zheng]

Add RamblockDirtyInfo to store sampled page info of each ramblock.

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/dirtyrate.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 5be9714..479e222 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -19,11 +19,29 @@
  */
 #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
 
+/*
+ * Record ramblock idstr
+ */
+#define RAMBLOCK_INFO_MAX_LEN                     256
+
 struct DirtyRateConfig {
     uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
     int64_t sample_period_seconds; /* time duration between two sampling */
 };
 
+/*
+ * Store dirtypage info for each ramblock.
+ */
+struct RamblockDirtyInfo {
+    char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */
+    uint8_t *ramblock_addr; /* base address of ramblock we measure */
+    uint64_t ramblock_pages; /* ramblock size in TARGET_PAGE_SIZE */
+    uint64_t *sample_page_vfn; /* relative offset address for sampled page */
+    uint64_t sample_pages_count; /* count of sampled pages */
+    uint64_t sample_dirty_count; /* count of dirty pages we measure */
+    uint32_t *hash_result; /* array of hash result for sampled pages */
+};
+
 void *get_dirtyrate_thread(void *arg);
 #endif
 
-- 
1.8.3.1

[PATCH v6 04/12] migration/dirtyrate: Add dirtyrate statistics series functions

[Chuan Zheng]

Add dirtyrate statistics functions to record/update dirtyrate info.

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
---
 migration/dirtyrate.c | 32 ++++++++++++++++++++++++++++++++
 migration/dirtyrate.h | 12 ++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 035ccd1..fa7a1db 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -23,6 +23,7 @@
 #include "dirtyrate.h"
 
 static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
+static struct DirtyRateStat DirtyStat;
 
 static int dirtyrate_set_state(int *state, int old_state, int new_state)
 {
@@ -34,6 +35,37 @@ static int dirtyrate_set_state(int *state, int old_state, int new_state)
     }
 }
 
+static void reset_dirtyrate_stat(void)
+{
+    DirtyStat.total_dirty_samples = 0;
+    DirtyStat.total_sample_count = 0;
+    DirtyStat.total_block_mem_MB = 0;
+    DirtyStat.dirty_rate = 0;
+    DirtyStat.start_time = 0;
+    DirtyStat.calc_time = 0;
+}
+
+static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
+{
+    DirtyStat.total_dirty_samples += info->sample_dirty_count;
+    DirtyStat.total_sample_count += info->sample_pages_count;
+    /* size of total pages in MB */
+    DirtyStat.total_block_mem_MB += (info->ramblock_pages *
+                                     TARGET_PAGE_SIZE) >> 20;
+}
+
+static void update_dirtyrate(uint64_t msec)
+{
+    uint64_t dirtyrate;
+    uint64_t total_dirty_samples = DirtyStat.total_dirty_samples;
+    uint64_t total_sample_count = DirtyStat.total_sample_count;
+    uint64_t total_block_mem_MB = DirtyStat.total_block_mem_MB;
+
+    dirtyrate = total_dirty_samples * total_block_mem_MB *
+                1000 / (total_sample_count * msec);
+
+    DirtyStat.dirty_rate = dirtyrate;
+}
 
 static void calculate_dirtyrate(struct DirtyRateConfig config)
 {
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 479e222..a3ee305 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -42,6 +42,18 @@ struct RamblockDirtyInfo {
     uint32_t *hash_result; /* array of hash result for sampled pages */
 };
 
+/*
+ * Store calculation statistics for each measure.
+ */
+struct DirtyRateStat {
+    uint64_t total_dirty_samples; /* total dirty sampled page */
+    uint64_t total_sample_count; /* total sampled pages */
+    uint64_t total_block_mem_MB; /* size of total sampled pages in MB */
+    int64_t dirty_rate; /* dirty rate in MB/s */
+    int64_t start_time; /* calculation start time in units of second */
+    int64_t calc_time; /* time duration of two sampling in units of second */
+};
+
 void *get_dirtyrate_thread(void *arg);
 #endif
 
-- 
1.8.3.1

[PATCH v6 05/12] migration/dirtyrate: move RAMBLOCK_FOREACH_MIGRATABLE into ram.h

[Chuan Zheng]

RAMBLOCK_FOREACH_MIGRATABLE is need in dirtyrate measure,
move the existing definition up into migration/ram.h

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/dirtyrate.c |  1 +
 migration/ram.c       | 11 +----------
 migration/ram.h       | 10 ++++++++++
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index fa7a1db..35b5c69 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -20,6 +20,7 @@
 #include "qemu/rcu_queue.h"
 #include "qapi/qapi-commands-migration.h"
 #include "migration.h"
+#include "ram.h"
 #include "dirtyrate.h"
 
 static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
diff --git a/migration/ram.c b/migration/ram.c
index 76d4fee..37ef0da 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -158,21 +158,12 @@ out:
     return ret;
 }
 
-static bool ramblock_is_ignored(RAMBlock *block)
+bool ramblock_is_ignored(RAMBlock *block)
 {
     return !qemu_ram_is_migratable(block) ||
            (migrate_ignore_shared() && qemu_ram_is_shared(block));
 }
 
-/* Should be holding either ram_list.mutex, or the RCU lock. */
-#define RAMBLOCK_FOREACH_NOT_IGNORED(block)            \
-    INTERNAL_RAMBLOCK_FOREACH(block)                   \
-        if (ramblock_is_ignored(block)) {} else
-
-#define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
-    INTERNAL_RAMBLOCK_FOREACH(block)                   \
-        if (!qemu_ram_is_migratable(block)) {} else
-
 #undef RAMBLOCK_FOREACH
 
 int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque)
diff --git a/migration/ram.h b/migration/ram.h
index 2eeaacf..011e854 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -37,6 +37,16 @@ extern MigrationStats ram_counters;
 extern XBZRLECacheStats xbzrle_counters;
 extern CompressionStats compression_counters;
 
+bool ramblock_is_ignored(RAMBlock *block);
+/* Should be holding either ram_list.mutex, or the RCU lock. */
+#define RAMBLOCK_FOREACH_NOT_IGNORED(block)            \
+    INTERNAL_RAMBLOCK_FOREACH(block)                   \
+        if (ramblock_is_ignored(block)) {} else
+
+#define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
+    INTERNAL_RAMBLOCK_FOREACH(block)                   \
+        if (!qemu_ram_is_migratable(block)) {} else
+
 int xbzrle_cache_resize(int64_t new_size, Error **errp);
 uint64_t ram_bytes_remaining(void);
 uint64_t ram_bytes_total(void);
-- 
1.8.3.1

[PATCH v6 06/12] migration/dirtyrate: Record hash results for each sampled page

[Chuan Zheng]

Record hash results for each sampled page, crc32 is taken to calculate
hash results for each sampled length in TARGET_PAGE_SIZE.

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
---
 migration/dirtyrate.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 35b5c69..f4967fd 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -10,6 +10,7 @@
  * See the COPYING file in the top-level directory.
  */
 
+#include <zlib.h>
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "cpu.h"
@@ -68,6 +69,130 @@ static void update_dirtyrate(uint64_t msec)
     DirtyStat.dirty_rate = dirtyrate;
 }
 
+/*
+ * get hash result for the sampled memory with length of TARGET_PAGE_SIZE
+ * in ramblock, which starts from ramblock base address.
+ */
+static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
+                                      uint64_t vfn)
+{
+    uint32_t crc;
+
+    crc = crc32(0, (info->ramblock_addr +
+                vfn * TARGET_PAGE_SIZE), TARGET_PAGE_SIZE);
+
+    return crc;
+}
+
+static int save_ramblock_hash(struct RamblockDirtyInfo *info)
+{
+    unsigned int sample_pages_count;
+    int i;
+    GRand *rand;
+
+    sample_pages_count = info->sample_pages_count;
+
+    /* ramblock size less than one page, return success to skip this ramblock */
+    if (unlikely(info->ramblock_pages == 0 || sample_pages_count == 0)) {
+        return 0;
+    }
+
+    info->hash_result = g_try_malloc0_n(sample_pages_count,
+                                        sizeof(uint32_t));
+    if (!info->hash_result) {
+        return -1;
+    }
+
+    info->sample_page_vfn = g_try_malloc0_n(sample_pages_count,
+                                            sizeof(uint64_t));
+    if (!info->sample_page_vfn) {
+        g_free(info->hash_result);
+        return -1;
+    }
+
+    rand  = g_rand_new();
+    for (i = 0; i < sample_pages_count; i++) {
+        info->sample_page_vfn[i] = g_rand_int_range(rand, 0,
+                                                    info->ramblock_pages - 1);
+        info->hash_result[i] = get_ramblock_vfn_hash(info,
+                                                     info->sample_page_vfn[i]);
+    }
+    g_rand_free(rand);
+
+    return 0;
+}
+
+static void get_ramblock_dirty_info(RAMBlock *block,
+                                    struct RamblockDirtyInfo *info,
+                                    struct DirtyRateConfig *config)
+{
+    uint64_t sample_pages_per_gigabytes = config->sample_pages_per_gigabytes;
+
+    /* Right shift 30 bits to calc ramblock size in GB */
+    info->sample_pages_count = (qemu_ram_get_used_length(block) *
+                                sample_pages_per_gigabytes) >> 30;
+    /* Right shift TARGET_PAGE_BITS to calc page count */
+    info->ramblock_pages = qemu_ram_get_used_length(block) >>
+                           TARGET_PAGE_BITS;
+    info->ramblock_addr = qemu_ram_get_host_addr(block);
+    strcpy(info->idstr, qemu_ram_get_idstr(block));
+}
+
+static struct RamblockDirtyInfo *
+alloc_ramblock_dirty_info(int *block_index,
+                          struct RamblockDirtyInfo *block_dinfo)
+{
+    struct RamblockDirtyInfo *info = NULL;
+    int index = *block_index;
+
+    if (!block_dinfo) {
+        index = 0;
+        block_dinfo = g_try_new(struct RamblockDirtyInfo, 1);
+    } else {
+        index++;
+        block_dinfo = g_try_realloc(block_dinfo, (index + 1) *
+                                    sizeof(struct RamblockDirtyInfo));
+    }
+    if (!block_dinfo) {
+        return NULL;
+    }
+
+    info = &block_dinfo[index];
+    *block_index = index;
+    memset(info, 0, sizeof(struct RamblockDirtyInfo));
+
+    return block_dinfo;
+}
+
+static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
+                                     struct DirtyRateConfig config,
+                                     int *block_index)
+{
+    struct RamblockDirtyInfo *info = NULL;
+    struct RamblockDirtyInfo *dinfo = NULL;
+    RAMBlock *block = NULL;
+    int index = 0;
+
+    RAMBLOCK_FOREACH_MIGRATABLE(block) {
+        dinfo = alloc_ramblock_dirty_info(&index, dinfo);
+        if (dinfo == NULL) {
+            return -1;
+        }
+        info = &dinfo[index];
+        get_ramblock_dirty_info(block, info, &config);
+        if (save_ramblock_hash(info) < 0) {
+            *block_dinfo = dinfo;
+            *block_index = index;
+            return -1;
+        }
+    }
+
+    *block_dinfo = dinfo;
+    *block_index = index;
+
+    return 0;
+}
+
 static void calculate_dirtyrate(struct DirtyRateConfig config)
 {
     /* todo */
-- 
1.8.3.1

[PATCH v6 07/12] migration/dirtyrate: Compare page hash results for recorded sampled page

[Chuan Zheng]

Compare page hash results for recorded sampled page.

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
---
 migration/dirtyrate.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index f4967fd..9cc2cbb 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -193,6 +193,69 @@ static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
     return 0;
 }
 
+static void calc_page_dirty_rate(struct RamblockDirtyInfo *info)
+{
+    uint32_t crc;
+    int i;
+
+    for (i = 0; i < info->sample_pages_count; i++) {
+        crc = get_ramblock_vfn_hash(info, info->sample_page_vfn[i]);
+        if (crc != info->hash_result[i]) {
+            info->sample_dirty_count++;
+        }
+    }
+}
+
+static struct RamblockDirtyInfo *
+find_page_matched(RAMBlock *block, int count,
+                  struct RamblockDirtyInfo *infos)
+{
+    int i;
+    struct RamblockDirtyInfo *matched;
+
+    for (i = 0; i < count; i++) {
+        if (!strcmp(infos[i].idstr, qemu_ram_get_idstr(block))) {
+            break;
+        }
+    }
+
+    if (i == count) {
+        return NULL;
+    }
+
+    if (infos[i].ramblock_addr != qemu_ram_get_host_addr(block) ||
+        infos[i].ramblock_pages !=
+            (qemu_ram_get_used_length(block) >> TARGET_PAGE_BITS)) {
+        return NULL;
+    }
+
+    matched = &infos[i];
+
+    return matched;
+}
+
+static int compare_page_hash_info(struct RamblockDirtyInfo *info,
+                                  int block_index)
+{
+    struct RamblockDirtyInfo *block_dinfo = NULL;
+    RAMBlock *block = NULL;
+
+    RAMBLOCK_FOREACH_MIGRATABLE(block) {
+        block_dinfo = find_page_matched(block, block_index + 1, info);
+        if (block_dinfo == NULL) {
+            continue;
+        }
+        calc_page_dirty_rate(block_dinfo);
+        update_dirtyrate_stat(block_dinfo);
+    }
+
+    if (!DirtyStat.total_sample_count) {
+        return -1;
+    }
+
+    return 0;
+}
+
 static void calculate_dirtyrate(struct DirtyRateConfig config)
 {
     /* todo */
-- 
1.8.3.1

[PATCH v6 08/12] migration/dirtyrate: skip sampling ramblock with size below MIN_RAMBLOCK_SIZE

[Chuan Zheng]

In order to sample real RAM, skip ramblock with size below MIN_RAMBLOCK_SIZE
which is set as 128M.

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
---
 migration/dirtyrate.c | 19 +++++++++++++++++++
 migration/dirtyrate.h |  5 +++++
 2 files changed, 24 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 9cc2cbb..420fc59 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -164,6 +164,19 @@ alloc_ramblock_dirty_info(int *block_index,
     return block_dinfo;
 }
 
+static bool skip_sample_ramblock(RAMBlock *block)
+{
+    /*
+     * Consider ramblock with size larger than 128M is what we
+     * want to sample.
+     */
+    if (qemu_ram_get_used_length(block) < (MIN_RAMBLOCK_SIZE << 10)) {
+        return true;
+    }
+
+    return false;
+}
+
 static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
                                      struct DirtyRateConfig config,
                                      int *block_index)
@@ -174,6 +187,9 @@ static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
     int index = 0;
 
     RAMBLOCK_FOREACH_MIGRATABLE(block) {
+        if (skip_sample_ramblock(block)) {
+            continue;
+        }
         dinfo = alloc_ramblock_dirty_info(&index, dinfo);
         if (dinfo == NULL) {
             return -1;
@@ -241,6 +257,9 @@ static int compare_page_hash_info(struct RamblockDirtyInfo *info,
     RAMBlock *block = NULL;
 
     RAMBLOCK_FOREACH_MIGRATABLE(block) {
+        if (skip_sample_ramblock(block)) {
+            continue;
+        }
         block_dinfo = find_page_matched(block, block_index + 1, info);
         if (block_dinfo == NULL) {
             continue;
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index a3ee305..faaf9da 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -24,6 +24,11 @@
  */
 #define RAMBLOCK_INFO_MAX_LEN                     256
 
+/*
+ * Minimum RAMBlock size to sample, in megabytes.
+ */
+#define MIN_RAMBLOCK_SIZE                         128
+
 struct DirtyRateConfig {
     uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
     int64_t sample_period_seconds; /* time duration between two sampling */
-- 
1.8.3.1

[PATCH v6 09/12] migration/dirtyrate: Implement set_sample_page_period() and get_sample_page_period()

[Chuan Zheng]

Implement set_sample_page_period()/get_sample_page_period() to sleep
specific time between sample actions.

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/dirtyrate.c | 24 ++++++++++++++++++++++++
 migration/dirtyrate.h |  6 ++++++
 2 files changed, 30 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 420fc59..850126d 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -27,6 +27,30 @@
 static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
 static struct DirtyRateStat DirtyStat;
 
+static int64_t set_sample_page_period(int64_t msec, int64_t initial_time)
+{
+    int64_t current_time;
+
+    current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+    if ((current_time - initial_time) >= msec) {
+        msec = current_time - initial_time;
+    } else {
+        g_usleep((msec + initial_time - current_time) * 1000);
+    }
+
+    return msec;
+}
+
+static bool get_sample_page_period(int64_t sec)
+{
+    if (sec < MIN_FETCH_DIRTYRATE_TIME_SEC ||
+        sec > MAX_FETCH_DIRTYRATE_TIME_SEC) {
+        return false;
+    }
+
+    return true;
+}
+
 static int dirtyrate_set_state(int *state, int old_state, int new_state)
 {
     assert(new_state < DIRTY_RATE_STATUS__MAX);
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index faaf9da..8f9bc80 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -29,6 +29,12 @@
  */
 #define MIN_RAMBLOCK_SIZE                         128
 
+/*
+ * Take 1s as minimum time for calculation duration
+ */
+#define MIN_FETCH_DIRTYRATE_TIME_SEC              1
+#define MAX_FETCH_DIRTYRATE_TIME_SEC              60
+
 struct DirtyRateConfig {
     uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
     int64_t sample_period_seconds; /* time duration between two sampling */
-- 
1.8.3.1

[PATCH v6 10/12] migration/dirtyrate: Implement calculate_dirtyrate() function

[Chuan Zheng]

Implement calculate_dirtyrate() function.

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
---
 migration/dirtyrate.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 850126d..95ee23e 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -162,6 +162,21 @@ static void get_ramblock_dirty_info(RAMBlock *block,
     strcpy(info->idstr, qemu_ram_get_idstr(block));
 }
 
+static void free_ramblock_dirty_info(struct RamblockDirtyInfo *infos, int count)
+{
+    int i;
+
+    if (!infos) {
+        return;
+    }
+
+    for (i = 0; i < count; i++) {
+        g_free(infos[i].sample_page_vfn);
+        g_free(infos[i].hash_result);
+    }
+    g_free(infos);
+}
+
 static struct RamblockDirtyInfo *
 alloc_ramblock_dirty_info(int *block_index,
                           struct RamblockDirtyInfo *block_dinfo)
@@ -301,8 +316,34 @@ static int compare_page_hash_info(struct RamblockDirtyInfo *info,
 
 static void calculate_dirtyrate(struct DirtyRateConfig config)
 {
-    /* todo */
-    return;
+    struct RamblockDirtyInfo *block_dinfo = NULL;
+    int block_index = 0;
+    int64_t msec = 0;
+    int64_t initial_time;
+
+    rcu_register_thread();
+    reset_dirtyrate_stat();
+    rcu_read_lock();
+    initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+    if (record_ramblock_hash_info(&block_dinfo, config, &block_index) < 0) {
+        goto out;
+    }
+    rcu_read_unlock();
+
+    msec = config.sample_period_seconds * 1000;
+    msec = set_sample_page_period(msec, initial_time);
+
+    rcu_read_lock();
+    if (compare_page_hash_info(block_dinfo, block_index) < 0) {
+        goto out;
+    }
+
+    update_dirtyrate(msec);
+
+out:
+    rcu_read_unlock();
+    free_ramblock_dirty_info(block_dinfo, block_index + 1);
+    rcu_unregister_thread();
 }
 
 void *get_dirtyrate_thread(void *arg)
-- 
1.8.3.1

[PATCH v6 11/12] migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function

[Chuan Zheng]

Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
---
 migration/dirtyrate.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/migration.json   | 50 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 95ee23e..fa1a12d 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -61,6 +61,24 @@ static int dirtyrate_set_state(int *state, int old_state, int new_state)
     }
 }
 
+static struct DirtyRateInfo *query_dirty_rate_info(void)
+{
+    int64_t dirty_rate = DirtyStat.dirty_rate;
+    struct DirtyRateInfo *info = g_malloc0(sizeof(DirtyRateInfo));
+
+    if (CalculatingState == DIRTY_RATE_STATUS_MEASURED) {
+        info->dirty_rate = dirty_rate;
+    } else {
+        info->dirty_rate = -1;
+    }
+
+    info->status = CalculatingState;
+    info->start_time = DirtyStat.start_time;
+    info->calc_time = DirtyStat.calc_time;
+
+    return info;
+}
+
 static void reset_dirtyrate_stat(void)
 {
     DirtyStat.total_dirty_samples = 0;
@@ -332,6 +350,8 @@ static void calculate_dirtyrate(struct DirtyRateConfig config)
 
     msec = config.sample_period_seconds * 1000;
     msec = set_sample_page_period(msec, initial_time);
+    DirtyStat.start_time = initial_time / 1000;
+    DirtyStat.calc_time = msec / 1000;
 
     rcu_read_lock();
     if (compare_page_hash_info(block_dinfo, block_index) < 0) {
@@ -363,3 +383,45 @@ void *get_dirtyrate_thread(void *arg)
                               DIRTY_RATE_STATUS_MEASURED);
     return NULL;
 }
+
+void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
+{
+    static struct DirtyRateConfig config;
+    QemuThread thread;
+    int ret;
+
+    /*
+     * If the dirty rate is already being measured, don't attempt to start.
+     */
+    if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) {
+        error_setg(errp, "the dirty rate is already being measured.");
+        return;
+    }
+
+    if (!get_sample_page_period(calc_time)) {
+        error_setg(errp, "calc-time is out of range[%d, %d].",
+                         MIN_FETCH_DIRTYRATE_TIME_SEC,
+                         MAX_FETCH_DIRTYRATE_TIME_SEC);
+        return;
+    }
+
+    /*
+     * Init calculation state as unstarted.
+     */
+    ret = dirtyrate_set_state(&CalculatingState, CalculatingState,
+                              DIRTY_RATE_STATUS_UNSTARTED);
+    if (ret == -1) {
+        error_setg(errp, "init dirty rate calculation state failed.");
+        return;
+    }
+
+    config.sample_period_seconds = calc_time;
+    config.sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
+    qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
+                       (void *)&config, QEMU_THREAD_DETACHED);
+}
+
+struct DirtyRateInfo *qmp_query_dirty_rate(Error **errp)
+{
+    return query_dirty_rate_info();
+}
diff --git a/qapi/migration.json b/qapi/migration.json
index 061ff25..4b980a0 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1737,3 +1737,53 @@
 ##
 { 'enum': 'DirtyRateStatus',
   'data': [ 'unstarted', 'measuring', 'measured'] }
+
+##
+# @DirtyRateInfo:
+#
+# Information about current dirty page rate of vm.
+#
+# @dirty-rate: @dirtyrate describing the dirty page rate of vm
+#          in units of MB/s.
+#          If this field return '-1', it means querying is not
+#          start or not complete.
+#
+# @status: status containing dirtyrate query status includes
+#          'unstarted' or 'measuring' or 'measured'
+#
+# @start-time: start time in units of second for calculation
+#
+# @calc-time: time in units of second for sample dirty pages
+#
+# Since: 5.2
+#
+##
+{ 'struct': 'DirtyRateInfo',
+  'data': {'dirty-rate': 'int64',
+           'status': 'DirtyRateStatus',
+           'start-time': 'int64',
+           'calc-time': 'int64'} }
+
+##
+# @calc-dirty-rate:
+#
+# start calculating dirty page rate for vm
+#
+# @calc-time: time in units of second for sample dirty pages
+#
+# Since: 5.2
+#
+# Example:
+#   {"command": "calc-dirty-rate", "data": {"calc-time": 1} }
+#
+##
+{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64'} }
+
+##
+# @query-dirty-rate:
+#
+# query dirty page rate in units of MB/s for vm
+#
+# Since: 5.2
+##
+{ 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
-- 
1.8.3.1

[PATCH v6 12/12] migration/dirtyrate: Add trace_calls to make it easier to debug

[Chuan Zheng]

Add trace_calls to  make it easier to debug

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/dirtyrate.c  | 9 +++++++++
 migration/trace-events | 8 ++++++++
 2 files changed, 17 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index fa1a12d..2a8f4ff 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -22,6 +22,7 @@
 #include "qapi/qapi-commands-migration.h"
 #include "migration.h"
 #include "ram.h"
+#include "trace.h"
 #include "dirtyrate.h"
 
 static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
@@ -54,6 +55,7 @@ static bool get_sample_page_period(int64_t sec)
 static int dirtyrate_set_state(int *state, int old_state, int new_state)
 {
     assert(new_state < DIRTY_RATE_STATUS__MAX);
+    trace_dirtyrate_set_state(DirtyRateStatus_str(new_state));
     if (atomic_cmpxchg(state, old_state, new_state) == old_state) {
         return 0;
     } else {
@@ -76,6 +78,8 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
     info->start_time = DirtyStat.start_time;
     info->calc_time = DirtyStat.calc_time;
 
+    trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
+
     return info;
 }
 
@@ -123,6 +127,7 @@ static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
     crc = crc32(0, (info->ramblock_addr +
                 vfn * TARGET_PAGE_SIZE), TARGET_PAGE_SIZE);
 
+    trace_get_ramblock_vfn_hash(info->idstr, vfn, crc);
     return crc;
 }
 
@@ -228,6 +233,8 @@ static bool skip_sample_ramblock(RAMBlock *block)
      * want to sample.
      */
     if (qemu_ram_get_used_length(block) < (MIN_RAMBLOCK_SIZE << 10)) {
+        trace_skip_sample_ramblock(block->idstr,
+                                   qemu_ram_get_used_length(block));
         return true;
     }
 
@@ -274,6 +281,7 @@ static void calc_page_dirty_rate(struct RamblockDirtyInfo *info)
     for (i = 0; i < info->sample_pages_count; i++) {
         crc = get_ramblock_vfn_hash(info, info->sample_page_vfn[i]);
         if (crc != info->hash_result[i]) {
+            trace_calc_page_dirty_rate(info->idstr, crc, info->hash_result[i]);
             info->sample_dirty_count++;
         }
     }
@@ -299,6 +307,7 @@ find_page_matched(RAMBlock *block, int count,
     if (infos[i].ramblock_addr != qemu_ram_get_host_addr(block) ||
         infos[i].ramblock_pages !=
             (qemu_ram_get_used_length(block) >> TARGET_PAGE_BITS)) {
+        trace_find_page_matched(block->idstr);
         return NULL;
     }
 
diff --git a/migration/trace-events b/migration/trace-events
index 4ab0a50..8c2b58f 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -312,3 +312,11 @@ dirty_bitmap_load_bits_zeroes(void) ""
 dirty_bitmap_load_header(uint32_t flags) "flags 0x%x"
 dirty_bitmap_load_enter(void) ""
 dirty_bitmap_load_success(void) ""
+
+# dirtyrate.c
+dirtyrate_set_state(const char *new_state) "new state %s"
+query_dirty_rate_info(const char *new_state) "current state %s"
+get_ramblock_vfn_hash(const char *idstr, uint64_t vfn, uint32_t crc) "ramblock name: %s, vfn: %"PRIu64 ", crc: %" PRIu32
+calc_page_dirty_rate(const char *idstr, uint32_t new_crc, uint32_t old_crc) "ramblock name: %s, new crc: %" PRIu32 ", old crc: %" PRIu32
+skip_sample_ramblock(const char *idstr, uint64_t ramblock_size) "ramblock name: %s, ramblock size: %" PRIu64
+find_page_matched(const char *idstr) "ramblock %s addr or size changed"
-- 
1.8.3.1

Re: [PATCH v6 01/12] migration/dirtyrate: setup up query-dirtyrate framwork

[David Edmondson]

On Saturday, 2020-08-29 at 10:52:46 +08, Chuan Zheng wrote:

> Add get_dirtyrate_thread() functions to setup query-dirtyrate
> framework.
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Modulo the question below...

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
>  migration/dirtyrate.c | 38 ++++++++++++++++++++++++++++++++++++++
>  migration/dirtyrate.h | 29 +++++++++++++++++++++++++++++
>  migration/meson.build |  2 +-
>  3 files changed, 68 insertions(+), 1 deletion(-)
>  create mode 100644 migration/dirtyrate.c
>  create mode 100644 migration/dirtyrate.h
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> new file mode 100644
> index 0000000..44d673a
> --- /dev/null
> +++ b/migration/dirtyrate.c
> @@ -0,0 +1,38 @@
> +/*
> + * Dirtyrate implement code
> + *
> + * Copyright (c) 2017-2020 HUAWEI TECHNOLOGIES CO.,LTD.

Idle query, given that I'm not a lawyer, has this code really been
around since 2017?

> + *
> + * Authors:
> + *  Chuan Zheng <zhengchuan@huawei.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "cpu.h"
> +#include "qemu/config-file.h"
> +#include "exec/memory.h"
> +#include "exec/ramblock.h"
> +#include "exec/target_page.h"
> +#include "qemu/rcu_queue.h"
> +#include "qapi/qapi-commands-migration.h"
> +#include "migration.h"
> +#include "dirtyrate.h"
> +
> +static void calculate_dirtyrate(struct DirtyRateConfig config)
> +{
> +    /* todo */
> +    return;
> +}
> +
> +void *get_dirtyrate_thread(void *arg)
> +{
> +    struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
> +
> +    calculate_dirtyrate(config);
> +
> +    return NULL;
> +}
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> new file mode 100644
> index 0000000..5be9714
> --- /dev/null
> +++ b/migration/dirtyrate.h
> @@ -0,0 +1,29 @@
> +/*
> + *  Dirtyrate common functions
> + *
> + *  Copyright (c) 2020 HUAWEI TECHNOLOGIES CO., LTD.
> + *
> + *  Authors:
> + *  Chuan Zheng <zhengchuan@huawei.com>
> + *
> + *  This work is licensed under the terms of the GNU GPL, version 2 or later.
> + *  See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef QEMU_MIGRATION_DIRTYRATE_H
> +#define QEMU_MIGRATION_DIRTYRATE_H
> +
> +/*
> + * Sample 512 pages per GB as default.
> + * TODO: Make it configurable.
> + */
> +#define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
> +
> +struct DirtyRateConfig {
> +    uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
> +    int64_t sample_period_seconds; /* time duration between two sampling */
> +};
> +
> +void *get_dirtyrate_thread(void *arg);
> +#endif
> +
> diff --git a/migration/meson.build b/migration/meson.build
> index ac8ff14..b5b71c8 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -37,4 +37,4 @@ softmmu_ss.add(when: ['CONFIG_RDMA', rdma], if_true: files('rdma.c'))
>  softmmu_ss.add(when: 'CONFIG_LIVE_BLOCK_MIGRATION', if_true: files('block.c'))
>  softmmu_ss.add(when: 'CONFIG_ZSTD', if_true: [files('multifd-zstd.c'), zstd])
>  
> -specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: files('ram.c'))
> +specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: files('dirtyrate.c', 'ram.c'))
> -- 
> 1.8.3.1

dme.
-- 
Tonight I'm gonna bury that horse in the ground.

Re: [PATCH v6 00/12] *** A Method for evaluating dirty page rate ***

[David Edmondson]

Trying to think like a control plane developer and user (of which I am
neither) raised some questions about the overall interface provided
here. If everyone else is happy with the current interface, then I'll
shut up :-)

It seems like it should be possible to query the last measured dirty
rate at any time. In particular, it should be possible to query the
value before any rate has been measured (either returning an error, or
if that is unpalatable perhaps a result with a zero interval to indicate
"this data isn't useful"), but also *during* a subsequent measurement
period.

That is, the result of the previous measurement should always be
available on demand and a measurement becomes "current" when it
completes.

Given that we allow the caller to specify the measurement interval, some
callers might specify a long period. As only one measurement can be
taken at a time, a long running measurement rules out taking a short
measurement. That's probably okay, but does lead me to wonder whether
the API should include a mechanism allowing the cancellation of an
in-progress measurement.

dme.
-- 
I can't explain, you would not understand. This is not how I am.

Re: [PATCH v6 03/12] migration/dirtyrate: Add RamblockDirtyInfo to store sampled page info

[David Edmondson]

On Saturday, 2020-08-29 at 10:52:48 +08, Chuan Zheng wrote:

> Add RamblockDirtyInfo to store sampled page info of each ramblock.
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
>  migration/dirtyrate.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 5be9714..479e222 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -19,11 +19,29 @@
>   */
>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
>  
> +/*
> + * Record ramblock idstr
> + */
> +#define RAMBLOCK_INFO_MAX_LEN                     256
> +
>  struct DirtyRateConfig {
>      uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
>      int64_t sample_period_seconds; /* time duration between two sampling */
>  };
>  
> +/*
> + * Store dirtypage info for each ramblock.
> + */
> +struct RamblockDirtyInfo {
> +    char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */
> +    uint8_t *ramblock_addr; /* base address of ramblock we measure */
> +    uint64_t ramblock_pages; /* ramblock size in TARGET_PAGE_SIZE */
> +    uint64_t *sample_page_vfn; /* relative offset address for sampled page */
> +    uint64_t sample_pages_count; /* count of sampled pages */
> +    uint64_t sample_dirty_count; /* count of dirty pages we measure */
> +    uint32_t *hash_result; /* array of hash result for sampled pages */
> +};
> +
>  void *get_dirtyrate_thread(void *arg);
>  #endif
>  
> -- 
> 1.8.3.1

dme.
-- 
This is the time. This is the record of the time.

Re: [PATCH v6 03/12] migration/dirtyrate: Add RamblockDirtyInfo to store sampled page info

[David Edmondson]

On Saturday, 2020-08-29 at 10:52:48 +08, Chuan Zheng wrote:

> Add RamblockDirtyInfo to store sampled page info of each ramblock.
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
>  migration/dirtyrate.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 5be9714..479e222 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -19,11 +19,29 @@
>   */
>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
>  
> +/*
> + * Record ramblock idstr
> + */
> +#define RAMBLOCK_INFO_MAX_LEN                     256
> +
>  struct DirtyRateConfig {
>      uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
>      int64_t sample_period_seconds; /* time duration between two sampling */
>  };
>  
> +/*
> + * Store dirtypage info for each ramblock.
> + */
> +struct RamblockDirtyInfo {
> +    char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */
> +    uint8_t *ramblock_addr; /* base address of ramblock we measure */
> +    uint64_t ramblock_pages; /* ramblock size in TARGET_PAGE_SIZE */
> +    uint64_t *sample_page_vfn; /* relative offset address for sampled page */
> +    uint64_t sample_pages_count; /* count of sampled pages */
> +    uint64_t sample_dirty_count; /* count of dirty pages we measure */
> +    uint32_t *hash_result; /* array of hash result for sampled pages */
> +};
> +
>  void *get_dirtyrate_thread(void *arg);
>  #endif
>  
> -- 
> 1.8.3.1

dme.
-- 
You make me feel like a natural woman.

Re: [PATCH v6 05/12] migration/dirtyrate: move RAMBLOCK_FOREACH_MIGRATABLE into ram.h

[David Edmondson]

On Saturday, 2020-08-29 at 10:52:50 +08, Chuan Zheng wrote:

> RAMBLOCK_FOREACH_MIGRATABLE is need in dirtyrate measure,
> move the existing definition up into migration/ram.h
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
>  migration/dirtyrate.c |  1 +
>  migration/ram.c       | 11 +----------
>  migration/ram.h       | 10 ++++++++++
>  3 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index fa7a1db..35b5c69 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -20,6 +20,7 @@
>  #include "qemu/rcu_queue.h"
>  #include "qapi/qapi-commands-migration.h"
>  #include "migration.h"
> +#include "ram.h"
>  #include "dirtyrate.h"
>  
>  static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
> diff --git a/migration/ram.c b/migration/ram.c
> index 76d4fee..37ef0da 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -158,21 +158,12 @@ out:
>      return ret;
>  }
>  
> -static bool ramblock_is_ignored(RAMBlock *block)
> +bool ramblock_is_ignored(RAMBlock *block)
>  {
>      return !qemu_ram_is_migratable(block) ||
>             (migrate_ignore_shared() && qemu_ram_is_shared(block));
>  }
>  
> -/* Should be holding either ram_list.mutex, or the RCU lock. */
> -#define RAMBLOCK_FOREACH_NOT_IGNORED(block)            \
> -    INTERNAL_RAMBLOCK_FOREACH(block)                   \
> -        if (ramblock_is_ignored(block)) {} else
> -
> -#define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
> -    INTERNAL_RAMBLOCK_FOREACH(block)                   \
> -        if (!qemu_ram_is_migratable(block)) {} else
> -
>  #undef RAMBLOCK_FOREACH
>  
>  int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque)
> diff --git a/migration/ram.h b/migration/ram.h
> index 2eeaacf..011e854 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -37,6 +37,16 @@ extern MigrationStats ram_counters;
>  extern XBZRLECacheStats xbzrle_counters;
>  extern CompressionStats compression_counters;
>  
> +bool ramblock_is_ignored(RAMBlock *block);
> +/* Should be holding either ram_list.mutex, or the RCU lock. */
> +#define RAMBLOCK_FOREACH_NOT_IGNORED(block)            \
> +    INTERNAL_RAMBLOCK_FOREACH(block)                   \
> +        if (ramblock_is_ignored(block)) {} else
> +
> +#define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
> +    INTERNAL_RAMBLOCK_FOREACH(block)                   \
> +        if (!qemu_ram_is_migratable(block)) {} else
> +
>  int xbzrle_cache_resize(int64_t new_size, Error **errp);
>  uint64_t ram_bytes_remaining(void);
>  uint64_t ram_bytes_total(void);
> -- 
> 1.8.3.1

dme.
-- 
When you were the brightest star, who were the shadows?

Re: [PATCH v6 06/12] migration/dirtyrate: Record hash results for each sampled page

[David Edmondson]

On Saturday, 2020-08-29 at 10:52:51 +08, Chuan Zheng wrote:

> Record hash results for each sampled page, crc32 is taken to calculate
> hash results for each sampled length in TARGET_PAGE_SIZE.
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
>  migration/dirtyrate.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 125 insertions(+)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 35b5c69..f4967fd 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -10,6 +10,7 @@
>   * See the COPYING file in the top-level directory.
>   */
>  
> +#include <zlib.h>
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "cpu.h"
> @@ -68,6 +69,130 @@ static void update_dirtyrate(uint64_t msec)
>      DirtyStat.dirty_rate = dirtyrate;
>  }
>  
> +/*
> + * get hash result for the sampled memory with length of TARGET_PAGE_SIZE
> + * in ramblock, which starts from ramblock base address.
> + */
> +static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
> +                                      uint64_t vfn)
> +{
> +    uint32_t crc;
> +
> +    crc = crc32(0, (info->ramblock_addr +
> +                vfn * TARGET_PAGE_SIZE), TARGET_PAGE_SIZE);
> +
> +    return crc;
> +}
> +
> +static int save_ramblock_hash(struct RamblockDirtyInfo *info)
> +{
> +    unsigned int sample_pages_count;
> +    int i;
> +    GRand *rand;
> +
> +    sample_pages_count = info->sample_pages_count;
> +
> +    /* ramblock size less than one page, return success to skip this ramblock */
> +    if (unlikely(info->ramblock_pages == 0 || sample_pages_count == 0)) {
> +        return 0;
> +    }
> +
> +    info->hash_result = g_try_malloc0_n(sample_pages_count,
> +                                        sizeof(uint32_t));
> +    if (!info->hash_result) {
> +        return -1;
> +    }
> +
> +    info->sample_page_vfn = g_try_malloc0_n(sample_pages_count,
> +                                            sizeof(uint64_t));
> +    if (!info->sample_page_vfn) {
> +        g_free(info->hash_result);
> +        return -1;
> +    }
> +
> +    rand  = g_rand_new();
> +    for (i = 0; i < sample_pages_count; i++) {
> +        info->sample_page_vfn[i] = g_rand_int_range(rand, 0,
> +                                                    info->ramblock_pages - 1);
> +        info->hash_result[i] = get_ramblock_vfn_hash(info,
> +                                                     info->sample_page_vfn[i]);
> +    }
> +    g_rand_free(rand);
> +
> +    return 0;
> +}
> +
> +static void get_ramblock_dirty_info(RAMBlock *block,
> +                                    struct RamblockDirtyInfo *info,
> +                                    struct DirtyRateConfig *config)
> +{
> +    uint64_t sample_pages_per_gigabytes = config->sample_pages_per_gigabytes;
> +
> +    /* Right shift 30 bits to calc ramblock size in GB */
> +    info->sample_pages_count = (qemu_ram_get_used_length(block) *
> +                                sample_pages_per_gigabytes) >> 30;
> +    /* Right shift TARGET_PAGE_BITS to calc page count */
> +    info->ramblock_pages = qemu_ram_get_used_length(block) >>
> +                           TARGET_PAGE_BITS;
> +    info->ramblock_addr = qemu_ram_get_host_addr(block);
> +    strcpy(info->idstr, qemu_ram_get_idstr(block));
> +}
> +
> +static struct RamblockDirtyInfo *
> +alloc_ramblock_dirty_info(int *block_index,
> +                          struct RamblockDirtyInfo *block_dinfo)
> +{
> +    struct RamblockDirtyInfo *info = NULL;
> +    int index = *block_index;
> +
> +    if (!block_dinfo) {
> +        index = 0;
> +        block_dinfo = g_try_new(struct RamblockDirtyInfo, 1);
> +    } else {
> +        index++;
> +        block_dinfo = g_try_realloc(block_dinfo, (index + 1) *
> +                                    sizeof(struct RamblockDirtyInfo));
> +    }
> +    if (!block_dinfo) {
> +        return NULL;
> +    }
> +
> +    info = &block_dinfo[index];
> +    *block_index = index;
> +    memset(info, 0, sizeof(struct RamblockDirtyInfo));
> +
> +    return block_dinfo;
> +}
> +
> +static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
> +                                     struct DirtyRateConfig config,
> +                                     int *block_index)
> +{
> +    struct RamblockDirtyInfo *info = NULL;
> +    struct RamblockDirtyInfo *dinfo = NULL;
> +    RAMBlock *block = NULL;
> +    int index = 0;
> +
> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
> +        dinfo = alloc_ramblock_dirty_info(&index, dinfo);
> +        if (dinfo == NULL) {
> +            return -1;
> +        }
> +        info = &dinfo[index];
> +        get_ramblock_dirty_info(block, info, &config);
> +        if (save_ramblock_hash(info) < 0) {
> +            *block_dinfo = dinfo;
> +            *block_index = index;
> +            return -1;
> +        }
> +    }
> +
> +    *block_dinfo = dinfo;
> +    *block_index = index;
> +
> +    return 0;
> +}
> +
>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>  {
>      /* todo */
> -- 
> 1.8.3.1

dme.
-- 
Please don't stand so close to me.

Re: [PATCH v6 07/12] migration/dirtyrate: Compare page hash results for recorded sampled page

[David Edmondson]

On Saturday, 2020-08-29 at 10:52:52 +08, Chuan Zheng wrote:

> Compare page hash results for recorded sampled page.
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> ---
>  migration/dirtyrate.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index f4967fd..9cc2cbb 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -193,6 +193,69 @@ static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
>      return 0;
>  }
>  
> +static void calc_page_dirty_rate(struct RamblockDirtyInfo *info)
> +{
> +    uint32_t crc;
> +    int i;
> +
> +    for (i = 0; i < info->sample_pages_count; i++) {
> +        crc = get_ramblock_vfn_hash(info, info->sample_page_vfn[i]);
> +        if (crc != info->hash_result[i]) {
> +            info->sample_dirty_count++;
> +        }
> +    }
> +}
> +
> +static struct RamblockDirtyInfo *
> +find_page_matched(RAMBlock *block, int count,
> +                  struct RamblockDirtyInfo *infos)
> +{
> +    int i;
> +    struct RamblockDirtyInfo *matched;
> +
> +    for (i = 0; i < count; i++) {
> +        if (!strcmp(infos[i].idstr, qemu_ram_get_idstr(block))) {
> +            break;
> +        }
> +    }
> +
> +    if (i == count) {
> +        return NULL;
> +    }
> +
> +    if (infos[i].ramblock_addr != qemu_ram_get_host_addr(block) ||
> +        infos[i].ramblock_pages !=
> +            (qemu_ram_get_used_length(block) >> TARGET_PAGE_BITS)) {
> +        return NULL;
> +    }
> +
> +    matched = &infos[i];
> +
> +    return matched;
> +}
> +
> +static int compare_page_hash_info(struct RamblockDirtyInfo *info,
> +                                  int block_index)
> +{
> +    struct RamblockDirtyInfo *block_dinfo = NULL;
> +    RAMBlock *block = NULL;
> +
> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
> +        block_dinfo = find_page_matched(block, block_index + 1, info);
> +        if (block_dinfo == NULL) {
> +            continue;
> +        }
> +        calc_page_dirty_rate(block_dinfo);
> +        update_dirtyrate_stat(block_dinfo);
> +    }
> +
> +    if (!DirtyStat.total_sample_count) {

total_sample_count isn't a boolean or pointer - comparing against 0
would be clearer.

> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>  {
>      /* todo */
> -- 
> 1.8.3.1

dme.
-- 
You can't hide from the flipside.

Re: [PATCH v6 08/12] migration/dirtyrate: skip sampling ramblock with size below MIN_RAMBLOCK_SIZE

[David Edmondson]

On Saturday, 2020-08-29 at 10:52:53 +08, Chuan Zheng wrote:

> In order to sample real RAM, skip ramblock with size below MIN_RAMBLOCK_SIZE
> which is set as 128M.
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>

Minor wordsmithing below, but...

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
>  migration/dirtyrate.c | 19 +++++++++++++++++++
>  migration/dirtyrate.h |  5 +++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 9cc2cbb..420fc59 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -164,6 +164,19 @@ alloc_ramblock_dirty_info(int *block_index,
>      return block_dinfo;
>  }
>  
> +static bool skip_sample_ramblock(RAMBlock *block)
> +{
> +    /*
> +     * Consider ramblock with size larger than 128M is what we
> +     * want to sample.

"Sample only blocks larger than MIN_RAMBLOCK_SIZE."

> +     */
> +    if (qemu_ram_get_used_length(block) < (MIN_RAMBLOCK_SIZE << 10)) {
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
>  static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
>                                       struct DirtyRateConfig config,
>                                       int *block_index)
> @@ -174,6 +187,9 @@ static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
>      int index = 0;
>  
>      RAMBLOCK_FOREACH_MIGRATABLE(block) {
> +        if (skip_sample_ramblock(block)) {
> +            continue;
> +        }
>          dinfo = alloc_ramblock_dirty_info(&index, dinfo);
>          if (dinfo == NULL) {
>              return -1;
> @@ -241,6 +257,9 @@ static int compare_page_hash_info(struct RamblockDirtyInfo *info,
>      RAMBlock *block = NULL;
>  
>      RAMBLOCK_FOREACH_MIGRATABLE(block) {
> +        if (skip_sample_ramblock(block)) {
> +            continue;
> +        }
>          block_dinfo = find_page_matched(block, block_index + 1, info);
>          if (block_dinfo == NULL) {
>              continue;
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index a3ee305..faaf9da 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -24,6 +24,11 @@
>   */
>  #define RAMBLOCK_INFO_MAX_LEN                     256
>  
> +/*
> + * Minimum RAMBlock size to sample, in megabytes.
> + */
> +#define MIN_RAMBLOCK_SIZE                         128
> +
>  struct DirtyRateConfig {
>      uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
>      int64_t sample_period_seconds; /* time duration between two sampling */
> -- 
> 1.8.3.1

dme.
-- 
Jane was in her turtle neck, I was much happier then.

Re: [PATCH v6 09/12] migration/dirtyrate: Implement set_sample_page_period() and get_sample_page_period()

[David Edmondson]

On Saturday, 2020-08-29 at 10:52:54 +08, Chuan Zheng wrote:

> Implement set_sample_page_period()/get_sample_page_period() to sleep
> specific time between sample actions.
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
>  migration/dirtyrate.c | 24 ++++++++++++++++++++++++
>  migration/dirtyrate.h |  6 ++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 420fc59..850126d 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -27,6 +27,30 @@
>  static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
>  static struct DirtyRateStat DirtyStat;
>  
> +static int64_t set_sample_page_period(int64_t msec, int64_t initial_time)
> +{
> +    int64_t current_time;
> +
> +    current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    if ((current_time - initial_time) >= msec) {
> +        msec = current_time - initial_time;
> +    } else {
> +        g_usleep((msec + initial_time - current_time) * 1000);
> +    }
> +
> +    return msec;
> +}
> +
> +static bool get_sample_page_period(int64_t sec)
> +{
> +    if (sec < MIN_FETCH_DIRTYRATE_TIME_SEC ||
> +        sec > MAX_FETCH_DIRTYRATE_TIME_SEC) {
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>  static int dirtyrate_set_state(int *state, int old_state, int new_state)
>  {
>      assert(new_state < DIRTY_RATE_STATUS__MAX);
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index faaf9da..8f9bc80 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -29,6 +29,12 @@
>   */
>  #define MIN_RAMBLOCK_SIZE                         128
>  
> +/*
> + * Take 1s as minimum time for calculation duration
> + */
> +#define MIN_FETCH_DIRTYRATE_TIME_SEC              1
> +#define MAX_FETCH_DIRTYRATE_TIME_SEC              60
> +
>  struct DirtyRateConfig {
>      uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
>      int64_t sample_period_seconds; /* time duration between two sampling */
> -- 
> 1.8.3.1

dme.
-- 
Tonight I think I'll walk alone, I'll find my soul as I go home.

Re: [PATCH v6 10/12] migration/dirtyrate: Implement calculate_dirtyrate() function

[David Edmondson]

On Saturday, 2020-08-29 at 10:52:55 +08, Chuan Zheng wrote:

> Implement calculate_dirtyrate() function.
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> ---
>  migration/dirtyrate.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 850126d..95ee23e 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -162,6 +162,21 @@ static void get_ramblock_dirty_info(RAMBlock *block,
>      strcpy(info->idstr, qemu_ram_get_idstr(block));
>  }
>  
> +static void free_ramblock_dirty_info(struct RamblockDirtyInfo *infos, int count)
> +{
> +    int i;
> +
> +    if (!infos) {
> +        return;
> +    }
> +
> +    for (i = 0; i < count; i++) {
> +        g_free(infos[i].sample_page_vfn);
> +        g_free(infos[i].hash_result);
> +    }
> +    g_free(infos);
> +}
> +
>  static struct RamblockDirtyInfo *
>  alloc_ramblock_dirty_info(int *block_index,
>                            struct RamblockDirtyInfo *block_dinfo)
> @@ -301,8 +316,34 @@ static int compare_page_hash_info(struct RamblockDirtyInfo *info,
>  
>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>  {
> -    /* todo */
> -    return;
> +    struct RamblockDirtyInfo *block_dinfo = NULL;
> +    int block_index = 0;
> +    int64_t msec = 0;
> +    int64_t initial_time;
> +
> +    rcu_register_thread();
> +    reset_dirtyrate_stat();
> +    rcu_read_lock();
> +    initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    if (record_ramblock_hash_info(&block_dinfo, config, &block_index) < 0) {
> +        goto out;
> +    }
> +    rcu_read_unlock();
> +
> +    msec = config.sample_period_seconds * 1000;
> +    msec = set_sample_page_period(msec, initial_time);
> +
> +    rcu_read_lock();
> +    if (compare_page_hash_info(block_dinfo, block_index) < 0) {
> +        goto out;
> +    }
> +
> +    update_dirtyrate(msec);
> +
> +out:
> +    rcu_read_unlock();

This still holds the RCU lock across update_dirtyrate(), which I
understood to be unnecessary.

> +    free_ramblock_dirty_info(block_dinfo, block_index + 1);
> +    rcu_unregister_thread();
>  }
>  
>  void *get_dirtyrate_thread(void *arg)
> -- 
> 1.8.3.1

dme.
-- 
I'd come on over but I haven't got a raincoat.

Re: [PATCH v6 12/12] migration/dirtyrate: Add trace_calls to make it easier to debug

[David Edmondson]

On Saturday, 2020-08-29 at 10:52:57 +08, Chuan Zheng wrote:

> Add trace_calls to  make it easier to debug
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
>  migration/dirtyrate.c  | 9 +++++++++
>  migration/trace-events | 8 ++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index fa1a12d..2a8f4ff 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -22,6 +22,7 @@
>  #include "qapi/qapi-commands-migration.h"
>  #include "migration.h"
>  #include "ram.h"
> +#include "trace.h"
>  #include "dirtyrate.h"
>  
>  static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
> @@ -54,6 +55,7 @@ static bool get_sample_page_period(int64_t sec)
>  static int dirtyrate_set_state(int *state, int old_state, int new_state)
>  {
>      assert(new_state < DIRTY_RATE_STATUS__MAX);
> +    trace_dirtyrate_set_state(DirtyRateStatus_str(new_state));
>      if (atomic_cmpxchg(state, old_state, new_state) == old_state) {
>          return 0;
>      } else {
> @@ -76,6 +78,8 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
>      info->start_time = DirtyStat.start_time;
>      info->calc_time = DirtyStat.calc_time;
>  
> +    trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
> +
>      return info;
>  }
>  
> @@ -123,6 +127,7 @@ static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
>      crc = crc32(0, (info->ramblock_addr +
>                  vfn * TARGET_PAGE_SIZE), TARGET_PAGE_SIZE);
>  
> +    trace_get_ramblock_vfn_hash(info->idstr, vfn, crc);
>      return crc;
>  }
>  
> @@ -228,6 +233,8 @@ static bool skip_sample_ramblock(RAMBlock *block)
>       * want to sample.
>       */
>      if (qemu_ram_get_used_length(block) < (MIN_RAMBLOCK_SIZE << 10)) {
> +        trace_skip_sample_ramblock(block->idstr,
> +                                   qemu_ram_get_used_length(block));
>          return true;
>      }
>  
> @@ -274,6 +281,7 @@ static void calc_page_dirty_rate(struct RamblockDirtyInfo *info)
>      for (i = 0; i < info->sample_pages_count; i++) {
>          crc = get_ramblock_vfn_hash(info, info->sample_page_vfn[i]);
>          if (crc != info->hash_result[i]) {
> +            trace_calc_page_dirty_rate(info->idstr, crc, info->hash_result[i]);
>              info->sample_dirty_count++;
>          }
>      }
> @@ -299,6 +307,7 @@ find_page_matched(RAMBlock *block, int count,
>      if (infos[i].ramblock_addr != qemu_ram_get_host_addr(block) ||
>          infos[i].ramblock_pages !=
>              (qemu_ram_get_used_length(block) >> TARGET_PAGE_BITS)) {
> +        trace_find_page_matched(block->idstr);
>          return NULL;
>      }
>  
> diff --git a/migration/trace-events b/migration/trace-events
> index 4ab0a50..8c2b58f 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -312,3 +312,11 @@ dirty_bitmap_load_bits_zeroes(void) ""
>  dirty_bitmap_load_header(uint32_t flags) "flags 0x%x"
>  dirty_bitmap_load_enter(void) ""
>  dirty_bitmap_load_success(void) ""
> +
> +# dirtyrate.c
> +dirtyrate_set_state(const char *new_state) "new state %s"
> +query_dirty_rate_info(const char *new_state) "current state %s"
> +get_ramblock_vfn_hash(const char *idstr, uint64_t vfn, uint32_t crc) "ramblock name: %s, vfn: %"PRIu64 ", crc: %" PRIu32
> +calc_page_dirty_rate(const char *idstr, uint32_t new_crc, uint32_t old_crc) "ramblock name: %s, new crc: %" PRIu32 ", old crc: %" PRIu32
> +skip_sample_ramblock(const char *idstr, uint64_t ramblock_size) "ramblock name: %s, ramblock size: %" PRIu64
> +find_page_matched(const char *idstr) "ramblock %s addr or size changed"
> -- 
> 1.8.3.1

dme.
-- 
All of us, we're going out tonight. We're gonna walk all over your cars.

Re: [PATCH v6 00/12] *** A Method for evaluating dirty page rate ***

[Zheng Chuan]

On 2020/8/31 17:05, David Edmondson wrote:
> Trying to think like a control plane developer and user (of which I am
> neither) raised some questions about the overall interface provided
> here. If everyone else is happy with the current interface, then I'll
> shut up :-)
> 
> It seems like it should be possible to query the last measured dirty
> rate at any time. In particular, it should be possible to query the
> value before any rate has been measured (either returning an error, or
> if that is unpalatable perhaps a result with a zero interval to indicate
> "this data isn't useful"), but also *during* a subsequent measurement
> period.
> 
Hi, Thank you for your review.

For now,
i. if we query the value before any rate has been measured, it will return unstarted,
   and dirtyrate will return -1.
{"return":{"status":"unstarted","dirty-rate":-1,"start-time":0,"calc-time":0},"id":"libvirt-14"}

ii.if we specify the measurement interval like -1 or 61, it will return error
{"id":"libvirt-13","error":{"class":"GenericError","desc":"calc-time is out of range[1, 60]."}}

iii. We can query the last measured dirty rate at any time now as you expected in last patch version
     with returning the measurement timestamp and calc-time.

If i have missed some other scenes, please let me know:)

> That is, the result of the previous measurement should always be
> available on demand and a measurement becomes "current" when it
> completes.
> 
> Given that we allow the caller to specify the measurement interval, some
> callers might specify a long period. As only one measurement can be
> taken at a time, a long running measurement rules out taking a short
> measurement. That's probably okay, but does lead me to wonder whether
> the API should include a mechanism allowing the cancellation of an
> in-progress measurement.

That's why we restrict the maximum time to 60s, i think this is enough and also
not too long for user to observe the average dirty rate.
I think it is may a little expensive and hardly used to implement cancellation mechanism.

On the other hand, users could call several times with the measurement interval
like 1s, to improve its accuracy otherwise observe a long time.



> 
> dme.
> 

Re: [PATCH v6 00/12] *** A Method for evaluating dirty page rate ***

[David Edmondson]

On Monday, 2020-08-31 at 17:55:39 +08, Zheng Chuan wrote:

> On 2020/8/31 17:05, David Edmondson wrote:
>> Trying to think like a control plane developer and user (of which I am
>> neither) raised some questions about the overall interface provided
>> here. If everyone else is happy with the current interface, then I'll
>> shut up :-)
>> 
>> It seems like it should be possible to query the last measured dirty
>> rate at any time. In particular, it should be possible to query the
>> value before any rate has been measured (either returning an error, or
>> if that is unpalatable perhaps a result with a zero interval to indicate
>> "this data isn't useful"), but also *during* a subsequent measurement
>> period.
>> 
> Hi, Thank you for your review.
>
> For now,
> i. if we query the value before any rate has been measured, it will return unstarted,
>    and dirtyrate will return -1.
> {"return":{"status":"unstarted","dirty-rate":-1,"start-time":0,"calc-time":0},"id":"libvirt-14"}
>
> ii.if we specify the measurement interval like -1 or 61, it will return error
> {"id":"libvirt-13","error":{"class":"GenericError","desc":"calc-time is out of range[1, 60]."}}
>
> iii. We can query the last measured dirty rate at any time now as you expected in last patch version
>      with returning the measurement timestamp and calc-time.
>
> If i have missed some other scenes, please let me know:)

No, I think that you have everything. My aim was to see if other people
agreed with the usage scenarios.

>> That is, the result of the previous measurement should always be
>> available on demand and a measurement becomes "current" when it
>> completes.
>> 
>> Given that we allow the caller to specify the measurement interval, some
>> callers might specify a long period. As only one measurement can be
>> taken at a time, a long running measurement rules out taking a short
>> measurement. That's probably okay, but does lead me to wonder whether
>> the API should include a mechanism allowing the cancellation of an
>> in-progress measurement.
>
> That's why we restrict the maximum time to 60s, i think this is enough and also
> not too long for user to observe the average dirty rate.
> I think it is may a little expensive and hardly used to implement cancellation mechanism.
>
> On the other hand, users could call several times with the measurement interval
> like 1s, to improve its accuracy otherwise observe a long time.

Understood.

dme.
-- 
Modern people tend to dance.

Re: [PATCH v6 01/12] migration/dirtyrate: setup up query-dirtyrate framwork

[Zheng Chuan]

On 2020/8/31 16:54, David Edmondson wrote:
> On Saturday, 2020-08-29 at 10:52:46 +08, Chuan Zheng wrote:
> 
>> Add get_dirtyrate_thread() functions to setup query-dirtyrate
>> framework.
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Modulo the question below...
> 
> Reviewed-by: David Edmondson <david.edmondson@oracle.com>
> 
>> ---
>>  migration/dirtyrate.c | 38 ++++++++++++++++++++++++++++++++++++++
>>  migration/dirtyrate.h | 29 +++++++++++++++++++++++++++++
>>  migration/meson.build |  2 +-
>>  3 files changed, 68 insertions(+), 1 deletion(-)
>>  create mode 100644 migration/dirtyrate.c
>>  create mode 100644 migration/dirtyrate.h
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> new file mode 100644
>> index 0000000..44d673a
>> --- /dev/null
>> +++ b/migration/dirtyrate.c
>> @@ -0,0 +1,38 @@
>> +/*
>> + * Dirtyrate implement code
>> + *
>> + * Copyright (c) 2017-2020 HUAWEI TECHNOLOGIES CO.,LTD.
> 
> Idle query, given that I'm not a lawyer, has this code really been
> around since 2017?
> 
We have evaluated dirty rate by simulating dirtylog as qemu migration does in our first
inside-version since 2017, but it hurts vm performace, and the hash method replaced
the old method recently.
I will consider to write in since 2020, because it is indeed just refactored recently:)

>> + *
>> + * Authors:
>> + *  Chuan Zheng <zhengchuan@huawei.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "cpu.h"
>> +#include "qemu/config-file.h"
>> +#include "exec/memory.h"
>> +#include "exec/ramblock.h"
>> +#include "exec/target_page.h"
>> +#include "qemu/rcu_queue.h"
>> +#include "qapi/qapi-commands-migration.h"
>> +#include "migration.h"
>> +#include "dirtyrate.h"
>> +
>> +static void calculate_dirtyrate(struct DirtyRateConfig config)
>> +{
>> +    /* todo */
>> +    return;
>> +}
>> +
>> +void *get_dirtyrate_thread(void *arg)
>> +{
>> +    struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
>> +
>> +    calculate_dirtyrate(config);
>> +
>> +    return NULL;
>> +}
>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>> new file mode 100644
>> index 0000000..5be9714
>> --- /dev/null
>> +++ b/migration/dirtyrate.h
>> @@ -0,0 +1,29 @@
>> +/*
>> + *  Dirtyrate common functions
>> + *
>> + *  Copyright (c) 2020 HUAWEI TECHNOLOGIES CO., LTD.
>> + *
>> + *  Authors:
>> + *  Chuan Zheng <zhengchuan@huawei.com>
>> + *
>> + *  This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + *  See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef QEMU_MIGRATION_DIRTYRATE_H
>> +#define QEMU_MIGRATION_DIRTYRATE_H
>> +
>> +/*
>> + * Sample 512 pages per GB as default.
>> + * TODO: Make it configurable.
>> + */
>> +#define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
>> +
>> +struct DirtyRateConfig {
>> +    uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
>> +    int64_t sample_period_seconds; /* time duration between two sampling */
>> +};
>> +
>> +void *get_dirtyrate_thread(void *arg);
>> +#endif
>> +
>> diff --git a/migration/meson.build b/migration/meson.build
>> index ac8ff14..b5b71c8 100644
>> --- a/migration/meson.build
>> +++ b/migration/meson.build
>> @@ -37,4 +37,4 @@ softmmu_ss.add(when: ['CONFIG_RDMA', rdma], if_true: files('rdma.c'))
>>  softmmu_ss.add(when: 'CONFIG_LIVE_BLOCK_MIGRATION', if_true: files('block.c'))
>>  softmmu_ss.add(when: 'CONFIG_ZSTD', if_true: [files('multifd-zstd.c'), zstd])
>>  
>> -specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: files('ram.c'))
>> +specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: files('dirtyrate.c', 'ram.c'))
>> -- 
>> 1.8.3.1
> 
> dme.
> 

Re: [PATCH v6 07/12] migration/dirtyrate: Compare page hash results for recorded sampled page

[Zheng Chuan]

On 2020/8/31 17:10, David Edmondson wrote:
> On Saturday, 2020-08-29 at 10:52:52 +08, Chuan Zheng wrote:
> 
>> Compare page hash results for recorded sampled page.
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>> ---
>>  migration/dirtyrate.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 63 insertions(+)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index f4967fd..9cc2cbb 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -193,6 +193,69 @@ static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
>>      return 0;
>>  }
>>  
>> +static void calc_page_dirty_rate(struct RamblockDirtyInfo *info)
>> +{
>> +    uint32_t crc;
>> +    int i;
>> +
>> +    for (i = 0; i < info->sample_pages_count; i++) {
>> +        crc = get_ramblock_vfn_hash(info, info->sample_page_vfn[i]);
>> +        if (crc != info->hash_result[i]) {
>> +            info->sample_dirty_count++;
>> +        }
>> +    }
>> +}
>> +
>> +static struct RamblockDirtyInfo *
>> +find_page_matched(RAMBlock *block, int count,
>> +                  struct RamblockDirtyInfo *infos)
>> +{
>> +    int i;
>> +    struct RamblockDirtyInfo *matched;
>> +
>> +    for (i = 0; i < count; i++) {
>> +        if (!strcmp(infos[i].idstr, qemu_ram_get_idstr(block))) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (i == count) {
>> +        return NULL;
>> +    }
>> +
>> +    if (infos[i].ramblock_addr != qemu_ram_get_host_addr(block) ||
>> +        infos[i].ramblock_pages !=
>> +            (qemu_ram_get_used_length(block) >> TARGET_PAGE_BITS)) {
>> +        return NULL;
>> +    }
>> +
>> +    matched = &infos[i];
>> +
>> +    return matched;
>> +}
>> +
>> +static int compare_page_hash_info(struct RamblockDirtyInfo *info,
>> +                                  int block_index)
>> +{
>> +    struct RamblockDirtyInfo *block_dinfo = NULL;
>> +    RAMBlock *block = NULL;
>> +
>> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
>> +        block_dinfo = find_page_matched(block, block_index + 1, info);
>> +        if (block_dinfo == NULL) {
>> +            continue;
>> +        }
>> +        calc_page_dirty_rate(block_dinfo);
>> +        update_dirtyrate_stat(block_dinfo);
>> +    }
>> +
>> +    if (!DirtyStat.total_sample_count) {
> 
> total_sample_count isn't a boolean or pointer - comparing against 0
> would be clearer.
> OK, will fix it in V7

>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>>  {
>>      /* todo */
>> -- 
>> 1.8.3.1
> 
> dme.
> 

Re: [PATCH v6 10/12] migration/dirtyrate: Implement calculate_dirtyrate() function

[Zheng Chuan]

On 2020/8/31 17:13, David Edmondson wrote:
> On Saturday, 2020-08-29 at 10:52:55 +08, Chuan Zheng wrote:
> 
>> Implement calculate_dirtyrate() function.
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>> ---
>>  migration/dirtyrate.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 43 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index 850126d..95ee23e 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -162,6 +162,21 @@ static void get_ramblock_dirty_info(RAMBlock *block,
>>      strcpy(info->idstr, qemu_ram_get_idstr(block));
>>  }
>>  
>> +static void free_ramblock_dirty_info(struct RamblockDirtyInfo *infos, int count)
>> +{
>> +    int i;
>> +
>> +    if (!infos) {
>> +        return;
>> +    }
>> +
>> +    for (i = 0; i < count; i++) {
>> +        g_free(infos[i].sample_page_vfn);
>> +        g_free(infos[i].hash_result);
>> +    }
>> +    g_free(infos);
>> +}
>> +
>>  static struct RamblockDirtyInfo *
>>  alloc_ramblock_dirty_info(int *block_index,
>>                            struct RamblockDirtyInfo *block_dinfo)
>> @@ -301,8 +316,34 @@ static int compare_page_hash_info(struct RamblockDirtyInfo *info,
>>  
>>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>>  {
>> -    /* todo */
>> -    return;
>> +    struct RamblockDirtyInfo *block_dinfo = NULL;
>> +    int block_index = 0;
>> +    int64_t msec = 0;
>> +    int64_t initial_time;
>> +
>> +    rcu_register_thread();
>> +    reset_dirtyrate_stat();
>> +    rcu_read_lock();
>> +    initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>> +    if (record_ramblock_hash_info(&block_dinfo, config, &block_index) < 0) {
>> +        goto out;
>> +    }
>> +    rcu_read_unlock();
>> +
>> +    msec = config.sample_period_seconds * 1000;
>> +    msec = set_sample_page_period(msec, initial_time);
>> +
>> +    rcu_read_lock();
>> +    if (compare_page_hash_info(block_dinfo, block_index) < 0) {
>> +        goto out;
>> +    }
>> +
>> +    update_dirtyrate(msec);
>> +
>> +out:
>> +    rcu_read_unlock();
> 
> This still holds the RCU lock across update_dirtyrate(), which I
> understood to be unnecessary.
>It does need to update_dirtyrate if we goto out when erro happens.
In order to remove update_dirtyrate out of RCU, it need to add flag
like is_need_update, like:
if (record_ramblock_hash_info(&block_dinfo, config, &block_index) < 0) {
         is_need_update = false;
         goto out;
}

if (is_need_update)
    update_dirtyrate(msec);

I doubt it is worth doing that or it will does any hurt if i holds
the RCU lock across update_dirtyrate()?


>> +    free_ramblock_dirty_info(block_dinfo, block_index + 1);
>> +    rcu_unregister_thread();
>>  }
>>  
>>  void *get_dirtyrate_thread(void *arg)
>> -- 
>> 1.8.3.1
> 
> dme.
> 

Re: [PATCH v6 10/12] migration/dirtyrate: Implement calculate_dirtyrate() function

[David Edmondson]

On Monday, 2020-08-31 at 19:24:04 +08, Zheng Chuan wrote:

> On 2020/8/31 17:13, David Edmondson wrote:
>> On Saturday, 2020-08-29 at 10:52:55 +08, Chuan Zheng wrote:
>> 
>>> Implement calculate_dirtyrate() function.
>>>
>>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>>> ---
>>>  migration/dirtyrate.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 43 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>>> index 850126d..95ee23e 100644
>>> --- a/migration/dirtyrate.c
>>> +++ b/migration/dirtyrate.c
>>> @@ -162,6 +162,21 @@ static void get_ramblock_dirty_info(RAMBlock *block,
>>>      strcpy(info->idstr, qemu_ram_get_idstr(block));
>>>  }
>>>  
>>> +static void free_ramblock_dirty_info(struct RamblockDirtyInfo *infos, int count)
>>> +{
>>> +    int i;
>>> +
>>> +    if (!infos) {
>>> +        return;
>>> +    }
>>> +
>>> +    for (i = 0; i < count; i++) {
>>> +        g_free(infos[i].sample_page_vfn);
>>> +        g_free(infos[i].hash_result);
>>> +    }
>>> +    g_free(infos);
>>> +}
>>> +
>>>  static struct RamblockDirtyInfo *
>>>  alloc_ramblock_dirty_info(int *block_index,
>>>                            struct RamblockDirtyInfo *block_dinfo)
>>> @@ -301,8 +316,34 @@ static int compare_page_hash_info(struct RamblockDirtyInfo *info,
>>>  
>>>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>>>  {
>>> -    /* todo */
>>> -    return;
>>> +    struct RamblockDirtyInfo *block_dinfo = NULL;
>>> +    int block_index = 0;
>>> +    int64_t msec = 0;
>>> +    int64_t initial_time;
>>> +
>>> +    rcu_register_thread();
>>> +    reset_dirtyrate_stat();
>>> +    rcu_read_lock();
>>> +    initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>>> +    if (record_ramblock_hash_info(&block_dinfo, config, &block_index) < 0) {
>>> +        goto out;
>>> +    }
>>> +    rcu_read_unlock();
>>> +
>>> +    msec = config.sample_period_seconds * 1000;
>>> +    msec = set_sample_page_period(msec, initial_time);
>>> +
>>> +    rcu_read_lock();
>>> +    if (compare_page_hash_info(block_dinfo, block_index) < 0) {
>>> +        goto out;
>>> +    }
>>> +
>>> +    update_dirtyrate(msec);
>>> +
>>> +out:
>>> +    rcu_read_unlock();
>> 
>> This still holds the RCU lock across update_dirtyrate(), which I
>> understood to be unnecessary.
>>It does need to update_dirtyrate if we goto out when erro happens.
> In order to remove update_dirtyrate out of RCU, it need to add flag
> like is_need_update, like:
> if (record_ramblock_hash_info(&block_dinfo, config, &block_index) < 0) {
>          is_need_update = false;
>          goto out;
> }
>
> if (is_need_update)
>     update_dirtyrate(msec);
>
> I doubt it is worth doing that or it will does any hurt if i holds
> the RCU lock across update_dirtyrate()?

Honestly I'm not sure if holding the RCU lock a little longer will be
measurable or not, perhaps someone with more experience will have a
better idea.

>
>>> +    free_ramblock_dirty_info(block_dinfo, block_index + 1);
>>> +    rcu_unregister_thread();
>>>  }
>>>  
>>>  void *get_dirtyrate_thread(void *arg)
>>> -- 
>>> 1.8.3.1
>> 
>> dme.
>> 

dme.
-- 
We wanna wait, but here we go again.