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

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

[Chuan Zheng]

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"}'

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 RamlockDirtyInfo 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 get_sample_page_period() and
    block_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  | 432 +++++++++++++++++++++++++++++++++++++++++++++++++
 migration/dirtyrate.h  |  87 ++++++++++
 migration/meson.build  |   1 +
 migration/ram.c        |  11 +-
 migration/ram.h        |  10 ++
 migration/trace-events |   8 +
 qapi/migration.json    |  61 +++++++
 7 files changed, 600 insertions(+), 10 deletions(-)
 create mode 100644 migration/dirtyrate.c
 create mode 100644 migration/dirtyrate.h

-- 
1.8.3.1

[PATCH v5 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>
---
 migration/dirtyrate.c | 39 +++++++++++++++++++++++++++++++++++++++
 migration/dirtyrate.h | 32 ++++++++++++++++++++++++++++++++
 migration/meson.build |  1 +
 3 files changed, 72 insertions(+)
 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..366f4e9
--- /dev/null
+++ b/migration/dirtyrate.c
@@ -0,0 +1,39 @@
+/*
+ * 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 "crypto/hash.h"
+#include "crypto/random.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..33669b7
--- /dev/null
+++ b/migration/dirtyrate.h
@@ -0,0 +1,32 @@
+/*
+ *  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
+
+/* Take 1s as default for calculation duration */
+#define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
+
+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..30cc6c3 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -21,6 +21,7 @@ softmmu_ss.add(files(
   'channel.c',
   'colo-failover.c',
   'colo.c',
+  'dirtyrate.c',
   'exec.c',
   'fd.c',
   'global_state.c',
-- 
1.8.3.1

[PATCH v5 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>
---
 migration/dirtyrate.c | 22 ++++++++++++++++++++++
 qapi/migration.json   | 17 +++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 366f4e9..91987c5 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -23,6 +23,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 */
@@ -32,8 +45,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..d640165 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: query-dirtyrate thread is not initial.
+#
+# @measuring: query-dirtyrate thread is created and start to measure.
+#
+# @measured:  query-dirtyrate thread is end, we can get result.
+#
+# Since: 5.2
+#
+##
+{ 'enum': 'DirtyRateStatus',
+  'data': [ 'unstarted', 'measuring', 'measured'] }
-- 
1.8.3.1

[PATCH v5 03/12] migration/dirtyrate: Add RamlockDirtyInfo to store sampled page info

[Chuan Zheng]

Add RamlockDirtyInfo to store sampled page info of each ramblock.

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

diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 33669b7..70000da 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -19,6 +19,11 @@
  */
 #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
 
+/*
+ * Record ramblock idstr
+ */
+#define RAMBLOCK_INFO_MAX_LEN                     256
+
 /* Take 1s as default for calculation duration */
 #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
 
@@ -27,6 +32,19 @@ struct DirtyRateConfig {
     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 4K-page */
+    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; /* cout 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 v5 04/12] migration/dirtyrate: Add dirtyrate statistics series functions

[Chuan Zheng]

Add dirtyrate statistics to record/update dirtyrate info.

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

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 91987c5..0d7163f 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -24,6 +24,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)
 {
@@ -35,6 +36,34 @@ 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;
+}
+
+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 4K pages in MB */
+    DirtyStat.total_block_mem_MB += info->ramblock_pages / 256;
+}
+
+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 70000da..9db269d 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -45,6 +45,16 @@ 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 */
+};
+
 void *get_dirtyrate_thread(void *arg);
 #endif
 
-- 
1.8.3.1

[PATCH v5 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>
---
 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 0d7163f..f6a94d8 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -21,6 +21,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 v5 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 4K-page.

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
---
 migration/dirtyrate.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++
 migration/dirtyrate.h |  15 ++++++
 2 files changed, 151 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index f6a94d8..66de426 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 "crypto/hash.h"
@@ -66,6 +67,141 @@ static void update_dirtyrate(uint64_t msec)
     DirtyStat.dirty_rate = dirtyrate;
 }
 
+/*
+ * get hash result for the sampled memory with length of 4K byte in ramblock,
+ * which starts from ramblock base address.
+ */
+static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
+                                      uint64_t vfn)
+{
+    struct iovec iov_array;
+    uint32_t crc;
+
+    iov_array.iov_base = info->ramblock_addr +
+                         vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
+    iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;
+
+    crc = crc32(0, iov_array.iov_base, iov_array.iov_len);
+
+    return crc;
+}
+
+static int save_ramblock_hash(struct RamblockDirtyInfo *info)
+{
+    unsigned int sample_pages_count;
+    int i;
+    int ret = -1;
+    GRand *rand = g_rand_new();
+
+    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)) {
+        ret = 0;
+        goto out;
+    }
+
+    info->hash_result = g_try_malloc0_n(sample_pages_count,
+                                        sizeof(uint32_t));
+    if (!info->hash_result) {
+        ret = -1;
+        goto out;
+    }
+
+    info->sample_page_vfn = g_try_malloc0_n(sample_pages_count,
+                                            sizeof(uint64_t));
+    if (!info->sample_page_vfn) {
+        g_free(info->hash_result);
+        ret = -1;
+        goto out;
+    }
+
+    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]);
+    }
+    ret = 0;
+
+out:
+    g_rand_free(rand);
+    return ret;
+}
+
+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 block size in GB */
+    info->sample_pages_count = (qemu_ram_get_used_length(block) *
+                                sample_pages_per_gigabytes) >>
+                                DIRTYRATE_PAGE_SHIFT_GB;
+
+    /* Right shift 12 bits to calc page count in 4KB */
+    info->ramblock_pages = qemu_ram_get_used_length(block) >>
+                           DIRTYRATE_PAGE_SHIFT_KB;
+    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 */
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 9db269d..5050add 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -24,6 +24,21 @@
  */
 #define RAMBLOCK_INFO_MAX_LEN                     256
 
+/*
+ * Sample page size 4K as default.
+ */
+#define DIRTYRATE_SAMPLE_PAGE_SIZE                4096
+
+/*
+ * Sample page size 4K shift
+ */
+#define DIRTYRATE_PAGE_SHIFT_KB                   12
+
+/*
+ * Sample page size 1G shift
+ */
+#define DIRTYRATE_PAGE_SHIFT_GB                   30
+
 /* Take 1s as default for calculation duration */
 #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
 
-- 
1.8.3.1

[PATCH v5 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 | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 66de426..050270d 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -202,6 +202,70 @@ static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
     return 0;
 }
 
+static int 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++;
+        }
+    }
+
+    return 0;
+}
+
+static bool find_page_matched(RAMBlock *block, struct RamblockDirtyInfo *infos,
+                              int count, struct RamblockDirtyInfo **matched)
+{
+    int i;
+
+    for (i = 0; i < count; i++) {
+        if (!strcmp(infos[i].idstr, qemu_ram_get_idstr(block))) {
+            break;
+        }
+    }
+
+    if (i == count) {
+        return false;
+    }
+
+    if (infos[i].ramblock_addr != qemu_ram_get_host_addr(block) ||
+        infos[i].ramblock_pages !=
+            (qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SHIFT_KB)) {
+        return false;
+    }
+
+    *matched = &infos[i];
+    return true;
+}
+
+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 = NULL;
+        if (!find_page_matched(block, info, block_index + 1, &block_dinfo)) {
+            continue;
+        }
+        if (calc_page_dirty_rate(block_dinfo) < 0) {
+            return -1;
+        }
+        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 v5 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 | 24 ++++++++++++++++++++++++
 migration/dirtyrate.h | 10 ++++++++++
 2 files changed, 34 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 050270d..bd398b7 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -173,6 +173,24 @@ alloc_ramblock_dirty_info(int *block_index,
     return block_dinfo;
 }
 
+static int skip_sample_ramblock(RAMBlock *block)
+{
+    int64_t ramblock_size;
+
+    /* ramblock size in MB */
+    ramblock_size = qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SHIFT_MB;
+
+    /*
+     * Consider ramblock with size larger than 128M is what we
+     * want to sample.
+     */
+    if (ramblock_size < MIN_RAMBLOCK_SIZE) {
+        return -1;
+    }
+
+    return 0;
+}
+
 static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
                                      struct DirtyRateConfig config,
                                      int *block_index)
