[PATCH] hmp: Add "calc_dirty_rate" and "info dirty_rate" cmds

[PATCH] hmp: Add "calc_dirty_rate" and "info dirty_rate" cmds

[Peter Xu]

These two commands are missing when adding the QMP sister commands.  Add them,
so developers can play with them easier.

Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Leonardo Bras Soares Passos <lsoaresp@redhat.com>
Cc: Chuan Zheng <zhengchuan@huawei.com>
Cc: huangy81@chinatelecom.cn
Signed-off-by: Peter Xu <peterx@redhat.com>
---
PS: I really doubt whether this is working as expected... I ran one 200MB/s
workload inside, what I measured is 20MB/s with current algorithm...  Sampling
512 pages out of 1G mem is not wise enough I guess, especially that assumes
dirty workload is spread across the memories while it's normally not the case..
---
 hmp-commands-info.hx  | 13 +++++++++++++
 hmp-commands.hx       | 14 ++++++++++++++
 include/monitor/hmp.h |  2 ++
 migration/dirtyrate.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ab0c7aa5eea..f8a9141dd8a 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -880,3 +880,16 @@ SRST
   ``info replay``
     Display the record/replay information: mode and the current icount.
 ERST
+
+    {
+        .name       = "dirty_rate",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show dirty rate information",
+        .cmd        = hmp_info_dirty_rate,
+    },
+
+SRST
+  ``info dirty_rate``
+    Display the vcpu dirty rate information.
+ERST
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 2d21fe5ad42..4c27fb91f7d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1727,3 +1727,17 @@ ERST
         .flags      = "p",
     },
 
+SRST
+``calc_dirty_rate`` *second*
+  Start a round of dirty rate measurement with the period specified in *second*.
+  The result of the dirty rate measurement may be observed with ``info
+  dirty_rate`` command.
+ERST
+
+    {
+        .name       = "calc_dirty_rate",
+        .args_type  = "second:l",
+        .params     = "second",
+        .help       = "start a round of guest dirty rate measurement",
+        .cmd        = hmp_calc_dirty_rate,
+    },
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 605d57287ae..3baa1058e2c 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -129,5 +129,7 @@ void hmp_info_replay(Monitor *mon, const QDict *qdict);
 void hmp_replay_break(Monitor *mon, const QDict *qdict);
 void hmp_replay_delete_break(Monitor *mon, const QDict *qdict);
 void hmp_replay_seek(Monitor *mon, const QDict *qdict);
+void hmp_info_dirty_rate(Monitor *mon, const QDict *qdict);
+void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index ccb98147e89..382287d2912 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -20,6 +20,9 @@
 #include "ram.h"
 #include "trace.h"
 #include "dirtyrate.h"
+#include "monitor/hmp.h"
+#include "monitor/monitor.h"
+#include "qapi/qmp/qdict.h"
 
 static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
 static struct DirtyRateStat DirtyStat;
@@ -424,3 +427,43 @@ struct DirtyRateInfo *qmp_query_dirty_rate(Error **errp)
 {
     return query_dirty_rate_info();
 }
+
+void hmp_info_dirty_rate(Monitor *mon, const QDict *qdict)
+{
+    DirtyRateInfo *info = query_dirty_rate_info();
+
+    monitor_printf(mon, "Status: %s\n",
+                   DirtyRateStatus_str(info->status));
+    monitor_printf(mon, "Start Time: %"PRIi64" (ms)\n",
+                   info->start_time);
+    monitor_printf(mon, "Period: %"PRIi64" (sec)\n",
+                   info->calc_time);
+    monitor_printf(mon, "Dirty rate: ");
+    if (info->has_dirty_rate) {
+        monitor_printf(mon, "%"PRIi64" (MB/s)\n", info->dirty_rate);
+    } else {
+        monitor_printf(mon, "(not ready)\n");
+    }
+    g_free(info);
+}
+
+void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict)
+{
+    int64_t sec = qdict_get_try_int(qdict, "second", 0);
+    Error *err = NULL;
+
+    if (!sec) {
+        monitor_printf(mon, "Incorrect period length specified!\n");
+        return;
+    }
+
+    qmp_calc_dirty_rate(sec, &err);
+    if (err) {
+        hmp_handle_error(mon, err);
+        return;
+    }
+
+    monitor_printf(mon, "Starting dirty rate measurement with period %"PRIi64
+                   " seconds\n", sec);
+    monitor_printf(mon, "[Please use 'info dirty_rate' to check results]\n");
+}
-- 
2.31.1

