[RFC PATCH 0/8] *** A Method for evaluating dirty page rate ***

[RFC PATCH 0/8] *** A Method for evaluating dirty page rate ***

[Chuan Zheng]

From: Zheng Chuan <zhengchuan@huawei.com>

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.

We evaluate the dirtypage rate on running 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);

++++++++++++++++++++++++++++++++++++++++++
|                      |    dirtyrate    |
++++++++++++++++++++++++++++++++++++++++++
| no mempress          |     4MB/s       |
------------------------------------------
| mempress 4096 1024   |    1204MB/s     |
++++++++++++++++++++++++++++++++++++++++++
| mempress 4096 4096   |    4000Mb/s     |
++++++++++++++++++++++++++++++++++++++++++

Test dirtyrate by qmp command like this:
1.  virsh qemu-monitor-command [vmname] '{"execute":"cal_dirty_rate", "arguments": {"value": [sampletime]}}'
2.  virsh qemu-monitor-command [vmname] '{"execute":"get_dirty_rate"}'

Further test dirtyrate by libvirt api like this:
virsh getdirtyrate [vmname] [sampletime]

Zheng Chuan (8):
  migration/dirtyrate: Add get_dirtyrate_thread() function
  migration/dirtyrate: Add block_dirty_info to store dirtypage info
  migration/dirtyrate: Add dirtyrate statistics series functions
  migration/dirtyrate: Record hash results for each ramblock
  migration/dirtyrate: Compare hash results for recorded ramblock
  migration/dirtyrate: Implement get_sample_gap_period() and
    block_sample_gap_period()
  migration/dirtyrate: Implement calculate_dirtyrate() function
  migration/dirtyrate: Implement
    qmp_cal_dirty_rate()/qmp_get_dirty_rate() function

 migration/Makefile.objs |   1 +
 migration/dirtyrate.c   | 424 ++++++++++++++++++++++++++++++++++++++++++++++++
 migration/dirtyrate.h   |  67 ++++++++
 qapi/migration.json     |  24 +++
 qapi/pragma.json        |   3 +-
 5 files changed, 518 insertions(+), 1 deletion(-)
 create mode 100644 migration/dirtyrate.c
 create mode 100644 migration/dirtyrate.h

-- 
1.8.3.1

[RFC PATCH 1/8] migration/dirtyrate: Add get_dirtyrate_thread() function

[Chuan Zheng]

From: Zheng Chuan <zhengchuan@huawei.com>

Add get_dirtyrate_thread() functions

Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
---
 migration/dirtyrate.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
 migration/dirtyrate.h | 38 +++++++++++++++++++++++++++++++
 2 files changed, 101 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..fc652fb
--- /dev/null
+++ b/migration/dirtyrate.c
@@ -0,0 +1,63 @@
+/*
+ * 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 "dirtyrate.h"
+
+static uint64_t sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
+static uint64_t dirty_rate; /* MB/s */
+CalculatingDirtyRateStage calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
+
+static bool calculate_dirtyrate(struct dirtyrate_config config,
+                        uint64_t *dirty_rate, int64_t time)
+{
+    /* todo */
+    return true;
+}
+
+static void set_dirty_rate(uint64_t drate)
+{
+    dirty_rate = drate;
+}
+
+/*
+ * There are multithread will write/read *calculating_dirty_rate_stage*,
+ * we can protect only one thread write/read it by libvirt api.
+ * So we don't add mutex_lock to protect it here, but we must calculate
+ * dirty_rate by libvirt api.
+ */
+static void set_dirty_rate_stage(CalculatingDirtyRateStage ratestage)
+{
+    calculating_dirty_rate_stage = ratestage;
+}
+
+void *get_dirtyrate_thread(void *arg)
+{
+    struct dirtyrate_config config = *(struct dirtyrate_config *)arg;
+    uint64_t dirty_rate;
+    uint64_t hash_dirty_rate;
+    bool query_succ;
+    int64_t msec = 0;
+ 
+    set_dirty_rate_stage(CAL_DIRTY_RATE_ING);
+
+    query_succ = calculate_dirtyrate(config, &hash_dirty_rate, msec);
+    if (!query_succ) {
+        dirty_rate = 0;
+    } else {
+        dirty_rate = hash_dirty_rate;
+    }
+
+    set_dirty_rate(dirty_rate);
+    set_dirty_rate_stage(CAL_DIRTY_RATE_END);
+
+    return NULL;
+}
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
new file mode 100644
index 0000000..9a5c228
--- /dev/null
+++ b/migration/dirtyrate.h
@@ -0,0 +1,38 @@
+/*
+ *  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
+
+/* take 256 pages per GB for cal dirty rate */
+#define DIRTYRATE_DEFAULT_SAMPLE_PAGES    256
+
+struct dirtyrate_config {
+    uint64_t sample_pages_per_gigabytes;
+    int64_t sample_period_seconds;
+};
+
+/*
+ *  To record calculate dirty_rate status:
+ *  0: initial status, calculating thread is not be created here.
+ *  1: calculating thread is created.
+ *  2: calculating thread is end, we can get result.
+ */
+typedef enum {
+    CAL_DIRTY_RATE_INIT  = 0,
+    CAL_DIRTY_RATE_ING   = 1,
+    CAL_DIRTY_RATE_END   = 2,
+} CalculatingDirtyRateStage;
+
+void *get_dirtyrate_thread(void *arg);
+#endif
+
-- 
1.8.3.1

Re: [RFC PATCH 1/8] migration/dirtyrate: Add get_dirtyrate_thread() function

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> From: Zheng Chuan <zhengchuan@huawei.com>
> 
> Add get_dirtyrate_thread() functions
> 
> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
> ---
>  migration/dirtyrate.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  migration/dirtyrate.h | 38 +++++++++++++++++++++++++++++++
>  2 files changed, 101 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..fc652fb
> --- /dev/null
> +++ b/migration/dirtyrate.c
> @@ -0,0 +1,63 @@
> +/*
> + * 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 "dirtyrate.h"
> +
> +static uint64_t sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
> +static uint64_t dirty_rate; /* MB/s */
> +CalculatingDirtyRateStage calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
> +
> +static bool calculate_dirtyrate(struct dirtyrate_config config,
> +                        uint64_t *dirty_rate, int64_t time)
> +{
> +    /* todo */
> +    return true;

It would be better to make this return false until you fill it in!

> +}
> +
> +static void set_dirty_rate(uint64_t drate)
> +{
> +    dirty_rate = drate;
> +}
> +
> +/*
> + * There are multithread will write/read *calculating_dirty_rate_stage*,
> + * we can protect only one thread write/read it by libvirt api.
> + * So we don't add mutex_lock to protect it here, but we must calculate
> + * dirty_rate by libvirt api.
> + */
> +static void set_dirty_rate_stage(CalculatingDirtyRateStage ratestage)
> +{
> +    calculating_dirty_rate_stage = ratestage;
> +}

I don't think I understand the threading comment here; when you say the
'libvirt api' do youmean QMP?  Maybe you could do this with an
atomic_cmpxchg like we do in migrate_set_state?

> +
> +void *get_dirtyrate_thread(void *arg)
> +{
> +    struct dirtyrate_config config = *(struct dirtyrate_config *)arg;
> +    uint64_t dirty_rate;
> +    uint64_t hash_dirty_rate;
> +    bool query_succ;
> +    int64_t msec = 0;
> + 
> +    set_dirty_rate_stage(CAL_DIRTY_RATE_ING);
> +
> +    query_succ = calculate_dirtyrate(config, &hash_dirty_rate, msec);
> +    if (!query_succ) {
> +        dirty_rate = 0;
> +    } else {
> +        dirty_rate = hash_dirty_rate;
> +    }
> +
> +    set_dirty_rate(dirty_rate);
> +    set_dirty_rate_stage(CAL_DIRTY_RATE_END);
> +
> +    return NULL;
> +}
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> new file mode 100644
> index 0000000..9a5c228
> --- /dev/null
> +++ b/migration/dirtyrate.h
> @@ -0,0 +1,38 @@
> +/*
> + *  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
> +
> +/* take 256 pages per GB for cal dirty rate */
> +#define DIRTYRATE_DEFAULT_SAMPLE_PAGES    256
> +
> +struct dirtyrate_config {
> +    uint64_t sample_pages_per_gigabytes;
> +    int64_t sample_period_seconds;
> +};
> +
> +/*
> + *  To record calculate dirty_rate status:
> + *  0: initial status, calculating thread is not be created here.
> + *  1: calculating thread is created.
> + *  2: calculating thread is end, we can get result.
> + */
> +typedef enum {
> +    CAL_DIRTY_RATE_INIT  = 0,
> +    CAL_DIRTY_RATE_ING   = 1,

I'm not sure why ING?


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

Re: [RFC PATCH 1/8] migration/dirtyrate: Add get_dirtyrate_thread() function

[Zheng Chuan]

On 2020/8/5 0:23, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>> From: Zheng Chuan <zhengchuan@huawei.com>
>>
>> Add get_dirtyrate_thread() functions
>>
>> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
>> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
>> ---
>>  migration/dirtyrate.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  migration/dirtyrate.h | 38 +++++++++++++++++++++++++++++++
>>  2 files changed, 101 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..fc652fb
>> --- /dev/null
>> +++ b/migration/dirtyrate.c
>> @@ -0,0 +1,63 @@
>> +/*
>> + * 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 "dirtyrate.h"
>> +
>> +static uint64_t sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
>> +static uint64_t dirty_rate; /* MB/s */
>> +CalculatingDirtyRateStage calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
>> +
>> +static bool calculate_dirtyrate(struct dirtyrate_config config,
>> +                        uint64_t *dirty_rate, int64_t time)
>> +{
>> +    /* todo */
>> +    return true;
> 
> It would be better to make this return false until you fill it in!
> 
Sure, I'll fix that in V2.
>> +}
>> +
>> +static void set_dirty_rate(uint64_t drate)
>> +{
>> +    dirty_rate = drate;
>> +}
>> +
>> +/*
>> + * There are multithread will write/read *calculating_dirty_rate_stage*,
>> + * we can protect only one thread write/read it by libvirt api.
>> + * So we don't add mutex_lock to protect it here, but we must calculate
>> + * dirty_rate by libvirt api.
>> + */
>> +static void set_dirty_rate_stage(CalculatingDirtyRateStage ratestage)
>> +{
>> +    calculating_dirty_rate_stage = ratestage;
>> +}
> 
> I don't think I understand the threading comment here; when you say the
> 'libvirt api' do youmean QMP?  Maybe you could do this with an
> atomic_cmpxchg like we do in migrate_set_state?
> 
Yes, I mean QMP, atomic_cmpxchg should be better:)
I'll fix that and make comments more clear in V2.

>> +
>> +void *get_dirtyrate_thread(void *arg)
>> +{
>> +    struct dirtyrate_config config = *(struct dirtyrate_config *)arg;
>> +    uint64_t dirty_rate;
>> +    uint64_t hash_dirty_rate;
>> +    bool query_succ;
>> +    int64_t msec = 0;
>> + 
>> +    set_dirty_rate_stage(CAL_DIRTY_RATE_ING);
>> +
>> +    query_succ = calculate_dirtyrate(config, &hash_dirty_rate, msec);
>> +    if (!query_succ) {
>> +        dirty_rate = 0;
>> +    } else {
>> +        dirty_rate = hash_dirty_rate;
>> +    }
>> +
>> +    set_dirty_rate(dirty_rate);
>> +    set_dirty_rate_stage(CAL_DIRTY_RATE_END);
>> +
>> +    return NULL;
>> +}
>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>> new file mode 100644
>> index 0000000..9a5c228
>> --- /dev/null
>> +++ b/migration/dirtyrate.h
>> @@ -0,0 +1,38 @@
>> +/*
>> + *  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
>> +
>> +/* take 256 pages per GB for cal dirty rate */
>> +#define DIRTYRATE_DEFAULT_SAMPLE_PAGES    256
>> +
>> +struct dirtyrate_config {
>> +    uint64_t sample_pages_per_gigabytes;
>> +    int64_t sample_period_seconds;
>> +};
>> +
>> +/*
>> + *  To record calculate dirty_rate status:
>> + *  0: initial status, calculating thread is not be created here.
>> + *  1: calculating thread is created.
>> + *  2: calculating thread is end, we can get result.
>> + */
>> +typedef enum {
>> +    CAL_DIRTY_RATE_INIT  = 0,
>> +    CAL_DIRTY_RATE_ING   = 1,
> 
> I'm not sure why ING?
Maybe ACTIVE is better, i'll fix that.
> 
> 
>> +    CAL_DIRTY_RATE_END   = 2,
>> +} CalculatingDirtyRateStage;
>> +
>> +void *get_dirtyrate_thread(void *arg);
>> +#endif
>> +
>> -- 
>> 1.8.3.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 
> .
> 