@@ -183,6 +201,9 @@ static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
     int index = 0;
 
     RAMBLOCK_FOREACH_MIGRATABLE(block) {
+        if (skip_sample_ramblock(block) < 0) {
+            continue;
+        }
         dinfo = alloc_ramblock_dirty_info(&index, dinfo);
         if (dinfo == NULL) {
             return -1;
@@ -249,6 +270,9 @@ static int compare_page_hash_info(struct RamblockDirtyInfo *info,
     RAMBlock *block = NULL;
 
     RAMBLOCK_FOREACH_MIGRATABLE(block) {
+        if (skip_sample_ramblock(block) < 0) {
+            continue;
+        }
         block_dinfo = NULL;
         if (!find_page_matched(block, info, block_index + 1, &block_dinfo)) {
             continue;
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 5050add..41bc264 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -35,10 +35,20 @@
 #define DIRTYRATE_PAGE_SHIFT_KB                   12
 
 /*
+ * Sample page size MB shift
+ */
+#define DIRTYRATE_PAGE_SHIFT_MB                   20
+
+/*
  * Sample page size 1G shift
  */
 #define DIRTYRATE_PAGE_SHIFT_GB                   30
 
+/*
+ * minimum ramblock size to sampled
+ */
+#define MIN_RAMBLOCK_SIZE                         128
+
 /* Take 1s as default for calculation duration */
 #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
 
-- 
1.8.3.1

[PATCH v5 09/12] migration/dirtyrate: Implement get_sample_page_period() and block_sample_page_period()

[Chuan Zheng]

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

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

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index bd398b7..d1c0a78 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -28,6 +28,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 int64_t get_sample_page_period(int64_t sec)
+{
+    if (sec <= MIN_FETCH_DIRTYRATE_TIME_SEC ||
+        sec > MAX_FETCH_DIRTYRATE_TIME_SEC) {
+        sec = DEFAULT_FETCH_DIRTYRATE_TIME_SEC;
+    }
+
+    return sec;
+}
+
 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 41bc264..50a5636 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -51,6 +51,8 @@
 
 /* Take 1s as default for calculation duration */
 #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
+#define MIN_FETCH_DIRTYRATE_TIME_SEC              0
+#define MAX_FETCH_DIRTYRATE_TIME_SEC              60
 
 struct DirtyRateConfig {
     uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
-- 
1.8.3.1

[PATCH v5 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 d1c0a78..9f52f5f 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -171,6 +171,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)
@@ -316,8 +331,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();
+    initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+    rcu_read_lock();
+    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 v5 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 | 45 +++++++++++++++++++++++++++++++++++++++++++++
 qapi/migration.json   | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 9f52f5f..08c46d3 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -62,6 +62,28 @@ 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;
+    /*
+     * Only support query once for each calculation,
+     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
+     */
+    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
+                              DIRTY_RATE_STATUS_UNSTARTED);
+
+    return info;
+}
+
 static void reset_dirtyrate_stat(void)
 {
     DirtyStat.total_dirty_samples = 0;
@@ -378,3 +400,26 @@ 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;
+
+    /*
+     * We don't begin calculating thread only when it's in calculating status.
+     */
+    if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) {
+        return;
+    }
+
+    config.sample_period_seconds = get_sample_page_period(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 d640165..826bfd7 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1737,3 +1737,47 @@
 ##
 { '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'
+#
+# Since: 5.2
+#
+##
+{ 'struct': 'DirtyRateInfo',
+  'data': {'dirty-rate': 'int64',
+           'status': 'DirtyRateStatus'} }
+
+##
+# @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": "cal-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 v5 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>
---
 migration/dirtyrate.c  | 7 +++++++
 migration/trace-events | 8 ++++++++
 2 files changed, 15 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 08c46d3..3513ef3 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -23,6 +23,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;
@@ -55,6 +56,7 @@ static int64_t 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 {
@@ -78,6 +80,7 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
      * Only support query once for each calculation,
      * reset as DIRTY_RATE_STATUS_UNSTARTED after query
      */
+    trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
     (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
                               DIRTY_RATE_STATUS_UNSTARTED);
 
@@ -129,6 +132,7 @@ static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
 
     crc = crc32(0, iov_array.iov_base, iov_array.iov_len);
 
+    trace_get_ramblock_vfn_hash(info->idstr, vfn, crc);
     return crc;
 }
 
@@ -246,6 +250,7 @@ static int skip_sample_ramblock(RAMBlock *block)
      * want to sample.
      */
     if (ramblock_size < MIN_RAMBLOCK_SIZE) {
+        trace_skip_sample_ramblock(block->idstr, ramblock_size);
         return -1;
     }
 
@@ -292,6 +297,7 @@ static int 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++;
         }
     }
@@ -317,6 +323,7 @@ static bool find_page_matched(RAMBlock *block, struct RamblockDirtyInfo *infos,
     if (infos[i].ramblock_addr != qemu_ram_get_host_addr(block) ||
         infos[i].ramblock_pages !=
             (qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SHIFT_KB)) {
+        trace_find_page_matched(block->idstr);
         return false;
     }
 
diff --git a/migration/trace-events b/migration/trace-events
index 4ab0a50..34569b9 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, int64_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 v5 00/12] *** A Method for evaluating dirty page rate ***

[no-reply]

Patchew URL: https://patchew.org/QEMU/1598260480-64862-1-git-send-email-zhengchuan@huawei.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1598260480-64862-1-git-send-email-zhengchuan@huawei.com
Subject: [PATCH v5 00/12] *** A Method for evaluating dirty page rate ***

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
   dd8014e..df82aa7  master     -> master
 - [tag update]      patchew/20200727112307.343608-1-Filip.Bozuta@syrmia.com -> patchew/20200727112307.343608-1-Filip.Bozuta@syrmia.com
 - [tag update]      patchew/20200815013145.539409-1-richard.henderson@linaro.org -> patchew/20200815013145.539409-1-richard.henderson@linaro.org
 - [tag update]      patchew/20200822212129.97758-1-r.bolshakov@yadro.com -> patchew/20200822212129.97758-1-r.bolshakov@yadro.com
 - [tag update]      patchew/20200823083215.14952-1-thuth@redhat.com -> patchew/20200823083215.14952-1-thuth@redhat.com
 - [tag update]      patchew/20200823090546.47957-1-r.bolshakov@yadro.com -> patchew/20200823090546.47957-1-r.bolshakov@yadro.com
 - [tag update]      patchew/20200824074057.3673-1-kraxel@redhat.com -> patchew/20200824074057.3673-1-kraxel@redhat.com
 - [tag update]      patchew/20200824094811.15439-1-peter.maydell@linaro.org -> patchew/20200824094811.15439-1-peter.maydell@linaro.org
 - [tag update]      patchew/20200824100041.1864420-1-edgar.iglesias@gmail.com -> patchew/20200824100041.1864420-1-edgar.iglesias@gmail.com
 * [new tag]         patchew/20200824142934.20850-1-peter.maydell@linaro.org -> patchew/20200824142934.20850-1-peter.maydell@linaro.org
 * [new tag]         patchew/20200824152430.1844159-1-laurent@vivier.eu -> patchew/20200824152430.1844159-1-laurent@vivier.eu
 * [new tag]         patchew/20200824155111.789466-1-brogers@suse.com -> patchew/20200824155111.789466-1-brogers@suse.com
 * [new tag]         patchew/20200824155212.789568-1-brogers@suse.com -> patchew/20200824155212.789568-1-brogers@suse.com
 * [new tag]         patchew/20200824155236.789635-1-brogers@suse.com -> patchew/20200824155236.789635-1-brogers@suse.com
 * [new tag]         patchew/20200824161014.401882-1-ckuehl@redhat.com -> patchew/20200824161014.401882-1-ckuehl@redhat.com
 * [new tag]         patchew/20200824163109.96938-1-berrange@redhat.com -> patchew/20200824163109.96938-1-berrange@redhat.com
Switched to a new branch 'test'
e422c48 migration/dirtyrate: Add trace_calls to make it easier to debug
d544358 migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function
b245e3e migration/dirtyrate: Implement calculate_dirtyrate() function
f634d60 migration/dirtyrate: Implement get_sample_page_period() and block_sample_page_period()
039884c migration/dirtyrate: skip sampling ramblock with size below MIN_RAMBLOCK_SIZE
b62b0f3 migration/dirtyrate: Compare page hash results for recorded sampled page
a0c77f6 migration/dirtyrate: Record hash results for each sampled page
7e4c941 migration/dirtyrate: move RAMBLOCK_FOREACH_MIGRATABLE into ram.h
5bd531b migration/dirtyrate: Add dirtyrate statistics series functions
af4f569 migration/dirtyrate: Add RamlockDirtyInfo to store sampled page info
9d2abe8 migration/dirtyrate: add DirtyRateStatus to denote calculation status
9da875e migration/dirtyrate: setup up query-dirtyrate framwork

=== OUTPUT BEGIN ===
1/12 Checking commit 9da875ef425e (migration/dirtyrate: setup up query-dirtyrate framwork)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#15: 
new file mode 100644

total: 0 errors, 1 warnings, 78 lines checked

Patch 1/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/12 Checking commit 9d2abe8b480a (migration/dirtyrate: add DirtyRateStatus to denote calculation status)
3/12 Checking commit af4f56975987 (migration/dirtyrate: Add RamlockDirtyInfo to store sampled page info)
4/12 Checking commit 5bd531bb00ce (migration/dirtyrate: Add dirtyrate statistics series functions)
5/12 Checking commit 7e4c941b32bc (migration/dirtyrate: move RAMBLOCK_FOREACH_MIGRATABLE into ram.h)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#62: FILE: migration/ram.h:42:
+#define RAMBLOCK_FOREACH_NOT_IGNORED(block)            \
+    INTERNAL_RAMBLOCK_FOREACH(block)                   \
+        if (ramblock_is_ignored(block)) {} else

ERROR: trailing statements should be on next line
#64: FILE: migration/ram.h:44:
+        if (ramblock_is_ignored(block)) {} else

ERROR: Macros with multiple statements should be enclosed in a do - while loop
#66: FILE: migration/ram.h:46:
+#define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
+    INTERNAL_RAMBLOCK_FOREACH(block)                   \
+        if (!qemu_ram_is_migratable(block)) {} else

ERROR: trailing statements should be on next line
#68: FILE: migration/ram.h:48:
+        if (!qemu_ram_is_migratable(block)) {} else

ERROR: braces {} are necessary for all arms of this statement
#68: FILE: migration/ram.h:48:
+        if (!qemu_ram_is_migratable(block)) {} else
[...]
+        if (!qemu_ram_is_migratable(block)) {} else
[...]

total: 5 errors, 0 warnings, 45 lines checked

Patch 5/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

6/12 Checking commit a0c77f6034fc (migration/dirtyrate: Record hash results for each sampled page)
7/12 Checking commit b62b0f3c7033 (migration/dirtyrate: Compare page hash results for recorded sampled page)
8/12 Checking commit 039884cf410e (migration/dirtyrate: skip sampling ramblock with size below MIN_RAMBLOCK_SIZE)
9/12 Checking commit f634d60df875 (migration/dirtyrate: Implement get_sample_page_period() and block_sample_page_period())
10/12 Checking commit b245e3e9cd0d (migration/dirtyrate: Implement calculate_dirtyrate() function)
11/12 Checking commit d544358ff2b0 (migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function)
12/12 Checking commit e422c48502c8 (migration/dirtyrate: Add trace_calls to make it easier to debug)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/1598260480-64862-1-git-send-email-zhengchuan@huawei.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[David Edmondson]

Minor wordsmithing...

On Monday, 2020-08-24 at 17:14:30 +08, Chuan Zheng wrote:

> add DirtyRateStatus to denote calculating status.
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.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 366f4e9..91987c5 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -23,6 +23,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 */
> @@ -32,8 +45,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..d640165 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: query-dirtyrate thread is not initial.

@unstarted: the dirtyrate thread has not been started.

> +#
> +# @measuring: query-dirtyrate thread is created and start to measure.

@measuring: the dirtyrate thread is measuring.

> +#
> +# @measured:  query-dirtyrate thread is end, we can get result.

@measured: the dirtyrate thread has measured and results are available.

> +#
> +# Since: 5.2
> +#
> +##
> +{ 'enum': 'DirtyRateStatus',
> +  'data': [ 'unstarted', 'measuring', 'measured'] }
> -- 
> 1.8.3.1

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

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

[David Edmondson]

On Monday, 2020-08-24 at 17:14:31 +08, Chuan Zheng wrote:

> Add RamlockDirtyInfo to store sampled page info of each ramblock.
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> ---
>  migration/dirtyrate.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 33669b7..70000da 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -19,6 +19,11 @@
>   */
>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
>  
> +/*
> + * Record ramblock idstr
> + */
> +#define RAMBLOCK_INFO_MAX_LEN                     256
> +
>  /* Take 1s as default for calculation duration */
>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>  
> @@ -27,6 +32,19 @@ struct DirtyRateConfig {
>      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 4K-page */

It's probably a stupid question, but why not store a pointer to the
RAMBlock rather than copying some of the details?

> +    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; /* cout of dirty pages we measure */

"cout" -> "count"

> +    uint32_t *hash_result; /* array of hash result for sampled pages */
> +};
> +
>  void *get_dirtyrate_thread(void *arg);
>  #endif
>  
> -- 
> 1.8.3.1

dme.
-- 
Please forgive me if I act a little strange, for I know not what I do.

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

[Zheng Chuan]

On 2020/8/26 17:29, David Edmondson wrote:
> On Monday, 2020-08-24 at 17:14:31 +08, Chuan Zheng wrote:
> 
>> Add RamlockDirtyInfo to store sampled page info of each ramblock.
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> ---
>>  migration/dirtyrate.h | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>> index 33669b7..70000da 100644
>> --- a/migration/dirtyrate.h
>> +++ b/migration/dirtyrate.h
>> @@ -19,6 +19,11 @@
>>   */
>>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
>>  
>> +/*
>> + * Record ramblock idstr
>> + */
>> +#define RAMBLOCK_INFO_MAX_LEN                     256
>> +
>>  /* Take 1s as default for calculation duration */
>>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>>  
>> @@ -27,6 +32,19 @@ struct DirtyRateConfig {
>>      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 4K-page */
> 
> It's probably a stupid question, but why not store a pointer to the
> RAMBlock rather than copying some of the details?
> 
>> +    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; /* cout of dirty pages we measure */
> 
> "cout" -> "count"
> 
Hi, David.
Thank you for review.
Actually I have resent [PATCH V5], but it's my fault to forget to add resent tag which may lead to confusing.
In new patch series, this spell mistake is fixed.
Please refer to the newer patch series:)

>> +    uint32_t *hash_result; /* array of hash result for sampled pages */
>> +};
>> +
>>  void *get_dirtyrate_thread(void *arg);
>>  #endif
>>  
>> -- 
>> 1.8.3.1
> 
> dme.
> 

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

[David Edmondson]

On Monday, 2020-08-24 at 17:14:34 +08, Chuan Zheng wrote:

> Record hash results for each sampled page, crc32 is taken to calculate
> hash results for each sampled 4K-page.
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> ---
>  migration/dirtyrate.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  migration/dirtyrate.h |  15 ++++++
>  2 files changed, 151 insertions(+)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index f6a94d8..66de426 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 "crypto/hash.h"
> @@ -66,6 +67,141 @@ static void update_dirtyrate(uint64_t msec)
>      DirtyStat.dirty_rate = dirtyrate;
>  }
>  
> +/*
> + * get hash result for the sampled memory with length of 4K byte in ramblock,
> + * which starts from ramblock base address.
> + */
> +static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
> +                                      uint64_t vfn)
> +{
> +    struct iovec iov_array;

There's no need for an iovec now that crc32() is being used.

> +    uint32_t crc;
> +
> +    iov_array.iov_base = info->ramblock_addr +
> +                         vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
> +    iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;
> +
> +    crc = crc32(0, iov_array.iov_base, iov_array.iov_len);
> +
> +    return crc;
> +}
> +
> +static int save_ramblock_hash(struct RamblockDirtyInfo *info)
> +{
> +    unsigned int sample_pages_count;
> +    int i;
> +    int ret = -1;

There's no need to initialise "ret".

> +    GRand *rand = g_rand_new();

Calling g_rand_new() when the result may not be used (because of the
various conditions immediately below) seems as though it might waste
entropy. Could this be pushed down just above the loop? It would even
get rid of the gotos ;-)

> +    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)) {
> +        ret = 0;
> +        goto out;
> +    }
> +
> +    info->hash_result = g_try_malloc0_n(sample_pages_count,
> +                                        sizeof(uint32_t));
> +    if (!info->hash_result) {
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    info->sample_page_vfn = g_try_malloc0_n(sample_pages_count,
> +                                            sizeof(uint64_t));
> +    if (!info->sample_page_vfn) {
> +        g_free(info->hash_result);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    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]);
> +    }
> +    ret = 0;
> +
> +out:
> +    g_rand_free(rand);
> +    return ret;
> +}
> +
> +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 block size in GB */
> +    info->sample_pages_count = (qemu_ram_get_used_length(block) *
> +                                sample_pages_per_gigabytes) >>
> +                                DIRTYRATE_PAGE_SHIFT_GB;