Re: [PATCH] hmp: Add "calc_dirty_rate" and "info dirty_rate" cmds

[Hyman]

在 2021/6/1 8:57, Peter Xu 写道:
> These two commands are missing when adding the QMP sister commands.  Add them,
> so developers can play with them easier.
> 
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Leonardo Bras Soares Passos <lsoaresp@redhat.com>
> Cc: Chuan Zheng <zhengchuan@huawei.com>
> Cc: huangy81@chinatelecom.cn
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> PS: I really doubt whether this is working as expected... I ran one 200MB/s
> workload inside, what I measured is 20MB/s with current algorithm...  Sampling
> 512 pages out of 1G mem is not wise enough I guess, especially that assumes
> dirty workload is spread across the memories while it's normally not the case..
I doubt whether the sampling can cope with the situation that the guest 
dirty memory too fast so that the sampling within a given time can not 
finish, this may happens when vm is in large scale.
> ---
>   hmp-commands-info.hx  | 13 +++++++++++++
>   hmp-commands.hx       | 14 ++++++++++++++
>   include/monitor/hmp.h |  2 ++
>   migration/dirtyrate.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 72 insertions(+)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index ab0c7aa5eea..f8a9141dd8a 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -880,3 +880,16 @@ SRST
>     ``info replay``
>       Display the record/replay information: mode and the current icount.
>   ERST
> +
> +    {
> +        .name       = "dirty_rate",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show dirty rate information",
> +        .cmd        = hmp_info_dirty_rate,
> +    },
> +
> +SRST
> +  ``info dirty_rate``
> +    Display the vcpu dirty rate information.
> +ERST
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 2d21fe5ad42..4c27fb91f7d 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1727,3 +1727,17 @@ ERST
>           .flags      = "p",
>       },
>   
> +SRST
> +``calc_dirty_rate`` *second*
> +  Start a round of dirty rate measurement with the period specified in *second*.
> +  The result of the dirty rate measurement may be observed with ``info
> +  dirty_rate`` command.
> +ERST
> +
> +    {
> +        .name       = "calc_dirty_rate",
> +        .args_type  = "second:l",
> +        .params     = "second",
> +        .help       = "start a round of guest dirty rate measurement",
> +        .cmd        = hmp_calc_dirty_rate,
> +    },
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index 605d57287ae..3baa1058e2c 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -129,5 +129,7 @@ void hmp_info_replay(Monitor *mon, const QDict *qdict);
>   void hmp_replay_break(Monitor *mon, const QDict *qdict);
>   void hmp_replay_delete_break(Monitor *mon, const QDict *qdict);
>   void hmp_replay_seek(Monitor *mon, const QDict *qdict);
> +void hmp_info_dirty_rate(Monitor *mon, const QDict *qdict);
> +void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict);
>   
>   #endif
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index ccb98147e89..382287d2912 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -20,6 +20,9 @@
>   #include "ram.h"
>   #include "trace.h"
>   #include "dirtyrate.h"
> +#include "monitor/hmp.h"
> +#include "monitor/monitor.h"
> +#include "qapi/qmp/qdict.h"
>   
>   static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
>   static struct DirtyRateStat DirtyStat;
> @@ -424,3 +427,43 @@ struct DirtyRateInfo *qmp_query_dirty_rate(Error **errp)
>   {
>       return query_dirty_rate_info();
>   }
> +
> +void hmp_info_dirty_rate(Monitor *mon, const QDict *qdict)
> +{
> +    DirtyRateInfo *info = query_dirty_rate_info();
> +
> +    monitor_printf(mon, "Status: %s\n",
> +                   DirtyRateStatus_str(info->status));
> +    monitor_printf(mon, "Start Time: %"PRIi64" (ms)\n",
> +                   info->start_time);
> +    monitor_printf(mon, "Period: %"PRIi64" (sec)\n",
> +                   info->calc_time);
> +    monitor_printf(mon, "Dirty rate: ");
> +    if (info->has_dirty_rate) {
> +        monitor_printf(mon, "%"PRIi64" (MB/s)\n", info->dirty_rate);
> +    } else {
> +        monitor_printf(mon, "(not ready)\n");
> +    }
> +    g_free(info);
> +}
> +
> +void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict)
> +{
> +    int64_t sec = qdict_get_try_int(qdict, "second", 0);
> +    Error *err = NULL;
> +
> +    if (!sec) {
> +        monitor_printf(mon, "Incorrect period length specified!\n");
> +        return;
> +    }
> +
> +    qmp_calc_dirty_rate(sec, &err);
> +    if (err) {
> +        hmp_handle_error(mon, err);
> +        return;
> +    }
> +
> +    monitor_printf(mon, "Starting dirty rate measurement with period %"PRIi64
> +                   " seconds\n", sec);
> +    monitor_printf(mon, "[Please use 'info dirty_rate' to check results]\n");
> +}
> 

