[PATCH v1] migration/dirtyrate: make sample page count configurable

[PATCH v1] migration/dirtyrate: make sample page count configurable

[huangy81]

From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>

introduce optional sample-pages argument in calc-dirty-rate,
making sample page count per GB configurable so that more
accurate dirtyrate can be calculated.

Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
---
 migration/dirtyrate.c | 32 ++++++++++++++++++++++++++++----
 migration/dirtyrate.h |  8 +++++++-
 qapi/migration.json   | 13 ++++++++++---
 3 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index ccb9814..43a531c 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -48,6 +48,16 @@ static bool is_sample_period_valid(int64_t sec)
     return true;
 }
 
+static bool is_sample_pages_valid(int64_t pages)
+{
+    if (pages < MIN_SAMPLE_PAGE_COUNT ||
+        pages > MAX_SAMPLE_PAGE_COUNT) {
+        return false;
+    }
+
+    return true;
+}
+
 static int dirtyrate_set_state(int *state, int old_state, int new_state)
 {
     assert(new_state < DIRTY_RATE_STATUS__MAX);
@@ -72,13 +82,15 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
     info->status = CalculatingState;
     info->start_time = DirtyStat.start_time;
     info->calc_time = DirtyStat.calc_time;
+    info->sample_pages = DirtyStat.sample_pages;
 
     trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
 
     return info;
 }
 
-static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time)
+static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time,
+                                uint64_t sample_pages)
 {
     DirtyStat.total_dirty_samples = 0;
     DirtyStat.total_sample_count = 0;
@@ -86,6 +98,7 @@ static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time)
     DirtyStat.dirty_rate = -1;
     DirtyStat.start_time = start_time;
     DirtyStat.calc_time = calc_time;
+    DirtyStat.sample_pages = sample_pages;
 }
 
 static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
@@ -361,6 +374,7 @@ void *get_dirtyrate_thread(void *arg)
     int ret;
     int64_t start_time;
     int64_t calc_time;
+    uint64_t sample_pages;
 
     ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_UNSTARTED,
                               DIRTY_RATE_STATUS_MEASURING);
@@ -371,7 +385,8 @@ void *get_dirtyrate_thread(void *arg)
 
     start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) / 1000;
     calc_time = config.sample_period_seconds;
-    init_dirtyrate_stat(start_time, calc_time);
+    sample_pages = config.sample_pages_per_gigabytes;
+    init_dirtyrate_stat(start_time, calc_time, sample_pages);
 
     calculate_dirtyrate(config);
 
@@ -383,7 +398,8 @@ void *get_dirtyrate_thread(void *arg)
     return NULL;
 }
 
-void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
+void qmp_calc_dirty_rate(int64_t calc_time, bool has_sample_pages,
+                         int64_t sample_pages, Error **errp)
 {
     static struct DirtyRateConfig config;
     QemuThread thread;
@@ -404,6 +420,13 @@ void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
         return;
     }
 
+    if (has_sample_pages && !is_sample_pages_valid(sample_pages)) {
+        error_setg(errp, "sample-pages is out of range[%d, %d].",
+                         MIN_SAMPLE_PAGE_COUNT,
+                         MAX_SAMPLE_PAGE_COUNT);
+        return;
+    }
+
     /*
      * Init calculation state as unstarted.
      */
@@ -415,7 +438,8 @@ void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
     }
 
     config.sample_period_seconds = calc_time;
-    config.sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
+    config.sample_pages_per_gigabytes =
+        has_sample_pages ? sample_pages : DIRTYRATE_DEFAULT_SAMPLE_PAGES;
     qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
                        (void *)&config, QEMU_THREAD_DETACHED);
 }
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
index 6ec4295..5f987e2 100644
--- a/migration/dirtyrate.h
+++ b/migration/dirtyrate.h
@@ -15,7 +15,6 @@
 
 /*
  * Sample 512 pages per GB as default.
- * TODO: Make it configurable.
  */
 #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
 
@@ -35,6 +34,12 @@
 #define MIN_FETCH_DIRTYRATE_TIME_SEC              1
 #define MAX_FETCH_DIRTYRATE_TIME_SEC              60
 