Doing the calculation this way around seems odd. Why was it not:

    info->sample_pages_count = (qemu_ram_get_used_length(block) >>
                                DIRTYRATE_PAGE_SHIFT_GB) *
                                sample_pages_per_gigabytes;

?

> +
> +    /* Right shift 12 bits to calc page count in 4KB */
> +    info->ramblock_pages = qemu_ram_get_used_length(block) >>
> +                           DIRTYRATE_PAGE_SHIFT_KB;
> +    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));

g_try_renew() instead of g_try_realloc()?

> +    }
> +    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;

There's no need to initialise "info" or "block".

The declaration of "info" could be pushed into the loop.

> +    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 */
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 9db269d..5050add 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -24,6 +24,21 @@
>   */
>  #define RAMBLOCK_INFO_MAX_LEN                     256
>  
> +/*
> + * Sample page size 4K as default.
> + */
> +#define DIRTYRATE_SAMPLE_PAGE_SIZE                4096
> +
> +/*
> + * Sample page size 4K shift
> + */
> +#define DIRTYRATE_PAGE_SHIFT_KB                   12
> +
> +/*
> + * Sample page size 1G shift
> + */
> +#define DIRTYRATE_PAGE_SHIFT_GB                   30
> +
>  /* Take 1s as default for calculation duration */
>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>  
> -- 
> 1.8.3.1

dme.
-- 
You bring light in.

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

[Zheng Chuan]

On 2020/8/26 17:40, Zheng Chuan wrote:
> 
> 
> On 2020/8/26 17:29, David Edmondson wrote:
>> On Monday, 2020-08-24 at 17:14:31 +08, Chuan Zheng wrote:
>>
>>> Add RamlockDirtyInfo to store sampled page info of each ramblock.
>>>
>>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>>> ---
>>>  migration/dirtyrate.h | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>>> index 33669b7..70000da 100644
>>> --- a/migration/dirtyrate.h
>>> +++ b/migration/dirtyrate.h
>>> @@ -19,6 +19,11 @@
>>>   */
>>>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
>>>  
>>> +/*
>>> + * Record ramblock idstr
>>> + */
>>> +#define RAMBLOCK_INFO_MAX_LEN                     256
>>> +
>>>  /* Take 1s as default for calculation duration */
>>>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>>>  
>>> @@ -27,6 +32,19 @@ struct DirtyRateConfig {
>>>      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 4K-page */
>>
>> It's probably a stupid question, but why not store a pointer to the
>> RAMBlock rather than copying some of the details?
>>
Missing this question:)
Since I only hold the RCU around each part separately, the RAMBlock
could disappear between the initial hash, and the later compare;  so
using the name makes it safe.
Again, Please refer to the newer patch series:)

