Re: [PATCH v3 00/10] *** A Method for evaluating dirty page rate ***

Re: [PATCH v3 00/10] *** A Method for evaluating dirty page rate ***

[no-reply]

Patchew URL: https://patchew.org/QEMU/1597634433-18809-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: 1597634433-18809-1-git-send-email-zhengchuan@huawei.com
Subject: [PATCH v3 00/10] *** 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
 * [new tag]         patchew/1597634433-18809-1-git-send-email-zhengchuan@huawei.com -> patchew/1597634433-18809-1-git-send-email-zhengchuan@huawei.com
Switched to a new branch 'test'
ce6fe0d migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function
96ce007 migration/dirtyrate: Implement calculate_dirtyrate() function
42642d4 migration/dirtyrate: Implement get_sample_page_period() and block_sample_page_period()
3a4d4a2 migration/dirtyrate: skip sampling ramblock with size below MIN_RAMBLOCK_SIZE
31f35b7 migration/dirtyrate: Compare page hash results for recorded sampled page
a3d582d migration/dirtyrate: Record hash results for each sampled page
85c6447 migration/dirtyrate: move RAMBLOCK_FOREACH_MIGRATABLE into ram.h
c7c94cc migration/dirtyrate: Add dirtyrate statistics series functions
a76c0d0 migration/dirtyrate: Add RamlockDirtyInfo to store sampled page info
7f6092c migration/dirtyrate: Add get_dirtyrate_thread() function

=== OUTPUT BEGIN ===
1/10 Checking commit 7f6092c52d5c (migration/dirtyrate: Add get_dirtyrate_thread() function)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#26: 
new file mode 100644

total: 0 errors, 1 warnings, 115 lines checked

Patch 1/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/10 Checking commit a76c0d08e93d (migration/dirtyrate: Add RamlockDirtyInfo to store sampled page info)
3/10 Checking commit c7c94cc4c17d (migration/dirtyrate: Add dirtyrate statistics series functions)
4/10 Checking commit 85c64473ab70 (migration/dirtyrate: move RAMBLOCK_FOREACH_MIGRATABLE into ram.h)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#63: 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
#65: FILE: migration/ram.h:44:
+        if (ramblock_is_ignored(block)) {} else

ERROR: Macros with multiple statements should be enclosed in a do - while loop
#67: 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
#69: FILE: migration/ram.h:48:
+        if (!qemu_ram_is_migratable(block)) {} else

ERROR: braces {} are necessary for all arms of this statement
#69: 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 4/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/10 Checking commit a3d582d88093 (migration/dirtyrate: Record hash results for each sampled page)
6/10 Checking commit 31f35b73e761 (migration/dirtyrate: Compare page hash results for recorded sampled page)
7/10 Checking commit 3a4d4a207812 (migration/dirtyrate: skip sampling ramblock with size below MIN_RAMBLOCK_SIZE)
8/10 Checking commit 42642d45cf29 (migration/dirtyrate: Implement get_sample_page_period() and block_sample_page_period())
9/10 Checking commit 96ce00742d23 (migration/dirtyrate: Implement calculate_dirtyrate() function)
10/10 Checking commit ce6fe0d124c5 (migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/1597634433-18809-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

[PATCH v3 00/10] *** A Method for evaluating dirty page rate ***

[Chuan Zheng]

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   |  1188MB/s |   536MB/s ~ 1044MB/s (cpu throttle triggered) |
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
| mempress 4096 4096   |  4152MB/s |     608MB/s ~ 4125MB/s (migation failed)      |
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

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]

Zheng Chuan (10):
  migration/dirtyrate: Add get_dirtyrate_thread() function
  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/Makefile.objs |   1 +
 migration/dirtyrate.c   | 448 ++++++++++++++++++++++++++++++++++++++++++++++++
 migration/dirtyrate.h   |  86 ++++++++++
 migration/ram.c         |  11 +-
 migration/ram.h         |  10 ++
 qapi/migration.json     |  42 +++++
 6 files changed, 588 insertions(+), 10 deletions(-)
 create mode 100644 migration/dirtyrate.c
 create mode 100644 migration/dirtyrate.h

-- 
1.8.3.1

[PATCH v3 01/10] migration/dirtyrate: Add get_dirtyrate_thread() function

[Chuan Zheng]

Add get_dirtyrate_thread() functions

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
---
 migration/Makefile.objs |  1 +
 migration/dirtyrate.c   | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
 migration/dirtyrate.h   | 44 ++++++++++++++++++++++++++++++++++
 3 files changed, 109 insertions(+)
 create mode 100644 migration/dirtyrate.c
 create mode 100644 migration/dirtyrate.h

diff --git a/migration/Makefile.objs b/migration/Makefile.objs
index 0fc619e..12ae98c 100644
--- a/migration/Makefile.objs
+++ b/migration/Makefile.objs
@@ -6,6 +6,7 @@ common-obj-y += qemu-file.o global_state.o
 common-obj-y += qemu-file-channel.o
 common-obj-y += xbzrle.o postcopy-ram.o
 common-obj-y += qjson.o
+common-obj-y += dirtyrate.o
 common-obj-y += block-dirty-bitmap.o
 common-obj-y += multifd.o
 common-obj-y += multifd-zlib.o
diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
new file mode 100644
index 0000000..bb0ebe9
--- /dev/null
+++ b/migration/dirtyrate.c
@@ -0,0 +1,64 @@
+/*
+ * 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"
+
+CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
+
+static int dirty_rate_set_state(int new_state)
+{
+    int old_state = CalculatingState;
+
+    if (new_state == old_state) {
+        return -1;
+    }
+
+    if (atomic_cmpxchg(&CalculatingState, old_state, new_state) != old_state) {
+        return -1;
+    }
+
+    return 0;
+}
+
+static void calculate_dirtyrate(struct DirtyRateConfig config)
+{
+    /* todo */
+    return;
+}
+
+void *get_dirtyrate_thread(void *arg)
+{
+    struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
+    int ret;
+
+    ret = dirty_rate_set_state(CAL_DIRTY_RATE_ACTIVE);
+    if (ret == -1) {
+        return NULL;
+    }
+
+    calculate_dirtyrate(config);
+
+    ret = dirty_rate_set_state(CAL_DIRTY_RATE_END);
+
+    return NULL;
+}
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
new file mode 100644
index 0000000..914c363
--- /dev/null
+++ b/migration/dirtyrate.h
@@ -0,0 +1,44 @@
+/*
+ *  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 256 pages per GB as default.
+ * TODO: Make it configurable.
+ */
+#define DIRTYRATE_DEFAULT_SAMPLE_PAGES            256
+
+/* 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 */
+};
+
+/*
+ *  To record calculate dirty_rate status:
+ *  0: initial status, calculating thread is not be created here.
+ *  1: calculating thread is created.
+ *  2: calculating thread is end, we can get result.
+ */
+typedef enum {
+    CAL_DIRTY_RATE_INIT = 0,
+    CAL_DIRTY_RATE_ACTIVE,
+    CAL_DIRTY_RATE_END,
+} CalculatingDirtyRateState;
+
+void *get_dirtyrate_thread(void *arg);
+#endif
+
-- 
1.8.3.1

[PATCH v3 02/10] 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>
Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
---
 migration/dirtyrate.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 914c363..9650566 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -19,6 +19,11 @@
  */
 #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            256
 
+/*
+ * Record ramblock idstr
+ */
+#define RAMBLOCK_INFO_MAX_LEN                     256
+
 /* Take 1s as default for calculation duration */
 #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
 
@@ -39,6 +44,19 @@ typedef enum {
     CAL_DIRTY_RATE_END,
 } CalculatingDirtyRateState;
 
+/*
+ * 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 */
+    size_t ramblock_pages; /* sum of dividation by 4K pages for ramblock */
+    size_t *sample_page_vfn; /* relative offset address for sampled page */
+    unsigned int sample_pages_count; /* sum of sampled pages */
+    unsigned int sample_dirty_count; /* sum of dirty pages we measure */
+    uint8_t *hash_result; /* array of hash result for sampled pages */
+};
+
 void *get_dirtyrate_thread(void *arg);
 #endif
 
-- 
1.8.3.1

[PATCH v3 03/10] 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>
Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
---
 migration/dirtyrate.c | 30 ++++++++++++++++++++++++++++++
 migration/dirtyrate.h | 10 ++++++++++
 2 files changed, 40 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index bb0ebe9..8708090 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -24,6 +24,7 @@
 #include "dirtyrate.h"
 
 CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
+static struct DirtyRateStat dirty_stat;
 
 static int dirty_rate_set_state(int new_state)
 {
@@ -40,6 +41,35 @@ static int dirty_rate_set_state(int new_state)
     return 0;
 }
 
+static void reset_dirtyrate_stat(void)
+{
+    dirty_stat.total_dirty_samples = 0;
+    dirty_stat.total_sample_count = 0;
+    dirty_stat.total_block_mem_MB = 0;
+    dirty_stat.dirty_rate = 0;
+}
+
+static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
+{
+    dirty_stat.total_dirty_samples += info->sample_dirty_count;
+    dirty_stat.total_sample_count += info->sample_pages_count;
+    /* size of 4K pages in MB */
+    dirty_stat.total_block_mem_MB += info->ramblock_pages / 256;
+}
+
+static void update_dirtyrate(uint64_t msec)
+{
+    uint64_t dirty_rate;
+    unsigned int total_dirty_samples = dirty_stat.total_dirty_samples;
+    unsigned int total_sample_count = dirty_stat.total_sample_count;
+    size_t total_block_mem_MB = dirty_stat.total_block_mem_MB;
+
+    dirty_rate = total_dirty_samples * total_block_mem_MB *
+                 1000 / (total_sample_count * msec);
+
+    dirty_stat.dirty_rate = dirty_rate;
+}
+
 static void calculate_dirtyrate(struct DirtyRateConfig config)
 {
     /* todo */
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 9650566..af57c80 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -57,6 +57,16 @@ struct RamblockDirtyInfo {
     uint8_t *hash_result; /* array of hash result for sampled pages */
 };
 
+/*
+ * Store calculate statistics for each measure.
+ */
+struct DirtyRateStat {
+    unsigned int total_dirty_samples; /* total dirty pages for this measure */
+    unsigned int total_sample_count; /* total sampled pages for this measure */
+    size_t total_block_mem_MB; /* size of sampled pages in MB */
+    int64_t dirty_rate; /* dirty rate for this measure */
+};
+
 void *get_dirtyrate_thread(void *arg);
 #endif
 
-- 
1.8.3.1

[PATCH v3 04/10] 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>
Signed-off-by: YanYing Zhuang <ann.zhuangyanying@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 8708090..c4304ef 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"
 
 CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
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 v3 05/10] migration/dirtyrate: Record hash results for each sampled page

