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

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

[Chuan Zheng]

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

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

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

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

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

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

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

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

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

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

 migration/dirtyrate.c  | 432 +++++++++++++++++++++++++++++++++++++++++++++++++
 migration/dirtyrate.h  |  87 ++++++++++
 migration/meson.build  |   1 +
 migration/ram.c        |  11 +-
 migration/ram.h        |  10 ++
 migration/trace-events |   8 +
 qapi/migration.json    |  61 +++++++
 7 files changed, 600 insertions(+), 10 deletions(-)
 create mode 100644 migration/dirtyrate.c
 create mode 100644 migration/dirtyrate.h

-- 
1.8.3.1

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

[Chuan Zheng]

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

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
---
 migration/dirtyrate.c | 39 +++++++++++++++++++++++++++++++++++++++
 migration/dirtyrate.h | 32 ++++++++++++++++++++++++++++++++
 migration/meson.build |  1 +
 3 files changed, 72 insertions(+)
 create mode 100644 migration/dirtyrate.c
 create mode 100644 migration/dirtyrate.h

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
new file mode 100644
index 0000000..366f4e9
--- /dev/null
+++ b/migration/dirtyrate.c
@@ -0,0 +1,39 @@
+/*
+ * Dirtyrate implement code
+ *
+ * Copyright (c) 2017-2020 HUAWEI TECHNOLOGIES CO.,LTD.
+ *
+ * Authors:
+ *  Chuan Zheng <zhengchuan@huawei.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "crypto/hash.h"
+#include "crypto/random.h"
+#include "qemu/config-file.h"
+#include "exec/memory.h"
+#include "exec/ramblock.h"
+#include "exec/target_page.h"
+#include "qemu/rcu_queue.h"
+#include "qapi/qapi-commands-migration.h"
+#include "migration.h"
+#include "dirtyrate.h"
+
+static void calculate_dirtyrate(struct DirtyRateConfig config)
+{
+    /* todo */
+    return;
+}
+
+void *get_dirtyrate_thread(void *arg)
+{
+    struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
+
+    calculate_dirtyrate(config);
+
+    return NULL;
+}
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
new file mode 100644
index 0000000..33669b7
--- /dev/null
+++ b/migration/dirtyrate.h
@@ -0,0 +1,32 @@
+/*
+ *  Dirtyrate common functions
+ *
+ *  Copyright (c) 2020 HUAWEI TECHNOLOGIES CO., LTD.
+ *
+ *  Authors:
+ *  Chuan Zheng <zhengchuan@huawei.com>
+ *
+ *  This work is licensed under the terms of the GNU GPL, version 2 or later.
+ *  See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_MIGRATION_DIRTYRATE_H
+#define QEMU_MIGRATION_DIRTYRATE_H
+
+/*
+ * Sample 512 pages per GB as default.
+ * TODO: Make it configurable.
+ */
+#define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
+
+/* Take 1s as default for calculation duration */
+#define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
+
+struct DirtyRateConfig {
+    uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
+    int64_t sample_period_seconds; /* time duration between two sampling */
+};
+
+void *get_dirtyrate_thread(void *arg);
+#endif
+
diff --git a/migration/meson.build b/migration/meson.build
index ac8ff14..30cc6c3 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -21,6 +21,7 @@ softmmu_ss.add(files(
   'channel.c',
   'colo-failover.c',
   'colo.c',
+  'dirtyrate.c',
   'exec.c',
   'fd.c',
   'global_state.c',
-- 
1.8.3.1

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

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> Add get_dirtyrate_thread() functions to setup query-dirtyrate
> framework.
> 
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>

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

> ---
>  migration/dirtyrate.c | 39 +++++++++++++++++++++++++++++++++++++++
>  migration/dirtyrate.h | 32 ++++++++++++++++++++++++++++++++
>  migration/meson.build |  1 +
>  3 files changed, 72 insertions(+)
>  create mode 100644 migration/dirtyrate.c
>  create mode 100644 migration/dirtyrate.h
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> new file mode 100644
> index 0000000..366f4e9
> --- /dev/null
> +++ b/migration/dirtyrate.c
> @@ -0,0 +1,39 @@
> +/*
> + * Dirtyrate implement code
> + *
> + * Copyright (c) 2017-2020 HUAWEI TECHNOLOGIES CO.,LTD.
> + *
> + * Authors:
> + *  Chuan Zheng <zhengchuan@huawei.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "crypto/hash.h"
> +#include "crypto/random.h"
> +#include "qemu/config-file.h"
> +#include "exec/memory.h"
> +#include "exec/ramblock.h"
> +#include "exec/target_page.h"
> +#include "qemu/rcu_queue.h"
> +#include "qapi/qapi-commands-migration.h"
> +#include "migration.h"
> +#include "dirtyrate.h"
> +
> +static void calculate_dirtyrate(struct DirtyRateConfig config)
> +{
> +    /* todo */
> +    return;
> +}
> +
> +void *get_dirtyrate_thread(void *arg)
> +{
> +    struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
> +
> +    calculate_dirtyrate(config);
> +
> +    return NULL;
> +}
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> new file mode 100644
> index 0000000..33669b7
> --- /dev/null
> +++ b/migration/dirtyrate.h
> @@ -0,0 +1,32 @@
> +/*
> + *  Dirtyrate common functions
> + *
> + *  Copyright (c) 2020 HUAWEI TECHNOLOGIES CO., LTD.
> + *
> + *  Authors:
> + *  Chuan Zheng <zhengchuan@huawei.com>
> + *
> + *  This work is licensed under the terms of the GNU GPL, version 2 or later.
> + *  See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef QEMU_MIGRATION_DIRTYRATE_H
> +#define QEMU_MIGRATION_DIRTYRATE_H
> +
> +/*
> + * Sample 512 pages per GB as default.
> + * TODO: Make it configurable.
> + */
> +#define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
> +
> +/* Take 1s as default for calculation duration */
> +#define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
> +
> +struct DirtyRateConfig {
> +    uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
> +    int64_t sample_period_seconds; /* time duration between two sampling */
> +};
> +
> +void *get_dirtyrate_thread(void *arg);
> +#endif
> +
> diff --git a/migration/meson.build b/migration/meson.build
> index ac8ff14..30cc6c3 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -21,6 +21,7 @@ softmmu_ss.add(files(
>    'channel.c',
>    'colo-failover.c',
>    'colo.c',
> +  'dirtyrate.c',
>    'exec.c',
>    'fd.c',
>    'global_state.c',
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Chuan Zheng]

add DirtyRateStatus to denote calculating status.

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

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 366f4e9..91987c5 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -23,6 +23,19 @@
 #include "migration.h"
 #include "dirtyrate.h"
 
+static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
+
+static int dirtyrate_set_state(int *state, int old_state, int new_state)
+{
+    assert(new_state < DIRTY_RATE_STATUS__MAX);
+    if (atomic_cmpxchg(state, old_state, new_state) == old_state) {
+        return 0;
+    } else {
+        return -1;
+    }
+}
+
+
 static void calculate_dirtyrate(struct DirtyRateConfig config)
 {
     /* todo */
@@ -32,8 +45,17 @@ static void calculate_dirtyrate(struct DirtyRateConfig config)
 void *get_dirtyrate_thread(void *arg)
 {
     struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
+    int ret;
+
+    ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_UNSTARTED,
+                              DIRTY_RATE_STATUS_MEASURING);
+    if (ret == -1) {
+        return NULL;
+    }
 
     calculate_dirtyrate(config);
 
+    ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_MEASURING,
+                              DIRTY_RATE_STATUS_MEASURED);
     return NULL;
 }
diff --git a/qapi/migration.json b/qapi/migration.json
index 5f6b061..d640165 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1720,3 +1720,20 @@
 ##
 { 'event': 'UNPLUG_PRIMARY',
   'data': { 'device-id': 'str' } }
+
+##
+# @DirtyRateStatus:
+#
+# An enumeration of dirtyrate status.
+#
+# @unstarted: query-dirtyrate thread is not initial.
+#
+# @measuring: query-dirtyrate thread is created and start to measure.
+#
+# @measured:  query-dirtyrate thread is end, we can get result.
+#
+# Since: 5.2
+#
+##
+{ 'enum': 'DirtyRateStatus',
+  'data': [ 'unstarted', 'measuring', 'measured'] }
-- 
1.8.3.1

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

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> add DirtyRateStatus to denote calculating status.
> 
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>

With the minor wording changes from David Edmondson:


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

> ---
>  migration/dirtyrate.c | 22 ++++++++++++++++++++++
>  qapi/migration.json   | 17 +++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 366f4e9..91987c5 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -23,6 +23,19 @@
>  #include "migration.h"
>  #include "dirtyrate.h"
>  
> +static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
> +
> +static int dirtyrate_set_state(int *state, int old_state, int new_state)
> +{
> +    assert(new_state < DIRTY_RATE_STATUS__MAX);
> +    if (atomic_cmpxchg(state, old_state, new_state) == old_state) {
> +        return 0;
> +    } else {
> +        return -1;
> +    }
> +}
> +
> +
>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>  {
>      /* todo */
> @@ -32,8 +45,17 @@ static void calculate_dirtyrate(struct DirtyRateConfig config)
>  void *get_dirtyrate_thread(void *arg)
>  {
>      struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
> +    int ret;
> +
> +    ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_UNSTARTED,
> +                              DIRTY_RATE_STATUS_MEASURING);
> +    if (ret == -1) {
> +        return NULL;
> +    }
>  
>      calculate_dirtyrate(config);
>  
> +    ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_MEASURING,
> +                              DIRTY_RATE_STATUS_MEASURED);
>      return NULL;
>  }
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 5f6b061..d640165 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1720,3 +1720,20 @@
>  ##
>  { 'event': 'UNPLUG_PRIMARY',
>    'data': { 'device-id': 'str' } }
> +
> +##
> +# @DirtyRateStatus:
> +#
> +# An enumeration of dirtyrate status.
> +#
> +# @unstarted: query-dirtyrate thread is not initial.
> +#
> +# @measuring: query-dirtyrate thread is created and start to measure.
> +#
> +# @measured:  query-dirtyrate thread is end, we can get result.
> +#
> +# Since: 5.2
> +#
> +##
> +{ 'enum': 'DirtyRateStatus',
> +  'data': [ 'unstarted', 'measuring', 'measured'] }
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Zheng Chuan]