>>> +    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; /* cout of dirty pages we measure */
>>
>> "cout" -> "count"
>>
> Hi, David.
> Thank you for review.
> Actually I have resent [PATCH V5], but it's my fault to forget to add resent tag which may lead to confusing.
> In new patch series, this spell mistake is fixed.
> Please refer to the newer patch series:)
> 
>>> +    uint32_t *hash_result; /* array of hash result for sampled pages */
>>> +};
>>> +
>>>  void *get_dirtyrate_thread(void *arg);
>>>  #endif
>>>  
>>> -- 
>>> 1.8.3.1
>>
>> dme.
>>

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

[David Edmondson]

On Monday, 2020-08-24 at 17:14:35 +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 | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 66de426..050270d 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -202,6 +202,70 @@ static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
>      return 0;
>  }
>  
> +static int calc_page_dirty_rate(struct RamblockDirtyInfo *info)

This never fails - could be void?

> +{
> +    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++;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static bool find_page_matched(RAMBlock *block, struct RamblockDirtyInfo *infos,
> +                              int count, struct RamblockDirtyInfo **matched)

This might as well just return a struct RamblockDirtyInfo pointer (or NULL).

> +{
> +    int i;
> +
> +    for (i = 0; i < count; i++) {
> +        if (!strcmp(infos[i].idstr, qemu_ram_get_idstr(block))) {
> +            break;
> +        }
> +    }
> +
> +    if (i == count) {
> +        return false;
> +    }
> +
> +    if (infos[i].ramblock_addr != qemu_ram_get_host_addr(block) ||
> +        infos[i].ramblock_pages !=
> +            (qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SHIFT_KB)) {
> +        return false;
> +    }
> +
> +    *matched = &infos[i];
> +    return true;
> +}
> +
> +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 = NULL;
> +        if (!find_page_matched(block, info, block_index + 1, &block_dinfo)) {
> +            continue;
> +        }
> +        if (calc_page_dirty_rate(block_dinfo) < 0) {
> +            return -1;
> +        }
> +        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

dme.
-- 
Sometimes these eyes, forget the face they're peering from.

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

[David Edmondson]

On Monday, 2020-08-24 at 17:14:36 +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>
> ---
>  migration/dirtyrate.c | 24 ++++++++++++++++++++++++
>  migration/dirtyrate.h | 10 ++++++++++
>  2 files changed, 34 insertions(+)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 050270d..bd398b7 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -173,6 +173,24 @@ alloc_ramblock_dirty_info(int *block_index,
>      return block_dinfo;
>  }
>  
> +static int skip_sample_ramblock(RAMBlock *block)
> +{
> +    int64_t ramblock_size;
> +
> +    /* ramblock size in MB */
> +    ramblock_size = qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SHIFT_MB;
> +
> +    /*
> +     * Consider ramblock with size larger than 128M is what we
> +     * want to sample.
> +     */
> +    if (ramblock_size < MIN_RAMBLOCK_SIZE) {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
>                                       struct DirtyRateConfig config,
>                                       int *block_index)
> @@ -183,6 +201,9 @@ static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
>      int index = 0;
>  
>      RAMBLOCK_FOREACH_MIGRATABLE(block) {
> +        if (skip_sample_ramblock(block) < 0) {
> +            continue;
> +        }
>          dinfo = alloc_ramblock_dirty_info(&index, dinfo);
>          if (dinfo == NULL) {
>              return -1;
> @@ -249,6 +270,9 @@ static int compare_page_hash_info(struct RamblockDirtyInfo *info,
>      RAMBlock *block = NULL;
>  
>      RAMBLOCK_FOREACH_MIGRATABLE(block) {
> +        if (skip_sample_ramblock(block) < 0) {
> +            continue;
> +        }
>          block_dinfo = NULL;
>          if (!find_page_matched(block, info, block_index + 1, &block_dinfo)) {
>              continue;
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 5050add..41bc264 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -35,10 +35,20 @@
>  #define DIRTYRATE_PAGE_SHIFT_KB                   12
>  
>  /*
> + * Sample page size MB shift
> + */
> +#define DIRTYRATE_PAGE_SHIFT_MB                   20
> +
> +/*
>   * Sample page size 1G shift
>   */
>  #define DIRTYRATE_PAGE_SHIFT_GB                   30
>  
> +/*
> + * minimum ramblock size to sampled

"Minimum RAMBlock size to sample, in megabytes."

> + */
> +#define MIN_RAMBLOCK_SIZE                         128
> +
>  /* Take 1s as default for calculation duration */
>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>  
> -- 
> 1.8.3.1

dme.
-- 
I went starin' out of my window, been caught doin' it once or twice.

Re: [PATCH v5 09/12] migration/dirtyrate: Implement get_sample_page_period() and block_sample_page_period()

[David Edmondson]

On Monday, 2020-08-24 at 17:14:37 +08, Chuan Zheng wrote:

> Implement get_sample_page_period() and set_sample_page_period() to
> sleep specific time between sample actions.
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> ---
>  migration/dirtyrate.c | 24 ++++++++++++++++++++++++
>  migration/dirtyrate.h |  2 ++
>  2 files changed, 26 insertions(+)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index bd398b7..d1c0a78 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -28,6 +28,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 int64_t get_sample_page_period(int64_t sec)
> +{
> +    if (sec <= MIN_FETCH_DIRTYRATE_TIME_SEC ||

Shouldn't the minimum value be allowed?

That is, this test should be "sec < MIN_FETCH_DIRTYRATE_TIME_SEC" and
MIN_FETCH_DIRTYRATE_TIME_SEC should be 1.

> +        sec > MAX_FETCH_DIRTYRATE_TIME_SEC) {
> +        sec = DEFAULT_FETCH_DIRTYRATE_TIME_SEC;
> +    }
> +
> +    return sec;
> +}
> +
>  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 41bc264..50a5636 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -51,6 +51,8 @@
>  
>  /* Take 1s as default for calculation duration */
>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
> +#define MIN_FETCH_DIRTYRATE_TIME_SEC              0
> +#define MAX_FETCH_DIRTYRATE_TIME_SEC              60
>  
>  struct DirtyRateConfig {
>      uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
> -- 
> 1.8.3.1

dme.
-- 
Facts don't do what I want them to.

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

[David Edmondson]

On Monday, 2020-08-24 at 17:14:38 +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 d1c0a78..9f52f5f 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -171,6 +171,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)
> @@ -316,8 +331,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();
> +    initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    rcu_read_lock();

Page dirtying that happens while acquiring the lock will not be
accounted for, but is within the time window. Could we store the time
after acquiring the lock?

> +    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();

Is it necessary to hold the 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.
-- 
There's someone in my head but it's not me.

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

[David Edmondson]

On Monday, 2020-08-24 at 17:14:39 +08, Chuan Zheng wrote:

> 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 | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  qapi/migration.json   | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 9f52f5f..08c46d3 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -62,6 +62,28 @@ 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;
> +    /*
> +     * Only support query once for each calculation,
> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
> +     */
> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
> +                              DIRTY_RATE_STATUS_UNSTARTED);

Is there a reason for this restriction? Removing it would require
clarifying the state model, I suppose.

> +
> +    return info;
> +}
> +
>  static void reset_dirtyrate_stat(void)
>  {
>      DirtyStat.total_dirty_samples = 0;
> @@ -378,3 +400,26 @@ 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;
> +
> +    /*
> +     * We don't begin calculating thread only when it's in calculating status.

"If the dirty rate is already being measured, don't attempt to start."

> +     */
> +    if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) {

Should this return an error to the caller?

> +        return;
> +    }
> +
> +    config.sample_period_seconds = get_sample_page_period(calc_time);

If the specified sample period is outside the min/max, should an error
be returned to the caller?

> +    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 d640165..826bfd7 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1737,3 +1737,47 @@
>  ##
>  { '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'
> +#
> +# Since: 5.2
> +#
> +##
> +{ 'struct': 'DirtyRateInfo',
> +  'data': {'dirty-rate': 'int64',
> +           'status': 'DirtyRateStatus'} }
> +
> +##
> +# @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": "cal-dirty-rate", "data": {"calc-time": 1} }

"cal-dirty-rate" -> "calc-dirty-rate".

> +#
> +##
> +{ '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

dme.
-- 
I'm going in the out door, I'm doing all right.

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

[Dr. David Alan Gilbert]

* David Edmondson (dme@dme.org) wrote:
> On Monday, 2020-08-24 at 17:14:31 +08, Chuan Zheng wrote:
> 
> > Add RamlockDirtyInfo to store sampled page info of each ramblock.
> >
> > Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> > ---
> >  migration/dirtyrate.h | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> > index 33669b7..70000da 100644
> > --- a/migration/dirtyrate.h
> > +++ b/migration/dirtyrate.h
> > @@ -19,6 +19,11 @@
> >   */
> >  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
> >  
> > +/*
> > + * Record ramblock idstr
> > + */
> > +#define RAMBLOCK_INFO_MAX_LEN                     256
> > +
> >  /* Take 1s as default for calculation duration */
> >  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
> >  
> > @@ -27,6 +32,19 @@ struct DirtyRateConfig {
> >      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 4K-page */
> 
> It's probably a stupid question, but why not store a pointer to the
> RAMBlock rather than copying some of the details?

I think I figured that out in the last round;  this code runs as:

    rcu lock {
       calculate initial CRCs
    }

    <sleep 1 second ish>
    rcu lock {
       calculate new CRCs
    }

A RAMBlock might get deleted between the two.

Dave

       
> > +    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; /* cout of dirty pages we measure */
> 
> "cout" -> "count"
> 
> > +    uint32_t *hash_result; /* array of hash result for sampled pages */
> > +};
> > +
> >  void *get_dirtyrate_thread(void *arg);
> >  #endif
> >  
> > -- 
> > 1.8.3.1
> 
> dme.
> -- 
> Please forgive me if I act a little strange, for I know not what I do.
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[David Edmondson]

On Wednesday, 2020-08-26 at 11:33:30 +01, Dr. David Alan Gilbert wrote:

> * David Edmondson (dme@dme.org) wrote:
>> On Monday, 2020-08-24 at 17:14:31 +08, Chuan Zheng wrote:
>> 
>> > Add RamlockDirtyInfo to store sampled page info of each ramblock.
>> >
>> > Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> > ---
>> >  migration/dirtyrate.h | 18 ++++++++++++++++++
>> >  1 file changed, 18 insertions(+)
>> >
>> > diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>> > index 33669b7..70000da 100644
>> > --- a/migration/dirtyrate.h
>> > +++ b/migration/dirtyrate.h
>> > @@ -19,6 +19,11 @@
>> >   */
>> >  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
>> >  
>> > +/*
>> > + * Record ramblock idstr
>> > + */
>> > +#define RAMBLOCK_INFO_MAX_LEN                     256
>> > +
>> >  /* Take 1s as default for calculation duration */
>> >  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>> >  
>> > @@ -27,6 +32,19 @@ struct DirtyRateConfig {
>> >      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 4K-page */
>> 
>> It's probably a stupid question, but why not store a pointer to the
>> RAMBlock rather than copying some of the details?
>
> I think I figured that out in the last round;  this code runs as:
>
>     rcu lock {
>        calculate initial CRCs
>     }
>
>     <sleep 1 second ish>
>     rcu lock {
>        calculate new CRCs
>     }
>
> A RAMBlock might get deleted between the two.

Makes sense, thanks.

dme.
-- 
Why does it have to be like this? I can never tell.

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

[Dr. David Alan Gilbert]

* David Edmondson (dme@dme.org) wrote:
> On Monday, 2020-08-24 at 17:14:34 +08, Chuan Zheng wrote:
> 
> > Record hash results for each sampled page, crc32 is taken to calculate
> > hash results for each sampled 4K-page.
> >
> > Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> > Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> > ---
> >  migration/dirtyrate.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  migration/dirtyrate.h |  15 ++++++
> >  2 files changed, 151 insertions(+)
> >
> > diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> > index f6a94d8..66de426 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 "crypto/hash.h"
> > @@ -66,6 +67,141 @@ static void update_dirtyrate(uint64_t msec)
> >      DirtyStat.dirty_rate = dirtyrate;
> >  }
> >  
> > +/*
> > + * get hash result for the sampled memory with length of 4K byte in ramblock,
> > + * which starts from ramblock base address.
> > + */
> > +static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
> > +                                      uint64_t vfn)
> > +{
> > +    struct iovec iov_array;
> 
> There's no need for an iovec now that crc32() is being used.
> 
> > +    uint32_t crc;
> > +
> > +    iov_array.iov_base = info->ramblock_addr +
> > +                         vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
> > +    iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;
> > +
> > +    crc = crc32(0, iov_array.iov_base, iov_array.iov_len);
> > +
> > +    return crc;
> > +}
> > +
> > +static int save_ramblock_hash(struct RamblockDirtyInfo *info)
> > +{
> > +    unsigned int sample_pages_count;
> > +    int i;
> > +    int ret = -1;
> 
> There's no need to initialise "ret".
> 
> > +    GRand *rand = g_rand_new();
> 
> Calling g_rand_new() when the result may not be used (because of the
> various conditions immediately below) seems as though it might waste
> entropy. Could this be pushed down just above the loop? It would even
> get rid of the gotos ;-)
> 
> > +    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)) {
> > +        ret = 0;
> > +        goto out;
> > +    }
> > +
> > +    info->hash_result = g_try_malloc0_n(sample_pages_count,
> > +                                        sizeof(uint32_t));
> > +    if (!info->hash_result) {
> > +        ret = -1;
> > +        goto out;
> > +    }
> > +
> > +    info->sample_page_vfn = g_try_malloc0_n(sample_pages_count,
> > +                                            sizeof(uint64_t));
> > +    if (!info->sample_page_vfn) {
> > +        g_free(info->hash_result);
> > +        ret = -1;
> > +        goto out;
> > +    }
> > +
> > +    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]);
> > +    }
> > +    ret = 0;
> > +
> > +out:
> > +    g_rand_free(rand);
> > +    return ret;
> > +}
> > +
> > +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 block size in GB */
> > +    info->sample_pages_count = (qemu_ram_get_used_length(block) *
> > +                                sample_pages_per_gigabytes) >>
> > +                                DIRTYRATE_PAGE_SHIFT_GB;
> 
> Doing the calculation this way around seems odd. Why was it not:
> 
>     info->sample_pages_count = (qemu_ram_get_used_length(block) >>
>                                 DIRTYRATE_PAGE_SHIFT_GB) *
>                                 sample_pages_per_gigabytes;
> 
> ?