[Chuan Zheng]

Record hash results for each sampled page.

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

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index c4304ef..62b6f69 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -25,6 +25,7 @@
 #include "dirtyrate.h"
 
 CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
+static unsigned long int qcrypto_hash_len = QCRYPTO_HASH_LEN;
 static struct DirtyRateStat dirty_stat;
 
 static int dirty_rate_set_state(int new_state)
@@ -71,6 +72,149 @@ static void update_dirtyrate(uint64_t msec)
     dirty_stat.dirty_rate = dirty_rate;
 }
 
+/*
+ * get hash result for the sampled memory with length of 4K byte in ramblock,
+ * which starts from ramblock base address.
+ */
+static int get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
+                                 unsigned long vfn, uint8_t **md)
+{
+    struct iovec iov_array;
+    int ret = 0;
+    int nkey = 1;
+    size_t hash_len = qcrypto_hash_len;
+
+    iov_array.iov_base = info->ramblock_addr +
+                         vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
+    iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;
+
+    if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_MD5,
+                            &iov_array, nkey,
+                            md, &hash_len, NULL) < 0) {
+        ret = -1;
+    }
+
+    return ret;
+}
+
+static int save_ramblock_hash(struct RamblockDirtyInfo *info)
+{
+    unsigned int sample_pages_count;
+    uint8_t *md = NULL;
+    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(uint8_t) * qcrypto_hash_len);
+    if (!info->hash_result) {
+        ret = -1;
+        goto out;
+    }
+
+    info->sample_page_vfn = g_try_malloc0_n(sample_pages_count,
+                                            sizeof(unsigned long));
+    if (!info->sample_page_vfn) {
+        g_free(info->hash_result);
+        ret = -1;
+        goto out;
+    }
+
+    for (i = 0; i < sample_pages_count; i++) {
+        md = info->hash_result + i * qcrypto_hash_len;
+        info->sample_page_vfn[i] = g_rand_int_range(rand, 0,
+                                                    info->ramblock_pages - 1);
+        ret = get_ramblock_vfn_hash(info, info->sample_page_vfn[i], &md);
+        if (ret < 0) {
+            goto out;
+        }
+    }
+    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) >> 30;
+
+    /* Right shift 12 bits to calc page count in 4KB */
+    info->ramblock_pages = qemu_ram_get_used_length(block) >> 12;
+    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) {
+        block_dinfo = g_try_new(struct RamblockDirtyInfo, 1);
+        index = 0;
+    } else {
+        index++;
+        block_dinfo = g_try_realloc(block_dinfo, (index + 1) *
+                                    sizeof(struct RamblockDirtyInfo));
+    }
+    if (!block_dinfo) {
+        return NULL;
+    }
+
+    info = &block_dinfo[index];
+    memset(info, 0, sizeof(struct RamblockDirtyInfo));
+
+    *block_index = index;
+    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 af57c80..0812b16 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -20,10 +20,17 @@
 #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            256
 
 /*
+ * Sample page size 4K as default.
+ */
+#define DIRTYRATE_SAMPLE_PAGE_SIZE                4096
+
+/*
  * Record ramblock idstr
  */
 #define RAMBLOCK_INFO_MAX_LEN                     256
 
+#define QCRYPTO_HASH_LEN                          16
+
 /* Take 1s as default for calculation duration */
 #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
 
-- 
1.8.3.1

[PATCH v3 06/10] 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 | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 62b6f69..3ce25f5 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -215,6 +215,82 @@ static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
     return 0;
 }
 
+static int calc_page_dirty_rate(struct RamblockDirtyInfo *info)
+{
+    uint8_t *md = NULL;
+    int i;
+    int ret = 0;
+
+    md = g_try_new0(uint8_t, qcrypto_hash_len);
+    if (!md) {
+        return -1;
+    }
+
+    for (i = 0; i < info->sample_pages_count; i++) {
+        ret = get_ramblock_vfn_hash(info, info->sample_page_vfn[i], &md);
+        if (ret < 0) {
+            goto out;
+        }
+
+        if (memcmp(md, info->hash_result + i * qcrypto_hash_len,
+                   qcrypto_hash_len) != 0) {
+            info->sample_dirty_count++;
+        }
+    }
+
+out:
+    g_free(md);
+    return ret;
+}
+
+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) >> 12)) {
+        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 (!dirty_stat.total_sample_count) {
+        return -1;
+    }
+
+    return 0;
+}
+
 static void calculate_dirtyrate(struct DirtyRateConfig config)
 {
     /* todo */
-- 
1.8.3.1

[PATCH v3 07/10] migration/dirtyrate: skip sampling ramblock with size below MIN_RAMBLOCK_SIZE

[Chuan Zheng]

In order to sample real RAM, skip ramblock with size below

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

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 3ce25f5..6f30f67 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -186,6 +186,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) >> 20;
+
+    /*
+     * 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)
@@ -196,6 +214,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;
@@ -275,6 +296,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 0812b16..fce2e3b 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -31,6 +31,11 @@
 
 #define QCRYPTO_HASH_LEN                          16
 
+/*
+ * 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 v3 08/10] 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>
Signed-off-by: YanYing Zhuang <ann.zhuangyanying@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 6f30f67..4bbfcc3 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -28,6 +28,30 @@ CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
 static unsigned long int qcrypto_hash_len = QCRYPTO_HASH_LEN;
 static struct DirtyRateStat dirty_stat;
 
+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 dirty_rate_set_state(int new_state)
 {
     int old_state = CalculatingState;
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index fce2e3b..86d8fa0 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -38,6 +38,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 v3 09/10] 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 | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 4bbfcc3..041d0c6 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -184,6 +184,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)
@@ -341,8 +356,35 @@ 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 v3 10/10] 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 | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/migration.json   | 42 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 041d0c6..0e8c9c5 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -67,6 +67,39 @@ static int dirty_rate_set_state(int new_state)
     return 0;
 }
 
+static struct DirtyRateInfo *query_dirty_rate_info(void)
+{
+    int64_t dirty_rate = dirty_stat.dirty_rate;
+    struct DirtyRateInfo *info = g_malloc0(sizeof(DirtyRateInfo));
+
+    switch (CalculatingState) {
+    case CAL_DIRTY_RATE_INIT:
+        info->dirty_rate = -1;
+        info->status = g_strdup("Not start measuring");
+        break;
+    case CAL_DIRTY_RATE_ACTIVE:
+        info->dirty_rate = -1;
+        info->status = g_strdup("Still measuring");
+        break;
+    case CAL_DIRTY_RATE_END:
+        info->dirty_rate = dirty_rate;
+        info->status = g_strdup("Measured");
+        break;
+    default:
+        info->dirty_rate = -1;
+        info->status = g_strdup("Unknown status");
+        break;
+    }
+
+    /*
+     * Only support query once for each calculation,
+     * reset as CAL_DIRTY_RATE_INIT after query
+     */
+    (void)dirty_rate_set_state(CAL_DIRTY_RATE_INIT);
+
+    return info;
+}
+
 static void reset_dirtyrate_stat(void)
 {
     dirty_stat.total_dirty_samples = 0;
@@ -403,3 +436,26 @@ void *get_dirtyrate_thread(void *arg)
 
     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 == CAL_DIRTY_RATE_ACTIVE) {
+        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 d500055..ccc7a4e 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1621,3 +1621,45 @@
 ##
 { 'event': 'UNPLUG_PRIMARY',
   'data': { 'device-id': 'str' } }
+
+##
+# @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
+#       status with 'not start measuring' or
+#       'Still measuring' or 'measured'(since 5.2)
+##
+{ 'struct': 'DirtyRateInfo',
+  'data': {'dirty-rate': 'int64',
+           'status': 'str'} }
+
+##
+# @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

Re: [PATCH v3 01/10] migration/dirtyrate: Add get_dirtyrate_thread() function

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> Add get_dirtyrate_thread() functions
> 
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> ---
>  migration/Makefile.objs |  1 +
>  migration/dirtyrate.c   | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>  migration/dirtyrate.h   | 44 ++++++++++++++++++++++++++++++++++
>  3 files changed, 109 insertions(+)
>  create mode 100644 migration/dirtyrate.c
>  create mode 100644 migration/dirtyrate.h
> 
> diff --git a/migration/Makefile.objs b/migration/Makefile.objs
> index 0fc619e..12ae98c 100644
> --- a/migration/Makefile.objs
> +++ b/migration/Makefile.objs
> @@ -6,6 +6,7 @@ common-obj-y += qemu-file.o global_state.o
>  common-obj-y += qemu-file-channel.o
>  common-obj-y += xbzrle.o postcopy-ram.o
>  common-obj-y += qjson.o
> +common-obj-y += dirtyrate.o
>  common-obj-y += block-dirty-bitmap.o
>  common-obj-y += multifd.o
>  common-obj-y += multifd-zlib.o
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> new file mode 100644
> index 0000000..bb0ebe9
> --- /dev/null
> +++ b/migration/dirtyrate.c
> @@ -0,0 +1,64 @@
> +/*
> + * 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"
> +
> +CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
> +
> +static int dirty_rate_set_state(int new_state)
> +{
> +    int old_state = CalculatingState;
> +
> +    if (new_state == old_state) {
> +        return -1;
> +    }
> +
> +    if (atomic_cmpxchg(&CalculatingState, old_state, new_state) != old_state) {
> +        return -1;
> +    }

This is a little unusual; this has removed your comment from v1 about
what you're trying to protect; but not quite being clear about what it's
doing.

I think what you want here is closer to migrate_set_state, ie you
pass what you think the old state is, and the state you want to go to.

> +    return 0;
> +}
> +
> +static void calculate_dirtyrate(struct DirtyRateConfig config)
> +{
> +    /* todo */
> +    return;
> +}
> +
> +void *get_dirtyrate_thread(void *arg)
> +{
> +    struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
> +    int ret;
> +
> +    ret = dirty_rate_set_state(CAL_DIRTY_RATE_ACTIVE);

so this would become:
    ret = dirty_rate_set_state(CAL_DIRTY_RATE_INIT,
              CAL_DIRTY_RATE_ACTIVE);

> +    if (ret == -1) {
> +        return NULL;
> +    }
> +
> +    calculate_dirtyrate(config);
> +
> +    ret = dirty_rate_set_state(CAL_DIRTY_RATE_END);
> +
> +    return NULL;
> +}
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> new file mode 100644
> index 0000000..914c363
> --- /dev/null
> +++ b/migration/dirtyrate.h
> @@ -0,0 +1,44 @@
> +/*
> + *  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 256 pages per GB as default.
> + * TODO: Make it configurable.
> + */
> +#define DIRTYRATE_DEFAULT_SAMPLE_PAGES            256
> +
> +/* 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 */
> +};
> +
> +/*
> + *  To record calculate dirty_rate status:
> + *  0: initial status, calculating thread is not be created here.
> + *  1: calculating thread is created.
> + *  2: calculating thread is end, we can get result.
> + */
> +typedef enum {
> +    CAL_DIRTY_RATE_INIT = 0,
> +    CAL_DIRTY_RATE_ACTIVE,
> +    CAL_DIRTY_RATE_END,
> +} CalculatingDirtyRateState;
> +
> +void *get_dirtyrate_thread(void *arg);
> +#endif
> +
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 02/10] migration/dirtyrate: Add RamlockDirtyInfo to store sampled page info

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> Add RamlockDirtyInfo to store sampled page info of each ramblock.
> 
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> ---
>  migration/dirtyrate.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 914c363..9650566 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -19,6 +19,11 @@
>   */
>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            256
>  
> +/*
> + * Record ramblock idstr
> + */
> +#define RAMBLOCK_INFO_MAX_LEN                     256
> +
>  /* Take 1s as default for calculation duration */
>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>  
> @@ -39,6 +44,19 @@ typedef enum {
>      CAL_DIRTY_RATE_END,
>  } CalculatingDirtyRateState;
>  
> +/*
> + * Store dirtypage info for each ramblock.
> + */
> +struct RamblockDirtyInfo {
> +    char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */

Can you remind me; why not just use RAMBlock* here of the block you're
interested in, rather than storing the name?

> +    uint8_t *ramblock_addr; /* base address of ramblock we measure */
> +    size_t ramblock_pages; /* sum of dividation by 4K pages for ramblock */

'dividation' is the wrong word, and 'sum' is only needed where you're
adding things together.  I think this is 'ramblock size in TARGET_PAGEs'

> +    size_t *sample_page_vfn; /* relative offset address for sampled page */
> +    unsigned int sample_pages_count; /* sum of sampled pages */
> +    unsigned int sample_dirty_count; /* sum of dirty pages we measure */

These are both 'count' rather than 'sum'

> +    uint8_t *hash_result; /* array of hash result for sampled pages */
> +};
> +
>  void *get_dirtyrate_thread(void *arg);
>  #endif
>  
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 03/10] migration/dirtyrate: Add dirtyrate statistics series functions

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> Add dirtyrate statistics to record/update dirtyrate info.
> 
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> ---
>  migration/dirtyrate.c | 30 ++++++++++++++++++++++++++++++
>  migration/dirtyrate.h | 10 ++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index bb0ebe9..8708090 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -24,6 +24,7 @@
>  #include "dirtyrate.h"
>  
>  CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
> +static struct DirtyRateStat dirty_stat;
>  
>  static int dirty_rate_set_state(int new_state)
>  {
> @@ -40,6 +41,35 @@ static int dirty_rate_set_state(int new_state)
>      return 0;
>  }
>  
> +static void reset_dirtyrate_stat(void)
> +{
> +    dirty_stat.total_dirty_samples = 0;
> +    dirty_stat.total_sample_count = 0;
> +    dirty_stat.total_block_mem_MB = 0;
> +    dirty_stat.dirty_rate = 0;
> +}
> +
> +static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
> +{
> +    dirty_stat.total_dirty_samples += info->sample_dirty_count;
> +    dirty_stat.total_sample_count += info->sample_pages_count;
> +    /* size of 4K pages in MB */
> +    dirty_stat.total_block_mem_MB += info->ramblock_pages / 256;
> +}
> +
> +static void update_dirtyrate(uint64_t msec)
> +{
> +    uint64_t dirty_rate;
> +    unsigned int total_dirty_samples = dirty_stat.total_dirty_samples;
> +    unsigned int total_sample_count = dirty_stat.total_sample_count;
> +    size_t total_block_mem_MB = dirty_stat.total_block_mem_MB;
> +
> +    dirty_rate = total_dirty_samples * total_block_mem_MB *
> +                 1000 / (total_sample_count * msec);
> +
> +    dirty_stat.dirty_rate = dirty_rate;
> +}
> +
>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>  {
>      /* todo */
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 9650566..af57c80 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -57,6 +57,16 @@ struct RamblockDirtyInfo {
>      uint8_t *hash_result; /* array of hash result for sampled pages */
>  };
>  
> +/*
> + * Store calculate statistics for each measure.
> + */
> +struct DirtyRateStat {
> +    unsigned int total_dirty_samples; /* total dirty pages for this measure */
> +    unsigned int total_sample_count; /* total sampled pages for this measure */
> +    size_t total_block_mem_MB; /* size of sampled pages in MB */
> +    int64_t dirty_rate; /* dirty rate for this measure */

As I said in the previous review, please comment 'dirty_rate' with it's
units.

Dave

> +};
> +
>  void *get_dirtyrate_thread(void *arg);
>  #endif
>  
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 04/10] migration/dirtyrate: move RAMBLOCK_FOREACH_MIGRATABLE into ram.h

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> RAMBLOCK_FOREACH_MIGRATABLE is need in dirtyrate measure,
> move the existing definition up into migration/ram.h
> 
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