+/*
+ * Take 128 as minimum for sample dirty pages
+ */
+#define MIN_SAMPLE_PAGE_COUNT                     128
+#define MAX_SAMPLE_PAGE_COUNT                     4096
+
 struct DirtyRateConfig {
     uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
     int64_t sample_period_seconds; /* time duration between two sampling */
@@ -63,6 +68,7 @@ struct DirtyRateStat {
     int64_t dirty_rate; /* dirty rate in MB/s */
     int64_t start_time; /* calculation start time in units of second */
     int64_t calc_time; /* time duration of two sampling in units of second */
+    uint64_t sample_pages; /* sample pages per GB */
 };
 
 void *get_dirtyrate_thread(void *arg);
diff --git a/qapi/migration.json b/qapi/migration.json
index 9bf0bc4..868a867 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1741,6 +1741,9 @@
 #
 # @calc-time: time in units of second for sample dirty pages
 #
+# @sample-pages: page count per GB for sample dirty pages
+#                the default value is 512
+#
 # Since: 5.2
 #
 ##
@@ -1748,7 +1751,8 @@
   'data': {'*dirty-rate': 'int64',
            'status': 'DirtyRateStatus',
            'start-time': 'int64',
-           'calc-time': 'int64'} }
+           'calc-time': 'int64',
+           'sample-pages': 'uint64'} }
 
 ##
 # @calc-dirty-rate:
@@ -1757,13 +1761,16 @@
 #
 # @calc-time: time in units of second for sample dirty pages
 #
+# @sample-pages: page count per GB for sample dirty pages
+#                the default value is 512
+#
 # Since: 5.2
 #
 # Example:
-#   {"command": "calc-dirty-rate", "data": {"calc-time": 1} }
+#   {"command": "calc-dirty-rate", "data": {"calc-time": 1, 'sample-pages': 512} }
 #
 ##
-{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64'} }
+{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64', '*sample-pages': 'int'} }
 
 ##
 # @query-dirty-rate:
-- 
1.8.3.1

Re: [PATCH v1] migration/dirtyrate: make sample page count configurable

[David Edmondson]

On Thursday, 2021-04-15 at 01:23:54 +08, huangy wrote:

> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>
> introduce optional sample-pages argument in calc-dirty-rate,
> making sample page count per GB configurable so that more
> accurate dirtyrate can be calculated.
>
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> ---
>  migration/dirtyrate.c | 32 ++++++++++++++++++++++++++++----
>  migration/dirtyrate.h |  8 +++++++-
>  qapi/migration.json   | 13 ++++++++++---
>  3 files changed, 45 insertions(+), 8 deletions(-)
>
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index ccb9814..43a531c 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -48,6 +48,16 @@ static bool is_sample_period_valid(int64_t sec)
>      return true;
>  }
>  
> +static bool is_sample_pages_valid(int64_t pages)
> +{
> +    if (pages < MIN_SAMPLE_PAGE_COUNT ||
> +        pages > MAX_SAMPLE_PAGE_COUNT) {
> +        return false;
> +    }
> +
> +    return true;
> +}

Could be:

return pages >= MIN_SAMPLE_PAGE_COUNT &&
       pages <= MAX_SAMPLE_PAGE_COUNT;

?

> +
>  static int dirtyrate_set_state(int *state, int old_state, int new_state)
>  {
>      assert(new_state < DIRTY_RATE_STATUS__MAX);
> @@ -72,13 +82,15 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
>      info->status = CalculatingState;
>      info->start_time = DirtyStat.start_time;
>      info->calc_time = DirtyStat.calc_time;
> +    info->sample_pages = DirtyStat.sample_pages;
>  
>      trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
>  
>      return info;
>  }
>  
> -static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time)
> +static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time,
> +                                uint64_t sample_pages)
>  {
>      DirtyStat.total_dirty_samples = 0;
>      DirtyStat.total_sample_count = 0;
> @@ -86,6 +98,7 @@ static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time)
>      DirtyStat.dirty_rate = -1;
>      DirtyStat.start_time = start_time;
>      DirtyStat.calc_time = calc_time;
> +    DirtyStat.sample_pages = sample_pages;
>  }
>  
>  static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
> @@ -361,6 +374,7 @@ void *get_dirtyrate_thread(void *arg)
>      int ret;
>      int64_t start_time;
>      int64_t calc_time;
> +    uint64_t sample_pages;
>  
>      ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_UNSTARTED,
>                                DIRTY_RATE_STATUS_MEASURING);
> @@ -371,7 +385,8 @@ void *get_dirtyrate_thread(void *arg)
>  
>      start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) / 1000;
>      calc_time = config.sample_period_seconds;
> -    init_dirtyrate_stat(start_time, calc_time);
> +    sample_pages = config.sample_pages_per_gigabytes;
> +    init_dirtyrate_stat(start_time, calc_time, sample_pages);
>  
>      calculate_dirtyrate(config);
>  
> @@ -383,7 +398,8 @@ void *get_dirtyrate_thread(void *arg)
>      return NULL;
>  }
>  
> -void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
> +void qmp_calc_dirty_rate(int64_t calc_time, bool has_sample_pages,
> +                         int64_t sample_pages, Error **errp)
>  {
>      static struct DirtyRateConfig config;
>      QemuThread thread;
> @@ -404,6 +420,13 @@ void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
>          return;
>      }
>  
> +    if (has_sample_pages && !is_sample_pages_valid(sample_pages)) {
> +        error_setg(errp, "sample-pages is out of range[%d, %d].",
> +                         MIN_SAMPLE_PAGE_COUNT,
> +                         MAX_SAMPLE_PAGE_COUNT);
> +        return;
> +    }