Because that would give 0 for a 0.5GB block

Dave

> > +
> > +    /* Right shift 12 bits to calc page count in 4KB */
> > +    info->ramblock_pages = qemu_ram_get_used_length(block) >>
> > +                           DIRTYRATE_PAGE_SHIFT_KB;
> > +    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));
> 
> g_try_renew() instead of g_try_realloc()?
> 
> > +    }
> > +    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;
> 
> There's no need to initialise "info" or "block".
> 
> The declaration of "info" could be pushed into the loop.
> 
> > +    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 */
> > diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> > index 9db269d..5050add 100644
> > --- a/migration/dirtyrate.h
> > +++ b/migration/dirtyrate.h
> > @@ -24,6 +24,21 @@
> >   */
> >  #define RAMBLOCK_INFO_MAX_LEN                     256
> >  
> > +/*
> > + * Sample page size 4K as default.
> > + */
> > +#define DIRTYRATE_SAMPLE_PAGE_SIZE                4096
> > +
> > +/*
> > + * Sample page size 4K shift
> > + */
> > +#define DIRTYRATE_PAGE_SHIFT_KB                   12
> > +
> > +/*
> > + * Sample page size 1G shift
> > + */
> > +#define DIRTYRATE_PAGE_SHIFT_GB                   30
> > +
> >  /* Take 1s as default for calculation duration */
> >  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
> >  
> > -- 
> > 1.8.3.1
> 
> dme.
> -- 
> You bring light in.
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[David Edmondson]

On Wednesday, 2020-08-26 at 13:30:16 +01, Dr. David Alan Gilbert wrote:

> * David Edmondson (dme@dme.org) wrote:
>> On Monday, 2020-08-24 at 17:14:34 +08, Chuan Zheng wrote:
>> 
>> > Record hash results for each sampled page, crc32 is taken to calculate
>> > hash results for each sampled 4K-page.
>> >
>> > Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> > Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>> > ---
>> >  migration/dirtyrate.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  migration/dirtyrate.h |  15 ++++++
>> >  2 files changed, 151 insertions(+)
>> >
>> > diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> > index f6a94d8..66de426 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 "crypto/hash.h"
>> > @@ -66,6 +67,141 @@ static void update_dirtyrate(uint64_t msec)
>> >      DirtyStat.dirty_rate = dirtyrate;
>> >  }
>> >  
>> > +/*
>> > + * get hash result for the sampled memory with length of 4K byte in ramblock,
>> > + * which starts from ramblock base address.
>> > + */
>> > +static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
>> > +                                      uint64_t vfn)
>> > +{
>> > +    struct iovec iov_array;
>> 
>> There's no need for an iovec now that crc32() is being used.
>> 
>> > +    uint32_t crc;
>> > +
>> > +    iov_array.iov_base = info->ramblock_addr +
>> > +                         vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
>> > +    iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;
>> > +
>> > +    crc = crc32(0, iov_array.iov_base, iov_array.iov_len);
>> > +
>> > +    return crc;
>> > +}
>> > +
>> > +static int save_ramblock_hash(struct RamblockDirtyInfo *info)
>> > +{
>> > +    unsigned int sample_pages_count;
>> > +    int i;
>> > +    int ret = -1;
>> 
>> There's no need to initialise "ret".
>> 
>> > +    GRand *rand = g_rand_new();
>> 
>> Calling g_rand_new() when the result may not be used (because of the
>> various conditions immediately below) seems as though it might waste
>> entropy. Could this be pushed down just above the loop? It would even
>> get rid of the gotos ;-)
>> 
>> > +    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)) {
>> > +        ret = 0;
>> > +        goto out;
>> > +    }
>> > +
>> > +    info->hash_result = g_try_malloc0_n(sample_pages_count,
>> > +                                        sizeof(uint32_t));
>> > +    if (!info->hash_result) {
>> > +        ret = -1;
>> > +        goto out;
>> > +    }
>> > +
>> > +    info->sample_page_vfn = g_try_malloc0_n(sample_pages_count,
>> > +                                            sizeof(uint64_t));
>> > +    if (!info->sample_page_vfn) {
>> > +        g_free(info->hash_result);
>> > +        ret = -1;
>> > +        goto out;
>> > +    }
>> > +
>> > +    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]);
>> > +    }
>> > +    ret = 0;
>> > +
>> > +out:
>> > +    g_rand_free(rand);
>> > +    return ret;
>> > +}
>> > +
>> > +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 block size in GB */
>> > +    info->sample_pages_count = (qemu_ram_get_used_length(block) *
>> > +                                sample_pages_per_gigabytes) >>
>> > +                                DIRTYRATE_PAGE_SHIFT_GB;
>> 
>> Doing the calculation this way around seems odd. Why was it not:
>> 
>>     info->sample_pages_count = (qemu_ram_get_used_length(block) >>
>>                                 DIRTYRATE_PAGE_SHIFT_GB) *
>>                                 sample_pages_per_gigabytes;
>> 
>> ?
>
> Because that would give 0 for a 0.5GB block