(I noticed this triggered a few checker warnings, but this is just
moving an existing odd #define)



> ---
>  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 8708090..c4304ef 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"
>  
>  CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
> 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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 10/10] migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function

[Eric Blake]

On 8/16/20 10:20 PM, 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>
> ---

> +++ b/qapi/migration.json
> @@ -1621,3 +1621,45 @@
>   ##
>   { 'event': 'UNPLUG_PRIMARY',
>     'data': { 'device-id': 'str' } }
> +
> +##
> +# @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
> +#       status with 'not start measuring' or
> +#       'Still measuring' or 'measured'(since 5.2)

Missing space before (

> +##
> +{ 'struct': 'DirtyRateInfo',
> +  'data': {'dirty-rate': 'int64',
> +           'status': 'str'} }

Based on your description, this should be an enum type rather than an 
open-coded string.  Something like:

{ 'enum': 'DirtyRateStatus', 'data': [ 'unstarted', 'measuring', 
'measured' ] }
{ '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' }
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 05/10] migration/dirtyrate: Record hash results for each sampled page

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> Record hash results for each sampled page.
> 
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> ---
>  migration/dirtyrate.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  migration/dirtyrate.h |   7 +++
>  2 files changed, 151 insertions(+)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index c4304ef..62b6f69 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -25,6 +25,7 @@
>  #include "dirtyrate.h"
>  
>  CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
> +static unsigned long int qcrypto_hash_len = QCRYPTO_HASH_LEN;

Why do we need this static rather than just using the QCRYPTO_HASH_LEN ?
It's never going to change is it?
(and anyway it's just a MD5 len?)

>  static struct DirtyRateStat dirty_stat;
>  
>  static int dirty_rate_set_state(int new_state)
> @@ -71,6 +72,149 @@ static void update_dirtyrate(uint64_t msec)
>      dirty_stat.dirty_rate = dirty_rate;
>  }
>  
> +/*
> + * get hash result for the sampled memory with length of 4K byte in ramblock,
> + * which starts from ramblock base address.
> + */
> +static int get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
> +                                 unsigned long vfn, uint8_t **md)
> +{
> +    struct iovec iov_array;
> +    int ret = 0;
> +    int nkey = 1;
> +    size_t hash_len = qcrypto_hash_len;
> +
> +    iov_array.iov_base = info->ramblock_addr +
> +                         vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
> +    iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;
> +
> +    if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_MD5,
> +                            &iov_array, nkey,
> +                            md, &hash_len, NULL) < 0) {
> +        ret = -1;
> +    }
> +
> +    return ret;
> +}
> +
> +static int save_ramblock_hash(struct RamblockDirtyInfo *info)
> +{
> +    unsigned int sample_pages_count;
> +    uint8_t *md = NULL;
> +    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(uint8_t) * qcrypto_hash_len);
> +    if (!info->hash_result) {
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    info->sample_page_vfn = g_try_malloc0_n(sample_pages_count,
> +                                            sizeof(unsigned long));
> +    if (!info->sample_page_vfn) {
> +        g_free(info->hash_result);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    for (i = 0; i < sample_pages_count; i++) {
> +        md = info->hash_result + i * qcrypto_hash_len;
> +        info->sample_page_vfn[i] = g_rand_int_range(rand, 0,
> +                                                    info->ramblock_pages - 1);
> +        ret = get_ramblock_vfn_hash(info, info->sample_page_vfn[i], &md);
> +        if (ret < 0) {
> +            goto out;
> +        }
> +    }
> +    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) >> 30;
> +
> +    /* Right shift 12 bits to calc page count in 4KB */
> +    info->ramblock_pages = qemu_ram_get_used_length(block) >> 12;

Is this really >> 12 ?  Should it actually be 
   / DIRTYRATE_SAMPLE_PAGE_SIZE ?

(and should you need that or just use TARGET_PAGE_SIZE?)

> +    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) {
> +        block_dinfo = g_try_new(struct RamblockDirtyInfo, 1);
> +        index = 0;
> +    } else {
> +        index++;
> +        block_dinfo = g_try_realloc(block_dinfo, (index + 1) *
> +                                    sizeof(struct RamblockDirtyInfo));
> +    }
> +    if (!block_dinfo) {
> +        return NULL;
> +    }
> +
> +    info = &block_dinfo[index];
> +    memset(info, 0, sizeof(struct RamblockDirtyInfo));
> +
> +    *block_index = index;
> +    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;

You should add some trace_ calls at various places to make this easier
to debug.

Dave