Could set "sample_pages = DIRTYRATE_DEFAULT_SAMPLE_PAGES" in the else
clause, removing the need for...

> +
>      /*
>       * Init calculation state as unstarted.
>       */
> @@ -415,7 +438,8 @@ void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
>      }
>  
>      config.sample_period_seconds = calc_time;
> -    config.sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
> +    config.sample_pages_per_gigabytes =
> +        has_sample_pages ? sample_pages : DIRTYRATE_DEFAULT_SAMPLE_PAGES;

...checking again here.

>      qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
>                         (void *)&config, QEMU_THREAD_DETACHED);
>  }
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 6ec4295..5f987e2 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -15,7 +15,6 @@
>  
>  /*
>   * Sample 512 pages per GB as default.
> - * TODO: Make it configurable.
>   */
>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
>  
> @@ -35,6 +34,12 @@
>  #define MIN_FETCH_DIRTYRATE_TIME_SEC              1
>  #define MAX_FETCH_DIRTYRATE_TIME_SEC              60
>  
> +/*
> + * Take 128 as minimum for sample dirty pages
> + */

This comment doesn't add any value - is there a reason for these values?

> +#define MIN_SAMPLE_PAGE_COUNT                     128
> +#define MAX_SAMPLE_PAGE_COUNT                     4096
> +
>  struct DirtyRateConfig {
>      uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
>      int64_t sample_period_seconds; /* time duration between two sampling */
> @@ -63,6 +68,7 @@ struct DirtyRateStat {
>      int64_t dirty_rate; /* dirty rate in MB/s */
>      int64_t start_time; /* calculation start time in units of second */
>      int64_t calc_time; /* time duration of two sampling in units of second */
> +    uint64_t sample_pages; /* sample pages per GB */
>  };
>  
>  void *get_dirtyrate_thread(void *arg);
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 9bf0bc4..868a867 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1741,6 +1741,9 @@
>  #
>  # @calc-time: time in units of second for sample dirty pages
>  #
> +# @sample-pages: page count per GB for sample dirty pages
> +#                the default value is 512
> +#
>  # Since: 5.2
>  #
>  ##
> @@ -1748,7 +1751,8 @@
>    'data': {'*dirty-rate': 'int64',
>             'status': 'DirtyRateStatus',
>             'start-time': 'int64',
> -           'calc-time': 'int64'} }
> +           'calc-time': 'int64',
> +           'sample-pages': 'uint64'} }
>  
>  ##
>  # @calc-dirty-rate:
> @@ -1757,13 +1761,16 @@
>  #
>  # @calc-time: time in units of second for sample dirty pages
>  #
> +# @sample-pages: page count per GB for sample dirty pages
> +#                the default value is 512
> +#
>  # Since: 5.2
>  #
>  # Example:
> -#   {"command": "calc-dirty-rate", "data": {"calc-time": 1} }
> +#   {"command": "calc-dirty-rate", "data": {"calc-time": 1, 'sample-pages': 512} }
>  #
>  ##
> -{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64'} }
> +{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64', '*sample-pages': 'int'} }
>  
>  ##
>  # @query-dirty-rate:
> -- 
> 1.8.3.1

dme.
-- 
Don't you know you're never going to get to France.

Re: [PATCH v1] migration/dirtyrate: make sample page count configurable

[Hyman Huang]

在 2021/4/15 1:23, huangy81@chinatelecom.cn 写道:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> introduce optional sample-pages argument in calc-dirty-rate,
> making sample page count per GB configurable so that more
> accurate dirtyrate can be calculated.
> 
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> ---
>   migration/dirtyrate.c | 32 ++++++++++++++++++++++++++++----
>   migration/dirtyrate.h |  8 +++++++-
>   qapi/migration.json   | 13 ++++++++++---
>   3 files changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index ccb9814..43a531c 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -48,6 +48,16 @@ static bool is_sample_period_valid(int64_t sec)
>       return true;
>   }
>   
> +static bool is_sample_pages_valid(int64_t pages)
> +{
> +    if (pages < MIN_SAMPLE_PAGE_COUNT ||
> +        pages > MAX_SAMPLE_PAGE_COUNT) {
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>   static int dirtyrate_set_state(int *state, int old_state, int new_state)
>   {
>       assert(new_state < DIRTY_RATE_STATUS__MAX);
> @@ -72,13 +82,15 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
>       info->status = CalculatingState;
>       info->start_time = DirtyStat.start_time;
>       info->calc_time = DirtyStat.calc_time;
> +    info->sample_pages = DirtyStat.sample_pages;
>   
>       trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
>   
>       return info;
>   }
>   
> -static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time)
> +static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time,
> +                                uint64_t sample_pages)
>   {
>       DirtyStat.total_dirty_samples = 0;
>       DirtyStat.total_sample_count = 0;
> @@ -86,6 +98,7 @@ static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time)
>       DirtyStat.dirty_rate = -1;
>       DirtyStat.start_time = start_time;
>       DirtyStat.calc_time = calc_time;
> +    DirtyStat.sample_pages = sample_pages;
>   }
>   
>   static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
> @@ -361,6 +374,7 @@ void *get_dirtyrate_thread(void *arg)
>       int ret;
>       int64_t start_time;
>       int64_t calc_time;
> +    uint64_t sample_pages;
>   
>       ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_UNSTARTED,
>                                 DIRTY_RATE_STATUS_MEASURING);
> @@ -371,7 +385,8 @@ void *get_dirtyrate_thread(void *arg)
>   
>       start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) / 1000;
>       calc_time = config.sample_period_seconds;
> -    init_dirtyrate_stat(start_time, calc_time);
> +    sample_pages = config.sample_pages_per_gigabytes;
> +    init_dirtyrate_stat(start_time, calc_time, sample_pages);
>   
>       calculate_dirtyrate(config);
>   
> @@ -383,7 +398,8 @@ void *get_dirtyrate_thread(void *arg)
>       return NULL;
>   }
>   
> -void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
> +void qmp_calc_dirty_rate(int64_t calc_time, bool has_sample_pages,
> +                         int64_t sample_pages, Error **errp)
>   {
>       static struct DirtyRateConfig config;
>       QemuThread thread;
> @@ -404,6 +420,13 @@ void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
>           return;
>       }
>   
> +    if (has_sample_pages && !is_sample_pages_valid(sample_pages)) {
> +        error_setg(errp, "sample-pages is out of range[%d, %d].",
> +                         MIN_SAMPLE_PAGE_COUNT,
> +                         MAX_SAMPLE_PAGE_COUNT);
> +        return;
> +    }
> +
>       /*
>        * Init calculation state as unstarted.
>        */
> @@ -415,7 +438,8 @@ void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
>       }
>   
>       config.sample_period_seconds = calc_time;
> -    config.sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
> +    config.sample_pages_per_gigabytes =
> +        has_sample_pages ? sample_pages : DIRTYRATE_DEFAULT_SAMPLE_PAGES;
>       qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
>                          (void *)&config, QEMU_THREAD_DETACHED);
>   }
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 6ec4295..5f987e2 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -15,7 +15,6 @@
>   
>   /*
>    * Sample 512 pages per GB as default.
> - * TODO: Make it configurable.
>    */
>   #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
>   
> @@ -35,6 +34,12 @@
>   #define MIN_FETCH_DIRTYRATE_TIME_SEC              1
>   #define MAX_FETCH_DIRTYRATE_TIME_SEC              60
>   
> +/*
> + * Take 128 as minimum for sample dirty pages
> + */
> +#define MIN_SAMPLE_PAGE_COUNT                     128
> +#define MAX_SAMPLE_PAGE_COUNT                     4096
> +
>   struct DirtyRateConfig {
>       uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
>       int64_t sample_period_seconds; /* time duration between two sampling */
> @@ -63,6 +68,7 @@ struct DirtyRateStat {
>       int64_t dirty_rate; /* dirty rate in MB/s */
>       int64_t start_time; /* calculation start time in units of second */
>       int64_t calc_time; /* time duration of two sampling in units of second */
> +    uint64_t sample_pages; /* sample pages per GB */
>   };
>   
>   void *get_dirtyrate_thread(void *arg);
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 9bf0bc4..868a867 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1741,6 +1741,9 @@
>   #
>   # @calc-time: time in units of second for sample dirty pages
>   #
> +# @sample-pages: page count per GB for sample dirty pages
> +#                the default value is 512
> +#
>   # Since: 5.2
>   #
>   ##
> @@ -1748,7 +1751,8 @@
>     'data': {'*dirty-rate': 'int64',
>              'status': 'DirtyRateStatus',
>              'start-time': 'int64',
> -           'calc-time': 'int64'} }
> +           'calc-time': 'int64',
> +           'sample-pages': 'uint64'} }
>   
>   ##
>   # @calc-dirty-rate:
> @@ -1757,13 +1761,16 @@
>   #
>   # @calc-time: time in units of second for sample dirty pages
>   #
> +# @sample-pages: page count per GB for sample dirty pages
> +#                the default value is 512
> +#
>   # Since: 5.2
>   #
>   # Example:
> -#   {"command": "calc-dirty-rate", "data": {"calc-time": 1} }
> +#   {"command": "calc-dirty-rate", "data": {"calc-time": 1, 'sample-pages': 512} }
>   #
>   ##
> -{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64'} }
> +{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64', '*sample-pages': 'int'} }
>   
>   ##
>   # @query-dirty-rate:
> 

Ping - Hi, What would you think about this patch?

-- 
Best regard

Hyman Huang(黄勇)

Re: [PATCH v1] migration/dirtyrate: make sample page count configurable

[Dr. David Alan Gilbert]

* huangy81@chinatelecom.cn (huangy81@chinatelecom.cn) wrote:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> introduce optional sample-pages argument in calc-dirty-rate,
> making sample page count per GB configurable so that more
> accurate dirtyrate can be calculated.
> 
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>

I think this is mostly OK, so:

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

I'm curious; do you find that increasing the sample count is making much
difference to the results?

Dave

> ---
>  migration/dirtyrate.c | 32 ++++++++++++++++++++++++++++----
>  migration/dirtyrate.h |  8 +++++++-
>  qapi/migration.json   | 13 ++++++++++---
>  3 files changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index ccb9814..43a531c 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -48,6 +48,16 @@ static bool is_sample_period_valid(int64_t sec)
>      return true;
>  }
>  
> +static bool is_sample_pages_valid(int64_t pages)
> +{
> +    if (pages < MIN_SAMPLE_PAGE_COUNT ||
> +        pages > MAX_SAMPLE_PAGE_COUNT) {
> +        return false;
> +    }
> +
> +    return true;

David is right that could be simplified; but it's OK.

> +}
> +
>  static int dirtyrate_set_state(int *state, int old_state, int new_state)
>  {
>      assert(new_state < DIRTY_RATE_STATUS__MAX);
> @@ -72,13 +82,15 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
>      info->status = CalculatingState;
>      info->start_time = DirtyStat.start_time;
>      info->calc_time = DirtyStat.calc_time;
> +    info->sample_pages = DirtyStat.sample_pages;
>  
>      trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
>  
>      return info;
>  }
>  
> -static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time)
> +static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time,
> +                                uint64_t sample_pages)
>  {
>      DirtyStat.total_dirty_samples = 0;
>      DirtyStat.total_sample_count = 0;
> @@ -86,6 +98,7 @@ static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time)
>      DirtyStat.dirty_rate = -1;
>      DirtyStat.start_time = start_time;
>      DirtyStat.calc_time = calc_time;
> +    DirtyStat.sample_pages = sample_pages;
>  }
>  
>  static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
> @@ -361,6 +374,7 @@ void *get_dirtyrate_thread(void *arg)
>      int ret;
>      int64_t start_time;
>      int64_t calc_time;
> +    uint64_t sample_pages;
>  
>      ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_UNSTARTED,
>                                DIRTY_RATE_STATUS_MEASURING);
> @@ -371,7 +385,8 @@ void *get_dirtyrate_thread(void *arg)
>  
>      start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) / 1000;
>      calc_time = config.sample_period_seconds;
> -    init_dirtyrate_stat(start_time, calc_time);
> +    sample_pages = config.sample_pages_per_gigabytes;
> +    init_dirtyrate_stat(start_time, calc_time, sample_pages);
>  
>      calculate_dirtyrate(config);
>  
> @@ -383,7 +398,8 @@ void *get_dirtyrate_thread(void *arg)
>      return NULL;
>  }
>  
> -void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
> +void qmp_calc_dirty_rate(int64_t calc_time, bool has_sample_pages,
> +                         int64_t sample_pages, Error **errp)
>  {
>      static struct DirtyRateConfig config;
>      QemuThread thread;
> @@ -404,6 +420,13 @@ void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
>          return;
>      }
>  
> +    if (has_sample_pages && !is_sample_pages_valid(sample_pages)) {
> +        error_setg(errp, "sample-pages is out of range[%d, %d].",
> +                         MIN_SAMPLE_PAGE_COUNT,
> +                         MAX_SAMPLE_PAGE_COUNT);
> +        return;
> +    }
> +
>      /*
>       * Init calculation state as unstarted.
>       */
> @@ -415,7 +438,8 @@ void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
>      }
>  
>      config.sample_period_seconds = calc_time;
> -    config.sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
> +    config.sample_pages_per_gigabytes =
> +        has_sample_pages ? sample_pages : DIRTYRATE_DEFAULT_SAMPLE_PAGES;
>      qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
>                         (void *)&config, QEMU_THREAD_DETACHED);
>  }
> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
> index 6ec4295..5f987e2 100644
> --- a/migration/dirtyrate.h
> +++ b/migration/dirtyrate.h
> @@ -15,7 +15,6 @@
>  
>  /*
>   * Sample 512 pages per GB as default.
> - * TODO: Make it configurable.
>   */
>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
>  
> @@ -35,6 +34,12 @@
>  #define MIN_FETCH_DIRTYRATE_TIME_SEC              1
>  #define MAX_FETCH_DIRTYRATE_TIME_SEC              60
>  
> +/*
> + * Take 128 as minimum for sample dirty pages
> + */
> +#define MIN_SAMPLE_PAGE_COUNT                     128
> +#define MAX_SAMPLE_PAGE_COUNT                     4096
> +
>  struct DirtyRateConfig {
>      uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
>      int64_t sample_period_seconds; /* time duration between two sampling */
> @@ -63,6 +68,7 @@ struct DirtyRateStat {
>      int64_t dirty_rate; /* dirty rate in MB/s */
>      int64_t start_time; /* calculation start time in units of second */
>      int64_t calc_time; /* time duration of two sampling in units of second */
> +    uint64_t sample_pages; /* sample pages per GB */
>  };
>  
>  void *get_dirtyrate_thread(void *arg);
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 9bf0bc4..868a867 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1741,6 +1741,9 @@
>  #
>  # @calc-time: time in units of second for sample dirty pages
>  #
> +# @sample-pages: page count per GB for sample dirty pages
> +#                the default value is 512

That needs a (since 6.1) adding; I can do that during the merge.

> +#
>  # Since: 5.2
>  #
>  ##
> @@ -1748,7 +1751,8 @@
>    'data': {'*dirty-rate': 'int64',
>             'status': 'DirtyRateStatus',
>             'start-time': 'int64',
> -           'calc-time': 'int64'} }
> +           'calc-time': 'int64',
> +           'sample-pages': 'uint64'} }
>  
>  ##
>  # @calc-dirty-rate:
> @@ -1757,13 +1761,16 @@
>  #
>  # @calc-time: time in units of second for sample dirty pages
>  #
> +# @sample-pages: page count per GB for sample dirty pages
> +#                the default value is 512