Ouch, obviously :-) Thanks.

dme.
-- 
And removed his hat, in respect of her presence.

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

[Zheng Chuan]

On 2020/8/26 20:30, Dr. David Alan Gilbert wrote:
> * David Edmondson (dme@dme.org) wrote:
>> On Monday, 2020-08-24 at 17:14:34 +08, Chuan Zheng wrote:
>>
>>> Record hash results for each sampled page, crc32 is taken to calculate
>>> hash results for each sampled 4K-page.
>>>
>>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>>> ---
>>>  migration/dirtyrate.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  migration/dirtyrate.h |  15 ++++++
>>>  2 files changed, 151 insertions(+)
>>>
>>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>>> index f6a94d8..66de426 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 "crypto/hash.h"
>>> @@ -66,6 +67,141 @@ static void update_dirtyrate(uint64_t msec)
>>>      DirtyStat.dirty_rate = dirtyrate;
>>>  }
>>>  
>>> +/*
>>> + * get hash result for the sampled memory with length of 4K byte in ramblock,
>>> + * which starts from ramblock base address.
>>> + */
>>> +static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
>>> +                                      uint64_t vfn)
>>> +{
>>> +    struct iovec iov_array;
>>
>> There's no need for an iovec now that crc32() is being used.
>>
OK, will take two variables instead to represent base address and length.

>>> +    uint32_t crc;
>>> +
>>> +    iov_array.iov_base = info->ramblock_addr +
>>> +                         vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
>>> +    iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;
>>> +
>>> +    crc = crc32(0, iov_array.iov_base, iov_array.iov_len);
>>> +
>>> +    return crc;
>>> +}
>>> +
>>> +static int save_ramblock_hash(struct RamblockDirtyInfo *info)
>>> +{
>>> +    unsigned int sample_pages_count;
>>> +    int i;
>>> +    int ret = -1;
>>
>> There's no need to initialise "ret".
>>
>>> +    GRand *rand = g_rand_new();
>>
>> Calling g_rand_new() when the result may not be used (because of the
>> various conditions immediately below) seems as though it might waste
>> entropy. Could this be pushed down just above the loop? It would even
>> get rid of the gotos ;-)
>>
OK, i'll try it.

>>> +    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)) {
>>> +        ret = 0;
>>> +        goto out;
>>> +    }
>>> +
>>> +    info->hash_result = g_try_malloc0_n(sample_pages_count,
>>> +                                        sizeof(uint32_t));
>>> +    if (!info->hash_result) {
>>> +        ret = -1;
>>> +        goto out;
>>> +    }
>>> +
>>> +    info->sample_page_vfn = g_try_malloc0_n(sample_pages_count,
>>> +                                            sizeof(uint64_t));
>>> +    if (!info->sample_page_vfn) {
>>> +        g_free(info->hash_result);
>>> +        ret = -1;
>>> +        goto out;
>>> +    }
>>> +
>>> +    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]);
>>> +    }
>>> +    ret = 0;
>>> +
>>> +out:
>>> +    g_rand_free(rand);
>>> +    return ret;
>>> +}
>>> +
>>> +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 block size in GB */
>>> +    info->sample_pages_count = (qemu_ram_get_used_length(block) *
>>> +                                sample_pages_per_gigabytes) >>
>>> +                                DIRTYRATE_PAGE_SHIFT_GB;
>>
>> Doing the calculation this way around seems odd. Why was it not:
>>
>>     info->sample_pages_count = (qemu_ram_get_used_length(block) >>
>>                                 DIRTYRATE_PAGE_SHIFT_GB) *
>>                                 sample_pages_per_gigabytes;
>>
>> ?
> 
> Because that would give 0 for a 0.5GB block
> 
> Dave
> 
>>> +
>>> +    /* Right shift 12 bits to calc page count in 4KB */
>>> +    info->ramblock_pages = qemu_ram_get_used_length(block) >>
>>> +                           DIRTYRATE_PAGE_SHIFT_KB;
>>> +    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));
>>
>> g_try_renew() instead of g_try_realloc()?
>>
Hi,
I am not sure that because there only one place in qemu to use g_try_renew.
Could you tell me why, because i think g_try_realloc will also return NULL when error happen:)

>>> +    }
>>> +    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;
>>
>> There's no need to initialise "info" or "block".
>>
OK, will remove unused initialization in V6.
>> The declaration of "info" could be pushed into the loop.
>>
>>> +    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 */
>>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>>> index 9db269d..5050add 100644
>>> --- a/migration/dirtyrate.h
>>> +++ b/migration/dirtyrate.h
>>> @@ -24,6 +24,21 @@
>>>   */
>>>  #define RAMBLOCK_INFO_MAX_LEN                     256
>>>  
>>> +/*
>>> + * Sample page size 4K as default.
>>> + */
>>> +#define DIRTYRATE_SAMPLE_PAGE_SIZE                4096
>>> +
>>> +/*
>>> + * Sample page size 4K shift
>>> + */
>>> +#define DIRTYRATE_PAGE_SHIFT_KB                   12
>>> +
>>> +/*
>>> + * Sample page size 1G shift
>>> + */
>>> +#define DIRTYRATE_PAGE_SHIFT_GB                   30
>>> +
>>>  /* Take 1s as default for calculation duration */
>>>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>>>  
>>> -- 
>>> 1.8.3.1
>>
>> dme.
>> -- 
>> You bring light in.
>>

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

[David Edmondson]

On Thursday, 2020-08-27 at 14:28:03 +08, Zheng Chuan wrote:

>>>> +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));
>>>
>>> g_try_renew() instead of g_try_realloc()?
>>>
> Hi,
> I am not sure that because there only one place in qemu to use g_try_renew.
> Could you tell me why, because i think g_try_realloc will also return NULL when error happen:)

Only suggested because it would make the two branches of the code more
similar.

dme.
-- 
And you can't hold me down, 'cause I belong to the hurricane.

Re: [PATCH v5 09/12] migration/dirtyrate: Implement get_sample_page_period() and block_sample_page_period()

[Zheng Chuan]

On 2020/8/26 18:17, David Edmondson wrote:
> On Monday, 2020-08-24 at 17:14:37 +08, Chuan Zheng wrote:
> 
>> Implement get_sample_page_period() and set_sample_page_period() to
>> sleep specific time between sample actions.
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> ---
>>  migration/dirtyrate.c | 24 ++++++++++++++++++++++++
>>  migration/dirtyrate.h |  2 ++
>>  2 files changed, 26 insertions(+)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index bd398b7..d1c0a78 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -28,6 +28,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 int64_t get_sample_page_period(int64_t sec)
>> +{
>> +    if (sec <= MIN_FETCH_DIRTYRATE_TIME_SEC ||
> 
> Shouldn't the minimum value be allowed?
> 
> That is, this test should be "sec < MIN_FETCH_DIRTYRATE_TIME_SEC" and
> MIN_FETCH_DIRTYRATE_TIME_SEC should be 1.
> 
Well, Actually we could measure dirtyrate within duration below 1s, like 0.5s.
Howerver, I am reconsider that maybe taking 0.5s as MIN_FETCH_DIRTYRATE_TIME_SEC is better in case of someone to do nasty thing like setting
a meaningless time duration which is close to 0:)

>> +        sec > MAX_FETCH_DIRTYRATE_TIME_SEC) {
>> +        sec = DEFAULT_FETCH_DIRTYRATE_TIME_SEC;
>> +    }
>> +
>> +    return sec;
>> +}
>> +
>>  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 41bc264..50a5636 100644
>> --- a/migration/dirtyrate.h
>> +++ b/migration/dirtyrate.h
>> @@ -51,6 +51,8 @@
>>  
>>  /* Take 1s as default for calculation duration */
>>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>> +#define MIN_FETCH_DIRTYRATE_TIME_SEC              0
>> +#define MAX_FETCH_DIRTYRATE_TIME_SEC              60
>>  
>>  struct DirtyRateConfig {
>>      uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
>> -- 
>> 1.8.3.1
> 
> dme.
> 

Re: [PATCH v5 09/12] migration/dirtyrate: Implement get_sample_page_period() and block_sample_page_period()

[Zheng Chuan]

On 2020/8/27 16:01, Zheng Chuan wrote:
> 
> 
> On 2020/8/26 18:17, David Edmondson wrote:
>> On Monday, 2020-08-24 at 17:14:37 +08, Chuan Zheng wrote:
>>
>>> Implement get_sample_page_period() and set_sample_page_period() to
>>> sleep specific time between sample actions.
>>>
>>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>>> ---
>>>  migration/dirtyrate.c | 24 ++++++++++++++++++++++++
>>>  migration/dirtyrate.h |  2 ++
>>>  2 files changed, 26 insertions(+)
>>>
>>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>>> index bd398b7..d1c0a78 100644
>>> --- a/migration/dirtyrate.c
>>> +++ b/migration/dirtyrate.c
>>> @@ -28,6 +28,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 int64_t get_sample_page_period(int64_t sec)
>>> +{
>>> +    if (sec <= MIN_FETCH_DIRTYRATE_TIME_SEC ||
>>
>> Shouldn't the minimum value be allowed?
>>
>> That is, this test should be "sec < MIN_FETCH_DIRTYRATE_TIME_SEC" and
>> MIN_FETCH_DIRTYRATE_TIME_SEC should be 1.
>>
> Well, Actually we could measure dirtyrate within duration below 1s, like 0.5s.
> Howerver, I am reconsider that maybe taking 0.5s as MIN_FETCH_DIRTYRATE_TIME_SEC is better in case of someone to do nasty thing like setting
> a meaningless time duration which is close to 0:)
> 
Ouch, since i define sec as int64_t，it could never be 0.5s:-).
I will take 1s as minimum value because it will take about 100ms for large vm to hash its result.
Thanks for your review.

>>> +        sec > MAX_FETCH_DIRTYRATE_TIME_SEC) {
>>> +        sec = DEFAULT_FETCH_DIRTYRATE_TIME_SEC;
>>> +    }
>>> +
>>> +    return sec;
>>> +}
>>> +
>>>  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 41bc264..50a5636 100644
>>> --- a/migration/dirtyrate.h
>>> +++ b/migration/dirtyrate.h
>>> @@ -51,6 +51,8 @@
>>>  
>>>  /* Take 1s as default for calculation duration */
>>>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>>> +#define MIN_FETCH_DIRTYRATE_TIME_SEC              0
>>> +#define MAX_FETCH_DIRTYRATE_TIME_SEC              60
>>>  
>>>  struct DirtyRateConfig {
>>>      uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
>>> -- 
>>> 1.8.3.1
>>
>> dme.
>>

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

[Zheng Chuan]

On 2020/8/26 18:21, David Edmondson wrote:
> On Monday, 2020-08-24 at 17:14:38 +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 d1c0a78..9f52f5f 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -171,6 +171,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)
>> @@ -316,8 +331,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();
>> +    initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>> +    rcu_read_lock();
> 
> Page dirtying that happens while acquiring the lock will not be
> accounted for, but is within the time window. Could we store the time
> after acquiring the lock?
> 
Yes, it would be better.
will fix in V6.