> +}
> +
>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>  {
>      /* todo */
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index af57c80..0812b16 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -20,10 +20,17 @@
>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            256
>  
>  /*
> + * Sample page size 4K as default.
> + */
> +#define DIRTYRATE_SAMPLE_PAGE_SIZE                4096
> +
> +/*
>   * Record ramblock idstr
>   */
>  #define RAMBLOCK_INFO_MAX_LEN                     256
>  
> +#define QCRYPTO_HASH_LEN                          16
> +
>  /* Take 1s as default for calculation duration */
>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>  
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 06/10] migration/dirtyrate: Compare page hash results for recorded sampled page

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) 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 | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 62b6f69..3ce25f5 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -215,6 +215,82 @@ static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
>      return 0;
>  }
>  
> +static int calc_page_dirty_rate(struct RamblockDirtyInfo *info)
> +{
> +    uint8_t *md = NULL;
> +    int i;
> +    int ret = 0;
> +
> +    md = g_try_new0(uint8_t, qcrypto_hash_len);
> +    if (!md) {
> +        return -1;
> +    }

As previously asked; isn't this a nice small simple fixed length - no
need to allocate it?

> +
> +    for (i = 0; i < info->sample_pages_count; i++) {
> +        ret = get_ramblock_vfn_hash(info, info->sample_page_vfn[i], &md);
> +        if (ret < 0) {
> +            goto out;
> +        }
> +
> +        if (memcmp(md, info->hash_result + i * qcrypto_hash_len,
> +                   qcrypto_hash_len) != 0) {
> +            info->sample_dirty_count++;
> +        }
> +    }
> +
> +out:
> +    g_free(md);
> +    return ret;
> +}
> +
> +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) >> 12)) {
> +        return false;

I previously asked how this happens.
Also this was DIRTYRATE_PAGE_SIZE_SHIFT

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

So you've removed the selction of only some RAMBlocks now?

> +        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 (!dirty_stat.total_sample_count) {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>  {
>      /* todo */
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 07/10] migration/dirtyrate: skip sampling ramblock with size below MIN_RAMBLOCK_SIZE

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> In order to sample real RAM, skip ramblock with size below
> 
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>

OK,


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/dirtyrate.c | 24 ++++++++++++++++++++++++
>  migration/dirtyrate.h |  5 +++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 3ce25f5..6f30f67 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -186,6 +186,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) >> 20;
> +
> +    /*
> +     * 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)
> @@ -196,6 +214,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;
> @@ -275,6 +296,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 0812b16..fce2e3b 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -31,6 +31,11 @@
>  
>  #define QCRYPTO_HASH_LEN                          16
>  
> +/*
> + * 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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 05/10] migration/dirtyrate: Record hash results for each sampled page

[Daniel P. Berrangé]

On Thu, Aug 20, 2020 at 06:30:09PM +0100, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) wrote:
> > Record hash results for each sampled page.
> > 
> > Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> > Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> > ---
> >  migration/dirtyrate.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  migration/dirtyrate.h |   7 +++
> >  2 files changed, 151 insertions(+)
> > 
> > diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> > index c4304ef..62b6f69 100644
> > --- a/migration/dirtyrate.c
> > +++ b/migration/dirtyrate.c
> > @@ -25,6 +25,7 @@
> >  #include "dirtyrate.h"
> >  
> >  CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
> > +static unsigned long int qcrypto_hash_len = QCRYPTO_HASH_LEN;
> 
> Why do we need this static rather than just using the QCRYPTO_HASH_LEN ?
> It's never going to change is it?
> (and anyway it's just a MD5 len?)

I wouldn't want to bet on that given that this is use of MD5. We might
claim this isn't security critical, but surprises happen, and we will
certainly be dinged on security audits for introducing new use of MD5
no matter what.

If a cryptographic hash is required, then sha256 should be the choice
for any new code that doesn't have back compat requirements.

If a cryptographic hash is not required then how about crc32 

IOW, it doesn't make a whole lot of sense to say we need a cryptographic
hash, but then pick the most insecure one.

sha256 is slower than md5, but it is conceivable that in future we might
gain support for something like Blake2b which is similar security level
to SHA3, while being faster than MD5.

Overall I'm pretty unethusiastic about use of MD5 being introduced and
worse, being hardcoded as the only option.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 08/10] migration/dirtyrate: Implement get_sample_page_period() and block_sample_page_period()

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) 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>
> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@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 6f30f67..4bbfcc3 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -28,6 +28,30 @@ CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
>  static unsigned long int qcrypto_hash_len = QCRYPTO_HASH_LEN;
>  static struct DirtyRateStat dirty_stat;
>  
> +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;
> +}
> +

This is OK I think, but it does seem a bit complicated for just
waiting for a time.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>  static int dirty_rate_set_state(int new_state)
>  {
>      int old_state = CalculatingState;
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index fce2e3b..86d8fa0 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -38,6 +38,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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 05/10] migration/dirtyrate: Record hash results for each sampled page

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Thu, Aug 20, 2020 at 06:30:09PM +0100, Dr. David Alan Gilbert wrote:
> > * Chuan Zheng (zhengchuan@huawei.com) wrote:
> > > Record hash results for each sampled page.
> > > 
> > > Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> > > Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> > > ---
> > >  migration/dirtyrate.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  migration/dirtyrate.h |   7 +++
> > >  2 files changed, 151 insertions(+)
> > > 
> > > diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> > > index c4304ef..62b6f69 100644
> > > --- a/migration/dirtyrate.c
> > > +++ b/migration/dirtyrate.c
> > > @@ -25,6 +25,7 @@
> > >  #include "dirtyrate.h"
> > >  
> > >  CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
> > > +static unsigned long int qcrypto_hash_len = QCRYPTO_HASH_LEN;
> > 
> > Why do we need this static rather than just using the QCRYPTO_HASH_LEN ?
> > It's never going to change is it?
> > (and anyway it's just a MD5 len?)
> 
> I wouldn't want to bet on that given that this is use of MD5. We might
> claim this isn't security critical, but surprises happen, and we will
> certainly be dinged on security audits for introducing new use of MD5
> no matter what.
> 
> If a cryptographic hash is required, then sha256 should be the choice
> for any new code that doesn't have back compat requirements.
> 
> If a cryptographic hash is not required then how about crc32 

It doesn't need to be cryptographic; is crc32 the fastest reasonable hash for use
in large areas?

Dave

> IOW, it doesn't make a whole lot of sense to say we need a cryptographic
> hash, but then pick the most insecure one.
> 
> sha256 is slower than md5, but it is conceivable that in future we might
> gain support for something like Blake2b which is similar security level
> to SHA3, while being faster than MD5.
> 
> Overall I'm pretty unethusiastic about use of MD5 being introduced and
> worse, being hardcoded as the only option.
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 09/10] migration/dirtyrate: Implement calculate_dirtyrate() function

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) 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 | 46 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 4bbfcc3..041d0c6 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -184,6 +184,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)
> @@ -341,8 +356,35 @@ 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);

I think this is OK, so:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

However, please try the following test,  set it to 60 seconds,
start the dirty rate check, and in that time, shut the guest down
(e.g. shutdown -h now in the guest) - what happens?

Dave

> +
> +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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 10/10] migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 8/16/20 10:20 PM, 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>
> > ---
> 
> > +++ b/qapi/migration.json
> > @@ -1621,3 +1621,45 @@
> >   ##
> >   { 'event': 'UNPLUG_PRIMARY',
> >     'data': { 'device-id': 'str' } }
> > +
> > +##
> > +# @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
> > +#       status with 'not start measuring' or
> > +#       'Still measuring' or 'measured'(since 5.2)
> 
> Missing space before (
> 
> > +##
> > +{ 'struct': 'DirtyRateInfo',
> > +  'data': {'dirty-rate': 'int64',
> > +           'status': 'str'} }
> 
> Based on your description, this should be an enum type rather than an
> open-coded string.  Something like:
> 
> { 'enum': 'DirtyRateStatus', 'data': [ 'unstarted', 'measuring', 'measured'
> ] }
> { 'struct': 'DirtyRateInfo', 'data': { 'dirty-rate': 'int64', 'status':
> 'DirtyRateStatus' } }

Yes, and if you do that I think you'll find qmp would automatically
define a C enum type for you, so you don't need to define the
CalculatingDirtyRateStage yourself;   see how MigrationStatus works.

Dave


> 
> > +
> > +##
> > +# @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' }
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 01/10] migration/dirtyrate: Add get_dirtyrate_thread() function

[Zheng Chuan]

On 2020/8/21 0:11, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>> Add get_dirtyrate_thread() functions
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>> ---
>>  migration/Makefile.objs |  1 +
>>  migration/dirtyrate.c   | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  migration/dirtyrate.h   | 44 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 109 insertions(+)
>>  create mode 100644 migration/dirtyrate.c
>>  create mode 100644 migration/dirtyrate.h
>>
>> diff --git a/migration/Makefile.objs b/migration/Makefile.objs
>> index 0fc619e..12ae98c 100644
>> --- a/migration/Makefile.objs
>> +++ b/migration/Makefile.objs
>> @@ -6,6 +6,7 @@ common-obj-y += qemu-file.o global_state.o
>>  common-obj-y += qemu-file-channel.o
>>  common-obj-y += xbzrle.o postcopy-ram.o
>>  common-obj-y += qjson.o
>> +common-obj-y += dirtyrate.o
>>  common-obj-y += block-dirty-bitmap.o
>>  common-obj-y += multifd.o
>>  common-obj-y += multifd-zlib.o
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> new file mode 100644
>> index 0000000..bb0ebe9
>> --- /dev/null
>> +++ b/migration/dirtyrate.c
>> @@ -0,0 +1,64 @@
>> +/*
>> + * 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"
>> +
>> +CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
>> +
>> +static int dirty_rate_set_state(int new_state)
>> +{
>> +    int old_state = CalculatingState;
>> +
>> +    if (new_state == old_state) {
>> +        return -1;
>> +    }
>> +
>> +    if (atomic_cmpxchg(&CalculatingState, old_state, new_state) != old_state) {
>> +        return -1;
>> +    }
> 
> This is a little unusual; this has removed your comment from v1 about
> what you're trying to protect; but not quite being clear about what it's
> doing.
> 
> I think what you want here is closer to migrate_set_state, ie you
> pass what you think the old state is, and the state you want to go to.
> 
Hi, Dave.
Thank you for your review.

Yes, what I want to do is to protect concurrent scene lockless, i'll rewrite
according to migrate_set_state().


>> +    return 0;
>> +}
>> +
>> +static void calculate_dirtyrate(struct DirtyRateConfig config)
>> +{
>> +    /* todo */
>> +    return;
>> +}
>> +
>> +void *get_dirtyrate_thread(void *arg)
>> +{
>> +    struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
>> +    int ret;
>> +
>> +    ret = dirty_rate_set_state(CAL_DIRTY_RATE_ACTIVE);
> 
> so this would become:
>     ret = dirty_rate_set_state(CAL_DIRTY_RATE_INIT,
>               CAL_DIRTY_RATE_ACTIVE);
> 
>> +    if (ret == -1) {
>> +        return NULL;
>> +    }
>> +
>> +    calculate_dirtyrate(config);
>> +
>> +    ret = dirty_rate_set_state(CAL_DIRTY_RATE_END);
>> +
>> +    return NULL;
>> +}
>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>> new file mode 100644
>> index 0000000..914c363
>> --- /dev/null
>> +++ b/migration/dirtyrate.h
>> @@ -0,0 +1,44 @@
>> +/*
>> + *  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 256 pages per GB as default.
>> + * TODO: Make it configurable.
>> + */
>> +#define DIRTYRATE_DEFAULT_SAMPLE_PAGES            256
>> +
>> +/* 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 */
>> +};
>> +
>> +/*
>> + *  To record calculate dirty_rate status:
>> + *  0: initial status, calculating thread is not be created here.
>> + *  1: calculating thread is created.
>> + *  2: calculating thread is end, we can get result.
>> + */
>> +typedef enum {
>> +    CAL_DIRTY_RATE_INIT = 0,
>> +    CAL_DIRTY_RATE_ACTIVE,
>> +    CAL_DIRTY_RATE_END,
>> +} CalculatingDirtyRateState;
>> +
>> +void *get_dirtyrate_thread(void *arg);
>> +#endif
>> +
>> -- 
>> 1.8.3.1
>>