On 2020/8/26 19:49, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>> add DirtyRateStatus to denote calculating status.
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> 
> With the minor wording changes from David Edmondson:
> 
OK, will fix it in V6.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
>> ---
>>  migration/dirtyrate.c | 22 ++++++++++++++++++++++
>>  qapi/migration.json   | 17 +++++++++++++++++
>>  2 files changed, 39 insertions(+)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index 366f4e9..91987c5 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -23,6 +23,19 @@
>>  #include "migration.h"
>>  #include "dirtyrate.h"
>>  
>> +static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
>> +
>> +static int dirtyrate_set_state(int *state, int old_state, int new_state)
>> +{
>> +    assert(new_state < DIRTY_RATE_STATUS__MAX);
>> +    if (atomic_cmpxchg(state, old_state, new_state) == old_state) {
>> +        return 0;
>> +    } else {
>> +        return -1;
>> +    }
>> +}
>> +
>> +
>>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>>  {
>>      /* todo */
>> @@ -32,8 +45,17 @@ static void calculate_dirtyrate(struct DirtyRateConfig config)
>>  void *get_dirtyrate_thread(void *arg)
>>  {
>>      struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
>> +    int ret;
>> +
>> +    ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_UNSTARTED,
>> +                              DIRTY_RATE_STATUS_MEASURING);
>> +    if (ret == -1) {
>> +        return NULL;
>> +    }
>>  
>>      calculate_dirtyrate(config);
>>  
>> +    ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_MEASURING,
>> +                              DIRTY_RATE_STATUS_MEASURED);
>>      return NULL;
>>  }
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 5f6b061..d640165 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1720,3 +1720,20 @@
>>  ##
>>  { 'event': 'UNPLUG_PRIMARY',
>>    'data': { 'device-id': 'str' } }
>> +
>> +##
>> +# @DirtyRateStatus:
>> +#
>> +# An enumeration of dirtyrate status.
>> +#
>> +# @unstarted: query-dirtyrate thread is not initial.
>> +#
>> +# @measuring: query-dirtyrate thread is created and start to measure.
>> +#
>> +# @measured:  query-dirtyrate thread is end, we can get result.
>> +#
>> +# Since: 5.2
>> +#
>> +##
>> +{ 'enum': 'DirtyRateStatus',
>> +  'data': [ 'unstarted', 'measuring', 'measured'] }
>> -- 
>> 1.8.3.1
>>

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

[Chuan Zheng]

Add RamlockDirtyInfo to store sampled page info of each ramblock.

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

diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 33669b7..dc45419 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -19,6 +19,11 @@
  */
 #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
 
+/*
+ * Record ramblock idstr
+ */
+#define RAMBLOCK_INFO_MAX_LEN                     256
+
 /* Take 1s as default for calculation duration */
 #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
 
@@ -27,6 +32,19 @@ struct DirtyRateConfig {
     int64_t sample_period_seconds; /* time duration between two sampling */
 };
 
+/*
+ * Store dirtypage info for each ramblock.
+ */
+struct RamblockDirtyInfo {
+    char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */
+    uint8_t *ramblock_addr; /* base address of ramblock we measure */
+    uint64_t ramblock_pages; /* ramblock size in 4K-page */
+    uint64_t *sample_page_vfn; /* relative offset address for sampled page */
+    uint64_t sample_pages_count; /* count of sampled pages */
+    uint64_t sample_dirty_count; /* count of dirty pages we measure */
+    uint32_t *hash_result; /* array of hash result for sampled pages */
+};
+
 void *get_dirtyrate_thread(void *arg);
 #endif
 
-- 
1.8.3.1

Re: [PATCH v5 03/12] 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.

Note typo 'RAM*B*lockDirtyInfo' (and in the title.

> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> ---
>  migration/dirtyrate.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 33669b7..dc45419 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -19,6 +19,11 @@
>   */
>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
>  
> +/*
> + * Record ramblock idstr
> + */
> +#define RAMBLOCK_INFO_MAX_LEN                     256
> +
>  /* Take 1s as default for calculation duration */
>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>  
> @@ -27,6 +32,19 @@ struct DirtyRateConfig {
>      int64_t sample_period_seconds; /* time duration between two sampling */
>  };
>  
> +/*
> + * Store dirtypage info for each ramblock.
> + */
> +struct RamblockDirtyInfo {
> +    char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */
> +    uint8_t *ramblock_addr; /* base address of ramblock we measure */
> +    uint64_t ramblock_pages; /* ramblock size in 4K-page */
> +    uint64_t *sample_page_vfn; /* relative offset address for sampled page */
> +    uint64_t sample_pages_count; /* count of sampled pages */
> +    uint64_t sample_dirty_count; /* count of dirty pages we measure */

I see you've already fixed that one that David Edmondson commented on; I
thin you actually posted this as a second v5.

Since it's fixed, with the RAM*B*lock typo fixed;


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

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

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

[Chuan Zheng]

Add dirtyrate statistics to record/update dirtyrate info.

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

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 91987c5..0d7163f 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -24,6 +24,7 @@
 #include "dirtyrate.h"
 
 static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
+static struct DirtyRateStat DirtyStat;
 
 static int dirtyrate_set_state(int *state, int old_state, int new_state)
 {
@@ -35,6 +36,34 @@ static int dirtyrate_set_state(int *state, int old_state, int new_state)
     }
 }
 
+static void reset_dirtyrate_stat(void)
+{
+    DirtyStat.total_dirty_samples = 0;
+    DirtyStat.total_sample_count = 0;
+    DirtyStat.total_block_mem_MB = 0;
+    DirtyStat.dirty_rate = 0;
+}
+
+static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
+{
+    DirtyStat.total_dirty_samples += info->sample_dirty_count;
+    DirtyStat.total_sample_count += info->sample_pages_count;
+    /* size of 4K pages in MB */
+    DirtyStat.total_block_mem_MB += info->ramblock_pages / 256;
+}
+
+static void update_dirtyrate(uint64_t msec)
+{
+    uint64_t dirtyrate;
+    uint64_t total_dirty_samples = DirtyStat.total_dirty_samples;
+    uint64_t total_sample_count = DirtyStat.total_sample_count;
+    uint64_t total_block_mem_MB = DirtyStat.total_block_mem_MB;
+
+    dirtyrate = total_dirty_samples * total_block_mem_MB *
+                 1000 / (total_sample_count * msec);
+
+    DirtyStat.dirty_rate = dirtyrate;
+}
 
 static void calculate_dirtyrate(struct DirtyRateConfig config)
 {
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index dc45419..8e25d93 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -45,6 +45,16 @@ struct RamblockDirtyInfo {
     uint32_t *hash_result; /* array of hash result for sampled pages */
 };
 
+/*
+ * Store calculation statistics for each measure.
+ */
+struct DirtyRateStat {
+    uint64_t total_dirty_samples; /* total dirty sampled page */
+    uint64_t total_sample_count; /* total sampled pages */
+    uint64_t total_block_mem_MB; /* size of total sampled pages in MB */
+    int64_t dirty_rate; /* dirty rate in MB/s */
+};
+
 void *get_dirtyrate_thread(void *arg);
 #endif
 
-- 
1.8.3.1

Re: [PATCH v5 04/12] 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>
> ---
>  migration/dirtyrate.c | 29 +++++++++++++++++++++++++++++
>  migration/dirtyrate.h | 10 ++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 91987c5..0d7163f 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -24,6 +24,7 @@
>  #include "dirtyrate.h"
>  
>  static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
> +static struct DirtyRateStat DirtyStat;
>  
>  static int dirtyrate_set_state(int *state, int old_state, int new_state)
>  {
> @@ -35,6 +36,34 @@ static int dirtyrate_set_state(int *state, int old_state, int new_state)
>      }
>  }
>  
> +static void reset_dirtyrate_stat(void)
> +{
> +    DirtyStat.total_dirty_samples = 0;
> +    DirtyStat.total_sample_count = 0;
> +    DirtyStat.total_block_mem_MB = 0;
> +    DirtyStat.dirty_rate = 0;
> +}
> +
> +static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
> +{
> +    DirtyStat.total_dirty_samples += info->sample_dirty_count;
> +    DirtyStat.total_sample_count += info->sample_pages_count;
> +    /* size of 4K pages in MB */
> +    DirtyStat.total_block_mem_MB += info->ramblock_pages / 256;

You need to be consistent with your use of constants, you have
a DIRTYRATE_SAMPLE_PAGE_SIZE in a later patch; so use it rather than
hard coding 256 here.

Dave

> +}
> +
> +static void update_dirtyrate(uint64_t msec)
> +{
> +    uint64_t dirtyrate;
> +    uint64_t total_dirty_samples = DirtyStat.total_dirty_samples;
> +    uint64_t total_sample_count = DirtyStat.total_sample_count;
> +    uint64_t total_block_mem_MB = DirtyStat.total_block_mem_MB;
> +
> +    dirtyrate = total_dirty_samples * total_block_mem_MB *
> +                 1000 / (total_sample_count * msec);
> +
> +    DirtyStat.dirty_rate = dirtyrate;
> +}
>  
>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>  {
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index dc45419..8e25d93 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -45,6 +45,16 @@ struct RamblockDirtyInfo {
>      uint32_t *hash_result; /* array of hash result for sampled pages */
>  };
>  
> +/*
> + * Store calculation statistics for each measure.
> + */
> +struct DirtyRateStat {
> +    uint64_t total_dirty_samples; /* total dirty sampled page */
> +    uint64_t total_sample_count; /* total sampled pages */
> +    uint64_t total_block_mem_MB; /* size of total sampled pages in MB */
> +    int64_t dirty_rate; /* dirty rate in MB/s */
> +};
> +
>  void *get_dirtyrate_thread(void *arg);
>  #endif
>  
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Zheng Chuan]

On 2020/8/26 20:09, 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>
>> ---
>>  migration/dirtyrate.c | 29 +++++++++++++++++++++++++++++
>>  migration/dirtyrate.h | 10 ++++++++++
>>  2 files changed, 39 insertions(+)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index 91987c5..0d7163f 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -24,6 +24,7 @@
>>  #include "dirtyrate.h"
>>  
>>  static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
>> +static struct DirtyRateStat DirtyStat;
>>  
>>  static int dirtyrate_set_state(int *state, int old_state, int new_state)
>>  {
>> @@ -35,6 +36,34 @@ static int dirtyrate_set_state(int *state, int old_state, int new_state)
>>      }
>>  }
>>  
>> +static void reset_dirtyrate_stat(void)
>> +{
>> +    DirtyStat.total_dirty_samples = 0;
>> +    DirtyStat.total_sample_count = 0;
>> +    DirtyStat.total_block_mem_MB = 0;
>> +    DirtyStat.dirty_rate = 0;
>> +}
>> +
>> +static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
>> +{
>> +    DirtyStat.total_dirty_samples += info->sample_dirty_count;
>> +    DirtyStat.total_sample_count += info->sample_pages_count;
>> +    /* size of 4K pages in MB */
>> +    DirtyStat.total_block_mem_MB += info->ramblock_pages / 256;
> 
> You need to be consistent with your use of constants, you have
> a DIRTYRATE_SAMPLE_PAGE_SIZE in a later patch; so use it rather than
> hard coding 256 here.
> 
> Dave
> 
Sure.
I may take as
'DirtyStat.total_block_mem_MB += (info->ramblock_pages * DIRTYRATE_SAMPLE_PAGE_SIZE) >> 20';

>> +}
>> +
>> +static void update_dirtyrate(uint64_t msec)
>> +{
>> +    uint64_t dirtyrate;
>> +    uint64_t total_dirty_samples = DirtyStat.total_dirty_samples;
>> +    uint64_t total_sample_count = DirtyStat.total_sample_count;
>> +    uint64_t total_block_mem_MB = DirtyStat.total_block_mem_MB;
>> +
>> +    dirtyrate = total_dirty_samples * total_block_mem_MB *
>> +                 1000 / (total_sample_count * msec);
>> +
>> +    DirtyStat.dirty_rate = dirtyrate;
>> +}
>>  
>>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>>  {
>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>> index dc45419..8e25d93 100644
>> --- a/migration/dirtyrate.h
>> +++ b/migration/dirtyrate.h
>> @@ -45,6 +45,16 @@ struct RamblockDirtyInfo {
>>      uint32_t *hash_result; /* array of hash result for sampled pages */
>>  };
>>  
>> +/*
>> + * Store calculation statistics for each measure.
>> + */
>> +struct DirtyRateStat {
>> +    uint64_t total_dirty_samples; /* total dirty sampled page */
>> +    uint64_t total_sample_count; /* total sampled pages */
>> +    uint64_t total_block_mem_MB; /* size of total sampled pages in MB */
>> +    int64_t dirty_rate; /* dirty rate in MB/s */
>> +};
>> +
>>  void *get_dirtyrate_thread(void *arg);
>>  #endif
>>  
>> -- 
>> 1.8.3.1
>>

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

[Chuan Zheng]

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

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

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

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

[Chuan Zheng]

Record hash results for each sampled page, crc32 is taken to calculate
hash results for each sampled 4K-page.

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

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index f6a94d8..66de426 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -10,6 +10,7 @@
  * See the COPYING file in the top-level directory.
  */
 
+#include <zlib.h>
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "crypto/hash.h"
@@ -66,6 +67,141 @@ static void update_dirtyrate(uint64_t msec)
     DirtyStat.dirty_rate = dirtyrate;
 }
 