Re: [PATCH] hmp: Add "calc_dirty_rate" and "info dirty_rate" cmds

[Peter Xu]

On Tue, Jun 01, 2021 at 10:08:31PM +0800, Hyman wrote:
> 
> 
> 在 2021/6/1 8:57, Peter Xu 写道:
> > These two commands are missing when adding the QMP sister commands.  Add them,
> > so developers can play with them easier.
> > 
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Cc: Juan Quintela <quintela@redhat.com>
> > Cc: Leonardo Bras Soares Passos <lsoaresp@redhat.com>
> > Cc: Chuan Zheng <zhengchuan@huawei.com>
> > Cc: huangy81@chinatelecom.cn
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > PS: I really doubt whether this is working as expected... I ran one 200MB/s
> > workload inside, what I measured is 20MB/s with current algorithm...  Sampling
> > 512 pages out of 1G mem is not wise enough I guess, especially that assumes
> > dirty workload is spread across the memories while it's normally not the case..
> I doubt whether the sampling can cope with the situation that the guest
> dirty memory too fast so that the sampling within a given time can not
> finish, this may happens when vm is in large scale.

Not my case, though..  I'm with a 1G super small VM, starting a malloc()
workload with 200MB, dirty rate 200MB/s.  As I said in the other thread, I
think it's the algorithm that may not really work well with such workload,
while it could be one of the major workloads..

-- 
Peter Xu

Re: [PATCH] hmp: Add "calc_dirty_rate" and "info dirty_rate" cmds

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> These two commands are missing when adding the QMP sister commands.  Add them,
> so developers can play with them easier.
> 
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Leonardo Bras Soares Passos <lsoaresp@redhat.com>
> Cc: Chuan Zheng <zhengchuan@huawei.com>
> Cc: huangy81@chinatelecom.cn
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

> ---
> PS: I really doubt whether this is working as expected... I ran one 200MB/s
> workload inside, what I measured is 20MB/s with current algorithm...  Sampling
> 512 pages out of 1G mem is not wise enough I guess, especially that assumes
> dirty workload is spread across the memories while it's normally not the case..

What size of address space did you dirty - was it 20MB?

Dave