Re: [PATCH v3 02/10] migration/dirtyrate: Add RamlockDirtyInfo to store sampled page info

[Zheng Chuan]

On 2020/8/21 0:20, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>> Add RamlockDirtyInfo to store sampled page info of each ramblock.
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>> ---
>>  migration/dirtyrate.h | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>> index 914c363..9650566 100644
>> --- a/migration/dirtyrate.h
>> +++ b/migration/dirtyrate.h
>> @@ -19,6 +19,11 @@
>>   */
>>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            256
>>  
>> +/*
>> + * Record ramblock idstr
>> + */
>> +#define RAMBLOCK_INFO_MAX_LEN                     256
>> +
>>  /* Take 1s as default for calculation duration */
>>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>>  
>> @@ -39,6 +44,19 @@ typedef enum {
>>      CAL_DIRTY_RATE_END,
>>  } CalculatingDirtyRateState;
>>  
>> +/*
>> + * Store dirtypage info for each ramblock.
>> + */
>> +struct RamblockDirtyInfo {
>> +    char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */
>> Can you remind me; why not just use RAMBlock* here of the block you're
> interested in, rather than storing the name?
>
idstr is used to store which ramblock is sampled page in, we test it in
find_page_matched().
so you mean we just RAMBlock*, and take idstr out of RAMBlock* when it need to
find matched page?

>> +    uint8_t *ramblock_addr; /* base address of ramblock we measure */
>> +    size_t ramblock_pages; /* sum of dividation by 4K pages for ramblock */
> 
> 'dividation' is the wrong word, and 'sum' is only needed where you're
> adding things together.  I think this is 'ramblock size in TARGET_PAGEs'
> 
>> +    size_t *sample_page_vfn; /* relative offset address for sampled page */
>> +    unsigned int sample_pages_count; /* sum of sampled pages */
>> +    unsigned int sample_dirty_count; /* sum of dirty pages we measure */
> 
> These are both 'count' rather than 'sum'
> 
OK, will be fixed in V4:)

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

Re: [PATCH v3 03/10] migration/dirtyrate: Add dirtyrate statistics series functions

[Zheng Chuan]

On 2020/8/21 0:28, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>> Add dirtyrate statistics to record/update dirtyrate info.
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>> ---
>>  migration/dirtyrate.c | 30 ++++++++++++++++++++++++++++++
>>  migration/dirtyrate.h | 10 ++++++++++
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index bb0ebe9..8708090 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -24,6 +24,7 @@
>>  #include "dirtyrate.h"
>>  
>>  CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
>> +static struct DirtyRateStat dirty_stat;
>>  
>>  static int dirty_rate_set_state(int new_state)
>>  {
>> @@ -40,6 +41,35 @@ static int dirty_rate_set_state(int new_state)
>>      return 0;
>>  }
>>  
>> +static void reset_dirtyrate_stat(void)
>> +{
>> +    dirty_stat.total_dirty_samples = 0;
>> +    dirty_stat.total_sample_count = 0;
>> +    dirty_stat.total_block_mem_MB = 0;
>> +    dirty_stat.dirty_rate = 0;
>> +}
>> +
>> +static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
>> +{
>> +    dirty_stat.total_dirty_samples += info->sample_dirty_count;
>> +    dirty_stat.total_sample_count += info->sample_pages_count;
>> +    /* size of 4K pages in MB */
>> +    dirty_stat.total_block_mem_MB += info->ramblock_pages / 256;
>> +}
>> +
>> +static void update_dirtyrate(uint64_t msec)
>> +{
>> +    uint64_t dirty_rate;
>> +    unsigned int total_dirty_samples = dirty_stat.total_dirty_samples;
>> +    unsigned int total_sample_count = dirty_stat.total_sample_count;
>> +    size_t total_block_mem_MB = dirty_stat.total_block_mem_MB;
>> +
>> +    dirty_rate = total_dirty_samples * total_block_mem_MB *
>> +                 1000 / (total_sample_count * msec);
>> +
>> +    dirty_stat.dirty_rate = dirty_rate;
>> +}
>> +
>>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>>  {
>>      /* todo */
>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>> index 9650566..af57c80 100644
>> --- a/migration/dirtyrate.h
>> +++ b/migration/dirtyrate.h
>> @@ -57,6 +57,16 @@ struct RamblockDirtyInfo {
>>      uint8_t *hash_result; /* array of hash result for sampled pages */
>>  };
>>  
>> +/*
>> + * Store calculate statistics for each measure.
>> + */
>> +struct DirtyRateStat {
>> +    unsigned int total_dirty_samples; /* total dirty pages for this measure */
>> +    unsigned int total_sample_count; /* total sampled pages for this measure */
>> +    size_t total_block_mem_MB; /* size of sampled pages in MB */
>> +    int64_t dirty_rate; /* dirty rate for this measure */
> 
> As I said in the previous review, please comment 'dirty_rate' with it's
> units.
> 
Sorry, i missed that, will be fix in V4:)
> Dave
> 
>> +};
>> +
>>  void *get_dirtyrate_thread(void *arg);
>>  #endif
>>  
>> -- 
>> 1.8.3.1
>>

Re: [PATCH v3 09/10] migration/dirtyrate: Implement calculate_dirtyrate() function

[Zheng Chuan]

On 2020/8/21 1:57, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) 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 | 46 ++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index 4bbfcc3..041d0c6 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -184,6 +184,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)
>> @@ -341,8 +356,35 @@ 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);
> 
> I think this is OK, so:
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> However, please try the following test,  set it to 60 seconds,
> start the dirty rate check, and in that time, shut the guest down
> (e.g. shutdown -h now in the guest) - what happens?
> 
> Dave
> 
It is ok when shutdown corcurrent with query dirtyrate, the get_dirtyrate
thread is terminated by qemu.

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

Re: [PATCH v3 10/10] migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function

[Zheng Chuan]

On 2020/8/21 2:00, Dr. David Alan Gilbert wrote:
> * Eric Blake (eblake@redhat.com) wrote:
>> On 8/16/20 10:20 PM, 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>
>>> ---
>>
>>> +++ b/qapi/migration.json
>>> @@ -1621,3 +1621,45 @@
>>>   ##
>>>   { 'event': 'UNPLUG_PRIMARY',
>>>     'data': { 'device-id': 'str' } }
>>> +
>>> +##
>>> +# @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
>>> +#       status with 'not start measuring' or
>>> +#       'Still measuring' or 'measured'(since 5.2)
>>
>> Missing space before (
>>
>>> +##
>>> +{ 'struct': 'DirtyRateInfo',
>>> +  'data': {'dirty-rate': 'int64',
>>> +           'status': 'str'} }
>>
>> Based on your description, this should be an enum type rather than an
>> open-coded string.  Something like:
>>
>> { 'enum': 'DirtyRateStatus', 'data': [ 'unstarted', 'measuring', 'measured'
>> ] }
>> { 'struct': 'DirtyRateInfo', 'data': { 'dirty-rate': 'int64', 'status':
>> 'DirtyRateStatus' } }
> 
> Yes, and if you do that I think you'll find qmp would automatically
> define a C enum type for you, so you don't need to define the
> CalculatingDirtyRateStage yourself;   see how MigrationStatus works.
> 
> Dave
> 
Sure, it could be better，will fix it in V4:)
> 
>>
>>> +
>>> +##
>>> +# @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' }
>>>
>>
>> -- 
>> Eric Blake, Principal Software Engineer
>> Red Hat, Inc.           +1-919-301-3226
>> Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 06/10] migration/dirtyrate: Compare page hash results for recorded sampled page

[Zheng Chuan]

On 2020/8/21 1:36, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) 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 | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 76 insertions(+)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index 62b6f69..3ce25f5 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -215,6 +215,82 @@ static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
>>      return 0;
>>  }
>>  
>> +static int calc_page_dirty_rate(struct RamblockDirtyInfo *info)
>> +{
>> +    uint8_t *md = NULL;
>> +    int i;
>> +    int ret = 0;
>> +
>> +    md = g_try_new0(uint8_t, qcrypto_hash_len);
>> +    if (!md) {
>> +        return -1;
>> +    }
> 
> As previously asked; isn't this a nice small simple fixed length - no
> need to allocate it?
> 
Yes, it could use QCRYPTO_HASH_LEN to define an array.
>> +
>> +    for (i = 0; i < info->sample_pages_count; i++) {
>> +        ret = get_ramblock_vfn_hash(info, info->sample_page_vfn[i], &md);
>> +        if (ret < 0) {
>> +            goto out;
>> +        }
>> +
>> +        if (memcmp(md, info->hash_result + i * qcrypto_hash_len,
>> +                   qcrypto_hash_len) != 0) {
>> +            info->sample_dirty_count++;
>> +        }
>> +    }
>> +
>> +out:
>> +    g_free(md);
>> +    return ret;
>> +}
>> +
>> +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) >> 12)) {
>> +        return false;
> 
> I previously asked how this happens.
> Also this was DIRTYRATE_PAGE_SIZE_SHIFT
> 
Here, we want to find same ramblock we sampled before.
We just ignore the ramblock if its hva address or size changed due to memory hot-plug during the measurement.

>> +    }
>> +
>> +    *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;
> 
> So you've removed the selction of only some RAMBlocks now?
> 
In next patch:), i add functions to skip sampling ramblock.