[RFC PATCH 2/8] migration/dirtyrate: Add block_dirty_info to store dirtypage info

[Chuan Zheng]

From: Zheng Chuan <zhengchuan@huawei.com>

Add block_dirty_info to store dirtypage info for each ramblock

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

diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 9a5c228..342b89f 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -33,6 +33,19 @@ typedef enum {
     CAL_DIRTY_RATE_END   = 2,
 } CalculatingDirtyRateStage;
 
+/* 
+ * Store dirtypage info for each block.
+ */
+struct block_dirty_info {
+    char idstr[BLOCK_INFO_MAX_LEN];
+    uint8_t *block_addr;
+    unsigned long block_pages;
+    unsigned long *sample_page_vfn;
+    unsigned int sample_pages_count;
+    unsigned int sample_dirty_count;
+    uint8_t *hash_result;
+};
+
 void *get_dirtyrate_thread(void *arg);
 #endif
 
-- 
1.8.3.1

Re: [RFC PATCH 2/8] migration/dirtyrate: Add block_dirty_info to store dirtypage info

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> From: Zheng Chuan <zhengchuan@huawei.com>
> 
> Add block_dirty_info to store dirtypage info for each ramblock
> 
> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
> ---
>  migration/dirtyrate.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 9a5c228..342b89f 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -33,6 +33,19 @@ typedef enum {
>      CAL_DIRTY_RATE_END   = 2,
>  } CalculatingDirtyRateStage;
>  
> +/* 
> + * Store dirtypage info for each block.
> + */
> +struct block_dirty_info {

Please call this ramblock_dirty_info; we use 'block' a lot to mean
disk block and it gets confusing.

> +    char idstr[BLOCK_INFO_MAX_LEN];

Is there a reason you don't just use a RAMBlock *  here?

> +    uint8_t *block_addr;
> +    unsigned long block_pages;
> +    unsigned long *sample_page_vfn;

Please comment these; if I understand correctly, that's an array
of page indexes into the block generated from the random numbers

> +    unsigned int sample_pages_count;
> +    unsigned int sample_dirty_count;
> +    uint8_t *hash_result;

If I understand, this is an array of hashes end-to-end for
all the pages in this RAMBlock?

Dave

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

Re: [RFC PATCH 2/8] migration/dirtyrate: Add block_dirty_info to store dirtypage info

[Zheng Chuan]

On 2020/8/5 0:28, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>> From: Zheng Chuan <zhengchuan@huawei.com>
>>
>> Add block_dirty_info to store dirtypage info for each ramblock
>>
>> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
>> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
>> ---
>>  migration/dirtyrate.h | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>> index 9a5c228..342b89f 100644
>> --- a/migration/dirtyrate.h
>> +++ b/migration/dirtyrate.h
>> @@ -33,6 +33,19 @@ typedef enum {
>>      CAL_DIRTY_RATE_END   = 2,
>>  } CalculatingDirtyRateStage;
>>  
>> +/* 
>> + * Store dirtypage info for each block.
>> + */
>> +struct block_dirty_info {
> 
> Please call this ramblock_dirty_info; we use 'block' a lot to mean
> disk block and it gets confusing.
> 
Sure, ramblock_dirty_info is better.

>> +    char idstr[BLOCK_INFO_MAX_LEN];
> 
> Is there a reason you don't just use a RAMBlock *  here?
> 
>> +    uint8_t *block_addr;
>> +    unsigned long block_pages;
>> +    unsigned long *sample_page_vfn;
> 
> Please comment these; if I understand correctly, that's an array
> of page indexes into the block generated from the random numbers
> 
>> +    unsigned int sample_pages_count;
>> +    unsigned int sample_dirty_count;
>> +    uint8_t *hash_result;
> 
> If I understand, this is an array of hashes end-to-end for
> all the pages in this RAMBlock?
> 
> Dave
> 
Actually, we do not go through all pages of the RAMBlock but sample
some pages （for example, 256 pages per Gigabit）to make it faster.
Obviously it will sacrifice accuracy, but it still looks good enough
under practical test.

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

Re: [RFC PATCH 2/8] migration/dirtyrate: Add block_dirty_info to store dirtypage info

[Dr. David Alan Gilbert]

* Zheng Chuan (zhengchuan@huawei.com) wrote:
> 
> 
> On 2020/8/5 0:28, Dr. David Alan Gilbert wrote:
> > * Chuan Zheng (zhengchuan@huawei.com) wrote:
> >> From: Zheng Chuan <zhengchuan@huawei.com>
> >>
> >> Add block_dirty_info to store dirtypage info for each ramblock
> >>
> >> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
> >> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
> >> ---
> >>  migration/dirtyrate.h | 13 +++++++++++++
> >>  1 file changed, 13 insertions(+)
> >>
> >> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> >> index 9a5c228..342b89f 100644
> >> --- a/migration/dirtyrate.h
> >> +++ b/migration/dirtyrate.h
> >> @@ -33,6 +33,19 @@ typedef enum {
> >>      CAL_DIRTY_RATE_END   = 2,
> >>  } CalculatingDirtyRateStage;
> >>  
> >> +/* 
> >> + * Store dirtypage info for each block.
> >> + */
> >> +struct block_dirty_info {
> > 
> > Please call this ramblock_dirty_info; we use 'block' a lot to mean
> > disk block and it gets confusing.
> > 
> Sure, ramblock_dirty_info is better.
> 
> >> +    char idstr[BLOCK_INFO_MAX_LEN];
> > 
> > Is there a reason you don't just use a RAMBlock *  here?
> > 
> >> +    uint8_t *block_addr;
> >> +    unsigned long block_pages;
> >> +    unsigned long *sample_page_vfn;
> > 
> > Please comment these; if I understand correctly, that's an array
> > of page indexes into the block generated from the random numbers
> > 
> >> +    unsigned int sample_pages_count;
> >> +    unsigned int sample_dirty_count;
> >> +    uint8_t *hash_result;
> > 
> > If I understand, this is an array of hashes end-to-end for
> > all the pages in this RAMBlock?
> > 
> > Dave
> > 
> Actually, we do not go through all pages of the RAMBlock but sample
> some pages （for example, 256 pages per Gigabit）to make it faster.
> Obviously it will sacrifice accuracy, but it still looks good enough
> under practical test.

Right yes; but that 'hash_result' is an array of hash values, one
for each of the pages that you did measure?

Dave

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

Re: [RFC PATCH 2/8] migration/dirtyrate: Add block_dirty_info to store dirtypage info

[Zheng Chuan]

On 2020/8/7 0:59, Dr. David Alan Gilbert wrote:
> * Zheng Chuan (zhengchuan@huawei.com) wrote:
>>
>>
>> On 2020/8/5 0:28, Dr. David Alan Gilbert wrote:
>>> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>>>> From: Zheng Chuan <zhengchuan@huawei.com>
>>>>
>>>> Add block_dirty_info to store dirtypage info for each ramblock
>>>>
>>>> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
>>>> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
>>>> ---
>>>>  migration/dirtyrate.h | 13 +++++++++++++
>>>>  1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>>>> index 9a5c228..342b89f 100644
>>>> --- a/migration/dirtyrate.h
>>>> +++ b/migration/dirtyrate.h
>>>> @@ -33,6 +33,19 @@ typedef enum {
>>>>      CAL_DIRTY_RATE_END   = 2,
>>>>  } CalculatingDirtyRateStage;
>>>>  
>>>> +/* 
>>>> + * Store dirtypage info for each block.
>>>> + */
>>>> +struct block_dirty_info {
>>>
>>> Please call this ramblock_dirty_info; we use 'block' a lot to mean
>>> disk block and it gets confusing.
>>>
>> Sure, ramblock_dirty_info is better.
>>
>>>> +    char idstr[BLOCK_INFO_MAX_LEN];
>>>
>>> Is there a reason you don't just use a RAMBlock *  here?
>>>
>>>> +    uint8_t *block_addr;
>>>> +    unsigned long block_pages;
>>>> +    unsigned long *sample_page_vfn;
>>>
>>> Please comment these; if I understand correctly, that's an array
>>> of page indexes into the block generated from the random numbers
>>>
>>>> +    unsigned int sample_pages_count;
>>>> +    unsigned int sample_dirty_count;
>>>> +    uint8_t *hash_result;
>>>
>>> If I understand, this is an array of hashes end-to-end for
>>> all the pages in this RAMBlock?
>>>
>>> Dave
>>>
>> Actually, we do not go through all pages of the RAMBlock but sample
>> some pages （for example, 256 pages per Gigabit）to make it faster.
>> Obviously it will sacrifice accuracy, but it still looks good enough
>> under practical test.
> 
> Right yes; but that 'hash_result' is an array of hash values, one
> for each of the pages that you did measure?
> 
> Dave
> 
Yes, hash_result stores the results from qcrypto_hash_bytesv() for each of the 4K pages i sampled.

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

[RFC PATCH 3/8] migration/dirtyrate: Add dirtyrate statistics series functions

[Chuan Zheng]

From: Zheng Chuan <zhengchuan@huawei.com>

Add dirtyrate statistics to record/update dirtyrate info.

Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
---
 migration/dirtyrate.c | 47 ++++++++++++++++++++++++++++++-----------------
 migration/dirtyrate.h | 11 +++++++++++
 2 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index fc652fb..6baf674 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -13,19 +13,41 @@
 #include "dirtyrate.h"
 
 static uint64_t sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
-static uint64_t dirty_rate; /* MB/s */
+static struct dirtyrate_statistics dirty_stat;
 CalculatingDirtyRateStage calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
 
-static bool calculate_dirtyrate(struct dirtyrate_config config,
-                        uint64_t *dirty_rate, int64_t time)
+static void reset_dirtyrate_stat(void)
 {
-    /* todo */
-    return true;
+    dirty_stat.total_dirty_samples = 0;
+    dirty_stat.total_sample_count = 0;
+    dirty_stat.total_block_mem_MB = 0;
+    dirty_stat.dirty_rate = 0;
+}
+
+static void update_dirtyrate_stat(struct block_dirty_info *info)
+{
+    dirty_stat.total_dirty_samples += info->sample_dirty_count;
+    dirty_stat.total_sample_count += info->sample_pages_count;
+    dirty_stat.total_block_mem_MB += (info->block_pages << DIRTYRATE_PAGE_SIZE_SHIFT) >> PAGE_SIZE_SHIFT;
 }
 
-static void set_dirty_rate(uint64_t drate)
+static void update_dirtyrate(int64_t msec)
 {
-    dirty_rate = drate;
+    uint64_t dirty_rate;
+    unsigned int total_dirty_samples = dirty_stat.total_dirty_samples;
+    unsigned int total_sample_count = dirty_stat.total_sample_count;
+    unsigned long total_block_mem_MB = dirty_stat.total_block_mem_MB;
+
+    dirty_rate = total_dirty_samples * total_block_mem_MB *
+                 1000 / (total_sample_count * msec);
+
+    dirty_stat.dirty_rate = dirty_rate;
+}
+
+
+static void calculate_dirtyrate(struct dirtyrate_config config, int64_t time)
+{
+    /* todo */
 }
 
 /*
@@ -42,21 +64,12 @@ static void set_dirty_rate_stage(CalculatingDirtyRateStage ratestage)
 void *get_dirtyrate_thread(void *arg)
 {
     struct dirtyrate_config config = *(struct dirtyrate_config *)arg;
-    uint64_t dirty_rate;
-    uint64_t hash_dirty_rate;
-    bool query_succ;
     int64_t msec = 0;
  
     set_dirty_rate_stage(CAL_DIRTY_RATE_ING);
 
-    query_succ = calculate_dirtyrate(config, &hash_dirty_rate, msec);
-    if (!query_succ) {
-        dirty_rate = 0;
-    } else {
-        dirty_rate = hash_dirty_rate;
-    }
+    calculate_dirtyrate(config, msec);
 
-    set_dirty_rate(dirty_rate);
     set_dirty_rate_stage(CAL_DIRTY_RATE_END);
 
     return NULL;
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 342b89f..2994535 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -15,6 +15,9 @@
 
 /* take 256 pages per GB for cal dirty rate */
 #define DIRTYRATE_DEFAULT_SAMPLE_PAGES    256
+#define DIRTYRATE_PAGE_SIZE_SHIFT       12
+#define BLOCK_INFO_MAX_LEN              256
+#define PAGE_SIZE_SHIFT                 20
 
 struct dirtyrate_config {
     uint64_t sample_pages_per_gigabytes;
@@ -33,6 +36,14 @@ typedef enum {
     CAL_DIRTY_RATE_END   = 2,
 } CalculatingDirtyRateStage;
 
+struct dirtyrate_statistics {
+    unsigned int total_dirty_samples;
+    unsigned int total_sample_count;
+    unsigned long total_block_mem_MB;
+    int64_t dirty_rate;
+};
+
+
 /* 
  * Store dirtypage info for each block.
  */
-- 
1.8.3.1

Re: [RFC PATCH 3/8] migration/dirtyrate: Add dirtyrate statistics series functions

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> From: Zheng Chuan <zhengchuan@huawei.com>
> 
> Add dirtyrate statistics to record/update dirtyrate info.
> 
> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
> ---
>  migration/dirtyrate.c | 47 ++++++++++++++++++++++++++++++-----------------
>  migration/dirtyrate.h | 11 +++++++++++
>  2 files changed, 41 insertions(+), 17 deletions(-)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index fc652fb..6baf674 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -13,19 +13,41 @@
>  #include "dirtyrate.h"
>  
>  static uint64_t sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
> -static uint64_t dirty_rate; /* MB/s */
> +static struct dirtyrate_statistics dirty_stat;
>  CalculatingDirtyRateStage calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
>  
> -static bool calculate_dirtyrate(struct dirtyrate_config config,
> -                        uint64_t *dirty_rate, int64_t time)
> +static void reset_dirtyrate_stat(void)
>  {
> -    /* todo */
> -    return true;
> +    dirty_stat.total_dirty_samples = 0;
> +    dirty_stat.total_sample_count = 0;
> +    dirty_stat.total_block_mem_MB = 0;
> +    dirty_stat.dirty_rate = 0;
> +}
> +
> +static void update_dirtyrate_stat(struct block_dirty_info *info)
> +{
> +    dirty_stat.total_dirty_samples += info->sample_dirty_count;
> +    dirty_stat.total_sample_count += info->sample_pages_count;
> +    dirty_stat.total_block_mem_MB += (info->block_pages << DIRTYRATE_PAGE_SIZE_SHIFT) >> PAGE_SIZE_SHIFT;
>  }
>  
> -static void set_dirty_rate(uint64_t drate)
> +static void update_dirtyrate(int64_t msec)
>  {
> -    dirty_rate = drate;
> +    uint64_t dirty_rate;
> +    unsigned int total_dirty_samples = dirty_stat.total_dirty_samples;
> +    unsigned int total_sample_count = dirty_stat.total_sample_count;
> +    unsigned long total_block_mem_MB = dirty_stat.total_block_mem_MB;
> +
> +    dirty_rate = total_dirty_samples * total_block_mem_MB *
> +                 1000 / (total_sample_count * msec);
> +
> +    dirty_stat.dirty_rate = dirty_rate;
> +}
> +
> +
> +static void calculate_dirtyrate(struct dirtyrate_config config, int64_t time)
> +{
> +    /* todo */
>  }
>  
>  /*
> @@ -42,21 +64,12 @@ static void set_dirty_rate_stage(CalculatingDirtyRateStage ratestage)
>  void *get_dirtyrate_thread(void *arg)
>  {
>      struct dirtyrate_config config = *(struct dirtyrate_config *)arg;
> -    uint64_t dirty_rate;
> -    uint64_t hash_dirty_rate;
> -    bool query_succ;
>      int64_t msec = 0;
>   
>      set_dirty_rate_stage(CAL_DIRTY_RATE_ING);
>  
> -    query_succ = calculate_dirtyrate(config, &hash_dirty_rate, msec);
> -    if (!query_succ) {
> -        dirty_rate = 0;
> -    } else {
> -        dirty_rate = hash_dirty_rate;
> -    }

All this was only just added; it might be easier to create the
update_dirtyrate function first.

> +    calculate_dirtyrate(config, msec);
>  
> -    set_dirty_rate(dirty_rate);
>      set_dirty_rate_stage(CAL_DIRTY_RATE_END);
>  
>      return NULL;
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 342b89f..2994535 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -15,6 +15,9 @@
>  
>  /* take 256 pages per GB for cal dirty rate */
>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES    256
> +#define DIRTYRATE_PAGE_SIZE_SHIFT       12
> +#define BLOCK_INFO_MAX_LEN              256
> +#define PAGE_SIZE_SHIFT                 20

I think you might also have used one of these #define's in a previous
patch; so make sure the patches each compile in order.
Also, can you please comment each one of these - I was confused by a lot
of the calculations above because I don't quite understand what each of
these is.
I don't thinl 'BLOCK_INFO_MAX_LEN' is needed - becuase it's just a
RAMBlock ID, and you can link to the RAMBlock.
I'm not sure what DIRTYRATE_PAGE_SIZE_SHIFT is, or why it's different
from TARGET_PAGE_BITS.

>  
>  struct dirtyrate_config {
>      uint64_t sample_pages_per_gigabytes;
> @@ -33,6 +36,14 @@ typedef enum {
>      CAL_DIRTY_RATE_END   = 2,
>  } CalculatingDirtyRateStage;
>  
> +struct dirtyrate_statistics {
> +    unsigned int total_dirty_samples;
> +    unsigned int total_sample_count;
> +    unsigned long total_block_mem_MB;

'long' is normally a bad idea - we use it in a few places and it
was generally a bad idea; size_t for a size is much better.

> +    int64_t dirty_rate;

Is this blocks/sec, MB/s what - please comment it.

Dave

> +};
> +
> +
>  /* 
>   * Store dirtypage info for each block.
>   */
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[RFC PATCH 4/8] migration/dirtyrate: Record hash results for each ramblock

[Chuan Zheng]

From: Zheng Chuan <zhengchuan@huawei.com>

Record hash results for each ramblock.

Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
---
 migration/dirtyrate.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++
 migration/dirtyrate.h |   1 +
 2 files changed, 158 insertions(+)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 6baf674..45cfc91 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -10,12 +10,27 @@
  * 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 uint64_t sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
 static struct dirtyrate_statistics dirty_stat;
 CalculatingDirtyRateStage calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
 
+#define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
+        INTERNAL_RAMBLOCK_FOREACH(block)                   \
+        if (!qemu_ram_is_migratable(block)) {} else
+
 static void reset_dirtyrate_stat(void)
 {
     dirty_stat.total_dirty_samples = 0;
@@ -44,6 +59,148 @@ static void update_dirtyrate(int64_t msec)
     dirty_stat.dirty_rate = dirty_rate;
 }
 
+static int get_block_vfn_hash(struct block_dirty_info *info, unsigned long vfn,
+                              uint8_t **md, size_t *hash_len)
+{
+    struct iovec iov_array;
+    int ret = 0;
+    int nkey = 1;
+
+    iov_array.iov_base = info->block_addr +
+                         vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
+    iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;
+
+    if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_MD5,
+                            &iov_array, nkey,
+                            md, hash_len, NULL) < 0) {
+        ret = -1;
+    }
+
+    return ret;
+}
+
+static int save_block_hash(struct block_dirty_info *info)
+{
+    unsigned long *rand_buf = NULL;
+    unsigned int sample_pages_count;
+    uint8_t *md = NULL;
+    size_t hash_len;
+    int i;
+    int ret = -1;
+
+    sample_pages_count = info->sample_pages_count;
+    /* block size less than one page, return success to skip this block */
+    if (unlikely(info->block_pages == 0 || sample_pages_count == 0)) {
+        ret = 0;
+        goto out;
+    }
+
+    /* use random bytes to pick sample page vfn */
+    rand_buf = g_malloc0_n(sample_pages_count, sizeof(unsigned long));
+    /* DEFAULT_READ_RANDOM_MAX_LIMIT 32M,
+     * can support 4T vm 1024 sample_pages_per_gigabytes
+     */
+    ret = qcrypto_random_bytes((unsigned char *)rand_buf,
+                               sample_pages_count * sizeof(unsigned long),
+                               NULL);
+    if (ret) {
+        ret = -1;
+        goto out;
+    }
+
+    hash_len = qcrypto_hash_digest_len(QCRYPTO_HASH_ALG_MD5);
+    info->hash_result = g_malloc0_n(sample_pages_count, sizeof(uint8_t) * hash_len);
+    info->sample_page_vfn = g_malloc0_n(sample_pages_count, sizeof(unsigned long));
+
+    for (i = 0; i < sample_pages_count; i++) {
+        md = info->hash_result + i * hash_len;
+        info->sample_page_vfn[i] = rand_buf[i] % info->block_pages;
+        ret = get_block_vfn_hash(info, info->sample_page_vfn[i], &md, &hash_len);
+        if (ret < 0) {
+            goto out;
+        }
+    }
+    ret = 0;
+out:
+    g_free(rand_buf);
+    return ret;
+}
+
+static void get_block_dirty_info(RAMBlock *block, struct block_dirty_info *info,
+                                 struct dirtyrate_config *config)
+{
+    uint64_t sample_pages_per_gigabytes = config->sample_pages_per_gigabytes;
+
+    /* Right shift 30 bits to calc block size in GB */
+    info->sample_pages_count = (qemu_ram_get_used_length(block) * sample_pages_per_gigabytes) >> 30;
+
+    info->block_pages = qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SIZE_SHIFT;
+    info->block_addr = qemu_ram_get_host_addr(block);
+    strcpy(info->idstr, qemu_ram_get_idstr(block));
+}
+
+static struct block_dirty_info *
+alloc_block_dirty_info(int *block_index,
+                       struct block_dirty_info *block_dinfo)
+{
+    struct block_dirty_info *info = NULL;
+    int index = *block_index;
+
+    if (!block_dinfo) {
+        block_dinfo = g_new(struct block_dirty_info, 1);
+        index = 0;
+    } else {
+        block_dinfo = g_realloc(block_dinfo, (index + 1) *
+                                sizeof(struct block_dirty_info));
+        index++;
+    }
+    info = &block_dinfo[index];
+    memset(info, 0, sizeof(struct block_dirty_info));
+
+    *block_index = index;
+    return block_dinfo;
+}
+
+static int ram_block_skip(RAMBlock *block)
+{
+    if (!strstr(qemu_ram_get_idstr(block), "ram-node") &&
+        !strstr(qemu_ram_get_idstr(block), "memdimm")) {
+        if (strcmp(qemu_ram_get_idstr(block), "mach-virt.ram") ||
+            strcmp(block->idstr, "pc.ram")) {
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
+static int record_block_hash_info(struct dirtyrate_config config,
+                                  struct block_dirty_info **block_dinfo, int *block_index)
+{
+    struct block_dirty_info *info = NULL;
+    struct block_dirty_info *dinfo = NULL;
+    RAMBlock *block = NULL;
+    int index = 0;
+
+    RAMBLOCK_FOREACH_MIGRATABLE(block) {
+        if (ram_block_skip(block) < 0) {
+            continue;
+        }
+        dinfo = alloc_block_dirty_info(&index, dinfo);
+        info = &dinfo[index];
+        get_block_dirty_info(block, info, &config);
+        if (save_block_hash(info) < 0) {
+            *block_dinfo = dinfo;
+            *block_index = index;
+            return -1;
+        }
+    }
+
+    *block_dinfo = dinfo;
+    *block_index = index;
+
+    return 0;
+}
 
 static void calculate_dirtyrate(struct dirtyrate_config config, int64_t time)
 {
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 2994535..4d9b3b8 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -15,6 +15,7 @@
 
 /* take 256 pages per GB for cal dirty rate */
 #define DIRTYRATE_DEFAULT_SAMPLE_PAGES    256
+#define DIRTYRATE_SAMPLE_PAGE_SIZE      4096
 #define DIRTYRATE_PAGE_SIZE_SHIFT       12
 #define BLOCK_INFO_MAX_LEN              256
 #define PAGE_SIZE_SHIFT                 20
-- 
1.8.3.1

Re: [RFC PATCH 4/8] migration/dirtyrate: Record hash results for each ramblock

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> From: Zheng Chuan <zhengchuan@huawei.com>
> 
> Record hash results for each ramblock.

Please be careful when talking about 'ramblock' since we already use
that for a chunk of memory in QEMU.

> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
> ---
>  migration/dirtyrate.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  migration/dirtyrate.h |   1 +
>  2 files changed, 158 insertions(+)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 6baf674..45cfc91 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -10,12 +10,27 @@
>   * 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 uint64_t sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
>  static struct dirtyrate_statistics dirty_stat;
>  CalculatingDirtyRateStage calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
>  
> +#define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
> +        INTERNAL_RAMBLOCK_FOREACH(block)                   \
> +        if (!qemu_ram_is_migratable(block)) {} else
> +

Instead of redefining this here, please move the existing definition up
into migration/ram.h ?

>  static void reset_dirtyrate_stat(void)
>  {
>      dirty_stat.total_dirty_samples = 0;
> @@ -44,6 +59,148 @@ static void update_dirtyrate(int64_t msec)
>      dirty_stat.dirty_rate = dirty_rate;
>  }
>  
> +static int get_block_vfn_hash(struct block_dirty_info *info, unsigned long vfn,
> +                              uint8_t **md, size_t *hash_len)
> +{
> +    struct iovec iov_array;
> +    int ret = 0;
> +    int nkey = 1;
> +
> +    iov_array.iov_base = info->block_addr +
> +                         vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
> +    iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;

I'm a bit confused by how this is working; is 'vfn' an index
in SAMPLE_PAGE_SIZE's rather than individual pages? So this is
going to hash over something the size of a 'DIRTYRATE_SAMPLE_PAGE_SIZE'?

> +    if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_MD5,
> +                            &iov_array, nkey,
> +                            md, hash_len, NULL) < 0) {
> +        ret = -1;
> +    }
> +
> +    return ret;
> +}
> +
> +static int save_block_hash(struct block_dirty_info *info)
> +{
> +    unsigned long *rand_buf = NULL;
> +    unsigned int sample_pages_count;
> +    uint8_t *md = NULL;
> +    size_t hash_len;
> +    int i;
> +    int ret = -1;
> +
> +    sample_pages_count = info->sample_pages_count;
> +    /* block size less than one page, return success to skip this block */
> +    if (unlikely(info->block_pages == 0 || sample_pages_count == 0)) {
> +        ret = 0;
> +        goto out;
> +    }
> +
> +    /* use random bytes to pick sample page vfn */
> +    rand_buf = g_malloc0_n(sample_pages_count, sizeof(unsigned long));
> +    /* DEFAULT_READ_RANDOM_MAX_LIMIT 32M,
> +     * can support 4T vm 1024 sample_pages_per_gigabytes
> +     */
> +    ret = qcrypto_random_bytes((unsigned char *)rand_buf,
> +                               sample_pages_count * sizeof(unsigned long),
> +                               NULL);

I think there may be a different way to do this without having to store
the rand_buf.
We already link to glib, and glib has a PRNG; https://developer.gnome.org/glib/stable/glib-Random-Numbers.html
If you create a new prng is g_rand_new() at the start of day, and take a
copy with g_rand_copy(), then you can replay the sequence of random
numbers it generates without actually storing the sequence.
So g_rand_new(), g_rand_copy() and then before you traverse the set of
pages you go and take a copy again and use the copy; it'll give the same
sequence every time.

Note also, because you're allocating a potentially large array,
for this and the hash_result please use g_try_malloc0_n (or g_try_new0)
and fail properly if it returns NULL.

Note we have users with more than 4T of RAM in their VMs, although I
doubt in a single RAMBlock

> +    if (ret) {
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    hash_len = qcrypto_hash_digest_len(QCRYPTO_HASH_ALG_MD5);
> +    info->hash_result = g_malloc0_n(sample_pages_count, sizeof(uint8_t) * hash_len);
> +    info->sample_page_vfn = g_malloc0_n(sample_pages_count, sizeof(unsigned long));
> +
> +    for (i = 0; i < sample_pages_count; i++) {
> +        md = info->hash_result + i * hash_len;
> +        info->sample_page_vfn[i] = rand_buf[i] % info->block_pages;
> +        ret = get_block_vfn_hash(info, info->sample_page_vfn[i], &md, &hash_len);
> +        if (ret < 0) {
> +            goto out;
> +        }
> +    }
> +    ret = 0;
> +out:
> +    g_free(rand_buf);
> +    return ret;
> +}
> +
> +static void get_block_dirty_info(RAMBlock *block, struct block_dirty_info *info,
> +                                 struct dirtyrate_config *config)
> +{
> +    uint64_t sample_pages_per_gigabytes = config->sample_pages_per_gigabytes;
> +
> +    /* Right shift 30 bits to calc block size in GB */
> +    info->sample_pages_count = (qemu_ram_get_used_length(block) * sample_pages_per_gigabytes) >> 30;
> +
> +    info->block_pages = qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SIZE_SHIFT;
> +    info->block_addr = qemu_ram_get_host_addr(block);
> +    strcpy(info->idstr, qemu_ram_get_idstr(block));
> +}
> +
> +static struct block_dirty_info *
> +alloc_block_dirty_info(int *block_index,
> +                       struct block_dirty_info *block_dinfo)
> +{
> +    struct block_dirty_info *info = NULL;
> +    int index = *block_index;
> +
> +    if (!block_dinfo) {
> +        block_dinfo = g_new(struct block_dirty_info, 1);
> +        index = 0;
> +    } else {
> +        block_dinfo = g_realloc(block_dinfo, (index + 1) *
> +                                sizeof(struct block_dirty_info));
> +        index++;

I think g_realloc works on a NULL pointer, so you might be able to
simplify.

> +    }
> +    info = &block_dinfo[index];
> +    memset(info, 0, sizeof(struct block_dirty_info));
> +
> +    *block_index = index;
> +    return block_dinfo;
> +}
> +
> +static int ram_block_skip(RAMBlock *block)
> +{
> +    if (!strstr(qemu_ram_get_idstr(block), "ram-node") &&
> +        !strstr(qemu_ram_get_idstr(block), "memdimm")) {
> +        if (strcmp(qemu_ram_get_idstr(block), "mach-virt.ram") ||
> +            strcmp(block->idstr, "pc.ram")) {
> +            return -1;
> +        }
> +    }

We can't tie this to the names you guess that RAMBlocks might have - 
things like 'memdimm' are settable by the caller and can be anything,
so you need to picka  different way of chosing which RAMBlocks to
use; I suggest anything larger than some cutoff size, that's really RAM.

> +    return 0;
> +}
> +
> +static int record_block_hash_info(struct dirtyrate_config config,
> +                                  struct block_dirty_info **block_dinfo, int *block_index)
> +{
> +    struct block_dirty_info *info = NULL;
> +    struct block_dirty_info *dinfo = NULL;
> +    RAMBlock *block = NULL;
> +    int index = 0;
> +
> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
> +        if (ram_block_skip(block) < 0) {
> +            continue;
> +        }
> +        dinfo = alloc_block_dirty_info(&index, dinfo);
> +        info = &dinfo[index];
> +        get_block_dirty_info(block, info, &config);
> +        if (save_block_hash(info) < 0) {
> +            *block_dinfo = dinfo;
> +            *block_index = index;
> +            return -1;
> +        }
> +    }
> +
> +    *block_dinfo = dinfo;
> +    *block_index = index;
> +
> +    return 0;
> +}
>  
>  static void calculate_dirtyrate(struct dirtyrate_config config, int64_t time)
>  {
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 2994535..4d9b3b8 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -15,6 +15,7 @@
>  
>  /* take 256 pages per GB for cal dirty rate */
>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES    256
> +#define DIRTYRATE_SAMPLE_PAGE_SIZE      4096
>  #define DIRTYRATE_PAGE_SIZE_SHIFT       12
>  #define BLOCK_INFO_MAX_LEN              256
>  #define PAGE_SIZE_SHIFT                 20
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC PATCH 4/8] migration/dirtyrate: Record hash results for each ramblock

[Zheng Chuan]

On 2020/8/5 1:00, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>> From: Zheng Chuan <zhengchuan@huawei.com>
>>
>> Record hash results for each ramblock.
> 
> Please be careful when talking about 'ramblock' since we already use
> that for a chunk of memory in QEMU.
> 
>> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
>> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
>> ---
>>  migration/dirtyrate.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  migration/dirtyrate.h |   1 +
>>  2 files changed, 158 insertions(+)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index 6baf674..45cfc91 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -10,12 +10,27 @@
>>   * 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 uint64_t sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
>>  static struct dirtyrate_statistics dirty_stat;
>>  CalculatingDirtyRateStage calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
>>  
>> +#define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
>> +        INTERNAL_RAMBLOCK_FOREACH(block)                   \
>> +        if (!qemu_ram_is_migratable(block)) {} else
>> +
> 
> Instead of redefining this here, please move the existing definition up
> into migration/ram.h ?
> 

OK, i'll do that in V2.

>>  static void reset_dirtyrate_stat(void)
>>  {
>>      dirty_stat.total_dirty_samples = 0;
>> @@ -44,6 +59,148 @@ static void update_dirtyrate(int64_t msec)
>>      dirty_stat.dirty_rate = dirty_rate;
>>  }
>>  
>> +static int get_block_vfn_hash(struct block_dirty_info *info, unsigned long vfn,
>> +                              uint8_t **md, size_t *hash_len)
>> +{
>> +    struct iovec iov_array;
>> +    int ret = 0;
>> +    int nkey = 1;
>> +
>> +    iov_array.iov_base = info->block_addr +
>> +                         vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
>> +    iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;
> 
> I'm a bit confused by how this is working; is 'vfn' an index
> in SAMPLE_PAGE_SIZE's rather than individual pages? So this is
> going to hash over something the size of a 'DIRTYRATE_SAMPLE_PAGE_SIZE'?
> 
Here is how sampling works for example.
1. assume block length is 1G, take 256 pages (sample_pages_count = 256) for sampling.
2. acquire rand_buf[256] and get 256 random pages over ramblock with "rand_buf[i] % info->block_pages;"
3. get hash results of 4K length for all 256 pages by qcrypto_hash_bytesv();
4. get hash results of 4K length for all 256 pages by qcrypto_hash_bytesv() again after sleep 1s;
5. compare hash results, and judge if it is the dirty during the sleep time;
6. calculate dirty rate.

Thus, vfn is an index, and we acquire hash result of the DIRTYRATE_SAMPLE_PAGE_SIZE length memory which starts from iov_base.

>> +    if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_MD5,
>> +                            &iov_array, nkey,
>> +                            md, hash_len, NULL) < 0) {
>> +        ret = -1;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static int save_block_hash(struct block_dirty_info *info)
>> +{
>> +    unsigned long *rand_buf = NULL;
>> +    unsigned int sample_pages_count;
>> +    uint8_t *md = NULL;
>> +    size_t hash_len;
>> +    int i;
>> +    int ret = -1;
>> +
>> +    sample_pages_count = info->sample_pages_count;
>> +    /* block size less than one page, return success to skip this block */
>> +    if (unlikely(info->block_pages == 0 || sample_pages_count == 0)) {
>> +        ret = 0;
>> +        goto out;
>> +    }
>> +
>> +    /* use random bytes to pick sample page vfn */
>> +    rand_buf = g_malloc0_n(sample_pages_count, sizeof(unsigned long));
>> +    /* DEFAULT_READ_RANDOM_MAX_LIMIT 32M,
>> +     * can support 4T vm 1024 sample_pages_per_gigabytes
>> +     */
>> +    ret = qcrypto_random_bytes((unsigned char *)rand_buf,
>> +                               sample_pages_count * sizeof(unsigned long),
>> +                               NULL);
> 
> I think there may be a different way to do this without having to store
> the rand_buf.
> We already link to glib, and glib has a PRNG; https://developer.gnome.org/glib/stable/glib-Random-Numbers.html
> If you create a new prng is g_rand_new() at the start of day, and take a
> copy with g_rand_copy(), then you can replay the sequence of random
> numbers it generates without actually storing the sequence.
> So g_rand_new(), g_rand_copy() and then before you traverse the set of
> pages you go and take a copy again and use the copy; it'll give the same
> sequence every time.
> 
> Note also, because you're allocating a potentially large array,
> for this and the hash_result please use g_try_malloc0_n (or g_try_new0)
> and fail properly if it returns NULL.
> 
Sure, i'll consider that in V2.

> Note we have users with more than 4T of RAM in their VMs, although I
> doubt in a single RAMBlock
> 
>> +    if (ret) {
>> +        ret = -1;
>> +        goto out;
>> +    }
>> +
>> +    hash_len = qcrypto_hash_digest_len(QCRYPTO_HASH_ALG_MD5);
>> +    info->hash_result = g_malloc0_n(sample_pages_count, sizeof(uint8_t) * hash_len);
>> +    info->sample_page_vfn = g_malloc0_n(sample_pages_count, sizeof(unsigned long));
>> +
>> +    for (i = 0; i < sample_pages_count; i++) {
>> +        md = info->hash_result + i * hash_len;
>> +        info->sample_page_vfn[i] = rand_buf[i] % info->block_pages;
>> +        ret = get_block_vfn_hash(info, info->sample_page_vfn[i], &md, &hash_len);
>> +        if (ret < 0) {
>> +            goto out;
>> +        }
>> +    }
>> +    ret = 0;
>> +out:
>> +    g_free(rand_buf);
>> +    return ret;
>> +}
>> +
>> +static void get_block_dirty_info(RAMBlock *block, struct block_dirty_info *info,
>> +                                 struct dirtyrate_config *config)
>> +{
>> +    uint64_t sample_pages_per_gigabytes = config->sample_pages_per_gigabytes;
>> +
>> +    /* Right shift 30 bits to calc block size in GB */
>> +    info->sample_pages_count = (qemu_ram_get_used_length(block) * sample_pages_per_gigabytes) >> 30;
>> +
>> +    info->block_pages = qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SIZE_SHIFT;
>> +    info->block_addr = qemu_ram_get_host_addr(block);
>> +    strcpy(info->idstr, qemu_ram_get_idstr(block));
>> +}
>> +
>> +static struct block_dirty_info *
>> +alloc_block_dirty_info(int *block_index,
>> +                       struct block_dirty_info *block_dinfo)
>> +{
>> +    struct block_dirty_info *info = NULL;
>> +    int index = *block_index;
>> +
>> +    if (!block_dinfo) {
>> +        block_dinfo = g_new(struct block_dirty_info, 1);
>> +        index = 0;
>> +    } else {
>> +        block_dinfo = g_realloc(block_dinfo, (index + 1) *
>> +                                sizeof(struct block_dirty_info));
>> +        index++;
> 
> I think g_realloc works on a NULL pointer, so you might be able to
> simplify.
> 
>> +    }
>> +    info = &block_dinfo[index];
>> +    memset(info, 0, sizeof(struct block_dirty_info));
>> +
>> +    *block_index = index;
>> +    return block_dinfo;
>> +}
>> +
>> +static int ram_block_skip(RAMBlock *block)
>> +{
>> +    if (!strstr(qemu_ram_get_idstr(block), "ram-node") &&
>> +        !strstr(qemu_ram_get_idstr(block), "memdimm")) {
>> +        if (strcmp(qemu_ram_get_idstr(block), "mach-virt.ram") ||
>> +            strcmp(block->idstr, "pc.ram")) {
>> +            return -1;
>> +        }
>> +    }
> 
> We can't tie this to the names you guess that RAMBlocks might have - 
> things like 'memdimm' are settable by the caller and can be anything,
> so you need to picka  different way of chosing which RAMBlocks to
> use; I suggest anything larger than some cutoff size, that's really RAM.
> 
OK, i'll consider it in a general way.
>> +    return 0;
>> +}
>> +
>> +static int record_block_hash_info(struct dirtyrate_config config,
>> +                                  struct block_dirty_info **block_dinfo, int *block_index)
>> +{
>> +    struct block_dirty_info *info = NULL;
>> +    struct block_dirty_info *dinfo = NULL;
>> +    RAMBlock *block = NULL;
>> +    int index = 0;
>> +
>> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
>> +        if (ram_block_skip(block) < 0) {
>> +            continue;
>> +        }
>> +        dinfo = alloc_block_dirty_info(&index, dinfo);
>> +        info = &dinfo[index];
>> +        get_block_dirty_info(block, info, &config);
>> +        if (save_block_hash(info) < 0) {
>> +            *block_dinfo = dinfo;
>> +            *block_index = index;
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    *block_dinfo = dinfo;
>> +    *block_index = index;
>> +
>> +    return 0;
>> +}
>>  
>>  static void calculate_dirtyrate(struct dirtyrate_config config, int64_t time)
>>  {
>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>> index 2994535..4d9b3b8 100644
>> --- a/migration/dirtyrate.h
>> +++ b/migration/dirtyrate.h
>> @@ -15,6 +15,7 @@
>>  
>>  /* take 256 pages per GB for cal dirty rate */
>>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES    256
>> +#define DIRTYRATE_SAMPLE_PAGE_SIZE      4096
>>  #define DIRTYRATE_PAGE_SIZE_SHIFT       12
>>  #define BLOCK_INFO_MAX_LEN              256
>>  #define PAGE_SIZE_SHIFT                 20
>> -- 
>> 1.8.3.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 
> .
> 

[RFC PATCH 5/8] migration/dirtyrate: Compare hash results for recorded ramblock

[Chuan Zheng]

From: Zheng Chuan <zhengchuan@huawei.com>

Compare hash results for recorded ramblock.

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

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 45cfc91..7badc53 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -202,6 +202,83 @@ static int record_block_hash_info(struct dirtyrate_config config,
     return 0;
 }
 
+static int cal_block_dirty_rate(struct block_dirty_info *info)
+{
+    uint8_t *md = NULL;
+    size_t hash_len;
+    int i;
+    int ret = 0;
+
+    hash_len = qcrypto_hash_digest_len(QCRYPTO_HASH_ALG_MD5);
+    md = g_new0(uint8_t, hash_len);
+
+    for (i = 0; i < info->sample_pages_count; i++) {
+        ret = get_block_vfn_hash(info, info->sample_page_vfn[i], &md, &hash_len);
+        if (ret < 0) {
+            goto out;
+        }
+
+        if (memcmp(md, info->hash_result + i * hash_len, hash_len) != 0) {
+            info->sample_dirty_count++;
+        }
+    }
+
+out:
+    g_free(md);
+    return ret;
+}
+
+static bool find_block_matched(RAMBlock *block, struct block_dirty_info *infos,
+                               int count, struct block_dirty_info **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].block_addr != qemu_ram_get_host_addr(block) ||
+        infos[i].block_pages !=
+            (qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SIZE_SHIFT)) {
+        return false;
+    }
+
+    *matched = &infos[i];
+    return true;
+}
+
+static int compare_block_hash_info(struct block_dirty_info *info, int block_index)
+{
+    struct block_dirty_info *block_dinfo = NULL;
+    RAMBlock *block = NULL;
+
+    RAMBLOCK_FOREACH_MIGRATABLE(block) {
+        if (ram_block_skip(block) < 0) {
+            continue;
+        }
+        block_dinfo = NULL;
+        if (!find_block_matched(block, info, block_index + 1, &block_dinfo)) {
+            continue;
+        }
+        if (cal_block_dirty_rate(block_dinfo) < 0) {
+            return -1;
+        }
+        update_dirtyrate_stat(block_dinfo);
+    }
+    if (!dirty_stat.total_sample_count) {
+        return -1;
+    }
+
+    return 0;
+}
+
+
 static void calculate_dirtyrate(struct dirtyrate_config config, int64_t time)
 {
     /* todo */
-- 
1.8.3.1

Re: [RFC PATCH 5/8] migration/dirtyrate: Compare hash results for recorded ramblock

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> From: Zheng Chuan <zhengchuan@huawei.com>
> 
> Compare hash results for recorded ramblock.
> 
> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
> ---
>  migration/dirtyrate.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 45cfc91..7badc53 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -202,6 +202,83 @@ static int record_block_hash_info(struct dirtyrate_config config,
>      return 0;
>  }
>  
> +static int cal_block_dirty_rate(struct block_dirty_info *info)
> +{
> +    uint8_t *md = NULL;
> +    size_t hash_len;
> +    int i;
> +    int ret = 0;
> +
> +    hash_len = qcrypto_hash_digest_len(QCRYPTO_HASH_ALG_MD5);
> +    md = g_new0(uint8_t, hash_len);

Is 'hash_len' actually constant for a given algorithm, like MD5 ?
i.e. can we just have a nice fixed size array?

> +    for (i = 0; i < info->sample_pages_count; i++) {
> +        ret = get_block_vfn_hash(info, info->sample_page_vfn[i], &md, &hash_len);
> +        if (ret < 0) {
> +            goto out;
> +        }
> +
> +        if (memcmp(md, info->hash_result + i * hash_len, hash_len) != 0) {
> +            info->sample_dirty_count++;

When the page doesn't match, do we have to update info->hash_result with
the new hash?   If the page is only modified once, and we catch it on
this cycle, we wouldn't want to catch it next time around.

> +        }
> +    }
> +
> +out:
> +    g_free(md);
> +    return ret;
> +}
> +
> +static bool find_block_matched(RAMBlock *block, struct block_dirty_info *infos,
> +                               int count, struct block_dirty_info **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].block_addr != qemu_ram_get_host_addr(block) ||
> +        infos[i].block_pages !=
> +            (qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SIZE_SHIFT)) {

How does this happen?

> +        return false;
> +    }
> +
> +    *matched = &infos[i];
> +    return true;
> +}
> +
> +static int compare_block_hash_info(struct block_dirty_info *info, int block_index)
> +{
> +    struct block_dirty_info *block_dinfo = NULL;
> +    RAMBlock *block = NULL;
> +
> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
> +        if (ram_block_skip(block) < 0) {
> +            continue;
> +        }
> +        block_dinfo = NULL;
> +        if (!find_block_matched(block, info, block_index + 1, &block_dinfo)) {
> +            continue;
> +        }
> +        if (cal_block_dirty_rate(block_dinfo) < 0) {
> +            return -1;
> +        }
> +        update_dirtyrate_stat(block_dinfo);
> +    }
> +    if (!dirty_stat.total_sample_count) {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
>  static void calculate_dirtyrate(struct dirtyrate_config config, int64_t time)
>  {
>      /* todo */
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC PATCH 5/8] migration/dirtyrate: Compare hash results for recorded ramblock

[Zheng Chuan]

On 2020/8/5 1:29, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>> From: Zheng Chuan <zhengchuan@huawei.com>
>>
>> Compare hash results for recorded ramblock.
>>
>> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
>> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
>> ---
>>  migration/dirtyrate.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 77 insertions(+)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index 45cfc91..7badc53 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -202,6 +202,83 @@ static int record_block_hash_info(struct dirtyrate_config config,
>>      return 0;
>>  }
>>  
>> +static int cal_block_dirty_rate(struct block_dirty_info *info)
>> +{
>> +    uint8_t *md = NULL;
>> +    size_t hash_len;
>> +    int i;
>> +    int ret = 0;
>> +
>> +    hash_len = qcrypto_hash_digest_len(QCRYPTO_HASH_ALG_MD5);
>> +    md = g_new0(uint8_t, hash_len);
> 
> Is 'hash_len' actually constant for a given algorithm, like MD5 ?
> i.e. can we just have a nice fixed size array?
> 
>> +    for (i = 0; i < info->sample_pages_count; i++) {
>> +        ret = get_block_vfn_hash(info, info->sample_page_vfn[i], &md, &hash_len);
>> +        if (ret < 0) {
>> +            goto out;
>> +        }
>> +
>> +        if (memcmp(md, info->hash_result + i * hash_len, hash_len) != 0) {
>> +            info->sample_dirty_count++;
> 
> When the page doesn't match, do we have to update info->hash_result with
> the new hash?   If the page is only modified once, and we catch it on
> this cycle, we wouldn't want to catch it next time around.
> 
For now, we only support calculate once for each qmp command, thus there is no need
to update it.

However, it is indeed in our plan to add support for calculate multiple times for each qmp command to enhance
dirty rate preciseness:)

>> +        }
>> +    }
>> +
>> +out:
>> +    g_free(md);
>> +    return ret;
>> +}
>> +
>> +static bool find_block_matched(RAMBlock *block, struct block_dirty_info *infos,
>> +                               int count, struct block_dirty_info **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].block_addr != qemu_ram_get_host_addr(block) ||
>> +        infos[i].block_pages !=
>> +            (qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SIZE_SHIFT)) {
> 
> How does this happen?
> 
>> +        return false;
>> +    }
>> +
>> +    *matched = &infos[i];
>> +    return true;
>> +}
>> +
>> +static int compare_block_hash_info(struct block_dirty_info *info, int block_index)
>> +{
>> +    struct block_dirty_info *block_dinfo = NULL;
>> +    RAMBlock *block = NULL;
>> +
>> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
>> +        if (ram_block_skip(block) < 0) {
>> +            continue;
>> +        }
>> +        block_dinfo = NULL;
>> +        if (!find_block_matched(block, info, block_index + 1, &block_dinfo)) {
>> +            continue;
>> +        }
>> +        if (cal_block_dirty_rate(block_dinfo) < 0) {
>> +            return -1;
>> +        }
>> +        update_dirtyrate_stat(block_dinfo);
>> +    }
>> +    if (!dirty_stat.total_sample_count) {
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +
>>  static void calculate_dirtyrate(struct dirtyrate_config config, int64_t time)
>>  {
>>      /* todo */
>> -- 
>> 1.8.3.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 
> .
> 

[RFC PATCH 6/8] migration/dirtyrate: Implement get_sample_gap_period() and block_sample_gap_period()

[Chuan Zheng]

From: Zheng Chuan <zhengchuan@huawei.com>

Implement get_sample_gap_period() and block_sample_gap_period() to
sleep specific time between sample actions.

Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
---
 migration/dirtyrate.c | 28 ++++++++++++++++++++++++++++
 migration/dirtyrate.h |  6 +++++-
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 7badc53..00abfa7 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -295,10 +295,38 @@ static void set_dirty_rate_stage(CalculatingDirtyRateStage ratestage)
     calculating_dirty_rate_stage = ratestage;
 }
 
+static int64_t block_sample_gap_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_gap_period(struct dirtyrate_config config)
+{
+    int64_t msec;
+
+    msec = config.sample_period_seconds * 1000;
+    if (msec <= MIN_FETCH_DIRTYRATE_TIME_MSEC || msec > MAX_FETCH_DIRTYRATE_TIME_MSEC) {
+        msec = DEFAULT_FETCH_DIRTYRATE_TIME_MSEC;
+    }
+    return msec;
+}
+
 void *get_dirtyrate_thread(void *arg)
 {
     struct dirtyrate_config config = *(struct dirtyrate_config *)arg;
     int64_t msec = 0;
+
+    /* max period is 60 seconds */
+    msec = get_sample_gap_period(config);
  
     set_dirty_rate_stage(CAL_DIRTY_RATE_ING);
 
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 4d9b3b8..5aef2d7 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -14,12 +14,16 @@
 #define QEMU_MIGRATION_DIRTYRATE_H
 
 /* take 256 pages per GB for cal dirty rate */
-#define DIRTYRATE_DEFAULT_SAMPLE_PAGES    256
+#define DIRTYRATE_DEFAULT_SAMPLE_PAGES  256
 #define DIRTYRATE_SAMPLE_PAGE_SIZE      4096
 #define DIRTYRATE_PAGE_SIZE_SHIFT       12
 #define BLOCK_INFO_MAX_LEN              256
 #define PAGE_SIZE_SHIFT                 20
 
+#define MIN_FETCH_DIRTYRATE_TIME_MSEC        0
+#define MAX_FETCH_DIRTYRATE_TIME_MSEC        60000
+#define DEFAULT_FETCH_DIRTYRATE_TIME_MSEC    1000
+
 struct dirtyrate_config {
     uint64_t sample_pages_per_gigabytes;
     int64_t sample_period_seconds;
-- 
1.8.3.1

Re: [RFC PATCH 6/8] migration/dirtyrate: Implement get_sample_gap_period() and block_sample_gap_period()

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> From: Zheng Chuan <zhengchuan@huawei.com>
> 
> Implement get_sample_gap_period() and block_sample_gap_period() to
> sleep specific time between sample actions.
> 
> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
> ---
>  migration/dirtyrate.c | 28 ++++++++++++++++++++++++++++
>  migration/dirtyrate.h |  6 +++++-
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 7badc53..00abfa7 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -295,10 +295,38 @@ static void set_dirty_rate_stage(CalculatingDirtyRateStage ratestage)
>      calculating_dirty_rate_stage = ratestage;
>  }
>  
> +static int64_t block_sample_gap_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);
> +    }

OK, so I think this is for sleeping in your own thread?
Since it's bounded at about 60s that's not too bad

> +    return msec;
> +}
> +
> +static int64_t get_sample_gap_period(struct dirtyrate_config config)
> +{
> +    int64_t msec;
> +
> +    msec = config.sample_period_seconds * 1000;
> +    if (msec <= MIN_FETCH_DIRTYRATE_TIME_MSEC || msec > MAX_FETCH_DIRTYRATE_TIME_MSEC) {
> +        msec = DEFAULT_FETCH_DIRTYRATE_TIME_MSEC;
> +    }
> +    return msec;
> +}
> +
>  void *get_dirtyrate_thread(void *arg)
>  {
>      struct dirtyrate_config config = *(struct dirtyrate_config *)arg;
>      int64_t msec = 0;
> +
> +    /* max period is 60 seconds */
> +    msec = get_sample_gap_period(config);

(I'm not sure I understood where the config was set?)

>      set_dirty_rate_stage(CAL_DIRTY_RATE_ING);
>  
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 4d9b3b8..5aef2d7 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -14,12 +14,16 @@
>  #define QEMU_MIGRATION_DIRTYRATE_H
>  
>  /* take 256 pages per GB for cal dirty rate */
> -#define DIRTYRATE_DEFAULT_SAMPLE_PAGES    256
> +#define DIRTYRATE_DEFAULT_SAMPLE_PAGES  256

Not for this patch.

>  #define DIRTYRATE_SAMPLE_PAGE_SIZE      4096
>  #define DIRTYRATE_PAGE_SIZE_SHIFT       12
>  #define BLOCK_INFO_MAX_LEN              256
>  #define PAGE_SIZE_SHIFT                 20
>  
> +#define MIN_FETCH_DIRTYRATE_TIME_MSEC        0
> +#define MAX_FETCH_DIRTYRATE_TIME_MSEC        60000
> +#define DEFAULT_FETCH_DIRTYRATE_TIME_MSEC    1000
> +
>  struct dirtyrate_config {
>      uint64_t sample_pages_per_gigabytes;
>      int64_t sample_period_seconds;
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[RFC PATCH 7/8] migration/dirtyrate: Implement calculate_dirtyrate() function

[Chuan Zheng]

From: Zheng Chuan <zhengchuan@huawei.com>

Implement calculate_dirtyrate() function.

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

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 00abfa7..d87e16d 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -161,6 +161,21 @@ alloc_block_dirty_info(int *block_index,
     return block_dinfo;
 }
 
+static void free_block_dirty_info(struct block_dirty_info *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 int ram_block_skip(RAMBlock *block)
 {
     if (!strstr(qemu_ram_get_idstr(block), "ram-node") &&
@@ -278,12 +293,6 @@ static int compare_block_hash_info(struct block_dirty_info *info, int block_inde
     return 0;
 }
 
-
-static void calculate_dirtyrate(struct dirtyrate_config config, int64_t time)
-{
-    /* todo */
-}
-
 /*
  * There are multithread will write/read *calculating_dirty_rate_stage*,
  * we can protect only one thread write/read it by libvirt api.
@@ -320,6 +329,38 @@ static int64_t get_sample_gap_period(struct dirtyrate_config config)
     return msec;
 }
 
+static void calculate_dirtyrate(struct dirtyrate_config config, int64_t time)
+{
+    struct block_dirty_info *block_dinfo = NULL;
+    int block_index = 0;
+    int64_t msec = time;
+    int64_t initial_time;
+
+    rcu_register_thread();
+    reset_dirtyrate_stat();
+    initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+    rcu_read_lock();
+    if (record_block_hash_info(config, &block_dinfo, &block_index) < 0) {
+        goto out;
+    }
+    rcu_read_unlock();
+
+    msec = block_sample_gap_period(msec, initial_time);
+
+    rcu_read_lock();
+    if (compare_block_hash_info(block_dinfo, block_index) < 0) {
+        goto out;
+    }
+
+    update_dirtyrate(msec);
+
+out:
+    rcu_read_unlock();
+    free_block_dirty_info(block_dinfo, block_index + 1);
+    rcu_unregister_thread();
+}
+
+
 void *get_dirtyrate_thread(void *arg)
 {
     struct dirtyrate_config config = *(struct dirtyrate_config *)arg;
-- 
1.8.3.1

Re: [RFC PATCH 7/8] migration/dirtyrate: Implement calculate_dirtyrate() function

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> From: Zheng Chuan <zhengchuan@huawei.com>
> 
> Implement calculate_dirtyrate() function.
> 
> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
> ---
>  migration/dirtyrate.c | 53 +++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 47 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 00abfa7..d87e16d 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -161,6 +161,21 @@ alloc_block_dirty_info(int *block_index,
>      return block_dinfo;
>  }
>  
> +static void free_block_dirty_info(struct block_dirty_info *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 int ram_block_skip(RAMBlock *block)
>  {
>      if (!strstr(qemu_ram_get_idstr(block), "ram-node") &&
> @@ -278,12 +293,6 @@ static int compare_block_hash_info(struct block_dirty_info *info, int block_inde
>      return 0;
>  }
>  
> -
> -static void calculate_dirtyrate(struct dirtyrate_config config, int64_t time)
> -{
> -    /* todo */
> -}

Please move this function in the earlier patches so that it's only added
once rather than moved later.

>  /*
>   * There are multithread will write/read *calculating_dirty_rate_stage*,
>   * we can protect only one thread write/read it by libvirt api.
> @@ -320,6 +329,38 @@ static int64_t get_sample_gap_period(struct dirtyrate_config config)
>      return msec;
>  }
>  
> +static void calculate_dirtyrate(struct dirtyrate_config config, int64_t time)
> +{
> +    struct block_dirty_info *block_dinfo = NULL;
> +    int block_index = 0;
> +    int64_t msec = time;
> +    int64_t initial_time;

you might like to add some trace_ calls to make this easier to debug.

Dave

> +    rcu_register_thread();
> +    reset_dirtyrate_stat();
> +    initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    rcu_read_lock();
> +    if (record_block_hash_info(config, &block_dinfo, &block_index) < 0) {
> +        goto out;
> +    }
> +    rcu_read_unlock();
> +
> +    msec = block_sample_gap_period(msec, initial_time);
> +
> +    rcu_read_lock();
> +    if (compare_block_hash_info(block_dinfo, block_index) < 0) {
> +        goto out;
> +    }
> +
> +    update_dirtyrate(msec);
> +
> +out:
> +    rcu_read_unlock();
> +    free_block_dirty_info(block_dinfo, block_index + 1);
> +    rcu_unregister_thread();
> +}
> +
> +
>  void *get_dirtyrate_thread(void *arg)
>  {
>      struct dirtyrate_config config = *(struct dirtyrate_config *)arg;
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[RFC PATCH 8/8] migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function

[Chuan Zheng]

From: Zheng Chuan <zhengchuan@huawei.com>

Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called
by libvirt api.

Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
---
 migration/Makefile.objs |  1 +
 migration/dirtyrate.c   | 45 +++++++++++++++++++++++++++++++++++++++++++++
 qapi/migration.json     | 24 ++++++++++++++++++++++++
 qapi/pragma.json        |  3 ++-
 4 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/migration/Makefile.objs b/migration/Makefile.objs
index 0fc619e..12ae98c 100644
--- a/migration/Makefile.objs
+++ b/migration/Makefile.objs
@@ -6,6 +6,7 @@ common-obj-y += qemu-file.o global_state.o
 common-obj-y += qemu-file-channel.o
 common-obj-y += xbzrle.o postcopy-ram.o
 common-obj-y += qjson.o
+common-obj-y += dirtyrate.o
 common-obj-y += block-dirty-bitmap.o
 common-obj-y += multifd.o
 common-obj-y += multifd-zlib.o
diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index d87e16d..5717521 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -31,6 +31,21 @@ CalculatingDirtyRateStage calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
         INTERNAL_RAMBLOCK_FOREACH(block)                   \
         if (!qemu_ram_is_migratable(block)) {} else
 
+static int64_t get_dirty_rate_info(void)
+{
+    int64_t dirty_rate = dirty_stat.dirty_rate;
+    /*
+     *    Return dirty_rate only when we get CAL_DIRTY_RATE_END.
+     *    And we must initial calculating_dirty_rate_stage.
+     */
+    if (calculating_dirty_rate_stage == CAL_DIRTY_RATE_END) {
+        calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
+        return dirty_rate;
+    }  else {
+        return -1;
+    }
+}
+
 static void reset_dirtyrate_stat(void)
 {
     dirty_stat.total_dirty_samples = 0;
@@ -377,3 +392,33 @@ void *get_dirtyrate_thread(void *arg)
 
     return NULL;
 }
+
+void qmp_cal_dirty_rate(int64_t value, Error **errp)
+{
+    static struct dirtyrate_config dirtyrate_config;
+    QemuThread thread;
+
+    /*
+     * We don't begin calculating thread only when it's in calculating status.
+     */
+    if (calculating_dirty_rate_stage == CAL_DIRTY_RATE_ING) {
+        return;
+    }
+
+    /*
+     * If we get CAL_DIRTY_RATE_END here, libvirtd may be restarted at last round,
+     * we need to initial it.
+     */
+    if (calculating_dirty_rate_stage == CAL_DIRTY_RATE_END)
+        calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
+
+    dirtyrate_config.sample_period_seconds = value;
+    dirtyrate_config.sample_pages_per_gigabytes = sample_pages_per_gigabytes;
+    qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
+                       (void *)&dirtyrate_config, QEMU_THREAD_DETACHED);
+}
+
+int64_t qmp_get_dirty_rate(Error **errp)
+{
+    return get_dirty_rate_info();
+}
diff --git a/qapi/migration.json b/qapi/migration.json
index d500055..59f35bb 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1621,3 +1621,27 @@
 ##
 { 'event': 'UNPLUG_PRIMARY',
   'data': { 'device-id': 'str' } }
+
+##
+# @cal_dirty_rate:
+#
+# start calculating dirty rate for vm
+#
+# @value: time for sample dirty pages
+#
+# Since: 5.1
+#
+# Example:
+#   {"command": "cal_dirty_rate", "data": {"value": 1} }
+#
+##
+{ 'command': 'cal_dirty_rate', 'data': {'value': 'int64'} }
+
+##
+# @get_dirty_rate:
+#
+# get dirty rate for vm
+#
+# Since: 5.1
+##
+{ 'command': 'get_dirty_rate', 'returns': 'int64' }
diff --git a/qapi/pragma.json b/qapi/pragma.json
index cffae27..ecd294b 100644
--- a/qapi/pragma.json
+++ b/qapi/pragma.json
@@ -10,7 +10,8 @@
         'query-migrate-cache-size',
         'query-tpm-models',
         'query-tpm-types',
-        'ringbuf-read' ],
+        'ringbuf-read',
+        'get_dirty_rate' ],
     'name-case-whitelist': [
         'ACPISlotType',             # DIMM, visible through query-acpi-ospm-status
         'CpuInfoMIPS',              # PC, visible through query-cpu
-- 
1.8.3.1

Re: [RFC PATCH 8/8] migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function

[Eric Blake]

On 7/24/20 10:11 PM, Chuan Zheng wrote:
> From: Zheng Chuan <zhengchuan@huawei.com>
> 
> Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called
> by libvirt api.
> 
> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
> ---

> +++ b/qapi/migration.json
> @@ -1621,3 +1621,27 @@
>   ##
>   { 'event': 'UNPLUG_PRIMARY',
>     'data': { 'device-id': 'str' } }
> +
> +##
> +# @cal_dirty_rate:

New QMP commands should be named favoring '-' over '_'; also, it doesn't 
hurt to spell it out:

calculate-dirty-rate

> +#
> +# start calculating dirty rate for vm
> +#
> +# @value: time for sample dirty pages

In what unit?

> +#
> +# Since: 5.1

We've missed 5.1; this will need to be updated to 5.2.

> +#
> +# Example:
> +#   {"command": "cal_dirty_rate", "data": {"value": 1} }
> +#
> +##
> +{ 'command': 'cal_dirty_rate', 'data': {'value': 'int64'} }
> +
> +##
> +# @get_dirty_rate:

get-dirty-rate, except that we tend to use 'query-' as the prefix for 
commands that read values.

> +#
> +# get dirty rate for vm
> +#
> +# Since: 5.1

5.2

What units is the rate expressed in?


> +##
> +{ 'command': 'get_dirty_rate', 'returns': 'int64' }
> diff --git a/qapi/pragma.json b/qapi/pragma.json
> index cffae27..ecd294b 100644
> --- a/qapi/pragma.json
> +++ b/qapi/pragma.json
> @@ -10,7 +10,8 @@
>           'query-migrate-cache-size',
>           'query-tpm-models',
>           'query-tpm-types',
> -        'ringbuf-read' ],
> +        'ringbuf-read',
> +        'get_dirty_rate' ],
>       'name-case-whitelist': [
>           'ACPISlotType',             # DIMM, visible through query-acpi-ospm-status
>           'CpuInfoMIPS',              # PC, visible through query-cpu
> 

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

Re: [RFC PATCH 8/8] migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function

[Zheng Chuan]

On 2020/8/5 0:28, Eric Blake wrote:
> On 7/24/20 10:11 PM, Chuan Zheng wrote:
>> From: Zheng Chuan <zhengchuan@huawei.com>
>>
>> Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called
>> by libvirt api.
>>
>> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
>> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
>> ---
> 
>> +++ b/qapi/migration.json
>> @@ -1621,3 +1621,27 @@
>>   ##
>>   { 'event': 'UNPLUG_PRIMARY',
>>     'data': { 'device-id': 'str' } }
>> +
>> +##
>> +# @cal_dirty_rate:
> 
> New QMP commands should be named favoring '-' over '_'; also, it doesn't hurt to spell it out:
> 
> calculate-dirty-rate
> 
Hi, Eric.
Thank you for your review.
I'll fix that in V2.

>> +#
>> +# start calculating dirty rate for vm
>> +#
>> +# @value: time for sample dirty pages
> 
> In what unit?
> 
The unit is second, i'll make comments in V2:)

>> +#
>> +# Since: 5.1
> 
> We've missed 5.1; this will need to be updated to 5.2.
> 
>> +#
>> +# Example:
>> +#   {"command": "cal_dirty_rate", "data": {"value": 1} }
>> +#
>> +##
>> +{ 'command': 'cal_dirty_rate', 'data': {'value': 'int64'} }
>> +
>> +##
>> +# @get_dirty_rate:
> 
> get-dirty-rate, except that we tend to use 'query-' as the prefix for commands that read values.
> 
>> +#
>> +# get dirty rate for vm
>> +#
>> +# Since: 5.1
> 
> 5.2
> 
> What units is the rate expressed in?
> 
The unit is MB/s, i'll make comments in V2:)
> 
>> +##
>> +{ 'command': 'get_dirty_rate', 'returns': 'int64' }
>> diff --git a/qapi/pragma.json b/qapi/pragma.json
>> index cffae27..ecd294b 100644
>> --- a/qapi/pragma.json
>> +++ b/qapi/pragma.json
>> @@ -10,7 +10,8 @@
>>           'query-migrate-cache-size',
>>           'query-tpm-models',
>>           'query-tpm-types',
>> -        'ringbuf-read' ],
>> +        'ringbuf-read',
>> +        'get_dirty_rate' ],
>>       'name-case-whitelist': [
>>           'ACPISlotType',             # DIMM, visible through query-acpi-ospm-status
>>           'CpuInfoMIPS',              # PC, visible through query-cpu
>>
> 

Re: [RFC PATCH 8/8] migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function

[Eric Blake]

On 7/24/20 10:11 PM, Chuan Zheng wrote:
> From: Zheng Chuan <zhengchuan@huawei.com>
> 
> Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called
> by libvirt api.
> 
> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>

> +##
> +{ 'command': 'get_dirty_rate', 'returns': 'int64' }
> diff --git a/qapi/pragma.json b/qapi/pragma.json
> index cffae27..ecd294b 100644
> --- a/qapi/pragma.json
> +++ b/qapi/pragma.json
> @@ -10,7 +10,8 @@
>           'query-migrate-cache-size',
>           'query-tpm-models',
>           'query-tpm-types',
> -        'ringbuf-read' ],
> +        'ringbuf-read',
> +        'get_dirty_rate' ],