>> +    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();
> 
> Is it necessary to hold the lock across update_dirtyrate()?
> 
There is no need for that.
Will fix it in V6.

>> +    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 v5 09/12] migration/dirtyrate: Implement get_sample_page_period() and block_sample_page_period()

[David Edmondson]

On Thursday, 2020-08-27 at 16:01:37 +08, Zheng Chuan wrote:

> On 2020/8/26 18:17, David Edmondson wrote:
>> On Monday, 2020-08-24 at 17:14:37 +08, Chuan Zheng wrote:
>> 
>>> Implement get_sample_page_period() and set_sample_page_period() to
>>> sleep specific time between sample actions.
>>>
>>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>>> ---
>>>  migration/dirtyrate.c | 24 ++++++++++++++++++++++++
>>>  migration/dirtyrate.h |  2 ++
>>>  2 files changed, 26 insertions(+)
>>>
>>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>>> index bd398b7..d1c0a78 100644
>>> --- a/migration/dirtyrate.c
>>> +++ b/migration/dirtyrate.c
>>> @@ -28,6 +28,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 int64_t get_sample_page_period(int64_t sec)
>>> +{
>>> +    if (sec <= MIN_FETCH_DIRTYRATE_TIME_SEC ||
>> 
>> Shouldn't the minimum value be allowed?
>> 
>> That is, this test should be "sec < MIN_FETCH_DIRTYRATE_TIME_SEC" and
>> MIN_FETCH_DIRTYRATE_TIME_SEC should be 1.
>> 
> Well, Actually we could measure dirtyrate within duration below 1s, like 0.5s.
> Howerver, I am reconsider that maybe taking 0.5s as MIN_FETCH_DIRTYRATE_TIME_SEC is better in case of someone to do nasty thing like setting
> a meaningless time duration which is close to 0:)

I think that a minimum of 1 second is fine. My concern is only that if
you say "the minimum is X" but then don't let me choose X, it seems
weird.

>>> +        sec > MAX_FETCH_DIRTYRATE_TIME_SEC) {
>>> +        sec = DEFAULT_FETCH_DIRTYRATE_TIME_SEC;
>>> +    }
>>> +
>>> +    return sec;
>>> +}
>>> +
>>>  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 41bc264..50a5636 100644
>>> --- a/migration/dirtyrate.h
>>> +++ b/migration/dirtyrate.h
>>> @@ -51,6 +51,8 @@
>>>  
>>>  /* Take 1s as default for calculation duration */
>>>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>>> +#define MIN_FETCH_DIRTYRATE_TIME_SEC              0
>>> +#define MAX_FETCH_DIRTYRATE_TIME_SEC              60
>>>  
>>>  struct DirtyRateConfig {
>>>      uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
>>> -- 
>>> 1.8.3.1
>> 
>> dme.
>> 

dme.
-- 
Hello? Is anybody home? Well, you don't know me, but I know you.

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

[Zheng Chuan]

On 2020/8/26 18:26, David Edmondson wrote:
> On Monday, 2020-08-24 at 17:14:39 +08, Chuan Zheng wrote:
> 
>> 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 | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>  qapi/migration.json   | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 89 insertions(+)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index 9f52f5f..08c46d3 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -62,6 +62,28 @@ 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;
>> +    /*
>> +     * Only support query once for each calculation,
>> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
>> +     */
>> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>> +                              DIRTY_RATE_STATUS_UNSTARTED);
> 
> Is there a reason for this restriction? Removing it would require
> clarifying the state model, I suppose.
> 
We only support query once for each calculation.
Otherwise, it could always query dirtyrate, but maybe the dirtyrate is calculated
long time ago.

>> +
>> +    return info;
>> +}
>> +
>>  static void reset_dirtyrate_stat(void)
>>  {
>>      DirtyStat.total_dirty_samples = 0;
>> @@ -378,3 +400,26 @@ 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;
>> +
>> +    /*
>> +     * We don't begin calculating thread only when it's in calculating status.
> 
> "If the dirty rate is already being measured, don't attempt to start."
> 
>> +     */
>> +    if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) {
> 
> Should this return an error to the caller?
> 
>> +        return;
>> +    }
>> +
>> +    config.sample_period_seconds = get_sample_page_period(calc_time);
> 
> If the specified sample period is outside the min/max, should an error
> be returned to the caller?
> 
>> +    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 d640165..826bfd7 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1737,3 +1737,47 @@
>>  ##
>>  { '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'
>> +#
>> +# Since: 5.2
>> +#
>> +##
>> +{ 'struct': 'DirtyRateInfo',
>> +  'data': {'dirty-rate': 'int64',
>> +           'status': 'DirtyRateStatus'} }
>> +
>> +##
>> +# @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": "cal-dirty-rate", "data": {"calc-time": 1} }
> 
> "cal-dirty-rate" -> "calc-dirty-rate".
> 
>> +#
>> +##
>> +{ '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
> 
> dme.
> 

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

[David Edmondson]

On Thursday, 2020-08-27 at 17:34:13 +08, Zheng Chuan wrote:

>>> +    /*
>>> +     * Only support query once for each calculation,
>>> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
>>> +     */
>>> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>>> +                              DIRTY_RATE_STATUS_UNSTARTED);
>> 
>> Is there a reason for this restriction? Removing it would require
>> clarifying the state model, I suppose.
>> 
> We only support query once for each calculation.
> Otherwise, it could always query dirtyrate, but maybe the dirtyrate is calculated
> long time ago.

There's nothing in the current interface that prevents this from being
the case already - the caller could initiate a 1 second sample, then
wait 24 hours to query the result.

Obviously this would generally be regarded as "d'oh - don't do that",
but the same argument would apply if the caller is allowed to query the
results multiple times.

Perhaps a complete solution would be to include information about the
sample period with the result. The caller could then determine whether
the sample is of adequate quality (sufficiently recent, taken over a
sufficiently long time period) for its' intended use.

dme.
-- 
I walk like a building, I never get wet.

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

[Zheng Chuan]

On 2020/8/27 19:58, David Edmondson wrote:
> On Thursday, 2020-08-27 at 17:34:13 +08, Zheng Chuan wrote:
> 
>>>> +    /*
>>>> +     * Only support query once for each calculation,
>>>> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
>>>> +     */
>>>> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>>>> +                              DIRTY_RATE_STATUS_UNSTARTED);
>>>
>>> Is there a reason for this restriction? Removing it would require
>>> clarifying the state model, I suppose.
>>>
>> We only support query once for each calculation.
>> Otherwise, it could always query dirtyrate, but maybe the dirtyrate is calculated
>> long time ago.
> 
> There's nothing in the current interface that prevents this from being
> the case already - the caller could initiate a 1 second sample, then
> wait 24 hours to query the result.
> 
> Obviously this would generally be regarded as "d'oh - don't do that",
> but the same argument would apply if the caller is allowed to query the
> results multiple times.
> 
> Perhaps a complete solution would be to include information about the
> sample period with the result. The caller could then determine whether
> the sample is of adequate quality (sufficiently recent, taken over a
> sufficiently long time period) for its' intended use.
> 
You mean add timestamp when i calculate?
Actually, I do not want make it complicate for qemu code,
maybe it could be left for user to implement both two qmp commands
like in libvirt-api.

On the other hand, it really bother me that we need to reset calculating state
to make sure the state model could be restart in next calculation.