>> +        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 (!dirty_stat.total_sample_count) {
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>>  {
>>      /* todo */
>> -- 
>> 1.8.3.1
>>

Re: [PATCH v3 02/10] migration/dirtyrate: Add RamlockDirtyInfo to store sampled page info

[Dr. David Alan Gilbert]

* Zheng Chuan (zhengchuan@huawei.com) wrote:
> 
> 
> On 2020/8/21 0:20, Dr. David Alan Gilbert wrote:
> > * Chuan Zheng (zhengchuan@huawei.com) wrote:
> >> Add RamlockDirtyInfo to store sampled page info of each ramblock.
> >>
> >> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> >> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> >> ---
> >>  migration/dirtyrate.h | 18 ++++++++++++++++++
> >>  1 file changed, 18 insertions(+)
> >>
> >> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> >> index 914c363..9650566 100644
> >> --- a/migration/dirtyrate.h
> >> +++ b/migration/dirtyrate.h
> >> @@ -19,6 +19,11 @@
> >>   */
> >>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            256
> >>  
> >> +/*
> >> + * Record ramblock idstr
> >> + */
> >> +#define RAMBLOCK_INFO_MAX_LEN                     256
> >> +
> >>  /* Take 1s as default for calculation duration */
> >>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
> >>  
> >> @@ -39,6 +44,19 @@ typedef enum {
> >>      CAL_DIRTY_RATE_END,
> >>  } CalculatingDirtyRateState;
> >>  
> >> +/*
> >> + * Store dirtypage info for each ramblock.
> >> + */
> >> +struct RamblockDirtyInfo {
> >> +    char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */
> >> Can you remind me; why not just use RAMBlock* here of the block you're
> > interested in, rather than storing the name?
> >
> idstr is used to store which ramblock is sampled page in, we test it in
> find_page_matched().
> so you mean we just RAMBlock*, and take idstr out of RAMBlock* when it need to
> find matched page?

I meant just use RAMBlock*, but I think I see why you don't;
because you 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.

Dave

> >> +    uint8_t *ramblock_addr; /* base address of ramblock we measure */
> >> +    size_t ramblock_pages; /* sum of dividation by 4K pages for ramblock */
> > 
> > 'dividation' is the wrong word, and 'sum' is only needed where you're
> > adding things together.  I think this is 'ramblock size in TARGET_PAGEs'
> > 
> >> +    size_t *sample_page_vfn; /* relative offset address for sampled page */
> >> +    unsigned int sample_pages_count; /* sum of sampled pages */
> >> +    unsigned int sample_dirty_count; /* sum of dirty pages we measure */
> > 
> > These are both 'count' rather than 'sum'
> > 
> OK, will be fixed in V4:)
> 
> >> +    uint8_t *hash_result; /* array of hash result for sampled pages */
> >> +};
> >> +
> >>  void *get_dirtyrate_thread(void *arg);
> >>  #endif
> >>  
> >> -- 
> >> 1.8.3.1
> >>
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 06/10] migration/dirtyrate: Compare page hash results for recorded sampled page

[Dr. David Alan Gilbert]

* Zheng Chuan (zhengchuan@huawei.com) wrote:
> 
> 
> On 2020/8/21 1:36, Dr. David Alan Gilbert wrote:
> > * Chuan Zheng (zhengchuan@huawei.com) 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 | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 76 insertions(+)
> >>
> >> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> >> index 62b6f69..3ce25f5 100644
> >> --- a/migration/dirtyrate.c
> >> +++ b/migration/dirtyrate.c
> >> @@ -215,6 +215,82 @@ static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
> >>      return 0;
> >>  }
> >>  
> >> +static int calc_page_dirty_rate(struct RamblockDirtyInfo *info)
> >> +{
> >> +    uint8_t *md = NULL;
> >> +    int i;
> >> +    int ret = 0;
> >> +
> >> +    md = g_try_new0(uint8_t, qcrypto_hash_len);
> >> +    if (!md) {
> >> +        return -1;
> >> +    }
> > 
> > As previously asked; isn't this a nice small simple fixed length - no
> > need to allocate it?
> > 
> Yes, it could use QCRYPTO_HASH_LEN to define an array.
> >> +
> >> +    for (i = 0; i < info->sample_pages_count; i++) {
> >> +        ret = get_ramblock_vfn_hash(info, info->sample_page_vfn[i], &md);
> >> +        if (ret < 0) {
> >> +            goto out;
> >> +        }
> >> +
> >> +        if (memcmp(md, info->hash_result + i * qcrypto_hash_len,
> >> +                   qcrypto_hash_len) != 0) {
> >> +            info->sample_dirty_count++;
> >> +        }
> >> +    }
> >> +
> >> +out:
> >> +    g_free(md);
> >> +    return ret;
> >> +}
> >> +
> >> +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) >> 12)) {
> >> +        return false;
> > 
> > I previously asked how this happens.
> > Also this was DIRTYRATE_PAGE_SIZE_SHIFT
> > 
> Here, we want to find same ramblock we sampled before.
> We just ignore the ramblock if its hva address or size changed due to memory hot-plug during the measurement.

OK, just try and make the code consistent between '12',
'DIRTYRATE_PAGE_SIZE_SHIFT'
or TARGET_PAGE_SIZE/SHIFT.

Dave

> >> +    }
> >> +
> >> +    *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;
> > 
> > So you've removed the selction of only some RAMBlocks now?
> > 
> In next patch:), i add functions to skip sampling ramblock.
> 
> >> +        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 (!dirty_stat.total_sample_count) {
> >> +        return -1;
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >>  static void calculate_dirtyrate(struct DirtyRateConfig config)
> >>  {
> >>      /* todo */
> >> -- 
> >> 1.8.3.1
> >>
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 05/10] migration/dirtyrate: Record hash results for each sampled page

[Zheng Chuan]

On 2020/8/21 1:55, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>> On Thu, Aug 20, 2020 at 06:30:09PM +0100, Dr. David Alan Gilbert wrote:
>>> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>>>> Record hash results for each sampled page.
>>>>
>>>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>>>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>>>> ---
>>>>  migration/dirtyrate.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  migration/dirtyrate.h |   7 +++
>>>>  2 files changed, 151 insertions(+)
>>>>
>>>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>>>> index c4304ef..62b6f69 100644
>>>> --- a/migration/dirtyrate.c
>>>> +++ b/migration/dirtyrate.c
>>>> @@ -25,6 +25,7 @@
>>>>  #include "dirtyrate.h"
>>>>  
>>>>  CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
>>>> +static unsigned long int qcrypto_hash_len = QCRYPTO_HASH_LEN;
>>>
>>> Why do we need this static rather than just using the QCRYPTO_HASH_LEN ?
>>> It's never going to change is it?
>>> (and anyway it's just a MD5 len?)
>>
>> I wouldn't want to bet on that given that this is use of MD5. We might
>> claim this isn't security critical, but surprises happen, and we will
>> certainly be dinged on security audits for introducing new use of MD5
>> no matter what.
>>
>> If a cryptographic hash is required, then sha256 should be the choice
>> for any new code that doesn't have back compat requirements.
>>
>> If a cryptographic hash is not required then how about crc32 
> 
> It doesn't need to be cryptographic; is crc32 the fastest reasonable hash for use
> in large areas?
> 
> Dave
> 
>> IOW, it doesn't make a whole lot of sense to say we need a cryptographic
>> hash, but then pick the most insecure one.
>>
>> sha256 is slower than md5, but it is conceivable that in future we might
>> gain support for something like Blake2b which is similar security level
>> to SHA3, while being faster than MD5.
>>
>> Overall I'm pretty unethusiastic about use of MD5 being introduced and
>> worse, being hardcoded as the only option.
>>
>> Regards,
>> Daniel
>> -- 
>> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Hi, Daniel, Dave.

I do compare MD5 and SHA256 with vm memory of 128G under mempress of 100G.

1. Calculation speed
1） MD5 takes about 500ms to sample and hash all pages by record_ramblock_hash_info().
2)  SHA256 takes about 750ms to sample all pages by record_ramblock_hash_info().

2. CPU Consumption
1)  MD5 may have instant rise up to 48% for dirtyrate thread
2)  SHA256 may have instant rise up to 75% for dirtyrate thread

3. Memory Consumption
SHA256 may need twice memory than MD5 due to its HASH_LEN.

I am trying to consider if crc32 is more faster and takes less memory and is more safer than MD5?
For now, i am trying to do how to implement crc32 instead of using qcrypto_hash_bytesv() to hash sampled page memory.

Re: [PATCH v3 05/10] migration/dirtyrate: Record hash results for each sampled page

[Daniel P. Berrangé]

On Fri, Aug 21, 2020 at 08:22:06PM +0800, Zheng Chuan wrote:
> 
> 
> On 2020/8/21 1:55, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> >> On Thu, Aug 20, 2020 at 06:30:09PM +0100, Dr. David Alan Gilbert wrote:
> >>> * Chuan Zheng (zhengchuan@huawei.com) wrote:
> >>>> Record hash results for each sampled page.
> >>>>
> >>>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> >>>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> >>>> ---
> >>>>  migration/dirtyrate.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  migration/dirtyrate.h |   7 +++
> >>>>  2 files changed, 151 insertions(+)
> >>>>
> >>>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> >>>> index c4304ef..62b6f69 100644
> >>>> --- a/migration/dirtyrate.c
> >>>> +++ b/migration/dirtyrate.c
> >>>> @@ -25,6 +25,7 @@
> >>>>  #include "dirtyrate.h"
> >>>>  
> >>>>  CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
> >>>> +static unsigned long int qcrypto_hash_len = QCRYPTO_HASH_LEN;
> >>>
> >>> Why do we need this static rather than just using the QCRYPTO_HASH_LEN ?
> >>> It's never going to change is it?
> >>> (and anyway it's just a MD5 len?)
> >>
> >> I wouldn't want to bet on that given that this is use of MD5. We might
> >> claim this isn't security critical, but surprises happen, and we will
> >> certainly be dinged on security audits for introducing new use of MD5
> >> no matter what.
> >>
> >> If a cryptographic hash is required, then sha256 should be the choice
> >> for any new code that doesn't have back compat requirements.
> >>
> >> If a cryptographic hash is not required then how about crc32 
> > 
> > It doesn't need to be cryptographic; is crc32 the fastest reasonable hash for use
> > in large areas?
> > 
> > Dave
> > 
> >> IOW, it doesn't make a whole lot of sense to say we need a cryptographic
> >> hash, but then pick the most insecure one.
> >>
> >> sha256 is slower than md5, but it is conceivable that in future we might
> >> gain support for something like Blake2b which is similar security level
> >> to SHA3, while being faster than MD5.
> >>
> >> Overall I'm pretty unethusiastic about use of MD5 being introduced and
> >> worse, being hardcoded as the only option.
> >>
> >> Regards,
> >> Daniel
> >> -- 
> >> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> >> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> >> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 
> Hi, Daniel, Dave.
> 
> I do compare MD5 and SHA256 with vm memory of 128G under mempress of 100G.
> 
> 1. Calculation speed
> 1） MD5 takes about 500ms to sample and hash all pages by record_ramblock_hash_info().
> 2)  SHA256 takes about 750ms to sample all pages by record_ramblock_hash_info().
> 
> 2. CPU Consumption
> 1)  MD5 may have instant rise up to 48% for dirtyrate thread
> 2)  SHA256 may have instant rise up to 75% for dirtyrate thread
> 
> 3. Memory Consumption
> SHA256 may need twice memory than MD5 due to its HASH_LEN.
> 
> I am trying to consider if crc32 is more faster and takes less memory and is more safer than MD5?