Nack.  You should not have to change the whitelist; this is evidence 
that your command is returning the wrong type.  Instead, you should be 
using:

{ 'command': 'get-dirty-rate', 'returns': { 'rate': 'int64' } }

and populating a struct, so that if we ever want to return more than 
just a single rate, we can extend the command in-place by adding to the 
struct.

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

Re: [RFC PATCH 8/8] migration/dirtyrate: Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> From: Zheng Chuan <zhengchuan@huawei.com>
> 
> Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called
> by libvirt api.
> 
> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
> ---
>  migration/Makefile.objs |  1 +
>  migration/dirtyrate.c   | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  qapi/migration.json     | 24 ++++++++++++++++++++++++
>  qapi/pragma.json        |  3 ++-
>  4 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/Makefile.objs b/migration/Makefile.objs
> index 0fc619e..12ae98c 100644
> --- a/migration/Makefile.objs
> +++ b/migration/Makefile.objs
> @@ -6,6 +6,7 @@ common-obj-y += qemu-file.o global_state.o
>  common-obj-y += qemu-file-channel.o
>  common-obj-y += xbzrle.o postcopy-ram.o
>  common-obj-y += qjson.o
> +common-obj-y += dirtyrate.o
>  common-obj-y += block-dirty-bitmap.o
>  common-obj-y += multifd.o
>  common-obj-y += multifd-zlib.o
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index d87e16d..5717521 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -31,6 +31,21 @@ CalculatingDirtyRateStage calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
>          INTERNAL_RAMBLOCK_FOREACH(block)                   \
>          if (!qemu_ram_is_migratable(block)) {} else
>  
> +static int64_t get_dirty_rate_info(void)
> +{
> +    int64_t dirty_rate = dirty_stat.dirty_rate;
> +    /*
> +     *    Return dirty_rate only when we get CAL_DIRTY_RATE_END.
> +     *    And we must initial calculating_dirty_rate_stage.
> +     */
> +    if (calculating_dirty_rate_stage == CAL_DIRTY_RATE_END) {
> +        calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;
> +        return dirty_rate;

I don't think a 'get' should change state - in particular I think we
should be able to call 'get' multiple times and get the result back.
Also, instead of just returning -1 you should probably give an error;
e.g. 'Still measuring' or 'not yet measured'

> +    }  else {
> +        return -1;
> +    }
> +}
> +
>  static void reset_dirtyrate_stat(void)
>  {
>      dirty_stat.total_dirty_samples = 0;
> @@ -377,3 +392,33 @@ void *get_dirtyrate_thread(void *arg)
>  
>      return NULL;
>  }
> +
> +void qmp_cal_dirty_rate(int64_t value, Error **errp)
> +{
> +    static struct dirtyrate_config dirtyrate_config;
> +    QemuThread thread;
> +
> +    /*
> +     * We don't begin calculating thread only when it's in calculating status.
> +     */
> +    if (calculating_dirty_rate_stage == CAL_DIRTY_RATE_ING) {
> +        return;
> +    }
> +
> +    /*
> +     * If we get CAL_DIRTY_RATE_END here, libvirtd may be restarted at last round,
> +     * we need to initial it.
> +     */
> +    if (calculating_dirty_rate_stage == CAL_DIRTY_RATE_END)
> +        calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT;

Note please run the patches through scripts/checkpatch.pl - you need
some {'s here and that script tells you about most of these things.

> +    dirtyrate_config.sample_period_seconds = value;

Ah, this is where the config comes from - OK, I think you said there was
a max, so you should probably check that the maximum is sensible here
and give an error if it's not.

Dave

> +    dirtyrate_config.sample_pages_per_gigabytes = sample_pages_per_gigabytes;
> +    qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
> +                       (void *)&dirtyrate_config, QEMU_THREAD_DETACHED);
> +}
> +
> +int64_t qmp_get_dirty_rate(Error **errp)
> +{
> +    return get_dirty_rate_info();
> +}
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d500055..59f35bb 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1621,3 +1621,27 @@
>  ##
>  { 'event': 'UNPLUG_PRIMARY',
>    'data': { 'device-id': 'str' } }
> +
> +##
> +# @cal_dirty_rate:
> +#
> +# start calculating dirty rate for vm
> +#
> +# @value: time for sample dirty pages
> +#
> +# Since: 5.1
> +#
> +# Example:
> +#   {"command": "cal_dirty_rate", "data": {"value": 1} }
> +#
> +##
> +{ 'command': 'cal_dirty_rate', 'data': {'value': 'int64'} }