That needs a (since 6.1) adding; I can do that during the merge; as does
that.

> +#
>  # Since: 5.2
>  #
>  # Example:
> -#   {"command": "calc-dirty-rate", "data": {"calc-time": 1} }
> +#   {"command": "calc-dirty-rate", "data": {"calc-time": 1, 'sample-pages': 512} }
>  #
>  ##
> -{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64'} }
> +{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64', '*sample-pages': 'int'} }
>  
>  ##
>  # @query-dirty-rate:
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v1] migration/dirtyrate: make sample page count configurable

[Hyman]

在 2021/5/6 21:29, Dr. David Alan Gilbert 写道:
> * huangy81@chinatelecom.cn (huangy81@chinatelecom.cn) wrote:
>> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>>
>> introduce optional sample-pages argument in calc-dirty-rate,
>> making sample page count per GB configurable so that more
>> accurate dirtyrate can be calculated.
>>
>> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> I think this is mostly OK, so:
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> I'm curious; do you find that increasing the sample count is making much
> difference to the results?
According to the test using stress tool in tests/migration, i havn't 
found that indeed. :(

 From my point of view, this depend on guest memory dirty type:

Using stress tool in tests/migration as the overload in guest, which 
dirty fixed memory section of 1G every 0.39 seconds with 1 vcpus, if we 
set the calculate time as 1s, the default value of sample page(512) 
makes result accurate enough, increasing sample count almost make no 
difference indeed.

But if we spread memory stress across more vcpus and shrink the memory 
section, dirting random-allocated memory section, we can find the 
results not accurate if we just adjust the calculate time and use the 
default value of sample page count.

Taking all types of workload in guest into account, it is resonable to 
provide one more method to make results accurate theoretically, this may 
benefit those who works for accelerate migration base on the analysis of 
dirty rate.

-- 
Best regard

Hyman Huang(黄勇)

> 
> Dave
> 
>> ---
>>   migration/dirtyrate.c | 32 ++++++++++++++++++++++++++++----
>>   migration/dirtyrate.h |  8 +++++++-
>>   qapi/migration.json   | 13 ++++++++++---
>>   3 files changed, 45 insertions(+), 8 deletions(-)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index ccb9814..43a531c 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -48,6 +48,16 @@ static bool is_sample_period_valid(int64_t sec)
>>       return true;
>>   }
>>   
>> +static bool is_sample_pages_valid(int64_t pages)
>> +{
>> +    if (pages < MIN_SAMPLE_PAGE_COUNT ||
>> +        pages > MAX_SAMPLE_PAGE_COUNT) {
>> +        return false;
>> +    }
>> +
>> +    return true;
> 
> David is right that could be simplified; but it's OK.
> 
>> +}
>> +
>>   static int dirtyrate_set_state(int *state, int old_state, int new_state)
>>   {
>>       assert(new_state < DIRTY_RATE_STATUS__MAX);
>> @@ -72,13 +82,15 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
>>       info->status = CalculatingState;
>>       info->start_time = DirtyStat.start_time;
>>       info->calc_time = DirtyStat.calc_time;
>> +    info->sample_pages = DirtyStat.sample_pages;
>>   
>>       trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
>>   
>>       return info;
>>   }
>>   
>> -static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time)
>> +static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time,
>> +                                uint64_t sample_pages)
>>   {
>>       DirtyStat.total_dirty_samples = 0;
>>       DirtyStat.total_sample_count = 0;
>> @@ -86,6 +98,7 @@ static void init_dirtyrate_stat(int64_t start_time, int64_t calc_time)
>>       DirtyStat.dirty_rate = -1;
>>       DirtyStat.start_time = start_time;
>>       DirtyStat.calc_time = calc_time;
>> +    DirtyStat.sample_pages = sample_pages;
>>   }
>>   
>>   static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
>> @@ -361,6 +374,7 @@ void *get_dirtyrate_thread(void *arg)
>>       int ret;
>>       int64_t start_time;
>>       int64_t calc_time;
>> +    uint64_t sample_pages;
>>   
>>       ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_UNSTARTED,
>>                                 DIRTY_RATE_STATUS_MEASURING);
>> @@ -371,7 +385,8 @@ void *get_dirtyrate_thread(void *arg)
>>   
>>       start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) / 1000;
>>       calc_time = config.sample_period_seconds;
>> -    init_dirtyrate_stat(start_time, calc_time);
>> +    sample_pages = config.sample_pages_per_gigabytes;
>> +    init_dirtyrate_stat(start_time, calc_time, sample_pages);
>>   
>>       calculate_dirtyrate(config);
>>   
>> @@ -383,7 +398,8 @@ void *get_dirtyrate_thread(void *arg)
>>       return NULL;
>>   }
>>   
>> -void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
>> +void qmp_calc_dirty_rate(int64_t calc_time, bool has_sample_pages,
>> +                         int64_t sample_pages, Error **errp)
>>   {
>>       static struct DirtyRateConfig config;
>>       QemuThread thread;
>> @@ -404,6 +420,13 @@ void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
>>           return;
>>       }
>>   
>> +    if (has_sample_pages && !is_sample_pages_valid(sample_pages)) {
>> +        error_setg(errp, "sample-pages is out of range[%d, %d].",
>> +                         MIN_SAMPLE_PAGE_COUNT,
>> +                         MAX_SAMPLE_PAGE_COUNT);
>> +        return;
>> +    }
>> +
>>       /*
>>        * Init calculation state as unstarted.
>>        */
>> @@ -415,7 +438,8 @@ void qmp_calc_dirty_rate(int64_t calc_time, Error **errp)
>>       }
>>   
>>       config.sample_period_seconds = calc_time;
>> -    config.sample_pages_per_gigabytes = DIRTYRATE_DEFAULT_SAMPLE_PAGES;
>> +    config.sample_pages_per_gigabytes =
>> +        has_sample_pages ? sample_pages : DIRTYRATE_DEFAULT_SAMPLE_PAGES;
>>       qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
>>                          (void *)&config, QEMU_THREAD_DETACHED);
>>   }
>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>> index 6ec4295..5f987e2 100644
>> --- a/migration/dirtyrate.h
>> +++ b/migration/dirtyrate.h
>> @@ -15,7 +15,6 @@
>>   
>>   /*
>>    * Sample 512 pages per GB as default.
>> - * TODO: Make it configurable.
>>    */
>>   #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            512
>>   
>> @@ -35,6 +34,12 @@
>>   #define MIN_FETCH_DIRTYRATE_TIME_SEC              1
>>   #define MAX_FETCH_DIRTYRATE_TIME_SEC              60
>>   
>> +/*
>> + * Take 128 as minimum for sample dirty pages
>> + */
>> +#define MIN_SAMPLE_PAGE_COUNT                     128
>> +#define MAX_SAMPLE_PAGE_COUNT                     4096
>> +
>>   struct DirtyRateConfig {
>>       uint64_t sample_pages_per_gigabytes; /* sample pages per GB */
>>       int64_t sample_period_seconds; /* time duration between two sampling */
>> @@ -63,6 +68,7 @@ struct DirtyRateStat {
>>       int64_t dirty_rate; /* dirty rate in MB/s */
>>       int64_t start_time; /* calculation start time in units of second */
>>       int64_t calc_time; /* time duration of two sampling in units of second */
>> +    uint64_t sample_pages; /* sample pages per GB */
>>   };
>>   
>>   void *get_dirtyrate_thread(void *arg);
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 9bf0bc4..868a867 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1741,6 +1741,9 @@
>>   #
>>   # @calc-time: time in units of second for sample dirty pages
>>   #
>> +# @sample-pages: page count per GB for sample dirty pages
>> +#                the default value is 512
> 
> That needs a (since 6.1) adding; I can do that during the merge.
> 
>> +#
>>   # Since: 5.2
>>   #
>>   ##
>> @@ -1748,7 +1751,8 @@
>>     'data': {'*dirty-rate': 'int64',
>>              'status': 'DirtyRateStatus',
>>              'start-time': 'int64',
>> -           'calc-time': 'int64'} }
>> +           'calc-time': 'int64',
>> +           'sample-pages': 'uint64'} }
>>   
>>   ##
>>   # @calc-dirty-rate:
>> @@ -1757,13 +1761,16 @@
>>   #
>>   # @calc-time: time in units of second for sample dirty pages
>>   #
>> +# @sample-pages: page count per GB for sample dirty pages
>> +#                the default value is 512
> 
> That needs a (since 6.1) adding; I can do that during the merge; as does
> that.
> 
>> +#
>>   # Since: 5.2
>>   #
>>   # Example:
>> -#   {"command": "calc-dirty-rate", "data": {"calc-time": 1} }
>> +#   {"command": "calc-dirty-rate", "data": {"calc-time": 1, 'sample-pages': 512} }
>>   #
>>   ##
>> -{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64'} }
>> +{ 'command': 'calc-dirty-rate', 'data': {'calc-time': 'int64', '*sample-pages': 'int'} }
>>   
>>   ##
>>   # @query-dirty-rate:
>> -- 
>> 1.8.3.1
>>