+/*
+ * get hash result for the sampled memory with length of 4K byte in ramblock,
+ * which starts from ramblock base address.
+ */
+static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
+                                      uint64_t vfn)
+{
+    struct iovec iov_array;
+    uint32_t crc;
+
+    iov_array.iov_base = info->ramblock_addr +
+                         vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
+    iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;
+
+    crc = crc32(0, iov_array.iov_base, iov_array.iov_len);
+
+    return crc;
+}
+
+static int save_ramblock_hash(struct RamblockDirtyInfo *info)
+{
+    unsigned int sample_pages_count;
+    int i;
+    int ret = -1;
+    GRand *rand = g_rand_new();
+
+    sample_pages_count = info->sample_pages_count;
+
+    /* ramblock size less than one page, return success to skip this ramblock */
+    if (unlikely(info->ramblock_pages == 0 || sample_pages_count == 0)) {
+        ret = 0;
+        goto out;
+    }
+
+    info->hash_result = g_try_malloc0_n(sample_pages_count,
+                                        sizeof(uint32_t));
+    if (!info->hash_result) {
+        ret = -1;
+        goto out;
+    }
+
+    info->sample_page_vfn = g_try_malloc0_n(sample_pages_count,
+                                            sizeof(uint64_t));
+    if (!info->sample_page_vfn) {
+        g_free(info->hash_result);
+        ret = -1;
+        goto out;
+    }
+
+    for (i = 0; i < sample_pages_count; i++) {
+        info->sample_page_vfn[i] = g_rand_int_range(rand, 0,
+                                                    info->ramblock_pages - 1);
+        info->hash_result[i] = get_ramblock_vfn_hash(info,
+                                                     info->sample_page_vfn[i]);
+    }
+    ret = 0;
+
+out:
+    g_rand_free(rand);
+    return ret;
+}
+
+static void get_ramblock_dirty_info(RAMBlock *block,
+                                    struct RamblockDirtyInfo *info,
+                                    struct DirtyRateConfig *config)
+{
+    uint64_t sample_pages_per_gigabytes = config->sample_pages_per_gigabytes;
+
+    /* Right shift 30 bits to calc block size in GB */
+    info->sample_pages_count = (qemu_ram_get_used_length(block) *
+                                sample_pages_per_gigabytes) >>
+                                DIRTYRATE_PAGE_SHIFT_GB;
+
+    /* Right shift 12 bits to calc page count in 4KB */
+    info->ramblock_pages = qemu_ram_get_used_length(block) >>
+                           DIRTYRATE_PAGE_SHIFT_KB;
+    info->ramblock_addr = qemu_ram_get_host_addr(block);
+    strcpy(info->idstr, qemu_ram_get_idstr(block));
+}
+
+static struct RamblockDirtyInfo *
+alloc_ramblock_dirty_info(int *block_index,
+                          struct RamblockDirtyInfo *block_dinfo)
+{
+    struct RamblockDirtyInfo *info = NULL;
+    int index = *block_index;
+
+    if (!block_dinfo) {
+        index = 0;
+        block_dinfo = g_try_new(struct RamblockDirtyInfo, 1);
+    } else {
+        index++;
+        block_dinfo = g_try_realloc(block_dinfo, (index + 1) *
+                                    sizeof(struct RamblockDirtyInfo));
+    }
+    if (!block_dinfo) {
+        return NULL;
+    }
+
+    info = &block_dinfo[index];
+    *block_index = index;
+    memset(info, 0, sizeof(struct RamblockDirtyInfo));
+
+    return block_dinfo;
+}
+
+static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
+                                     struct DirtyRateConfig config,
+                                     int *block_index)
+{
+    struct RamblockDirtyInfo *info = NULL;
+    struct RamblockDirtyInfo *dinfo = NULL;
+    RAMBlock *block = NULL;
+    int index = 0;
+
+    RAMBLOCK_FOREACH_MIGRATABLE(block) {
+        dinfo = alloc_ramblock_dirty_info(&index, dinfo);
+        if (dinfo == NULL) {
+            return -1;
+        }
+        info = &dinfo[index];
+        get_ramblock_dirty_info(block, info, &config);
+        if (save_ramblock_hash(info) < 0) {
+            *block_dinfo = dinfo;
+            *block_index = index;
+            return -1;
+        }
+    }
+
+    *block_dinfo = dinfo;
+    *block_index = index;
+
+    return 0;
+}
+
 static void calculate_dirtyrate(struct DirtyRateConfig config)
 {
     /* todo */
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 8e25d93..e3adead 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -24,6 +24,21 @@
  */
 #define RAMBLOCK_INFO_MAX_LEN                     256
 
+/*
+ * Sample page size 4K as default.
+ */
+#define DIRTYRATE_SAMPLE_PAGE_SIZE                4096
+
+/*
+ * Sample page size 4K shift
+ */
+#define DIRTYRATE_PAGE_SHIFT_KB                   12
+
+/*
+ * Sample page size 1G shift
+ */
+#define DIRTYRATE_PAGE_SHIFT_GB                   30
+
 /* Take 1s as default for calculation duration */
 #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
 
-- 
1.8.3.1

Re: [PATCH v5 06/12] 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, crc32 is taken to calculate
> hash results for each sampled 4K-page.
> 
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> ---
>  migration/dirtyrate.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  migration/dirtyrate.h |  15 ++++++
>  2 files changed, 151 insertions(+)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index f6a94d8..66de426 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -10,6 +10,7 @@
>   * See the COPYING file in the top-level directory.
>   */
>  
> +#include <zlib.h>
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "crypto/hash.h"
> @@ -66,6 +67,141 @@ static void update_dirtyrate(uint64_t msec)
>      DirtyStat.dirty_rate = dirtyrate;
>  }
>  
> +/*
> + * get hash result for the sampled memory with length of 4K byte in ramblock,
> + * which starts from ramblock base address.
> + */
> +static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
> +                                      uint64_t vfn)
> +{
> +    struct iovec iov_array;
> +    uint32_t crc;
> +
> +    iov_array.iov_base = info->ramblock_addr +
> +                         vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
> +    iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;
> +
> +    crc = crc32(0, iov_array.iov_base, iov_array.iov_len);
> +
> +    return crc;
> +}
> +
> +static int save_ramblock_hash(struct RamblockDirtyInfo *info)
> +{
> +    unsigned int sample_pages_count;
> +    int i;
> +    int ret = -1;
> +    GRand *rand = g_rand_new();
> +
> +    sample_pages_count = info->sample_pages_count;
> +
> +    /* ramblock size less than one page, return success to skip this ramblock */
> +    if (unlikely(info->ramblock_pages == 0 || sample_pages_count == 0)) {
> +        ret = 0;
> +        goto out;
> +    }
> +
> +    info->hash_result = g_try_malloc0_n(sample_pages_count,
> +                                        sizeof(uint32_t));
> +    if (!info->hash_result) {
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    info->sample_page_vfn = g_try_malloc0_n(sample_pages_count,
> +                                            sizeof(uint64_t));
> +    if (!info->sample_page_vfn) {
> +        g_free(info->hash_result);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    for (i = 0; i < sample_pages_count; i++) {
> +        info->sample_page_vfn[i] = g_rand_int_range(rand, 0,
> +                                                    info->ramblock_pages - 1);
> +        info->hash_result[i] = get_ramblock_vfn_hash(info,
> +                                                     info->sample_page_vfn[i]);
> +    }
> +    ret = 0;
> +
> +out:
> +    g_rand_free(rand);
> +    return ret;
> +}
> +
> +static void get_ramblock_dirty_info(RAMBlock *block,
> +                                    struct RamblockDirtyInfo *info,
> +                                    struct DirtyRateConfig *config)
> +{
> +    uint64_t sample_pages_per_gigabytes = config->sample_pages_per_gigabytes;
> +
> +    /* Right shift 30 bits to calc block size in GB */
> +    info->sample_pages_count = (qemu_ram_get_used_length(block) *
> +                                sample_pages_per_gigabytes) >>
> +                                DIRTYRATE_PAGE_SHIFT_GB;
> +
> +    /* Right shift 12 bits to calc page count in 4KB */
> +    info->ramblock_pages = qemu_ram_get_used_length(block) >>
> +                           DIRTYRATE_PAGE_SHIFT_KB;
> +    info->ramblock_addr = qemu_ram_get_host_addr(block);
> +    strcpy(info->idstr, qemu_ram_get_idstr(block));
> +}
> +
> +static struct RamblockDirtyInfo *
> +alloc_ramblock_dirty_info(int *block_index,
> +                          struct RamblockDirtyInfo *block_dinfo)
> +{
> +    struct RamblockDirtyInfo *info = NULL;
> +    int index = *block_index;
> +
> +    if (!block_dinfo) {
> +        index = 0;
> +        block_dinfo = g_try_new(struct RamblockDirtyInfo, 1);
> +    } else {
> +        index++;
> +        block_dinfo = g_try_realloc(block_dinfo, (index + 1) *
> +                                    sizeof(struct RamblockDirtyInfo));
> +    }
> +    if (!block_dinfo) {
> +        return NULL;
> +    }
> +
> +    info = &block_dinfo[index];
> +    *block_index = index;
> +    memset(info, 0, sizeof(struct RamblockDirtyInfo));
> +
> +    return block_dinfo;
> +}
> +
> +static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
> +                                     struct DirtyRateConfig config,
> +                                     int *block_index)
> +{
> +    struct RamblockDirtyInfo *info = NULL;
> +    struct RamblockDirtyInfo *dinfo = NULL;
> +    RAMBlock *block = NULL;
> +    int index = 0;
> +
> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
> +        dinfo = alloc_ramblock_dirty_info(&index, dinfo);
> +        if (dinfo == NULL) {
> +            return -1;
> +        }
> +        info = &dinfo[index];
> +        get_ramblock_dirty_info(block, info, &config);
> +        if (save_ramblock_hash(info) < 0) {
> +            *block_dinfo = dinfo;
> +            *block_index = index;
> +            return -1;
> +        }
> +    }
> +
> +    *block_dinfo = dinfo;
> +    *block_index = index;
> +
> +    return 0;
> +}
> +
>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>  {
>      /* todo */
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 8e25d93..e3adead 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -24,6 +24,21 @@
>   */
>  #define RAMBLOCK_INFO_MAX_LEN                     256
>  
> +/*
> + * Sample page size 4K as default.
> + */
> +#define DIRTYRATE_SAMPLE_PAGE_SIZE                4096
> +
> +/*
> + * Sample page size 4K shift
> + */
> +#define DIRTYRATE_PAGE_SHIFT_KB                   12
> +
> +/*
> + * Sample page size 1G shift
> + */
> +#define DIRTYRATE_PAGE_SHIFT_GB                   30

Your naming is really odd here;  'PAGE_SHIFT_KB' divides
by 4KB, where as 'PAGE_SHIFT_GB' divices by 1KB.

Simplify this;  you can just do >>30 for GB because it's well known;
you don't need a #define constant for simple KB,MB,GB since
we all know them.

Also, I've asked before - do you really want 4KB explicitly - or
should you just use TARGET_PAGE_SIZE and TARGET_PAGE_BITS ?

Dave

> +
>  /* 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 v5 06/12] migration/dirtyrate: Record hash results for each sampled page

[Zheng Chuan]

On 2020/8/26 20:35, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>> Record hash results for each sampled page, crc32 is taken to calculate
>> hash results for each sampled 4K-page.
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>> ---
>>  migration/dirtyrate.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  migration/dirtyrate.h |  15 ++++++
>>  2 files changed, 151 insertions(+)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index f6a94d8..66de426 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -10,6 +10,7 @@
>>   * See the COPYING file in the top-level directory.
>>   */
>>  
>> +#include <zlib.h>
>>  #include "qemu/osdep.h"
>>  #include "qapi/error.h"
>>  #include "crypto/hash.h"
>> @@ -66,6 +67,141 @@ static void update_dirtyrate(uint64_t msec)
>>      DirtyStat.dirty_rate = dirtyrate;
>>  }
>>  
>> +/*
>> + * get hash result for the sampled memory with length of 4K byte in ramblock,
>> + * which starts from ramblock base address.
>> + */
>> +static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
>> +                                      uint64_t vfn)
>> +{
>> +    struct iovec iov_array;
>> +    uint32_t crc;
>> +
>> +    iov_array.iov_base = info->ramblock_addr +
>> +                         vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
>> +    iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;
>> +
>> +    crc = crc32(0, iov_array.iov_base, iov_array.iov_len);
>> +
>> +    return crc;
>> +}
>> +
>> +static int save_ramblock_hash(struct RamblockDirtyInfo *info)
>> +{
>> +    unsigned int sample_pages_count;
>> +    int i;
>> +    int ret = -1;
>> +    GRand *rand = g_rand_new();
>> +
>> +    sample_pages_count = info->sample_pages_count;
>> +
>> +    /* ramblock size less than one page, return success to skip this ramblock */
>> +    if (unlikely(info->ramblock_pages == 0 || sample_pages_count == 0)) {
>> +        ret = 0;
>> +        goto out;
>> +    }
>> +
>> +    info->hash_result = g_try_malloc0_n(sample_pages_count,
>> +                                        sizeof(uint32_t));
>> +    if (!info->hash_result) {
>> +        ret = -1;
>> +        goto out;
>> +    }
>> +
>> +    info->sample_page_vfn = g_try_malloc0_n(sample_pages_count,
>> +                                            sizeof(uint64_t));
>> +    if (!info->sample_page_vfn) {
>> +        g_free(info->hash_result);
>> +        ret = -1;
>> +        goto out;
>> +    }
>> +
>> +    for (i = 0; i < sample_pages_count; i++) {
>> +        info->sample_page_vfn[i] = g_rand_int_range(rand, 0,
>> +                                                    info->ramblock_pages - 1);
>> +        info->hash_result[i] = get_ramblock_vfn_hash(info,
>> +                                                     info->sample_page_vfn[i]);
>> +    }
>> +    ret = 0;
>> +
>> +out:
>> +    g_rand_free(rand);
>> +    return ret;
>> +}
>> +
>> +static void get_ramblock_dirty_info(RAMBlock *block,
>> +                                    struct RamblockDirtyInfo *info,
>> +                                    struct DirtyRateConfig *config)
>> +{
>> +    uint64_t sample_pages_per_gigabytes = config->sample_pages_per_gigabytes;
>> +
>> +    /* Right shift 30 bits to calc block size in GB */
>> +    info->sample_pages_count = (qemu_ram_get_used_length(block) *
>> +                                sample_pages_per_gigabytes) >>
>> +                                DIRTYRATE_PAGE_SHIFT_GB;
>> +
>> +    /* Right shift 12 bits to calc page count in 4KB */
>> +    info->ramblock_pages = qemu_ram_get_used_length(block) >>
>> +                           DIRTYRATE_PAGE_SHIFT_KB;
>> +    info->ramblock_addr = qemu_ram_get_host_addr(block);
>> +    strcpy(info->idstr, qemu_ram_get_idstr(block));
>> +}
>> +
>> +static struct RamblockDirtyInfo *
>> +alloc_ramblock_dirty_info(int *block_index,
>> +                          struct RamblockDirtyInfo *block_dinfo)
>> +{
>> +    struct RamblockDirtyInfo *info = NULL;
>> +    int index = *block_index;
>> +
>> +    if (!block_dinfo) {
>> +        index = 0;
>> +        block_dinfo = g_try_new(struct RamblockDirtyInfo, 1);
>> +    } else {
>> +        index++;
>> +        block_dinfo = g_try_realloc(block_dinfo, (index + 1) *
>> +                                    sizeof(struct RamblockDirtyInfo));
>> +    }
>> +    if (!block_dinfo) {
>> +        return NULL;
>> +    }
>> +
>> +    info = &block_dinfo[index];
>> +    *block_index = index;
>> +    memset(info, 0, sizeof(struct RamblockDirtyInfo));
>> +
>> +    return block_dinfo;
>> +}
>> +
>> +static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
>> +                                     struct DirtyRateConfig config,
>> +                                     int *block_index)
>> +{
>> +    struct RamblockDirtyInfo *info = NULL;
>> +    struct RamblockDirtyInfo *dinfo = NULL;
>> +    RAMBlock *block = NULL;
>> +    int index = 0;
>> +
>> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
>> +        dinfo = alloc_ramblock_dirty_info(&index, dinfo);
>> +        if (dinfo == NULL) {
>> +            return -1;
>> +        }
>> +        info = &dinfo[index];
>> +        get_ramblock_dirty_info(block, info, &config);
>> +        if (save_ramblock_hash(info) < 0) {
>> +            *block_dinfo = dinfo;
>> +            *block_index = index;
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    *block_dinfo = dinfo;
>> +    *block_index = index;
>> +
>> +    return 0;
>> +}
>> +
>>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>>  {
>>      /* todo */
>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>> index 8e25d93..e3adead 100644
>> --- a/migration/dirtyrate.h
>> +++ b/migration/dirtyrate.h
>> @@ -24,6 +24,21 @@
>>   */
>>  #define RAMBLOCK_INFO_MAX_LEN                     256
>>  
>> +/*
>> + * Sample page size 4K as default.
>> + */
>> +#define DIRTYRATE_SAMPLE_PAGE_SIZE                4096
>> +
>> +/*
>> + * Sample page size 4K shift
>> + */
>> +#define DIRTYRATE_PAGE_SHIFT_KB                   12
>> +
>> +/*
>> + * Sample page size 1G shift
>> + */
>> +#define DIRTYRATE_PAGE_SHIFT_GB                   30
> 
> Your naming is really odd here;  'PAGE_SHIFT_KB' divides
> by 4KB, where as 'PAGE_SHIFT_GB' divices by 1KB.
> 
> Simplify this;  you can just do >>30 for GB because it's well known;
> you don't need a #define constant for simple KB,MB,GB since
> we all know them.
> 
Hi, Dave.
Thank you for review.
OK, i will fix that in V6:)
> Also, I've asked before - do you really want 4KB explicitly - or
> should you just use TARGET_PAGE_SIZE and TARGET_PAGE_BITS ?
> 
> Dave
>
TARGET_PAGE_SIZE will be 2M or 1G for HugePage.
As you see, what we get is hash result of every 'virtual' 4K-page.
We care about if it is dirty within 4K length in ramblock, which would be more
accurate than TARGET_PAGE_SIZE which could be 2M or 1G.
On the other hand, the hugepage will be broken up into 4K during migration.

I think it is better we do hash at 'virtual' 4K-page granularity.

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

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

[Dr. David Alan Gilbert]

* Zheng Chuan (zhengchuan@huawei.com) wrote:
> 
> 
> On 2020/8/26 20:35, Dr. David Alan Gilbert wrote:
> > * Chuan Zheng (zhengchuan@huawei.com) wrote:
> >> Record hash results for each sampled page, crc32 is taken to calculate
> >> hash results for each sampled 4K-page.
> >>
> >> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> >> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
> >> ---
> >>  migration/dirtyrate.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  migration/dirtyrate.h |  15 ++++++
> >>  2 files changed, 151 insertions(+)
> >>
> >> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> >> index f6a94d8..66de426 100644
> >> --- a/migration/dirtyrate.c
> >> +++ b/migration/dirtyrate.c
> >> @@ -10,6 +10,7 @@
> >>   * See the COPYING file in the top-level directory.
> >>   */
> >>  
> >> +#include <zlib.h>
> >>  #include "qemu/osdep.h"
> >>  #include "qapi/error.h"
> >>  #include "crypto/hash.h"
> >> @@ -66,6 +67,141 @@ static void update_dirtyrate(uint64_t msec)
> >>      DirtyStat.dirty_rate = dirtyrate;
> >>  }
> >>  
> >> +/*
> >> + * get hash result for the sampled memory with length of 4K byte in ramblock,
> >> + * which starts from ramblock base address.
> >> + */
> >> +static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
> >> +                                      uint64_t vfn)
> >> +{
> >> +    struct iovec iov_array;
> >> +    uint32_t crc;
> >> +
> >> +    iov_array.iov_base = info->ramblock_addr +
> >> +                         vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
> >> +    iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;
> >> +
> >> +    crc = crc32(0, iov_array.iov_base, iov_array.iov_len);
> >> +
> >> +    return crc;
> >> +}
> >> +
> >> +static int save_ramblock_hash(struct RamblockDirtyInfo *info)
> >> +{
> >> +    unsigned int sample_pages_count;
> >> +    int i;
> >> +    int ret = -1;
> >> +    GRand *rand = g_rand_new();
> >> +
> >> +    sample_pages_count = info->sample_pages_count;
> >> +
> >> +    /* ramblock size less than one page, return success to skip this ramblock */
> >> +    if (unlikely(info->ramblock_pages == 0 || sample_pages_count == 0)) {
> >> +        ret = 0;
> >> +        goto out;
> >> +    }
> >> +
> >> +    info->hash_result = g_try_malloc0_n(sample_pages_count,
> >> +                                        sizeof(uint32_t));
> >> +    if (!info->hash_result) {
> >> +        ret = -1;
> >> +        goto out;
> >> +    }
> >> +
> >> +    info->sample_page_vfn = g_try_malloc0_n(sample_pages_count,
> >> +                                            sizeof(uint64_t));
> >> +    if (!info->sample_page_vfn) {
> >> +        g_free(info->hash_result);
> >> +        ret = -1;
> >> +        goto out;
> >> +    }
> >> +
> >> +    for (i = 0; i < sample_pages_count; i++) {
> >> +        info->sample_page_vfn[i] = g_rand_int_range(rand, 0,
> >> +                                                    info->ramblock_pages - 1);
> >> +        info->hash_result[i] = get_ramblock_vfn_hash(info,
> >> +                                                     info->sample_page_vfn[i]);
> >> +    }
> >> +    ret = 0;
> >> +
> >> +out:
> >> +    g_rand_free(rand);
> >> +    return ret;
> >> +}
> >> +
> >> +static void get_ramblock_dirty_info(RAMBlock *block,
> >> +                                    struct RamblockDirtyInfo *info,
> >> +                                    struct DirtyRateConfig *config)
> >> +{
> >> +    uint64_t sample_pages_per_gigabytes = config->sample_pages_per_gigabytes;
> >> +
> >> +    /* Right shift 30 bits to calc block size in GB */
> >> +    info->sample_pages_count = (qemu_ram_get_used_length(block) *
> >> +                                sample_pages_per_gigabytes) >>
> >> +                                DIRTYRATE_PAGE_SHIFT_GB;
> >> +
> >> +    /* Right shift 12 bits to calc page count in 4KB */
> >> +    info->ramblock_pages = qemu_ram_get_used_length(block) >>
> >> +                           DIRTYRATE_PAGE_SHIFT_KB;
> >> +    info->ramblock_addr = qemu_ram_get_host_addr(block);
> >> +    strcpy(info->idstr, qemu_ram_get_idstr(block));
> >> +}
> >> +
> >> +static struct RamblockDirtyInfo *
> >> +alloc_ramblock_dirty_info(int *block_index,
> >> +                          struct RamblockDirtyInfo *block_dinfo)
> >> +{
> >> +    struct RamblockDirtyInfo *info = NULL;
> >> +    int index = *block_index;
> >> +
> >> +    if (!block_dinfo) {
> >> +        index = 0;
> >> +        block_dinfo = g_try_new(struct RamblockDirtyInfo, 1);
> >> +    } else {
> >> +        index++;
> >> +        block_dinfo = g_try_realloc(block_dinfo, (index + 1) *
> >> +                                    sizeof(struct RamblockDirtyInfo));
> >> +    }
> >> +    if (!block_dinfo) {
> >> +        return NULL;
> >> +    }
> >> +
> >> +    info = &block_dinfo[index];
> >> +    *block_index = index;
> >> +    memset(info, 0, sizeof(struct RamblockDirtyInfo));
> >> +
> >> +    return block_dinfo;
> >> +}
> >> +
> >> +static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
> >> +                                     struct DirtyRateConfig config,
> >> +                                     int *block_index)
> >> +{
> >> +    struct RamblockDirtyInfo *info = NULL;
> >> +    struct RamblockDirtyInfo *dinfo = NULL;
> >> +    RAMBlock *block = NULL;
> >> +    int index = 0;
> >> +
> >> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
> >> +        dinfo = alloc_ramblock_dirty_info(&index, dinfo);
> >> +        if (dinfo == NULL) {
> >> +            return -1;
> >> +        }
> >> +        info = &dinfo[index];
> >> +        get_ramblock_dirty_info(block, info, &config);
> >> +        if (save_ramblock_hash(info) < 0) {
> >> +            *block_dinfo = dinfo;
> >> +            *block_index = index;
> >> +            return -1;
> >> +        }
> >> +    }
> >> +
> >> +    *block_dinfo = dinfo;
> >> +    *block_index = index;
> >> +
> >> +    return 0;
> >> +}
> >> +
> >>  static void calculate_dirtyrate(struct DirtyRateConfig config)
> >>  {
> >>      /* todo */
> >> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> >> index 8e25d93..e3adead 100644
> >> --- a/migration/dirtyrate.h
> >> +++ b/migration/dirtyrate.h
> >> @@ -24,6 +24,21 @@
> >>   */
> >>  #define RAMBLOCK_INFO_MAX_LEN                     256
> >>  
> >> +/*
> >> + * Sample page size 4K as default.
> >> + */
> >> +#define DIRTYRATE_SAMPLE_PAGE_SIZE                4096
> >> +
> >> +/*
> >> + * Sample page size 4K shift
> >> + */
> >> +#define DIRTYRATE_PAGE_SHIFT_KB                   12
> >> +
> >> +/*
> >> + * Sample page size 1G shift
> >> + */
> >> +#define DIRTYRATE_PAGE_SHIFT_GB                   30
> > 
> > Your naming is really odd here;  'PAGE_SHIFT_KB' divides
> > by 4KB, where as 'PAGE_SHIFT_GB' divices by 1KB.
> > 
> > Simplify this;  you can just do >>30 for GB because it's well known;
> > you don't need a #define constant for simple KB,MB,GB since
> > we all know them.
> > 
> Hi, Dave.
> Thank you for review.
> OK, i will fix that in V6:)
> > Also, I've asked before - do you really want 4KB explicitly - or
> > should you just use TARGET_PAGE_SIZE and TARGET_PAGE_BITS ?
> > 
> > Dave
> >
> TARGET_PAGE_SIZE will be 2M or 1G for HugePage.
> As you see, what we get is hash result of every 'virtual' 4K-page.
> We care about if it is dirty within 4K length in ramblock, which would be more
> accurate than TARGET_PAGE_SIZE which could be 2M or 1G.
> On the other hand, the hugepage will be broken up into 4K during migration.
> 
> I think it is better we do hash at 'virtual' 4K-page granularity.