I'm OK with abbreviating 'calculate' but prefer calc.

> +##
> +# @get_dirty_rate:
> +#
> +# get dirty rate for vm
> +#
> +# Since: 5.1
> +##
> +{ 'command': 'get_dirty_rate', 'returns': 'int64' }
> diff --git a/qapi/pragma.json b/qapi/pragma.json
> index cffae27..ecd294b 100644
> --- a/qapi/pragma.json
> +++ b/qapi/pragma.json
> @@ -10,7 +10,8 @@
>          'query-migrate-cache-size',
>          'query-tpm-models',
>          'query-tpm-types',
> -        'ringbuf-read' ],
> +        'ringbuf-read',
> +        'get_dirty_rate' ],
>      'name-case-whitelist': [
>          'ACPISlotType',             # DIMM, visible through query-acpi-ospm-status
>          'CpuInfoMIPS',              # PC, visible through query-cpu
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC PATCH 0/8] *** A Method for evaluating dirty page rate ***

[Dr. David Alan Gilbert]

* Chuan Zheng (zhengchuan@huawei.com) wrote:
> From: Zheng Chuan <zhengchuan@huawei.com>

Hi,

> 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.
> 
> We evaluate the dirtypage rate on running 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);
> 
> ++++++++++++++++++++++++++++++++++++++++++
> |                      |    dirtyrate    |
> ++++++++++++++++++++++++++++++++++++++++++
> | no mempress          |     4MB/s       |
> ------------------------------------------
> | mempress 4096 1024   |    1204MB/s     |
> ++++++++++++++++++++++++++++++++++++++++++
> | mempress 4096 4096   |    4000Mb/s     |
> ++++++++++++++++++++++++++++++++++++++++++