For now, i put it after query_dirty_rate_info is finished as you see, it should not be a good idea:(

Maybe it is better to initialize at the beginning of qmp_calc_dirty_rate().

> dme.
> 

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

[David Edmondson]

On Thursday, 2020-08-27 at 20:55:51 +08, Zheng Chuan wrote:

> On 2020/8/27 19:58, David Edmondson wrote:
>> On Thursday, 2020-08-27 at 17:34:13 +08, Zheng Chuan wrote:
>> 
>>>>> +    /*
>>>>> +     * Only support query once for each calculation,
>>>>> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
>>>>> +     */
>>>>> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>>>>> +                              DIRTY_RATE_STATUS_UNSTARTED);
>>>>
>>>> Is there a reason for this restriction? Removing it would require
>>>> clarifying the state model, I suppose.
>>>>
>>> We only support query once for each calculation.
>>> Otherwise, it could always query dirtyrate, but maybe the dirtyrate is calculated
>>> long time ago.
>> 
>> There's nothing in the current interface that prevents this from being
>> the case already - the caller could initiate a 1 second sample, then
>> wait 24 hours to query the result.
>> 
>> Obviously this would generally be regarded as "d'oh - don't do that",
>> but the same argument would apply if the caller is allowed to query the
>> results multiple times.
>> 
>> Perhaps a complete solution would be to include information about the
>> sample period with the result. The caller could then determine whether
>> the sample is of adequate quality (sufficiently recent, taken over a
>> sufficiently long time period) for its' intended use.
>> 
> You mean add timestamp when i calculate?

You already have a timestamp, though I'm not sure if it is one that is
appropriate to report to a user.

I was thinking that you would include both the start time and duration
of the sample in the output of the query-dirty-rate QMP command, as well
as the dirty rate itself. That way the caller can make a decision about
whether the data is useful.

> Actually, I do not want make it complicate for qemu code,
> maybe it could be left for user to implement both two qmp commands
> like in libvirt-api.

Sorry, I didn't understand this comment.

> On the other hand, it really bother me that we need to reset calculating state
> to make sure the state model could be restart in next calculation.
>
> For now, i put it after query_dirty_rate_info is finished as you see, it should not be a good idea:(
>
> Maybe it is better to initialize at the beginning of qmp_calc_dirty_rate().
>
>> dme.
>> 

dme.
-- 
Another lonely day, no one here but me-o.

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

[Zheng Chuan]

On 2020/8/27 21:07, David Edmondson wrote:
> On Thursday, 2020-08-27 at 20:55:51 +08, Zheng Chuan wrote:
> 
>> On 2020/8/27 19:58, David Edmondson wrote:
>>> On Thursday, 2020-08-27 at 17:34:13 +08, Zheng Chuan wrote:
>>>
>>>>>> +    /*
>>>>>> +     * Only support query once for each calculation,
>>>>>> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
>>>>>> +     */
>>>>>> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>>>>>> +                              DIRTY_RATE_STATUS_UNSTARTED);
>>>>>
>>>>> Is there a reason for this restriction? Removing it would require
>>>>> clarifying the state model, I suppose.
>>>>>
>>>> We only support query once for each calculation.
>>>> Otherwise, it could always query dirtyrate, but maybe the dirtyrate is calculated
>>>> long time ago.
>>>
>>> There's nothing in the current interface that prevents this from being
>>> the case already - the caller could initiate a 1 second sample, then
>>> wait 24 hours to query the result.
>>>
>>> Obviously this would generally be regarded as "d'oh - don't do that",
>>> but the same argument would apply if the caller is allowed to query the
>>> results multiple times.
>>>
>>> Perhaps a complete solution would be to include information about the
>>> sample period with the result. The caller could then determine whether
>>> the sample is of adequate quality (sufficiently recent, taken over a
>>> sufficiently long time period) for its' intended use.
>>>
>> You mean add timestamp when i calculate?
> 
> You already have a timestamp, though I'm not sure if it is one that is
> appropriate to report to a user.
> 
> I was thinking that you would include both the start time and duration
> of the sample in the output of the query-dirty-rate QMP command, as well
> as the dirty rate itself. That way the caller can make a decision about
> whether the data is useful.
> 
OK, i understand.
I may add it like this:
+##
+{ 'struct': 'DirtyRateInfo',
+  'data': {'dirty-rate': 'int64',
+           'status': 'DirtyRateStatus',
+           'start-timestamp': 'int64',
+           'calc-time': 'int64'} }
+
+##
the stat-timestamp would be initial_time which gets from qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
at the beginning of calculation while calc_time is time-duration in microsecond.

But i reconsider that, it maybe still need to reset the CalculatingState as DIRTY_RATE_STATUS_UNSTARTED
here?

Initialization like:
void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
{
   XXXX

    if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) {
        return;
    }


    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
                              DIRTY_RATE_STATUS_UNSTARTED);
    XXXX
}

It could not prevent concurrent scene which may lead to disorder state:(


>> Actually, I do not want make it complicate for qemu code,
>> maybe it could be left for user to implement both two qmp commands
>> like in libvirt-api.
> 
> Sorry, I didn't understand this comment.
> 
>> On the other hand, it really bother me that we need to reset calculating state
>> to make sure the state model could be restart in next calculation.
>>
>> For now, i put it after query_dirty_rate_info is finished as you see, it should not be a good idea:(
>>
>> Maybe it is better to initialize at the beginning of qmp_calc_dirty_rate().
>>
>>> dme.
>>>
> 
> dme.
> 

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

[David Edmondson]

On Thursday, 2020-08-27 at 22:47:15 +08, Zheng Chuan wrote:

> On 2020/8/27 21:07, David Edmondson wrote:
>> On Thursday, 2020-08-27 at 20:55:51 +08, Zheng Chuan wrote:
>> 
>>> On 2020/8/27 19:58, David Edmondson wrote:
>>>> On Thursday, 2020-08-27 at 17:34:13 +08, Zheng Chuan wrote:
>>>>
>>>>>>> +    /*
>>>>>>> +     * Only support query once for each calculation,
>>>>>>> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
>>>>>>> +     */
>>>>>>> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>>>>>>> +                              DIRTY_RATE_STATUS_UNSTARTED);
>>>>>>
>>>>>> Is there a reason for this restriction? Removing it would require
>>>>>> clarifying the state model, I suppose.
>>>>>>
>>>>> We only support query once for each calculation.
>>>>> Otherwise, it could always query dirtyrate, but maybe the dirtyrate is calculated
>>>>> long time ago.
>>>>
>>>> There's nothing in the current interface that prevents this from being
>>>> the case already - the caller could initiate a 1 second sample, then
>>>> wait 24 hours to query the result.
>>>>
>>>> Obviously this would generally be regarded as "d'oh - don't do that",
>>>> but the same argument would apply if the caller is allowed to query the
>>>> results multiple times.
>>>>
>>>> Perhaps a complete solution would be to include information about the
>>>> sample period with the result. The caller could then determine whether
>>>> the sample is of adequate quality (sufficiently recent, taken over a
>>>> sufficiently long time period) for its' intended use.
>>>>
>>> You mean add timestamp when i calculate?
>> 
>> You already have a timestamp, though I'm not sure if it is one that is
>> appropriate to report to a user.
>> 
>> I was thinking that you would include both the start time and duration
>> of the sample in the output of the query-dirty-rate QMP command, as well
>> as the dirty rate itself. That way the caller can make a decision about
>> whether the data is useful.
>> 
> OK, i understand.
> I may add it like this:
> +##
> +{ 'struct': 'DirtyRateInfo',
> +  'data': {'dirty-rate': 'int64',
> +           'status': 'DirtyRateStatus',
> +           'start-timestamp': 'int64',
> +           'calc-time': 'int64'} }
> +
> +##
> the stat-timestamp would be initial_time which gets from qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
> at the beginning of calculation while calc_time is time-duration in microsecond.

The calc-time reported here should be in the same units as when it is
specified in calc-dirty-rate (seconds for both seems fine).

I suspect that providing the start-timestamp in seconds would also be
fine - it's not obvious that knowing the value in milliseconds adds much
value.

> But i reconsider that, it maybe still need to reset the CalculatingState as DIRTY_RATE_STATUS_UNSTARTED
> here?
>
> Initialization like:
> void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
> {
>    XXXX
>
>     if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) {
>         return;
>     }
>
>
>     (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>                               DIRTY_RATE_STATUS_UNSTARTED);
>     XXXX
> }
>
> It could not prevent concurrent scene which may lead to disorder state:(

It should be possible to initiate measurement when the state is either
UNSTARTED or MEASURED - only MEASURING should rule it out.

>
>
>>> Actually, I do not want make it complicate for qemu code,
>>> maybe it could be left for user to implement both two qmp commands
>>> like in libvirt-api.
>> 
>> Sorry, I didn't understand this comment.
>> 
>>> On the other hand, it really bother me that we need to reset calculating state
>>> to make sure the state model could be restart in next calculation.
>>>
>>> For now, i put it after query_dirty_rate_info is finished as you see, it should not be a good idea:(
>>>
>>> Maybe it is better to initialize at the beginning of qmp_calc_dirty_rate().
>>>
>>>> dme.
>>>>
>> 
>> dme.
>> 

dme.
-- 
Everyone I know, goes away in the end.

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

[Chuan Zheng]

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"}'

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 RamlockDirtyInfo 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 get_sample_page_period() and
    block_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  | 432 +++++++++++++++++++++++++++++++++++++++++++++++++
 migration/dirtyrate.h  |  87 ++++++++++
 migration/meson.build  |   1 +
 migration/ram.c        |  11 +-
 migration/ram.h        |  10 ++
 migration/trace-events |   8 +
 qapi/migration.json    |  61 +++++++
 7 files changed, 600 insertions(+), 10 deletions(-)
 create mode 100644 migration/dirtyrate.c
 create mode 100644 migration/dirtyrate.h

-- 
1.8.3.1