> ---
>  hmp-commands-info.hx  | 13 +++++++++++++
>  hmp-commands.hx       | 14 ++++++++++++++
>  include/monitor/hmp.h |  2 ++
>  migration/dirtyrate.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 72 insertions(+)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index ab0c7aa5eea..f8a9141dd8a 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -880,3 +880,16 @@ SRST
>    ``info replay``
>      Display the record/replay information: mode and the current icount.
>  ERST
> +
> +    {
> +        .name       = "dirty_rate",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show dirty rate information",
> +        .cmd        = hmp_info_dirty_rate,
> +    },
> +
> +SRST
> +  ``info dirty_rate``
> +    Display the vcpu dirty rate information.
> +ERST
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 2d21fe5ad42..4c27fb91f7d 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1727,3 +1727,17 @@ ERST
>          .flags      = "p",
>      },
>  
> +SRST
> +``calc_dirty_rate`` *second*
> +  Start a round of dirty rate measurement with the period specified in *second*.
> +  The result of the dirty rate measurement may be observed with ``info
> +  dirty_rate`` command.
> +ERST
> +
> +    {
> +        .name       = "calc_dirty_rate",
> +        .args_type  = "second:l",
> +        .params     = "second",
> +        .help       = "start a round of guest dirty rate measurement",
> +        .cmd        = hmp_calc_dirty_rate,
> +    },
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index 605d57287ae..3baa1058e2c 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -129,5 +129,7 @@ void hmp_info_replay(Monitor *mon, const QDict *qdict);
>  void hmp_replay_break(Monitor *mon, const QDict *qdict);
>  void hmp_replay_delete_break(Monitor *mon, const QDict *qdict);
>  void hmp_replay_seek(Monitor *mon, const QDict *qdict);
> +void hmp_info_dirty_rate(Monitor *mon, const QDict *qdict);
> +void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index ccb98147e89..382287d2912 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -20,6 +20,9 @@
>  #include "ram.h"
>  #include "trace.h"
>  #include "dirtyrate.h"
> +#include "monitor/hmp.h"
> +#include "monitor/monitor.h"
> +#include "qapi/qmp/qdict.h"
>  
>  static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
>  static struct DirtyRateStat DirtyStat;
> @@ -424,3 +427,43 @@ struct DirtyRateInfo *qmp_query_dirty_rate(Error **errp)
>  {
>      return query_dirty_rate_info();
>  }
> +
> +void hmp_info_dirty_rate(Monitor *mon, const QDict *qdict)
> +{
> +    DirtyRateInfo *info = query_dirty_rate_info();
> +
> +    monitor_printf(mon, "Status: %s\n",
> +                   DirtyRateStatus_str(info->status));
> +    monitor_printf(mon, "Start Time: %"PRIi64" (ms)\n",
> +                   info->start_time);
> +    monitor_printf(mon, "Period: %"PRIi64" (sec)\n",
> +                   info->calc_time);
> +    monitor_printf(mon, "Dirty rate: ");
> +    if (info->has_dirty_rate) {
> +        monitor_printf(mon, "%"PRIi64" (MB/s)\n", info->dirty_rate);
> +    } else {
> +        monitor_printf(mon, "(not ready)\n");
> +    }
> +    g_free(info);
> +}
> +
> +void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict)
> +{
> +    int64_t sec = qdict_get_try_int(qdict, "second", 0);
> +    Error *err = NULL;
> +
> +    if (!sec) {
> +        monitor_printf(mon, "Incorrect period length specified!\n");
> +        return;
> +    }
> +
> +    qmp_calc_dirty_rate(sec, &err);
> +    if (err) {
> +        hmp_handle_error(mon, err);
> +        return;
> +    }
> +
> +    monitor_printf(mon, "Starting dirty rate measurement with period %"PRIi64
> +                   " seconds\n", sec);
> +    monitor_printf(mon, "[Please use 'info dirty_rate' to check results]\n");
> +}
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] hmp: Add "calc_dirty_rate" and "info dirty_rate" cmds

[Peter Xu]