This is quite neat; I know we've got other people who have asked
for a similar feature!
Have you tried to validate these numbers against a real migration - e.g.
try setting mempress to dirty just under 1GByte/s and see if you can
migrate it over a 10Gbps link?

Dave

> Test dirtyrate by qmp command like this:
> 1.  virsh qemu-monitor-command [vmname] '{"execute":"cal_dirty_rate", "arguments": {"value": [sampletime]}}'
> 2.  virsh qemu-monitor-command [vmname] '{"execute":"get_dirty_rate"}'
> 
> Further test dirtyrate by libvirt api like this:
> virsh getdirtyrate [vmname] [sampletime]
> 
> Zheng Chuan (8):
>   migration/dirtyrate: Add get_dirtyrate_thread() function
>   migration/dirtyrate: Add block_dirty_info to store dirtypage info
>   migration/dirtyrate: Add dirtyrate statistics series functions
>   migration/dirtyrate: Record hash results for each ramblock
>   migration/dirtyrate: Compare hash results for recorded ramblock
>   migration/dirtyrate: Implement get_sample_gap_period() and
>     block_sample_gap_period()
>   migration/dirtyrate: Implement calculate_dirtyrate() function
>   migration/dirtyrate: Implement
>     qmp_cal_dirty_rate()/qmp_get_dirty_rate() function
> 
>  migration/Makefile.objs |   1 +
>  migration/dirtyrate.c   | 424 ++++++++++++++++++++++++++++++++++++++++++++++++
>  migration/dirtyrate.h   |  67 ++++++++
>  qapi/migration.json     |  24 +++
>  qapi/pragma.json        |   3 +-
>  5 files changed, 518 insertions(+), 1 deletion(-)
>  create mode 100644 migration/dirtyrate.c
>  create mode 100644 migration/dirtyrate.h
> 
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC PATCH 0/8] *** A Method for evaluating dirty page rate ***