TARGET_PAGE_SIZE is never 2M or 1G; it's always based on the smallest
MMU page on the platform; on x86 it's always 4kB; it's the unit that the
migration code works in when dealing with pages.
(use TARGET_PAGE_BITS to shift by).

Dave

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

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

[Chuan Zheng]

Compare page hash results for recorded sampled page.

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

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

Re: [PATCH v5 07/12] 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>

So I think this is OK, with the minor clean up suggestions of David E.

> ---
>  migration/dirtyrate.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 66de426..050270d 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -202,6 +202,70 @@ static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
>      return 0;
>  }
>  
> +static int calc_page_dirty_rate(struct RamblockDirtyInfo *info)
> +{
> +    uint32_t crc;
> +    int i;
> +
> +    for (i = 0; i < info->sample_pages_count; i++) {
> +        crc = get_ramblock_vfn_hash(info, info->sample_page_vfn[i]);
> +        if (crc != info->hash_result[i]) {
> +            info->sample_dirty_count++;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static bool find_page_matched(RAMBlock *block, struct RamblockDirtyInfo *infos,
> +                              int count, struct RamblockDirtyInfo **matched)
> +{
> +    int i;
> +
> +    for (i = 0; i < count; i++) {
> +        if (!strcmp(infos[i].idstr, qemu_ram_get_idstr(block))) {
> +            break;
> +        }
> +    }
> +
> +    if (i == count) {
> +        return false;
> +    }
> +
> +    if (infos[i].ramblock_addr != qemu_ram_get_host_addr(block) ||
> +        infos[i].ramblock_pages !=
> +            (qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SHIFT_KB)) {
> +        return false;
> +    }
> +
> +    *matched = &infos[i];
> +    return true;
> +}
> +
> +static int compare_page_hash_info(struct RamblockDirtyInfo *info,
> +                                  int block_index)
> +{
> +    struct RamblockDirtyInfo *block_dinfo = NULL;
> +    RAMBlock *block = NULL;
> +
> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
> +        block_dinfo = NULL;
> +        if (!find_page_matched(block, info, block_index + 1, &block_dinfo)) {
> +            continue;
> +        }
> +        if (calc_page_dirty_rate(block_dinfo) < 0) {
> +            return -1;
> +        }
> +        update_dirtyrate_stat(block_dinfo);
> +    }
> +
> +    if (!DirtyStat.total_sample_count) {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  static void calculate_dirtyrate(struct DirtyRateConfig config)
>  {
>      /* todo */
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Chuan Zheng]

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

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

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

Re: [PATCH v5 08/12] 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 MIN_RAMBLOCK_SIZE
> which is set as 128M.
> 
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> ---
>  migration/dirtyrate.c | 24 ++++++++++++++++++++++++
>  migration/dirtyrate.h | 10 ++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 050270d..bd398b7 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -173,6 +173,24 @@ alloc_ramblock_dirty_info(int *block_index,
>      return block_dinfo;
>  }
>  
> +static int skip_sample_ramblock(RAMBlock *block)
> +{
> +    int64_t ramblock_size;
> +
> +    /* ramblock size in MB */
> +    ramblock_size = qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SHIFT_MB;
> +
> +    /*
> +     * Consider ramblock with size larger than 128M is what we
> +     * want to sample.
> +     */
> +    if (ramblock_size < MIN_RAMBLOCK_SIZE) {
> +        return -1;
> +    }

This seems way too complicated for:

  if (qemu_ram_get_used_length(block) < (MIN_RAMBLOCK_SIZE << 10)) {
      return -1;
  }

  return 0;

(even easier if it just returned bool from the comparison)

Dave

> +    return 0;
> +}
> +
>  static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
>                                       struct DirtyRateConfig config,
>                                       int *block_index)
> @@ -183,6 +201,9 @@ static int record_ramblock_hash_info(struct RamblockDirtyInfo **block_dinfo,
>      int index = 0;
>  
>      RAMBLOCK_FOREACH_MIGRATABLE(block) {
> +        if (skip_sample_ramblock(block) < 0) {
> +            continue;
> +        }
>          dinfo = alloc_ramblock_dirty_info(&index, dinfo);
>          if (dinfo == NULL) {
>              return -1;
> @@ -249,6 +270,9 @@ static int compare_page_hash_info(struct RamblockDirtyInfo *info,
>      RAMBlock *block = NULL;
>  
>      RAMBLOCK_FOREACH_MIGRATABLE(block) {
> +        if (skip_sample_ramblock(block) < 0) {
> +            continue;
> +        }
>          block_dinfo = NULL;
>          if (!find_page_matched(block, info, block_index + 1, &block_dinfo)) {
>              continue;
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index e3adead..600bceb 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -35,10 +35,20 @@
>  #define DIRTYRATE_PAGE_SHIFT_KB                   12
>  
>  /*
> + * Sample page size MB shift
> + */
> +#define DIRTYRATE_PAGE_SHIFT_MB                   20
> +
> +/*
>   * Sample page size 1G shift
>   */
>  #define DIRTYRATE_PAGE_SHIFT_GB                   30
>  
> +/*
> + * minimum ramblock size to sampled
> + */
> +#define MIN_RAMBLOCK_SIZE                         128
> +
>  /* Take 1s as default for calculation duration */
>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>  
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Chuan Zheng]

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

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

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index bd398b7..d1c0a78 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -28,6 +28,30 @@
 static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
 static struct DirtyRateStat DirtyStat;
 
+static int64_t set_sample_page_period(int64_t msec, int64_t initial_time)
+{
+    int64_t current_time;
+
+    current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+    if ((current_time - initial_time) >= msec) {
+        msec = current_time - initial_time;
+    } else {
+        g_usleep((msec + initial_time - current_time) * 1000);
+    }
+
+    return msec;
+}
+
+static int64_t get_sample_page_period(int64_t sec)
+{
+    if (sec <= MIN_FETCH_DIRTYRATE_TIME_SEC ||
+        sec > MAX_FETCH_DIRTYRATE_TIME_SEC) {
+        sec = DEFAULT_FETCH_DIRTYRATE_TIME_SEC;
+    }
+
+    return sec;
+}
+
 static int dirtyrate_set_state(int *state, int old_state, int new_state)
 {
     assert(new_state < DIRTY_RATE_STATUS__MAX);
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 600bceb..cf14647 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -51,6 +51,8 @@
 
 /* Take 1s as default for calculation duration */
 #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
+#define MIN_FETCH_DIRTYRATE_TIME_SEC              0
+#define MAX_FETCH_DIRTYRATE_TIME_SEC              60
 
 struct DirtyRateConfig {
     uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
-- 
1.8.3.1

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

[Chuan Zheng]

Implement calculate_dirtyrate() function.

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

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

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

[Chuan Zheng]

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

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

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 9f52f5f..08c46d3 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -62,6 +62,28 @@ static int dirtyrate_set_state(int *state, int old_state, int new_state)
     }
 }
 
+static struct DirtyRateInfo *query_dirty_rate_info(void)
+{
+    int64_t dirty_rate = DirtyStat.dirty_rate;
+    struct DirtyRateInfo *info = g_malloc0(sizeof(DirtyRateInfo));
+
+    if (CalculatingState == DIRTY_RATE_STATUS_MEASURED) {
+        info->dirty_rate = dirty_rate;
+    } else {
+        info->dirty_rate = -1;
+    }
+
+    info->status = CalculatingState;
+    /*
+     * Only support query once for each calculation,
+     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
+     */
+    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
+                              DIRTY_RATE_STATUS_UNSTARTED);
+
+    return info;
+}
+
 static void reset_dirtyrate_stat(void)
 {
     DirtyStat.total_dirty_samples = 0;
@@ -378,3 +400,26 @@ void *get_dirtyrate_thread(void *arg)
                               DIRTY_RATE_STATUS_MEASURED);
     return NULL;
 }
+
+void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
+{
+    static struct DirtyRateConfig config;
+    QemuThread thread;
+
+    /*
+     * We don't begin calculating thread only when it's in calculating status.
+     */
+    if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) {
+        return;
+    }
+
+    config.sample_period_seconds = get_sample_page_period(calc_time);
+    config.sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
+    qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
+                       (void *)&config, QEMU_THREAD_DETACHED);
+}
+
+struct DirtyRateInfo *qmp_query_dirty_rate(Error **errp)
+{
+    return query_dirty_rate_info();
+}
diff --git a/qapi/migration.json b/qapi/migration.json
index d640165..826bfd7 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1737,3 +1737,47 @@
 ##
 { 'enum': 'DirtyRateStatus',
   'data': [ 'unstarted', 'measuring', 'measured'] }
+
+##
+# @DirtyRateInfo:
+#
+# Information about current dirty page rate of vm.
+#
+# @dirty-rate: @dirtyrate describing the dirty page rate of vm
+#          in units of MB/s.
+#          If this field return '-1', it means querying is not
+#          start or not complete.
+#
+# @status: status containing dirtyrate query status includes
+#          'unstarted' or 'measuring' or 'measured'
+#
+# Since: 5.2
+#
+##
+{ 'struct': 'DirtyRateInfo',
+  'data': {'dirty-rate': 'int64',
+           'status': 'DirtyRateStatus'} }
+
+##
+# @calc-dirty-rate:
+#
+# start calculating dirty page rate for vm
+#
+# @calc-time: time in units of second for sample dirty pages
+#
+# Since: 5.2
+#
+# Example:
+#   {"command": "cal-dirty-rate", "data": {"calc-time": 1} }
+#
+##
+{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64'} }
+
+##
+# @query-dirty-rate:
+#
+# query dirty page rate in units of MB/s for vm
+#
+# Since: 5.2
+##
+{ 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
-- 
1.8.3.1

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

[Chuan Zheng]

Add trace_calls to  make it easier to debug

Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
---
 migration/dirtyrate.c  | 7 +++++++
 migration/trace-events | 8 ++++++++
 2 files changed, 15 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 08c46d3..3513ef3 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -23,6 +23,7 @@
 #include "qapi/qapi-commands-migration.h"
 #include "migration.h"
 #include "ram.h"
+#include "trace.h"
 #include "dirtyrate.h"
 
 static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
@@ -55,6 +56,7 @@ static int64_t get_sample_page_period(int64_t sec)
 static int dirtyrate_set_state(int *state, int old_state, int new_state)
 {
     assert(new_state < DIRTY_RATE_STATUS__MAX);
+    trace_dirtyrate_set_state(DirtyRateStatus_str(new_state));
     if (atomic_cmpxchg(state, old_state, new_state) == old_state) {
         return 0;
     } else {
@@ -78,6 +80,7 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
      * Only support query once for each calculation,
      * reset as DIRTY_RATE_STATUS_UNSTARTED after query
      */
+    trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
     (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
                               DIRTY_RATE_STATUS_UNSTARTED);
 
@@ -129,6 +132,7 @@ static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
 
     crc = crc32(0, iov_array.iov_base, iov_array.iov_len);
 
+    trace_get_ramblock_vfn_hash(info->idstr, vfn, crc);
     return crc;
 }
 
@@ -246,6 +250,7 @@ static int skip_sample_ramblock(RAMBlock *block)
      * want to sample.
      */
     if (ramblock_size < MIN_RAMBLOCK_SIZE) {
+        trace_skip_sample_ramblock(block->idstr, ramblock_size);
         return -1;
     }
 
@@ -292,6 +297,7 @@ static int calc_page_dirty_rate(struct RamblockDirtyInfo *info)
     for (i = 0; i < info->sample_pages_count; i++) {
         crc = get_ramblock_vfn_hash(info, info->sample_page_vfn[i]);
         if (crc != info->hash_result[i]) {
+            trace_calc_page_dirty_rate(info->idstr, crc, info->hash_result[i]);
             info->sample_dirty_count++;
         }
     }
@@ -317,6 +323,7 @@ static bool find_page_matched(RAMBlock *block, struct RamblockDirtyInfo *infos,
     if (infos[i].ramblock_addr != qemu_ram_get_host_addr(block) ||
         infos[i].ramblock_pages !=
             (qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SHIFT_KB)) {
+        trace_find_page_matched(block->idstr);
         return false;
     }
 
diff --git a/migration/trace-events b/migration/trace-events
index 4ab0a50..34569b9 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -312,3 +312,11 @@ dirty_bitmap_load_bits_zeroes(void) ""
 dirty_bitmap_load_header(uint32_t flags) "flags 0x%x"
 dirty_bitmap_load_enter(void) ""
 dirty_bitmap_load_success(void) ""
+
+# dirtyrate.c
+dirtyrate_set_state(const char *new_state) "new state %s"
+query_dirty_rate_info(const char *new_state) "current state %s"
+get_ramblock_vfn_hash(const char *idstr, uint64_t vfn, uint32_t crc) "ramblock name: %s, vfn: %"PRIu64 ", crc: %" PRIu32
+calc_page_dirty_rate(const char *idstr, uint32_t new_crc, uint32_t old_crc) "ramblock name: %s, new crc: %" PRIu32 ", old crc: %" PRIu32
+skip_sample_ramblock(const char *idstr, int64_t ramblock_size) "ramblock name: %s, ramblock size: %" PRIu64
+find_page_matched(const char *idstr) "ramblock %s addr or size changed"
-- 
1.8.3.1

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

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> Add trace_calls to  make it easier to debug
> 
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>

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

> ---
>  migration/dirtyrate.c  | 7 +++++++
>  migration/trace-events | 8 ++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 08c46d3..3513ef3 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -23,6 +23,7 @@
>  #include "qapi/qapi-commands-migration.h"
>  #include "migration.h"
>  #include "ram.h"
> +#include "trace.h"
>  #include "dirtyrate.h"
>  
>  static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
> @@ -55,6 +56,7 @@ static int64_t get_sample_page_period(int64_t sec)
>  static int dirtyrate_set_state(int *state, int old_state, int new_state)
>  {
>      assert(new_state < DIRTY_RATE_STATUS__MAX);
> +    trace_dirtyrate_set_state(DirtyRateStatus_str(new_state));
>      if (atomic_cmpxchg(state, old_state, new_state) == old_state) {
>          return 0;
>      } else {
> @@ -78,6 +80,7 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
>       * Only support query once for each calculation,
>       * reset as DIRTY_RATE_STATUS_UNSTARTED after query
>       */
> +    trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
>      (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>                                DIRTY_RATE_STATUS_UNSTARTED);
>  
> @@ -129,6 +132,7 @@ static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
>  
>      crc = crc32(0, iov_array.iov_base, iov_array.iov_len);
>  
> +    trace_get_ramblock_vfn_hash(info->idstr, vfn, crc);
>      return crc;
>  }
>  
> @@ -246,6 +250,7 @@ static int skip_sample_ramblock(RAMBlock *block)
>       * want to sample.
>       */
>      if (ramblock_size < MIN_RAMBLOCK_SIZE) {
> +        trace_skip_sample_ramblock(block->idstr, ramblock_size);
>          return -1;
>      }
>  
> @@ -292,6 +297,7 @@ static int calc_page_dirty_rate(struct RamblockDirtyInfo *info)
>      for (i = 0; i < info->sample_pages_count; i++) {
>          crc = get_ramblock_vfn_hash(info, info->sample_page_vfn[i]);
>          if (crc != info->hash_result[i]) {
> +            trace_calc_page_dirty_rate(info->idstr, crc, info->hash_result[i]);
>              info->sample_dirty_count++;
>          }
>      }
> @@ -317,6 +323,7 @@ static bool find_page_matched(RAMBlock *block, struct RamblockDirtyInfo *infos,
>      if (infos[i].ramblock_addr != qemu_ram_get_host_addr(block) ||
>          infos[i].ramblock_pages !=
>              (qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SHIFT_KB)) {
> +        trace_find_page_matched(block->idstr);
>          return false;
>      }
>  
> diff --git a/migration/trace-events b/migration/trace-events
> index 4ab0a50..34569b9 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -312,3 +312,11 @@ dirty_bitmap_load_bits_zeroes(void) ""
>  dirty_bitmap_load_header(uint32_t flags) "flags 0x%x"
>  dirty_bitmap_load_enter(void) ""
>  dirty_bitmap_load_success(void) ""
> +
> +# dirtyrate.c
> +dirtyrate_set_state(const char *new_state) "new state %s"
> +query_dirty_rate_info(const char *new_state) "current state %s"
> +get_ramblock_vfn_hash(const char *idstr, uint64_t vfn, uint32_t crc) "ramblock name: %s, vfn: %"PRIu64 ", crc: %" PRIu32
> +calc_page_dirty_rate(const char *idstr, uint32_t new_crc, uint32_t old_crc) "ramblock name: %s, new crc: %" PRIu32 ", old crc: %" PRIu32
> +skip_sample_ramblock(const char *idstr, int64_t ramblock_size) "ramblock name: %s, ramblock size: %" PRIu64
> +find_page_matched(const char *idstr) "ramblock %s addr or size changed"
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Chuan Zheng]

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

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

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

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

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

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

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

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

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

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

 migration/dirtyrate.c  | 432 +++++++++++++++++++++++++++++++++++++++++++++++++
 migration/dirtyrate.h  |  87 ++++++++++
 migration/meson.build  |   1 +
 migration/ram.c        |  11 +-
 migration/ram.h        |  10 ++
 migration/trace-events |   8 +
 qapi/migration.json    |  61 +++++++
 7 files changed, 600 insertions(+), 10 deletions(-)
 create mode 100644 migration/dirtyrate.c
 create mode 100644 migration/dirtyrate.h

-- 
1.8.3.1

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

[Chuan Zheng]

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

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

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 9f52f5f..08c46d3 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -62,6 +62,28 @@ static int dirtyrate_set_state(int *state, int old_state, int new_state)
     }
 }
 
+static struct DirtyRateInfo *query_dirty_rate_info(void)
+{
+    int64_t dirty_rate = DirtyStat.dirty_rate;
+    struct DirtyRateInfo *info = g_malloc0(sizeof(DirtyRateInfo));
+
+    if (CalculatingState == DIRTY_RATE_STATUS_MEASURED) {
+        info->dirty_rate = dirty_rate;
+    } else {
+        info->dirty_rate = -1;
+    }
+
+    info->status = CalculatingState;
+    /*
+     * Only support query once for each calculation,
+     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
+     */
+    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
+                              DIRTY_RATE_STATUS_UNSTARTED);
+
+    return info;
+}
+
 static void reset_dirtyrate_stat(void)
 {
     DirtyStat.total_dirty_samples = 0;
@@ -378,3 +400,26 @@ void *get_dirtyrate_thread(void *arg)
                               DIRTY_RATE_STATUS_MEASURED);
     return NULL;
 }
+
+void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
+{
+    static struct DirtyRateConfig config;
+    QemuThread thread;
+
+    /*
+     * We don't begin calculating thread only when it's in calculating status.
+     */
+    if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) {
+        return;
+    }
+
+    config.sample_period_seconds = get_sample_page_period(calc_time);
+    config.sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
+    qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
+                       (void *)&config, QEMU_THREAD_DETACHED);
+}
+
+struct DirtyRateInfo *qmp_query_dirty_rate(Error **errp)
+{
+    return query_dirty_rate_info();
+}
diff --git a/qapi/migration.json b/qapi/migration.json
index d640165..826bfd7 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1737,3 +1737,47 @@
 ##
 { 'enum': 'DirtyRateStatus',
   'data': [ 'unstarted', 'measuring', 'measured'] }
+
+##
+# @DirtyRateInfo:
+#
+# Information about current dirty page rate of vm.
+#
+# @dirty-rate: @dirtyrate describing the dirty page rate of vm
+#          in units of MB/s.
+#          If this field return '-1', it means querying is not
+#          start or not complete.
+#
+# @status: status containing dirtyrate query status includes
+#          'unstarted' or 'measuring' or 'measured'
+#
+# Since: 5.2
+#
+##
+{ 'struct': 'DirtyRateInfo',
+  'data': {'dirty-rate': 'int64',
+           'status': 'DirtyRateStatus'} }
+
+##
+# @calc-dirty-rate:
+#
+# start calculating dirty page rate for vm
+#
+# @calc-time: time in units of second for sample dirty pages
+#
+# Since: 5.2
+#
+# Example:
+#   {"command": "cal-dirty-rate", "data": {"calc-time": 1} }
+#
+##
+{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64'} }
+
+##
+# @query-dirty-rate:
+#
+# query dirty page rate in units of MB/s for vm
+#
+# Since: 5.2
+##
+{ 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
-- 
1.8.3.1

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

[David Edmondson]

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

> Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called
>
> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
> ---
>  migration/dirtyrate.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  qapi/migration.json   | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 9f52f5f..08c46d3 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -62,6 +62,28 @@ static int dirtyrate_set_state(int *state, int old_state, int new_state)
>      }
>  }
>  
> +static struct DirtyRateInfo *query_dirty_rate_info(void)
> +{
> +    int64_t dirty_rate = DirtyStat.dirty_rate;
> +    struct DirtyRateInfo *info = g_malloc0(sizeof(DirtyRateInfo));
> +
> +    if (CalculatingState == DIRTY_RATE_STATUS_MEASURED) {
> +        info->dirty_rate = dirty_rate;
> +    } else {
> +        info->dirty_rate = -1;
> +    }
> +
> +    info->status = CalculatingState;
> +    /*
> +     * Only support query once for each calculation,
> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
> +     */
> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
> +                              DIRTY_RATE_STATUS_UNSTARTED);

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

> +
> +    return info;
> +}
> +
>  static void reset_dirtyrate_stat(void)
>  {
>      DirtyStat.total_dirty_samples = 0;
> @@ -378,3 +400,26 @@ void *get_dirtyrate_thread(void *arg)
>                                DIRTY_RATE_STATUS_MEASURED);
>      return NULL;
>  }
> +
> +void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
> +{
> +    static struct DirtyRateConfig config;
> +    QemuThread thread;
> +
> +    /*
> +     * We don't begin calculating thread only when it's in calculating status.

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

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

Should this return an error to the caller?

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

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

> +    config.sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
> +    qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
> +                       (void *)&config, QEMU_THREAD_DETACHED);
> +}
> +
> +struct DirtyRateInfo *qmp_query_dirty_rate(Error **errp)
> +{
> +    return query_dirty_rate_info();
> +}
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d640165..826bfd7 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1737,3 +1737,47 @@
>  ##
>  { 'enum': 'DirtyRateStatus',
>    'data': [ 'unstarted', 'measuring', 'measured'] }
> +
> +##
> +# @DirtyRateInfo:
> +#
> +# Information about current dirty page rate of vm.
> +#
> +# @dirty-rate: @dirtyrate describing the dirty page rate of vm
> +#          in units of MB/s.
> +#          If this field return '-1', it means querying is not
> +#          start or not complete.
> +#
> +# @status: status containing dirtyrate query status includes
> +#          'unstarted' or 'measuring' or 'measured'
> +#
> +# Since: 5.2
> +#
> +##
> +{ 'struct': 'DirtyRateInfo',
> +  'data': {'dirty-rate': 'int64',
> +           'status': 'DirtyRateStatus'} }
> +
> +##
> +# @calc-dirty-rate:
> +#
> +# start calculating dirty page rate for vm
> +#
> +# @calc-time: time in units of second for sample dirty pages
> +#
> +# Since: 5.2
> +#
> +# Example:
> +#   {"command": "cal-dirty-rate", "data": {"calc-time": 1} }

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

> +#
> +##
> +{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64'} }
> +
> +##
> +# @query-dirty-rate:
> +#
> +# query dirty page rate in units of MB/s for vm
> +#
> +# Since: 5.2
> +##
> +{ 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
> -- 
> 1.8.3.1

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

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

[Zheng Chuan]

On 2020/8/26 18:26, David Edmondson wrote:
> On Monday, 2020-08-24 at 17:14:39 +08, Chuan Zheng wrote:
> 
>> Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> ---
>>  migration/dirtyrate.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>  qapi/migration.json   | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 89 insertions(+)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index 9f52f5f..08c46d3 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -62,6 +62,28 @@ static int dirtyrate_set_state(int *state, int old_state, int new_state)
>>      }
>>  }
>>  
>> +static struct DirtyRateInfo *query_dirty_rate_info(void)
>> +{
>> +    int64_t dirty_rate = DirtyStat.dirty_rate;
>> +    struct DirtyRateInfo *info = g_malloc0(sizeof(DirtyRateInfo));
>> +
>> +    if (CalculatingState == DIRTY_RATE_STATUS_MEASURED) {
>> +        info->dirty_rate = dirty_rate;
>> +    } else {
>> +        info->dirty_rate = -1;
>> +    }
>> +
>> +    info->status = CalculatingState;
>> +    /*
>> +     * Only support query once for each calculation,
>> +     * reset as DIRTY_RATE_STATUS_UNSTARTED after query
>> +     */
>> +    (void)dirtyrate_set_state(&CalculatingState, CalculatingState,
>> +                              DIRTY_RATE_STATUS_UNSTARTED);
> 
> Is there a reason for this restriction? Removing it would require
> clarifying the state model, I suppose.
> 
We only support query once for each calculation.
Otherwise, it could always query dirtyrate, but maybe the dirtyrate is calculated
long time ago.

>> +
>> +    return info;
>> +}
>> +
>>  static void reset_dirtyrate_stat(void)
>>  {
>>      DirtyStat.total_dirty_samples = 0;
>> @@ -378,3 +400,26 @@ void *get_dirtyrate_thread(void *arg)
>>                                DIRTY_RATE_STATUS_MEASURED);
>>      return NULL;
>>  }
>> +
>> +void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
>> +{
>> +    static struct DirtyRateConfig config;
>> +    QemuThread thread;
>> +
>> +    /*
>> +     * We don't begin calculating thread only when it's in calculating status.
> 
> "If the dirty rate is already being measured, don't attempt to start."
> 
>> +     */
>> +    if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) {
> 
> Should this return an error to the caller?
> 
>> +        return;
>> +    }
>> +
>> +    config.sample_period_seconds = get_sample_page_period(calc_time);
> 
> If the specified sample period is outside the min/max, should an error
> be returned to the caller?
> 
>> +    config.sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
>> +    qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
>> +                       (void *)&config, QEMU_THREAD_DETACHED);
>> +}
>> +
>> +struct DirtyRateInfo *qmp_query_dirty_rate(Error **errp)
>> +{
>> +    return query_dirty_rate_info();
>> +}
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index d640165..826bfd7 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1737,3 +1737,47 @@
>>  ##
>>  { 'enum': 'DirtyRateStatus',
>>    'data': [ 'unstarted', 'measuring', 'measured'] }
>> +
>> +##
>> +# @DirtyRateInfo:
>> +#
>> +# Information about current dirty page rate of vm.
>> +#
>> +# @dirty-rate: @dirtyrate describing the dirty page rate of vm
>> +#          in units of MB/s.
>> +#          If this field return '-1', it means querying is not
>> +#          start or not complete.
>> +#
>> +# @status: status containing dirtyrate query status includes
>> +#          'unstarted' or 'measuring' or 'measured'
>> +#
>> +# Since: 5.2
>> +#
>> +##
>> +{ 'struct': 'DirtyRateInfo',
>> +  'data': {'dirty-rate': 'int64',
>> +           'status': 'DirtyRateStatus'} }
>> +
>> +##
>> +# @calc-dirty-rate:
>> +#
>> +# start calculating dirty page rate for vm
>> +#
>> +# @calc-time: time in units of second for sample dirty pages
>> +#
>> +# Since: 5.2
>> +#
>> +# Example:
>> +#   {"command": "cal-dirty-rate", "data": {"calc-time": 1} }
> 
> "cal-dirty-rate" -> "calc-dirty-rate".
> 
>> +#
>> +##
>> +{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64'} }
>> +
>> +##
>> +# @query-dirty-rate:
>> +#
>> +# query dirty page rate in units of MB/s for vm
>> +#
>> +# Since: 5.2
>> +##
>> +{ 'command': 'query-dirty-rate', 'returns': 'DirtyRateInfo' }
>> -- 
>> 1.8.3.1
> 
> dme.
> 

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

[David Edmondson]

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

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

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

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

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

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

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

[Zheng Chuan]

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

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

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

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

> dme.
> 

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

[David Edmondson]

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

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

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

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

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

Sorry, I didn't understand this comment.

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

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

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

[Zheng Chuan]

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

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

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

    if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) {
        return;
    }


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

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


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

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

[David Edmondson]

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

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

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

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

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

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

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

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