On Tue, Jun 08, 2021 at 07:49:56PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > These two commands are missing when adding the QMP sister commands.  Add them,
> > so developers can play with them easier.
> > 
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Cc: Juan Quintela <quintela@redhat.com>
> > Cc: Leonardo Bras Soares Passos <lsoaresp@redhat.com>
> > Cc: Chuan Zheng <zhengchuan@huawei.com>
> > Cc: huangy81@chinatelecom.cn
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> > ---
> > PS: I really doubt whether this is working as expected... I ran one 200MB/s
> > workload inside, what I measured is 20MB/s with current algorithm...  Sampling
> > 512 pages out of 1G mem is not wise enough I guess, especially that assumes
> > dirty workload is spread across the memories while it's normally not the case..
> 
> What size of address space did you dirty - was it 20MB?

IIRC it was either 200M or 500M, based on a 1G small VM.

-- 
Peter Xu

Re: [PATCH] hmp: Add "calc_dirty_rate" and "info dirty_rate" cmds

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Tue, Jun 08, 2021 at 07:49:56PM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > These two commands are missing when adding the QMP sister commands.  Add them,
> > > so developers can play with them easier.
> > > 
> > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > Cc: Juan Quintela <quintela@redhat.com>
> > > Cc: Leonardo Bras Soares Passos <lsoaresp@redhat.com>
> > > Cc: Chuan Zheng <zhengchuan@huawei.com>
> > > Cc: huangy81@chinatelecom.cn
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > > ---
> > > PS: I really doubt whether this is working as expected... I ran one 200MB/s
> > > workload inside, what I measured is 20MB/s with current algorithm...  Sampling
> > > 512 pages out of 1G mem is not wise enough I guess, especially that assumes
> > > dirty workload is spread across the memories while it's normally not the case..
> > 
> > What size of address space did you dirty - was it 20MB?
> 
> IIRC it was either 200M or 500M, based on a 1G small VM.

What was your sample time ?

Dave

> -- 
> Peter Xu
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] hmp: Add "calc_dirty_rate" and "info dirty_rate" cmds

[Peter Xu]

On Tue, Jun 08, 2021 at 08:36:23PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Tue, Jun 08, 2021 at 07:49:56PM +0100, Dr. David Alan Gilbert wrote:
> > > * Peter Xu (peterx@redhat.com) wrote:
> > > > These two commands are missing when adding the QMP sister commands.  Add them,
> > > > so developers can play with them easier.
> > > > 
> > > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > Cc: Juan Quintela <quintela@redhat.com>
> > > > Cc: Leonardo Bras Soares Passos <lsoaresp@redhat.com>
> > > > Cc: Chuan Zheng <zhengchuan@huawei.com>
> > > > Cc: huangy81@chinatelecom.cn
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > 
> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > 
> > > > ---
> > > > PS: I really doubt whether this is working as expected... I ran one 200MB/s
> > > > workload inside, what I measured is 20MB/s with current algorithm...  Sampling
> > > > 512 pages out of 1G mem is not wise enough I guess, especially that assumes
> > > > dirty workload is spread across the memories while it's normally not the case..
> > > 
> > > What size of address space did you dirty - was it 20MB?
> > 
> > IIRC it was either 200M or 500M, based on a 1G small VM.
> 
> What was your sample time ?

10 seconds; I used the same sample time for below runs:

https://lore.kernel.org/qemu-devel/YMEFqfYZVhsinNN+@t490s/

A large sample time does make dirty rate less indeed, as the same dirty page
could be written again as 1 single page dirtyed in the host (while it's counted
twice in the guest dirty workload).

This effect should happen too if we further extend calc_dirty_rate with
KVM_GET_DIRTY_LOG in the future as the 3rd method besides dirty ring.

From that pov, dirty ring is easier to be more "accurate" (I don't know whether
it's suitable to say it's accurate; it's just easier to trap cases like
writting to same page multiple times within a period), as the ring size is
normally very limited (e.g. 4096 pages per vcpu), so even the guest workload
writes the same page twice, as long as there's a ring collect between the two
writes, they'll be counted twice too (each collect will reprotect the pages).

-- 
Peter Xu