[Zheng Chuan]

On 2020/8/5 0:19, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>> From: Zheng Chuan <zhengchuan@huawei.com>
> 
> Hi,
> 
>> 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.
>>
>> We evaluate the dirtypage rate on running 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);
>>
>> ++++++++++++++++++++++++++++++++++++++++++
>> |                      |    dirtyrate    |
>> ++++++++++++++++++++++++++++++++++++++++++
>> | no mempress          |     4MB/s       |
>> ------------------------------------------
>> | mempress 4096 1024   |    1204MB/s     |
>> ++++++++++++++++++++++++++++++++++++++++++
>> | mempress 4096 4096   |    4000Mb/s     |
>> ++++++++++++++++++++++++++++++++++++++++++
> 
> This is quite neat; I know we've got other people who have asked
> for a similar feature!
> Have you tried to validate these numbers against a real migration - e.g.
> try setting mempress to dirty just under 1GByte/s and see if you can
> migrate it over a 10Gbps link?
> 
> Dave
> 
Hi, Dave.
Thank you for your review.

Note that, the original intention is evaluating dirty rate before migration.

However, I test dirty rate against a real migration over a bandwidth of 10Gps with various mempress, which shows as below:
++++++++++++++++++++++++++++++++++++++++++
|                      |    dirtyrate    |
++++++++++++++++++++++++++++++++++++++++++
| no mempress          |     8MB/s       |
------------------------------------------
| mempress 4096 1024   |    1188MB/s     |
++++++++++++++++++++++++++++++++++++++++++