No, crc32 is absolutely *weaker* than MD5. It is NOT a cryptographic
hash so does not try to guarantee collision resistance. It only has
2^32 possible outputs.

MD5 does try to guarantee collision resistance, but MD5 is considered
broken these days, so a malicious attacker can cause collisions if they
are motivated enough.

IOW if you need collision resistance that SHA256 should be used.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 05/10] migration/dirtyrate: Record hash results for each sampled page

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Fri, Aug 21, 2020 at 08:22:06PM +0800, Zheng Chuan wrote:
> > 
> > 
> > On 2020/8/21 1:55, Dr. David Alan Gilbert wrote:
> > > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > >> On Thu, Aug 20, 2020 at 06:30:09PM +0100, Dr. David Alan Gilbert wrote:
> > >>> * Chuan Zheng (zhengchuan@huawei.com) wrote:
> > >>>> Record hash results for each sampled page.
> > >>>>
> > >>>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> > >>>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> > >>>> ---
> > >>>>  migration/dirtyrate.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >>>>  migration/dirtyrate.h |   7 +++
> > >>>>  2 files changed, 151 insertions(+)
> > >>>>
> > >>>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> > >>>> index c4304ef..62b6f69 100644
> > >>>> --- a/migration/dirtyrate.c
> > >>>> +++ b/migration/dirtyrate.c
> > >>>> @@ -25,6 +25,7 @@
> > >>>>  #include "dirtyrate.h"
> > >>>>  
> > >>>>  CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
> > >>>> +static unsigned long int qcrypto_hash_len = QCRYPTO_HASH_LEN;
> > >>>
> > >>> Why do we need this static rather than just using the QCRYPTO_HASH_LEN ?
> > >>> It's never going to change is it?
> > >>> (and anyway it's just a MD5 len?)
> > >>
> > >> I wouldn't want to bet on that given that this is use of MD5. We might
> > >> claim this isn't security critical, but surprises happen, and we will
> > >> certainly be dinged on security audits for introducing new use of MD5
> > >> no matter what.
> > >>
> > >> If a cryptographic hash is required, then sha256 should be the choice
> > >> for any new code that doesn't have back compat requirements.
> > >>
> > >> If a cryptographic hash is not required then how about crc32 
> > > 
> > > It doesn't need to be cryptographic; is crc32 the fastest reasonable hash for use
> > > in large areas?
> > > 
> > > Dave
> > > 
> > >> IOW, it doesn't make a whole lot of sense to say we need a cryptographic
> > >> hash, but then pick the most insecure one.
> > >>
> > >> sha256 is slower than md5, but it is conceivable that in future we might
> > >> gain support for something like Blake2b which is similar security level
> > >> to SHA3, while being faster than MD5.
> > >>
> > >> Overall I'm pretty unethusiastic about use of MD5 being introduced and
> > >> worse, being hardcoded as the only option.
> > >>
> > >> Regards,
> > >> Daniel
> > >> -- 
> > >> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> > >> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> > >> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> > 
> > Hi, Daniel, Dave.
> > 
> > I do compare MD5 and SHA256 with vm memory of 128G under mempress of 100G.
> > 
> > 1. Calculation speed
> > 1） MD5 takes about 500ms to sample and hash all pages by record_ramblock_hash_info().
> > 2)  SHA256 takes about 750ms to sample all pages by record_ramblock_hash_info().
> > 
> > 2. CPU Consumption
> > 1)  MD5 may have instant rise up to 48% for dirtyrate thread
> > 2)  SHA256 may have instant rise up to 75% for dirtyrate thread
> > 
> > 3. Memory Consumption
> > SHA256 may need twice memory than MD5 due to its HASH_LEN.
> > 
> > I am trying to consider if crc32 is more faster and takes less memory and is more safer than MD5?
> 
> No, crc32 is absolutely *weaker* than MD5. It is NOT a cryptographic
> hash so does not try to guarantee collision resistance. It only has
> 2^32 possible outputs.
> 
> MD5 does try to guarantee collision resistance, but MD5 is considered
> broken these days, so a malicious attacker can cause collisions if they
> are motivated enough.
> 
> IOW if you need collision resistance that SHA256 should be used.

There's no need to guard against malicious behaviour here - this is just
a stat to guide migration.
If CRC32 is likely to be faster than md5 I suspect it's enough.

Dave

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 05/10] migration/dirtyrate: Record hash results for each sampled page

[Zheng Chuan]

On 2020/8/21 20:39, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>> On Fri, Aug 21, 2020 at 08:22:06PM +0800, Zheng Chuan wrote:
>>>
>>>
>>> On 2020/8/21 1:55, Dr. David Alan Gilbert wrote:
>>>> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>>>>> On Thu, Aug 20, 2020 at 06:30:09PM +0100, Dr. David Alan Gilbert wrote:
>>>>>> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>>>>>>> Record hash results for each sampled page.
>>>>>>>
>>>>>>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>>>>>>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>>>>>>> ---
>>>>>>>  migration/dirtyrate.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>  migration/dirtyrate.h |   7 +++
>>>>>>>  2 files changed, 151 insertions(+)
>>>>>>>
>>>>>>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>>>>>>> index c4304ef..62b6f69 100644
>>>>>>> --- a/migration/dirtyrate.c
>>>>>>> +++ b/migration/dirtyrate.c
>>>>>>> @@ -25,6 +25,7 @@
>>>>>>>  #include "dirtyrate.h"
>>>>>>>  
>>>>>>>  CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
>>>>>>> +static unsigned long int qcrypto_hash_len = QCRYPTO_HASH_LEN;
>>>>>>
>>>>>> Why do we need this static rather than just using the QCRYPTO_HASH_LEN ?
>>>>>> It's never going to change is it?
>>>>>> (and anyway it's just a MD5 len?)
>>>>>
>>>>> I wouldn't want to bet on that given that this is use of MD5. We might
>>>>> claim this isn't security critical, but surprises happen, and we will
>>>>> certainly be dinged on security audits for introducing new use of MD5
>>>>> no matter what.
>>>>>
>>>>> If a cryptographic hash is required, then sha256 should be the choice
>>>>> for any new code that doesn't have back compat requirements.
>>>>>
>>>>> If a cryptographic hash is not required then how about crc32 
>>>>
>>>> It doesn't need to be cryptographic; is crc32 the fastest reasonable hash for use
>>>> in large areas?
>>>>
>>>> Dave
>>>>
>>>>> IOW, it doesn't make a whole lot of sense to say we need a cryptographic
>>>>> hash, but then pick the most insecure one.
>>>>>
>>>>> sha256 is slower than md5, but it is conceivable that in future we might
>>>>> gain support for something like Blake2b which is similar security level
>>>>> to SHA3, while being faster than MD5.
>>>>>
>>>>> Overall I'm pretty unethusiastic about use of MD5 being introduced and
>>>>> worse, being hardcoded as the only option.
>>>>>
>>>>> Regards,
>>>>> Daniel
>>>>> -- 
>>>>> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>>>>> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>>>>> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>>>
>>> Hi, Daniel, Dave.
>>>
>>> I do compare MD5 and SHA256 with vm memory of 128G under mempress of 100G.
>>>
>>> 1. Calculation speed
>>> 1） MD5 takes about 500ms to sample and hash all pages by record_ramblock_hash_info().
>>> 2)  SHA256 takes about 750ms to sample all pages by record_ramblock_hash_info().
>>>
>>> 2. CPU Consumption
>>> 1)  MD5 may have instant rise up to 48% for dirtyrate thread
>>> 2)  SHA256 may have instant rise up to 75% for dirtyrate thread
>>>
>>> 3. Memory Consumption
>>> SHA256 may need twice memory than MD5 due to its HASH_LEN.
>>>
>>> I am trying to consider if crc32 is more faster and takes less memory and is more safer than MD5?
>>
>> No, crc32 is absolutely *weaker* than MD5. It is NOT a cryptographic
>> hash so does not try to guarantee collision resistance. It only has
>> 2^32 possible outputs.
>>
>> MD5 does try to guarantee collision resistance, but MD5 is considered
>> broken these days, so a malicious attacker can cause collisions if they
>> are motivated enough.
>>
>> IOW if you need collision resistance that SHA256 should be used.
> 
> There's no need to guard against malicious behaviour here - this is just
> a stat to guide migration.
> If CRC32 is likely to be faster than md5 I suspect it's enough.
> 
> Dave
> 
OK, i'll take a test by crc32.

>>
>> Regards,
>> Daniel
>> -- 
>> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 05/10] migration/dirtyrate: Record hash results for each sampled page

[Zheng Chuan]