It looks still close to actual dirty rate:)

Test results against a real migration will be posted in V2.

>> Test dirtyrate by qmp command like this:
>> 1.  virsh qemu-monitor-command [vmname] '{"execute":"cal_dirty_rate", "arguments": {"value": [sampletime]}}'
>> 2.  virsh qemu-monitor-command [vmname] '{"execute":"get_dirty_rate"}'
>>
>> Further test dirtyrate by libvirt api like this:
>> virsh getdirtyrate [vmname] [sampletime]
>>
>> Zheng Chuan (8):
>>   migration/dirtyrate: Add get_dirtyrate_thread() function
>>   migration/dirtyrate: Add block_dirty_info to store dirtypage info
>>   migration/dirtyrate: Add dirtyrate statistics series functions
>>   migration/dirtyrate: Record hash results for each ramblock
>>   migration/dirtyrate: Compare hash results for recorded ramblock
>>   migration/dirtyrate: Implement get_sample_gap_period() and
>>     block_sample_gap_period()
>>   migration/dirtyrate: Implement calculate_dirtyrate() function
>>   migration/dirtyrate: Implement
>>     qmp_cal_dirty_rate()/qmp_get_dirty_rate() function
>>
>>  migration/Makefile.objs |   1 +
>>  migration/dirtyrate.c   | 424 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  migration/dirtyrate.h   |  67 ++++++++
>>  qapi/migration.json     |  24 +++
>>  qapi/pragma.json        |   3 +-
>>  5 files changed, 518 insertions(+), 1 deletion(-)
>>  create mode 100644 migration/dirtyrate.c
>>  create mode 100644 migration/dirtyrate.h
>>
>> -- 
>> 1.8.3.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 
> .
> 

Re: [RFC PATCH 0/8] *** A Method for evaluating dirty page rate ***

[Dr. David Alan Gilbert]

* Zheng Chuan (zhengchuan@huawei.com) wrote:
> 
> 
> On 2020/8/5 0:19, Dr. David Alan Gilbert wrote:
> > * Chuan Zheng (zhengchuan@huawei.com) wrote:
> >> From: Zheng Chuan <zhengchuan@huawei.com>
> > 
> > Hi,
> > 
> >> 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.
> >>
> >> We evaluate the dirtypage rate on running 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);
> >>
> >> ++++++++++++++++++++++++++++++++++++++++++
> >> |                      |    dirtyrate    |
> >> ++++++++++++++++++++++++++++++++++++++++++
> >> | no mempress          |     4MB/s       |
> >> ------------------------------------------
> >> | mempress 4096 1024   |    1204MB/s     |
> >> ++++++++++++++++++++++++++++++++++++++++++
> >> | mempress 4096 4096   |    4000Mb/s     |
> >> ++++++++++++++++++++++++++++++++++++++++++
> > 
> > This is quite neat; I know we've got other people who have asked
> > for a similar feature!
> > Have you tried to validate these numbers against a real migration - e.g.
> > try setting mempress to dirty just under 1GByte/s and see if you can
> > migrate it over a 10Gbps link?
> > 
> > Dave
> > 
> Hi, Dave.
> Thank you for your review.
> 
> Note that, the original intention is evaluating dirty rate before migration.

Right, but the reason you want to evaluate the dirty rate is, I guess,
to figure out whether a migration is likely to coverge?

> However, I test dirty rate against a real migration over a bandwidth of 10Gps with various mempress, which shows as below:
> ++++++++++++++++++++++++++++++++++++++++++
> |                      |    dirtyrate    |
> ++++++++++++++++++++++++++++++++++++++++++
> | no mempress          |     8MB/s       |
> ------------------------------------------
> | mempress 4096 1024   |    1188MB/s     |
> ++++++++++++++++++++++++++++++++++++++++++
> 
> It looks still close to actual dirty rate:)

I don't quite understand that comparison you just gave.
But what I was expecting was that a mempress that
just fits teh ~1100MB/s is just the limit of what you can get down
a 10Gbps link.

Dave

> Test results against a real migration will be posted in V2.
> 
> >> Test dirtyrate by qmp command like this:
> >> 1.  virsh qemu-monitor-command [vmname] '{"execute":"cal_dirty_rate", "arguments": {"value": [sampletime]}}'
> >> 2.  virsh qemu-monitor-command [vmname] '{"execute":"get_dirty_rate"}'
> >>
> >> Further test dirtyrate by libvirt api like this:
> >> virsh getdirtyrate [vmname] [sampletime]
> >>
> >> Zheng Chuan (8):
> >>   migration/dirtyrate: Add get_dirtyrate_thread() function
> >>   migration/dirtyrate: Add block_dirty_info to store dirtypage info
> >>   migration/dirtyrate: Add dirtyrate statistics series functions
> >>   migration/dirtyrate: Record hash results for each ramblock
> >>   migration/dirtyrate: Compare hash results for recorded ramblock
> >>   migration/dirtyrate: Implement get_sample_gap_period() and
> >>     block_sample_gap_period()
> >>   migration/dirtyrate: Implement calculate_dirtyrate() function
> >>   migration/dirtyrate: Implement
> >>     qmp_cal_dirty_rate()/qmp_get_dirty_rate() function
> >>
> >>  migration/Makefile.objs |   1 +
> >>  migration/dirtyrate.c   | 424 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>  migration/dirtyrate.h   |  67 ++++++++
> >>  qapi/migration.json     |  24 +++
> >>  qapi/pragma.json        |   3 +-
> >>  5 files changed, 518 insertions(+), 1 deletion(-)
> >>  create mode 100644 migration/dirtyrate.c
> >>  create mode 100644 migration/dirtyrate.h
> >>
> >> -- 
> >> 1.8.3.1
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> > 
> > .
> > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC PATCH 0/8] *** A Method for evaluating dirty page rate ***

[Zheng Chuan]

On 2020/8/7 0:58, Dr. David Alan Gilbert wrote:
> * Zheng Chuan (zhengchuan@huawei.com) wrote:
>>
>>
>> On 2020/8/5 0:19, Dr. David Alan Gilbert wrote:
>>> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>>>> From: Zheng Chuan <zhengchuan@huawei.com>
>>>
>>> Hi,
>>>
>>>> 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.
>>>>
>>>> We evaluate the dirtypage rate on running 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);
>>>>
>>>> ++++++++++++++++++++++++++++++++++++++++++
>>>> |                      |    dirtyrate    |
>>>> ++++++++++++++++++++++++++++++++++++++++++
>>>> | no mempress          |     4MB/s       |
>>>> ------------------------------------------
>>>> | mempress 4096 1024   |    1204MB/s     |
>>>> ++++++++++++++++++++++++++++++++++++++++++
>>>> | mempress 4096 4096   |    4000Mb/s     |
>>>> ++++++++++++++++++++++++++++++++++++++++++
>>>
>>> This is quite neat; I know we've got other people who have asked
>>> for a similar feature!
>>> Have you tried to validate these numbers against a real migration - e.g.
>>> try setting mempress to dirty just under 1GByte/s and see if you can
>>> migrate it over a 10Gbps link?
>>>
>>> Dave
>>>
>> Hi, Dave.
>> Thank you for your review.
>>
>> Note that, the original intention is evaluating dirty rate before migration.
> 
> Right, but the reason you want to evaluate the dirty rate is, I guess,
> to figure out whether a migration is likely to coverge?
> 
Yes, in our practice, we use this feature to evaluate dirty rate before migration.
if the dirty rate is too high, users could consider do not migration in case of
migration failure and vm performance.
However, i think it could extend to use at all stages of migration which includes the migrating stage:)

>> However, I test dirty rate against a real migration over a bandwidth of 10Gps with various mempress, which shows as below:
>> ++++++++++++++++++++++++++++++++++++++++++
>> |                      |    dirtyrate    |
>> ++++++++++++++++++++++++++++++++++++++++++
>> | no mempress          |     8MB/s       |
>> ------------------------------------------
>> | mempress 4096 1024   |    1188MB/s     |
>> ++++++++++++++++++++++++++++++++++++++++++
>>
>> It looks still close to actual dirty rate:)
> 
> I don't quite understand that comparison you just gave.
> But what I was expecting was that a mempress that
> just fits teh ~1100MB/s is just the limit of what you can get down
> a 10Gbps link.
> 
> Dave
> 
Well, what i mean is that the comparison between before-migration and migrating stage under 10Gbps link,
the dirty-rate calculating by our method is very close.
We could use this method both at the stage of before-migration and migrating.

+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
|                   |                          |    dirtyrate       |
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
| before migration  |   mempress 4096 1024     |     1204MB/s       |
--------------------------------------------------------------------
| migrating         |   mempress 4096 1024     |     1188MB/s       |
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

I am not sure if it is the comparison you want.
if not, please let me know and i'll supplement it in V2:)

>> Test results against a real migration will be posted in V2.
>>
>>>> Test dirtyrate by qmp command like this:
>>>> 1.  virsh qemu-monitor-command [vmname] '{"execute":"cal_dirty_rate", "arguments": {"value": [sampletime]}}'
>>>> 2.  virsh qemu-monitor-command [vmname] '{"execute":"get_dirty_rate"}'
>>>>
>>>> Further test dirtyrate by libvirt api like this:
>>>> virsh getdirtyrate [vmname] [sampletime]
>>>>
>>>> Zheng Chuan (8):
>>>>   migration/dirtyrate: Add get_dirtyrate_thread() function
>>>>   migration/dirtyrate: Add block_dirty_info to store dirtypage info
>>>>   migration/dirtyrate: Add dirtyrate statistics series functions
>>>>   migration/dirtyrate: Record hash results for each ramblock
>>>>   migration/dirtyrate: Compare hash results for recorded ramblock
>>>>   migration/dirtyrate: Implement get_sample_gap_period() and
>>>>     block_sample_gap_period()
>>>>   migration/dirtyrate: Implement calculate_dirtyrate() function
>>>>   migration/dirtyrate: Implement
>>>>     qmp_cal_dirty_rate()/qmp_get_dirty_rate() function
>>>>
>>>>  migration/Makefile.objs |   1 +
>>>>  migration/dirtyrate.c   | 424 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  migration/dirtyrate.h   |  67 ++++++++
>>>>  qapi/migration.json     |  24 +++
>>>>  qapi/pragma.json        |   3 +-
>>>>  5 files changed, 518 insertions(+), 1 deletion(-)
>>>>  create mode 100644 migration/dirtyrate.c
>>>>  create mode 100644 migration/dirtyrate.h
>>>>
>>>> -- 
>>>> 1.8.3.1
>>>>
>>> --
>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>>
>>>
>>> .
>>>
>>