On 2020/8/21 20:39, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>> On Fri, Aug 21, 2020 at 08:22:06PM +0800, Zheng Chuan wrote:
>>>
>>>
>>> On 2020/8/21 1:55, Dr. David Alan Gilbert wrote:
>>>> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>>>>> On Thu, Aug 20, 2020 at 06:30:09PM +0100, Dr. David Alan Gilbert wrote:
>>>>>> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>>>>>>> Record hash results for each sampled page.
>>>>>>>
>>>>>>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>>>>>>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>>>>>>> ---
>>>>>>>  migration/dirtyrate.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>  migration/dirtyrate.h |   7 +++
>>>>>>>  2 files changed, 151 insertions(+)
>>>>>>>
>>>>>>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>>>>>>> index c4304ef..62b6f69 100644
>>>>>>> --- a/migration/dirtyrate.c
>>>>>>> +++ b/migration/dirtyrate.c
>>>>>>> @@ -25,6 +25,7 @@
>>>>>>>  #include "dirtyrate.h"
>>>>>>>  
>>>>>>>  CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
>>>>>>> +static unsigned long int qcrypto_hash_len = QCRYPTO_HASH_LEN;
>>>>>>
>>>>>> Why do we need this static rather than just using the QCRYPTO_HASH_LEN ?
>>>>>> It's never going to change is it?
>>>>>> (and anyway it's just a MD5 len?)
>>>>>
>>>>> I wouldn't want to bet on that given that this is use of MD5. We might
>>>>> claim this isn't security critical, but surprises happen, and we will
>>>>> certainly be dinged on security audits for introducing new use of MD5
>>>>> no matter what.
>>>>>
>>>>> If a cryptographic hash is required, then sha256 should be the choice
>>>>> for any new code that doesn't have back compat requirements.
>>>>>
>>>>> If a cryptographic hash is not required then how about crc32 
>>>>
>>>> It doesn't need to be cryptographic; is crc32 the fastest reasonable hash for use
>>>> in large areas?
>>>>
>>>> Dave
>>>>
>>>>> IOW, it doesn't make a whole lot of sense to say we need a cryptographic
>>>>> hash, but then pick the most insecure one.
>>>>>
>>>>> sha256 is slower than md5, but it is conceivable that in future we might
>>>>> gain support for something like Blake2b which is similar security level
>>>>> to SHA3, while being faster than MD5.
>>>>>
>>>>> Overall I'm pretty unethusiastic about use of MD5 being introduced and
>>>>> worse, being hardcoded as the only option.
>>>>>
>>>>> Regards,
>>>>> Daniel
>>>>> -- 
>>>>> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>>>>> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>>>>> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>>>
>>> Hi, Daniel, Dave.
>>>
>>> I do compare MD5 and SHA256 with vm memory of 128G under mempress of 100G.
>>>
>>> 1. Calculation speed
>>> 1） MD5 takes about 500ms to sample and hash all pages by record_ramblock_hash_info().
>>> 2)  SHA256 takes about 750ms to sample all pages by record_ramblock_hash_info().
>>>
>>> 2. CPU Consumption
>>> 1)  MD5 may have instant rise up to 48% for dirtyrate thread
>>> 2)  SHA256 may have instant rise up to 75% for dirtyrate thread
>>>
>>> 3. Memory Consumption
>>> SHA256 may need twice memory than MD5 due to its HASH_LEN.
>>>
>>> I am trying to consider if crc32 is more faster and takes less memory and is more safer than MD5?
>>
>> No, crc32 is absolutely *weaker* than MD5. It is NOT a cryptographic
>> hash so does not try to guarantee collision resistance. It only has
>> 2^32 possible outputs.
>>
>> MD5 does try to guarantee collision resistance, but MD5 is considered
>> broken these days, so a malicious attacker can cause collisions if they
>> are motivated enough.
>>
>> IOW if you need collision resistance that SHA256 should be used.
> 
> There's no need to guard against malicious behaviour here - this is just
> a stat to guide migration.
> If CRC32 is likely to be faster than md5 I suspect it's enough.
> 
> Dave
> 
Hi，Dave, Daniel.

I did test by crc32，it is much faster than MD5 and SHA256:)

As for 128G vm it takes only about 50ms to sample and hash all pages by record_ramblock_hash_info().
And the dirtyrate calculation is still good enough:)
++++++++++++++++++++++++++++++++++++++++++
|                      |    dirtyrate    |
++++++++++++++++++++++++++++++++++++++++++
| no mempress          |     4MB/s       |
------------------------------------------
| mempress 4096 1024   |    1248MB/s     |
++++++++++++++++++++++++++++++++++++++++++
| mempress 4096 4096   |    4060MB/s     |
++++++++++++++++++++++++++++++++++++++++++

I will take crc32 in PatchV4, is that OK from the perspective of safety?

In my opinion, it should be safe.
The crc32 is only for compare and the recorder will be free after calculation is over.
The output is just dirtyrate for user to guide migration.

What's more, i consider increase DIRTYRATE_DEFAULT_SAMPLE_PAGES from 256 to 512
which may takes about 75ms to sample and hash all pages by record_ramblock_hash_info().

>>
>> Regards,
>> Daniel
>> -- 
>> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 05/10] migration/dirtyrate: Record hash results for each sampled page

[Dr. David Alan Gilbert]

* Zheng Chuan (zhengchuan@huawei.com) wrote:
> 
> 
> On 2020/8/21 20:39, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> >> On Fri, Aug 21, 2020 at 08:22:06PM +0800, Zheng Chuan wrote:
> >>>
> >>>
> >>> On 2020/8/21 1:55, Dr. David Alan Gilbert wrote:
> >>>> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> >>>>> On Thu, Aug 20, 2020 at 06:30:09PM +0100, Dr. David Alan Gilbert wrote:
> >>>>>> * Chuan Zheng (zhengchuan@huawei.com) wrote:
> >>>>>>> Record hash results for each sampled page.
> >>>>>>>
> >>>>>>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> >>>>>>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> >>>>>>> ---
> >>>>>>>  migration/dirtyrate.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>  migration/dirtyrate.h |   7 +++
> >>>>>>>  2 files changed, 151 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> >>>>>>> index c4304ef..62b6f69 100644
> >>>>>>> --- a/migration/dirtyrate.c
> >>>>>>> +++ b/migration/dirtyrate.c
> >>>>>>> @@ -25,6 +25,7 @@
> >>>>>>>  #include "dirtyrate.h"
> >>>>>>>  
> >>>>>>>  CalculatingDirtyRateState CalculatingState = CAL_DIRTY_RATE_INIT;
> >>>>>>> +static unsigned long int qcrypto_hash_len = QCRYPTO_HASH_LEN;
> >>>>>>
> >>>>>> Why do we need this static rather than just using the QCRYPTO_HASH_LEN ?
> >>>>>> It's never going to change is it?
> >>>>>> (and anyway it's just a MD5 len?)
> >>>>>
> >>>>> I wouldn't want to bet on that given that this is use of MD5. We might
> >>>>> claim this isn't security critical, but surprises happen, and we will
> >>>>> certainly be dinged on security audits for introducing new use of MD5
> >>>>> no matter what.
> >>>>>
> >>>>> If a cryptographic hash is required, then sha256 should be the choice
> >>>>> for any new code that doesn't have back compat requirements.
> >>>>>
> >>>>> If a cryptographic hash is not required then how about crc32 
> >>>>
> >>>> It doesn't need to be cryptographic; is crc32 the fastest reasonable hash for use
> >>>> in large areas?
> >>>>
> >>>> Dave
> >>>>
> >>>>> IOW, it doesn't make a whole lot of sense to say we need a cryptographic
> >>>>> hash, but then pick the most insecure one.
> >>>>>
> >>>>> sha256 is slower than md5, but it is conceivable that in future we might
> >>>>> gain support for something like Blake2b which is similar security level
> >>>>> to SHA3, while being faster than MD5.
> >>>>>
> >>>>> Overall I'm pretty unethusiastic about use of MD5 being introduced and
> >>>>> worse, being hardcoded as the only option.
> >>>>>
> >>>>> Regards,
> >>>>> Daniel
> >>>>> -- 
> >>>>> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> >>>>> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> >>>>> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> >>>
> >>> Hi, Daniel, Dave.
> >>>
> >>> I do compare MD5 and SHA256 with vm memory of 128G under mempress of 100G.
> >>>
> >>> 1. Calculation speed
> >>> 1） MD5 takes about 500ms to sample and hash all pages by record_ramblock_hash_info().
> >>> 2)  SHA256 takes about 750ms to sample all pages by record_ramblock_hash_info().
> >>>
> >>> 2. CPU Consumption
> >>> 1)  MD5 may have instant rise up to 48% for dirtyrate thread
> >>> 2)  SHA256 may have instant rise up to 75% for dirtyrate thread
> >>>
> >>> 3. Memory Consumption
> >>> SHA256 may need twice memory than MD5 due to its HASH_LEN.
> >>>
> >>> I am trying to consider if crc32 is more faster and takes less memory and is more safer than MD5?
> >>
> >> No, crc32 is absolutely *weaker* than MD5. It is NOT a cryptographic
> >> hash so does not try to guarantee collision resistance. It only has
> >> 2^32 possible outputs.
> >>
> >> MD5 does try to guarantee collision resistance, but MD5 is considered
> >> broken these days, so a malicious attacker can cause collisions if they
> >> are motivated enough.
> >>
> >> IOW if you need collision resistance that SHA256 should be used.
> > 
> > There's no need to guard against malicious behaviour here - this is just
> > a stat to guide migration.
> > If CRC32 is likely to be faster than md5 I suspect it's enough.
> > 
> > Dave
> > 
> Hi，Dave, Daniel.
> 
> I did test by crc32，it is much faster than MD5 and SHA256:)
> 
> As for 128G vm it takes only about 50ms to sample and hash all pages by record_ramblock_hash_info().
> And the dirtyrate calculation is still good enough:)
> ++++++++++++++++++++++++++++++++++++++++++
> |                      |    dirtyrate    |
> ++++++++++++++++++++++++++++++++++++++++++
> | no mempress          |     4MB/s       |
> ------------------------------------------
> | mempress 4096 1024   |    1248MB/s     |
> ++++++++++++++++++++++++++++++++++++++++++
> | mempress 4096 4096   |    4060MB/s     |
> ++++++++++++++++++++++++++++++++++++++++++
> 
> I will take crc32 in PatchV4, is that OK from the perspective of safety?

Yes, it's fine since it's only a heuristic anyway.

Dave

> In my opinion, it should be safe.
> The crc32 is only for compare and the recorder will be free after calculation is over.
> The output is just dirtyrate for user to guide migration.
> 
> What's more, i consider increase DIRTYRATE_DEFAULT_SAMPLE_PAGES from 256 to 512
> which may takes about 75ms to sample and hash all pages by record_ramblock_hash_info().
> 
> >>
> >> Regards,
> >> Daniel
> >> -- 
> >> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> >> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> >> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK