qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH QEMU v8 1/9] softmmu/dirtylimit: Add parameter check for hmp "set_vcpu_dirty_limit"
  2023-07-07  4:10 [PATCH QEMU v8 0/9] migration: introduce dirtylimit capability ~hyman
@ 2022-11-18  2:08 ` ~hyman
  2023-06-07 13:32 ` [PATCH QEMU v8 2/9] qapi/migration: Introduce x-vcpu-dirty-limit-period parameter ~hyman
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 20+ messages in thread
From: ~hyman @ 2022-11-18  2:08 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Xu, Paolo Bonzini, Juan Quintela, Dr. David Alan Gilbert,
	Eric Blake, Markus Armbruster, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé,
	Hyman Huang(黄勇)

From: Hyman Huang(黄勇) <yong.huang@smartx.com>

dirty_rate paraemter of hmp command "set_vcpu_dirty_limit" is invalid
if less than 0, so add parameter check for it.

Note that this patch also delete the unsolicited help message and
clean up the code.

Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 softmmu/dirtylimit.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
index 015a9038d1..5c12d26d49 100644
--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -515,14 +515,15 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict)
     int64_t cpu_index = qdict_get_try_int(qdict, "cpu_index", -1);
     Error *err = NULL;
 
-    qmp_set_vcpu_dirty_limit(!!(cpu_index != -1), cpu_index, dirty_rate, &err);
-    if (err) {
-        hmp_handle_error(mon, err);
-        return;
+    if (dirty_rate < 0) {
+        error_setg(&err, "invalid dirty page limit %ld", dirty_rate);
+        goto out;
     }
 
-    monitor_printf(mon, "[Please use 'info vcpu_dirty_limit' to query "
-                   "dirty limit for virtual CPU]\n");
+    qmp_set_vcpu_dirty_limit(!!(cpu_index != -1), cpu_index, dirty_rate, &err);
+
+out:
+    hmp_handle_error(mon, err);
 }
 
 static struct DirtyLimitInfo *dirtylimit_query_vcpu(int cpu_index)
-- 
2.38.5



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH QEMU v8 2/9] qapi/migration: Introduce x-vcpu-dirty-limit-period parameter
  2023-07-07  4:10 [PATCH QEMU v8 0/9] migration: introduce dirtylimit capability ~hyman
  2022-11-18  2:08 ` [PATCH QEMU v8 1/9] softmmu/dirtylimit: Add parameter check for hmp "set_vcpu_dirty_limit" ~hyman
@ 2023-06-07 13:32 ` ~hyman
  2023-07-13 12:32   ` Markus Armbruster
  2023-06-07 14:58 ` [PATCH QEMU v8 3/9] qapi/migration: Introduce vcpu-dirty-limit parameters ~hyman
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 20+ messages in thread
From: ~hyman @ 2023-06-07 13:32 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Xu, Paolo Bonzini, Juan Quintela, Dr. David Alan Gilbert,
	Eric Blake, Markus Armbruster, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé,
	Hyman Huang(黄勇)

From: Hyman Huang(黄勇) <yong.huang@smartx.com>

Introduce "x-vcpu-dirty-limit-period" migration experimental
parameter, which is in the range of 1 to 1000ms and used to
make dirtyrate calculation period configurable.

Currently with the "x-vcpu-dirty-limit-period" varies, the
total time of live migration changes, test results show the
optimal value of "x-vcpu-dirty-limit-period" ranges from
500ms to 1000 ms. "x-vcpu-dirty-limit-period" should be made
stable once it proves best value can not be determined with
developer's experiments.

Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-hmp-cmds.c |  8 ++++++++
 migration/options.c            | 28 +++++++++++++++++++++++++++
 qapi/migration.json            | 35 +++++++++++++++++++++++++++-------
 3 files changed, 64 insertions(+), 7 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 9885d7c9f7..352e9ec716 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -364,6 +364,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
                 }
             }
         }
+
+        monitor_printf(mon, "%s: %" PRIu64 " ms\n",
+        MigrationParameter_str(MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD),
+        params->x_vcpu_dirty_limit_period);
     }
 
     qapi_free_MigrationParameters(params);
@@ -620,6 +624,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         error_setg(&err, "The block-bitmap-mapping parameter can only be set "
                    "through QMP");
         break;
+    case MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD:
+        p->has_x_vcpu_dirty_limit_period = true;
+        visit_type_size(v, param, &p->x_vcpu_dirty_limit_period, &err);
+        break;
     default:
         assert(0);
     }
diff --git a/migration/options.c b/migration/options.c
index 5a9505adf7..1de63ba775 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -80,6 +80,8 @@
 #define DEFINE_PROP_MIG_CAP(name, x)             \
     DEFINE_PROP_BOOL(name, MigrationState, capabilities[x], false)
 
+#define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD     1000    /* milliseconds */
+
 Property migration_properties[] = {
     DEFINE_PROP_BOOL("store-global-state", MigrationState,
                      store_global_state, true),
@@ -163,6 +165,9 @@ Property migration_properties[] = {
     DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds),
     DEFINE_PROP_STRING("tls-hostname", MigrationState, parameters.tls_hostname),
     DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz),
+    DEFINE_PROP_UINT64("x-vcpu-dirty-limit-period", MigrationState,
+                       parameters.x_vcpu_dirty_limit_period,
+                       DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD),
 
     /* Migration capabilities */
     DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
@@ -908,6 +913,9 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
                        s->parameters.block_bitmap_mapping);
     }
 
+    params->has_x_vcpu_dirty_limit_period = true;
+    params->x_vcpu_dirty_limit_period = s->parameters.x_vcpu_dirty_limit_period;
+
     return params;
 }
 
@@ -940,6 +948,7 @@ void migrate_params_init(MigrationParameters *params)
     params->has_announce_max = true;
     params->has_announce_rounds = true;
     params->has_announce_step = true;
+    params->has_x_vcpu_dirty_limit_period = true;
 }
 
 /*
@@ -1100,6 +1109,15 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
     }
 #endif
 
+    if (params->has_x_vcpu_dirty_limit_period &&
+        (params->x_vcpu_dirty_limit_period < 1 ||
+         params->x_vcpu_dirty_limit_period > 1000)) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   "x-vcpu-dirty-limit-period",
+                   "a value between 1 and 1000");
+        return false;
+    }
+
     return true;
 }
 
@@ -1199,6 +1217,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
         dest->has_block_bitmap_mapping = true;
         dest->block_bitmap_mapping = params->block_bitmap_mapping;
     }
+
+    if (params->has_x_vcpu_dirty_limit_period) {
+        dest->x_vcpu_dirty_limit_period =
+            params->x_vcpu_dirty_limit_period;
+    }
 }
 
 static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
@@ -1317,6 +1340,11 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
             QAPI_CLONE(BitmapMigrationNodeAliasList,
                        params->block_bitmap_mapping);
     }
+
+    if (params->has_x_vcpu_dirty_limit_period) {
+        s->parameters.x_vcpu_dirty_limit_period =
+            params->x_vcpu_dirty_limit_period;
+    }
 }
 
 void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
diff --git a/qapi/migration.json b/qapi/migration.json
index 47dfef0278..2041d336d5 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -789,9 +789,14 @@
 #     Nodes are mapped to their block device name if there is one, and
 #     to their node name otherwise.  (Since 5.2)
 #
+# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty
+#     limit during live migration. Should be in the range 1 to 1000ms,
+#     defaults to 1000ms. (Since 8.1)
+#
 # Features:
 #
-# @unstable: Member @x-checkpoint-delay is experimental.
+# @unstable: Members @x-checkpoint-delay and
+#     @x-vcpu-dirty-limit-period are experimental.
 #
 # Since: 2.4
 ##
@@ -809,8 +814,10 @@
            'multifd-channels',
            'xbzrle-cache-size', 'max-postcopy-bandwidth',
            'max-cpu-throttle', 'multifd-compression',
-           'multifd-zlib-level' ,'multifd-zstd-level',
-           'block-bitmap-mapping' ] }
+           'multifd-zlib-level', 'multifd-zstd-level',
+           'block-bitmap-mapping',
+           { 'name': 'x-vcpu-dirty-limit-period',
+             'features': ['unstable'] } ] }
 
 ##
 # @MigrateSetParameters:
@@ -945,9 +952,14 @@
 #     Nodes are mapped to their block device name if there is one, and
 #     to their node name otherwise.  (Since 5.2)
 #
+# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty
+#     limit during live migration. Should be in the range 1 to 1000ms,
+#     defaults to 1000ms. (Since 8.1)
+#
 # Features:
 #
-# @unstable: Member @x-checkpoint-delay is experimental.
+# @unstable: Members @x-checkpoint-delay and
+#     @x-vcpu-dirty-limit-period are experimental.
 #
 # TODO: either fuse back into MigrationParameters, or make
 #     MigrationParameters members mandatory
@@ -982,7 +994,9 @@
             '*multifd-compression': 'MultiFDCompression',
             '*multifd-zlib-level': 'uint8',
             '*multifd-zstd-level': 'uint8',
-            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
+            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
+            '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
+                                            'features': [ 'unstable' ] } } }
 
 ##
 # @migrate-set-parameters:
@@ -1137,9 +1151,14 @@
 #     Nodes are mapped to their block device name if there is one, and
 #     to their node name otherwise.  (Since 5.2)
 #
+# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty
+#     limit during live migration. Should be in the range 1 to 1000ms,
+#     defaults to 1000ms. (Since 8.1)
+#
 # Features:
 #
-# @unstable: Member @x-checkpoint-delay is experimental.
+# @unstable: Members @x-checkpoint-delay and
+#     @x-vcpu-dirty-limit-period are experimental.
 #
 # Since: 2.4
 ##
@@ -1171,7 +1190,9 @@
             '*multifd-compression': 'MultiFDCompression',
             '*multifd-zlib-level': 'uint8',
             '*multifd-zstd-level': 'uint8',
-            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
+            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
+            '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
+                                            'features': [ 'unstable' ] } } }
 
 ##
 # @query-migrate-parameters:
-- 
2.38.5



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH QEMU v8 3/9] qapi/migration: Introduce vcpu-dirty-limit parameters
  2023-07-07  4:10 [PATCH QEMU v8 0/9] migration: introduce dirtylimit capability ~hyman
  2022-11-18  2:08 ` [PATCH QEMU v8 1/9] softmmu/dirtylimit: Add parameter check for hmp "set_vcpu_dirty_limit" ~hyman
  2023-06-07 13:32 ` [PATCH QEMU v8 2/9] qapi/migration: Introduce x-vcpu-dirty-limit-period parameter ~hyman
@ 2023-06-07 14:58 ` ~hyman
  2023-06-07 15:30 ` [PATCH QEMU v8 4/9] migration: Introduce dirty-limit capability ~hyman
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 20+ messages in thread
From: ~hyman @ 2023-06-07 14:58 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Xu, Paolo Bonzini, Juan Quintela, Dr. David Alan Gilbert,
	Eric Blake, Markus Armbruster, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé,
	Hyman Huang(黄勇)

From: Hyman Huang(黄勇) <yong.huang@smartx.com>

Introduce "vcpu-dirty-limit" migration parameter used
to limit dirty page rate during live migration.

"vcpu-dirty-limit" and "x-vcpu-dirty-limit-period" are
two dirty-limit-related migration parameters, which can
be set before and during live migration by qmp
migrate-set-parameters.

This two parameters are used to help implement the dirty
page rate limit algo of migration.

Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
Acked-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-hmp-cmds.c |  8 ++++++++
 migration/options.c            | 21 +++++++++++++++++++++
 qapi/migration.json            | 18 +++++++++++++++---
 3 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 352e9ec716..35e8020bbf 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -368,6 +368,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: %" PRIu64 " ms\n",
         MigrationParameter_str(MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD),
         params->x_vcpu_dirty_limit_period);
+
+        monitor_printf(mon, "%s: %" PRIu64 " MB/s\n",
+            MigrationParameter_str(MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT),
+            params->vcpu_dirty_limit);
     }
 
     qapi_free_MigrationParameters(params);
@@ -628,6 +632,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         p->has_x_vcpu_dirty_limit_period = true;
         visit_type_size(v, param, &p->x_vcpu_dirty_limit_period, &err);
         break;
+    case MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT:
+        p->has_vcpu_dirty_limit = true;
+        visit_type_size(v, param, &p->vcpu_dirty_limit, &err);
+        break;
     default:
         assert(0);
     }
diff --git a/migration/options.c b/migration/options.c
index 1de63ba775..7d2d98830e 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -81,6 +81,7 @@
     DEFINE_PROP_BOOL(name, MigrationState, capabilities[x], false)
 
 #define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD     1000    /* milliseconds */
+#define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT            1       /* MB/s */
 
 Property migration_properties[] = {
     DEFINE_PROP_BOOL("store-global-state", MigrationState,
@@ -168,6 +169,9 @@ Property migration_properties[] = {
     DEFINE_PROP_UINT64("x-vcpu-dirty-limit-period", MigrationState,
                        parameters.x_vcpu_dirty_limit_period,
                        DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD),
+    DEFINE_PROP_UINT64("vcpu-dirty-limit", MigrationState,
+                       parameters.vcpu_dirty_limit,
+                       DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT),
 
     /* Migration capabilities */
     DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
@@ -915,6 +919,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
 
     params->has_x_vcpu_dirty_limit_period = true;
     params->x_vcpu_dirty_limit_period = s->parameters.x_vcpu_dirty_limit_period;
+    params->has_vcpu_dirty_limit = true;
+    params->vcpu_dirty_limit = s->parameters.vcpu_dirty_limit;
 
     return params;
 }
@@ -949,6 +955,7 @@ void migrate_params_init(MigrationParameters *params)
     params->has_announce_rounds = true;
     params->has_announce_step = true;
     params->has_x_vcpu_dirty_limit_period = true;
+    params->has_vcpu_dirty_limit = true;
 }
 
 /*
@@ -1118,6 +1125,14 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
         return false;
     }
 
+    if (params->has_vcpu_dirty_limit &&
+        (params->vcpu_dirty_limit < 1)) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   "vcpu_dirty_limit",
+                   "is invalid, it must greater then 1 MB/s");
+        return false;
+    }
+
     return true;
 }
 
@@ -1222,6 +1237,9 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
         dest->x_vcpu_dirty_limit_period =
             params->x_vcpu_dirty_limit_period;
     }
+    if (params->has_vcpu_dirty_limit) {
+        dest->vcpu_dirty_limit = params->vcpu_dirty_limit;
+    }
 }
 
 static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
@@ -1345,6 +1363,9 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
         s->parameters.x_vcpu_dirty_limit_period =
             params->x_vcpu_dirty_limit_period;
     }
+    if (params->has_vcpu_dirty_limit) {
+        s->parameters.vcpu_dirty_limit = params->vcpu_dirty_limit;
+    }
 }
 
 void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
diff --git a/qapi/migration.json b/qapi/migration.json
index 2041d336d5..e43371955a 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -793,6 +793,9 @@
 #     limit during live migration. Should be in the range 1 to 1000ms,
 #     defaults to 1000ms. (Since 8.1)
 #
+# @vcpu-dirty-limit: Dirty page rate limit (MB/s) during live
+#     migration, defaults to 1. (Since 8.1)
+#
 # Features:
 #
 # @unstable: Members @x-checkpoint-delay and
@@ -817,7 +820,8 @@
            'multifd-zlib-level', 'multifd-zstd-level',
            'block-bitmap-mapping',
            { 'name': 'x-vcpu-dirty-limit-period',
-             'features': ['unstable'] } ] }
+             'features': ['unstable'] },
+           'vcpu-dirty-limit'] }
 
 ##
 # @MigrateSetParameters:
@@ -956,6 +960,9 @@
 #     limit during live migration. Should be in the range 1 to 1000ms,
 #     defaults to 1000ms. (Since 8.1)
 #
+# @vcpu-dirty-limit: Dirty page rate limit (MB/s) during live
+#     migration, defaults to 1. (Since 8.1)
+#
 # Features:
 #
 # @unstable: Members @x-checkpoint-delay and
@@ -996,7 +1003,8 @@
             '*multifd-zstd-level': 'uint8',
             '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
             '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
-                                            'features': [ 'unstable' ] } } }
+                                            'features': [ 'unstable' ] },
+            '*vcpu-dirty-limit': 'uint64'} }
 
 ##
 # @migrate-set-parameters:
@@ -1155,6 +1163,9 @@
 #     limit during live migration. Should be in the range 1 to 1000ms,
 #     defaults to 1000ms. (Since 8.1)
 #
+# @vcpu-dirty-limit: Dirty page rate limit (MB/s) during live
+#     migration, defaults to 1. (Since 8.1)
+#
 # Features:
 #
 # @unstable: Members @x-checkpoint-delay and
@@ -1192,7 +1203,8 @@
             '*multifd-zstd-level': 'uint8',
             '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
             '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
-                                            'features': [ 'unstable' ] } } }
+                                            'features': [ 'unstable' ] },
+            '*vcpu-dirty-limit': 'uint64'} }
 
 ##
 # @query-migrate-parameters:
-- 
2.38.5



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH QEMU v8 4/9] migration: Introduce dirty-limit capability
  2023-07-07  4:10 [PATCH QEMU v8 0/9] migration: introduce dirtylimit capability ~hyman
                   ` (2 preceding siblings ...)
  2023-06-07 14:58 ` [PATCH QEMU v8 3/9] qapi/migration: Introduce vcpu-dirty-limit parameters ~hyman
@ 2023-06-07 15:30 ` ~hyman
  2023-07-13 12:44   ` Markus Armbruster
  2023-06-07 15:32 ` [PATCH QEMU v8 5/9] migration: Refactor auto-converge capability logic ~hyman
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 20+ messages in thread
From: ~hyman @ 2023-06-07 15:30 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Xu, Paolo Bonzini, Juan Quintela, Dr. David Alan Gilbert,
	Eric Blake, Markus Armbruster, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé,
	Hyman Huang(黄勇)

From: Hyman Huang(黄勇) <yong.huang@smartx.com>

Introduce migration dirty-limit capability, which can
be turned on before live migration and limit dirty
page rate durty live migration.

Introduce migrate_dirty_limit function to help check
if dirty-limit capability enabled during live migration.

Meanwhile, refactor vcpu_dirty_rate_stat_collect
so that period can be configured instead of hardcoded.

dirty-limit capability is kind of like auto-converge
but using dirty limit instead of traditional cpu-throttle
to throttle guest down. To enable this feature, turn on
the dirty-limit capability before live migration using
migrate-set-capabilities, and set the parameters
"x-vcpu-dirty-limit-period", "vcpu-dirty-limit" suitably
to speed up convergence.

Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
Acked-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 migration/options.c  | 24 ++++++++++++++++++++++++
 migration/options.h  |  1 +
 qapi/migration.json  | 12 +++++++++++-
 softmmu/dirtylimit.c | 12 +++++++++++-
 4 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index 7d2d98830e..631c12cf32 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -27,6 +27,7 @@
 #include "qemu-file.h"
 #include "ram.h"
 #include "options.h"
+#include "sysemu/kvm.h"
 
 /* Maximum migrate downtime set to 2000 seconds */
 #define MAX_MIGRATE_DOWNTIME_SECONDS 2000
@@ -196,6 +197,8 @@ Property migration_properties[] = {
 #endif
     DEFINE_PROP_MIG_CAP("x-switchover-ack",
                         MIGRATION_CAPABILITY_SWITCHOVER_ACK),
+    DEFINE_PROP_MIG_CAP("x-dirty-limit",
+                        MIGRATION_CAPABILITY_DIRTY_LIMIT),
 
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -242,6 +245,13 @@ bool migrate_dirty_bitmaps(void)
     return s->capabilities[MIGRATION_CAPABILITY_DIRTY_BITMAPS];
 }
 
+bool migrate_dirty_limit(void)
+{
+    MigrationState *s = migrate_get_current();
+
+    return s->capabilities[MIGRATION_CAPABILITY_DIRTY_LIMIT];
+}
+
 bool migrate_events(void)
 {
     MigrationState *s = migrate_get_current();
@@ -573,6 +583,20 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
         }
     }
 
+    if (new_caps[MIGRATION_CAPABILITY_DIRTY_LIMIT]) {
+        if (new_caps[MIGRATION_CAPABILITY_AUTO_CONVERGE]) {
+            error_setg(errp, "dirty-limit conflicts with auto-converge"
+                       " either of then available currently");
+            return false;
+        }
+
+        if (!kvm_enabled() || !kvm_dirty_ring_enabled()) {
+            error_setg(errp, "dirty-limit requires KVM with accelerator"
+                       " property 'dirty-ring-size' set");
+            return false;
+        }
+    }
+
     return true;
 }
 
diff --git a/migration/options.h b/migration/options.h
index 9aaf363322..b5a950d4e4 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -24,6 +24,7 @@ extern Property migration_properties[];
 /* capabilities */
 
 bool migrate_auto_converge(void);
+bool migrate_dirty_limit(void);
 bool migrate_background_snapshot(void);
 bool migrate_block(void);
 bool migrate_colo(void);
diff --git a/qapi/migration.json b/qapi/migration.json
index e43371955a..031832cde5 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -497,6 +497,15 @@
 #     are present.  'return-path' capability must be enabled to use
 #     it.  (since 8.1)
 #
+# @dirty-limit: If enabled, migration will use the dirty-limit
+#     algorithm to throttle down guest instead of auto-converge
+#     algorithm. This algorithm only works when vCPU's dirtyrate
+#     greater than 'vcpu-dirty-limit', read processes in guest os
+#     aren't penalized any more, so the algorithm can improve
+#     performance of vCPU during live migration. This is an optional
+#     performance feature and should not affect the correctness of the
+#     existing auto-converge algorithm. (since 8.1)
+#
 # Features:
 #
 # @unstable: Members @x-colo and @x-ignore-shared are experimental.
@@ -512,7 +521,8 @@
            'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
            { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
            'validate-uuid', 'background-snapshot',
-           'zero-copy-send', 'postcopy-preempt', 'switchover-ack'] }
+           'zero-copy-send', 'postcopy-preempt', 'switchover-ack',
+           'dirty-limit'] }
 
 ##
 # @MigrationCapabilityStatus:
diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
index 5c12d26d49..953ef934bc 100644
--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -24,6 +24,9 @@
 #include "hw/boards.h"
 #include "sysemu/kvm.h"
 #include "trace.h"
+#include "migration/misc.h"
+#include "migration/migration.h"
+#include "migration/options.h"
 
 /*
  * Dirtylimit stop working if dirty page rate error
@@ -75,11 +78,18 @@ static bool dirtylimit_quit;
 
 static void vcpu_dirty_rate_stat_collect(void)
 {
+    MigrationState *s = migrate_get_current();
     VcpuStat stat;
     int i = 0;
+    int64_t period = DIRTYLIMIT_CALC_TIME_MS;
+
+    if (migrate_dirty_limit() &&
+        migration_is_active(s)) {
+        period = s->parameters.x_vcpu_dirty_limit_period;
+    }
 
     /* calculate vcpu dirtyrate */
-    vcpu_calculate_dirtyrate(DIRTYLIMIT_CALC_TIME_MS,
+    vcpu_calculate_dirtyrate(period,
                              &stat,
                              GLOBAL_DIRTY_LIMIT,
                              false);
-- 
2.38.5



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH QEMU v8 5/9] migration: Refactor auto-converge capability logic
  2023-07-07  4:10 [PATCH QEMU v8 0/9] migration: introduce dirtylimit capability ~hyman
                   ` (3 preceding siblings ...)
  2023-06-07 15:30 ` [PATCH QEMU v8 4/9] migration: Introduce dirty-limit capability ~hyman
@ 2023-06-07 15:32 ` ~hyman
  2023-06-07 16:12 ` [PATCH QEMU v8 7/9] migration: Implement dirty-limit convergence algo ~hyman
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 20+ messages in thread
From: ~hyman @ 2023-06-07 15:32 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Xu, Paolo Bonzini, Juan Quintela, Dr. David Alan Gilbert,
	Eric Blake, Markus Armbruster, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé,
	Hyman Huang(黄勇)

From: Hyman Huang(黄勇) <yong.huang@smartx.com>

Check if block migration is running before throttling
guest down in auto-converge way.

Note that this modification is kind of like code clean,
because block migration does not depend on auto-converge
capability, so the order of checks can be adjusted.

Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
Acked-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 5283a75f02..78746849b5 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -995,7 +995,11 @@ static void migration_trigger_throttle(RAMState *rs)
     /* During block migration the auto-converge logic incorrectly detects
      * that ram migration makes no progress. Avoid this by disabling the
      * throttling logic during the bulk phase of block migration. */
-    if (migrate_auto_converge() && !blk_mig_bulk_active()) {
+    if (blk_mig_bulk_active()) {
+        return;
+    }
+
+    if (migrate_auto_converge()) {
         /* The following detection logic can be refined later. For now:
            Check to see if the ratio between dirtied bytes and the approx.
            amount of bytes that just got transferred since the last time
-- 
2.38.5



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH QEMU v8 7/9] migration: Implement dirty-limit convergence algo
  2023-07-07  4:10 [PATCH QEMU v8 0/9] migration: introduce dirtylimit capability ~hyman
                   ` (4 preceding siblings ...)
  2023-06-07 15:32 ` [PATCH QEMU v8 5/9] migration: Refactor auto-converge capability logic ~hyman
@ 2023-06-07 16:12 ` ~hyman
  2023-06-07 16:21 ` [PATCH QEMU v8 8/9] migration: Extend query-migrate to provide dirty page limit info ~hyman
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 20+ messages in thread
From: ~hyman @ 2023-06-07 16:12 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Xu, Paolo Bonzini, Juan Quintela, Dr. David Alan Gilbert,
	Eric Blake, Markus Armbruster, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé,
	Hyman Huang(黄勇)

From: Hyman Huang(黄勇) <yong.huang@smartx.com>

Implement dirty-limit convergence algo for live migration,
which is kind of like auto-converge algo but using dirty-limit
instead of cpu throttle to make migration convergent.

Enable dirty page limit if dirty_rate_high_cnt greater than 2
when dirty-limit capability enabled, Disable dirty-limit if
migration be canceled.

Note that "set_vcpu_dirty_limit", "cancel_vcpu_dirty_limit"
commands are not allowed during dirty-limit live migration.

Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c  |  3 +++
 migration/ram.c        | 36 ++++++++++++++++++++++++++++++++++++
 migration/trace-events |  1 +
 softmmu/dirtylimit.c   | 29 +++++++++++++++++++++++++++++
 4 files changed, 69 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 096e8191d1..a3791900fd 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -166,6 +166,9 @@ void migration_cancel(const Error *error)
     if (error) {
         migrate_set_error(current_migration, error);
     }
+    if (migrate_dirty_limit()) {
+        qmp_cancel_vcpu_dirty_limit(false, -1, NULL);
+    }
     migrate_fd_cancel(current_migration);
 }
 
diff --git a/migration/ram.c b/migration/ram.c
index b6559f9312..8a86363216 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -46,6 +46,7 @@
 #include "qapi/error.h"
 #include "qapi/qapi-types-migration.h"
 #include "qapi/qapi-events-migration.h"
+#include "qapi/qapi-commands-migration.h"
 #include "qapi/qmp/qerror.h"
 #include "trace.h"
 #include "exec/ram_addr.h"
@@ -59,6 +60,8 @@
 #include "multifd.h"
 #include "sysemu/runstate.h"
 #include "options.h"
+#include "sysemu/dirtylimit.h"
+#include "sysemu/kvm.h"
 
 #include "hw/boards.h" /* for machine_dump_guest_core() */
 
@@ -984,6 +987,37 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
     }
 }
 
+/*
+ * Enable dirty-limit to throttle down the guest
+ */
+static void migration_dirty_limit_guest(void)
+{
+    /*
+     * dirty page rate quota for all vCPUs fetched from
+     * migration parameter 'vcpu_dirty_limit'
+     */
+    static int64_t quota_dirtyrate;
+    MigrationState *s = migrate_get_current();
+
+    /*
+     * If dirty limit already enabled and migration parameter
+     * vcpu-dirty-limit untouched.
+     */
+    if (dirtylimit_in_service() &&
+        quota_dirtyrate == s->parameters.vcpu_dirty_limit) {
+        return;
+    }
+
+    quota_dirtyrate = s->parameters.vcpu_dirty_limit;
+
+    /*
+     * Set all vCPU a quota dirtyrate, note that the second
+     * parameter will be ignored if setting all vCPU for the vm
+     */
+    qmp_set_vcpu_dirty_limit(false, -1, quota_dirtyrate, NULL);
+    trace_migration_dirty_limit_guest(quota_dirtyrate);
+}
+
 static void migration_trigger_throttle(RAMState *rs)
 {
     uint64_t threshold = migrate_throttle_trigger_threshold();
@@ -1013,6 +1047,8 @@ static void migration_trigger_throttle(RAMState *rs)
             trace_migration_throttle();
             mig_throttle_guest_down(bytes_dirty_period,
                                     bytes_dirty_threshold);
+        } else if (migrate_dirty_limit()) {
+            migration_dirty_limit_guest();
         }
     }
 }
diff --git a/migration/trace-events b/migration/trace-events
index 5259c1044b..580895e86e 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -93,6 +93,7 @@ migration_bitmap_sync_start(void) ""
 migration_bitmap_sync_end(uint64_t dirty_pages) "dirty_pages %" PRIu64
 migration_bitmap_clear_dirty(char *str, uint64_t start, uint64_t size, unsigned long page) "rb %s start 0x%"PRIx64" size 0x%"PRIx64" page 0x%lx"
 migration_throttle(void) ""
+migration_dirty_limit_guest(int64_t dirtyrate) "guest dirty page rate limit %" PRIi64 " MB/s"
 ram_discard_range(const char *rbname, uint64_t start, size_t len) "%s: start: %" PRIx64 " %zx"
 ram_load_loop(const char *rbname, uint64_t addr, int flags, void *host) "%s: addr: 0x%" PRIx64 " flags: 0x%x host: %p"
 ram_load_postcopy_loop(int channel, uint64_t addr, int flags) "chan=%d addr=0x%" PRIx64 " flags=0x%x"
diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
index 953ef934bc..5134296667 100644
--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -436,6 +436,23 @@ static void dirtylimit_cleanup(void)
     dirtylimit_state_finalize();
 }
 
+/*
+ * dirty page rate limit is not allowed to set if migration
+ * is running with dirty-limit capability enabled.
+ */
+static bool dirtylimit_is_allowed(void)
+{
+    MigrationState *ms = migrate_get_current();
+
+    if (migration_is_running(ms->state) &&
+        (!qemu_thread_is_self(&ms->thread)) &&
+        migrate_dirty_limit() &&
+        dirtylimit_in_service()) {
+        return false;
+    }
+    return true;
+}
+
 void qmp_cancel_vcpu_dirty_limit(bool has_cpu_index,
                                  int64_t cpu_index,
                                  Error **errp)
@@ -449,6 +466,12 @@ void qmp_cancel_vcpu_dirty_limit(bool has_cpu_index,
         return;
     }
 
+    if (!dirtylimit_is_allowed()) {
+        error_setg(errp, "can't cancel dirty page rate limit while"
+                   " migration is running");
+        return;
+    }
+
     if (!dirtylimit_in_service()) {
         return;
     }
@@ -499,6 +522,12 @@ void qmp_set_vcpu_dirty_limit(bool has_cpu_index,
         return;
     }
 
+    if (!dirtylimit_is_allowed()) {
+        error_setg(errp, "can't set dirty page rate limit while"
+                   " migration is running");
+        return;
+    }
+
     if (!dirty_rate) {
         qmp_cancel_vcpu_dirty_limit(has_cpu_index, cpu_index, errp);
         return;
-- 
2.38.5



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH QEMU v8 8/9] migration: Extend query-migrate to provide dirty page limit info
  2023-07-07  4:10 [PATCH QEMU v8 0/9] migration: introduce dirtylimit capability ~hyman
                   ` (5 preceding siblings ...)
  2023-06-07 16:12 ` [PATCH QEMU v8 7/9] migration: Implement dirty-limit convergence algo ~hyman
@ 2023-06-07 16:21 ` ~hyman
  2023-07-13 12:45   ` Markus Armbruster
  2023-06-07 16:46 ` [PATCH QEMU v8 9/9] tests: Add migration dirty-limit capability test ~hyman
  2023-06-15 13:29 ` [PATCH QEMU v8 6/9] migration: Put the detection logic before auto-converge checking ~hyman
  8 siblings, 1 reply; 20+ messages in thread
From: ~hyman @ 2023-06-07 16:21 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Xu, Paolo Bonzini, Juan Quintela, Dr. David Alan Gilbert,
	Eric Blake, Markus Armbruster, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé,
	Hyman Huang(黄勇)

From: Hyman Huang(黄勇) <yong.huang@smartx.com>

Extend query-migrate to provide throttle time and estimated
ring full time with dirty-limit capability enabled, through which
we can observe if dirty limit take effect during live migration.

Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 include/sysemu/dirtylimit.h    |  2 ++
 migration/migration-hmp-cmds.c | 10 +++++++++
 migration/migration.c          | 10 +++++++++
 qapi/migration.json            | 16 +++++++++++++-
 softmmu/dirtylimit.c           | 39 ++++++++++++++++++++++++++++++++++
 5 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/dirtylimit.h b/include/sysemu/dirtylimit.h
index 8d2c1f3a6b..d11ebbbbdb 100644
--- a/include/sysemu/dirtylimit.h
+++ b/include/sysemu/dirtylimit.h
@@ -34,4 +34,6 @@ void dirtylimit_set_vcpu(int cpu_index,
 void dirtylimit_set_all(uint64_t quota,
                         bool enable);
 void dirtylimit_vcpu_execute(CPUState *cpu);
+uint64_t dirtylimit_throttle_time_per_round(void);
+uint64_t dirtylimit_ring_full_time(void);
 #endif
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 35e8020bbf..c115ef2d23 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -190,6 +190,16 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
                        info->cpu_throttle_percentage);
     }
 
+    if (info->has_dirty_limit_throttle_time_per_round) {
+        monitor_printf(mon, "dirty-limit throttle time: %" PRIu64 " us\n",
+                       info->dirty_limit_throttle_time_per_round);
+    }
+
+    if (info->has_dirty_limit_ring_full_time) {
+        monitor_printf(mon, "dirty-limit ring full time: %" PRIu64 " us\n",
+                       info->dirty_limit_ring_full_time);
+    }
+
     if (info->has_postcopy_blocktime) {
         monitor_printf(mon, "postcopy blocktime: %u\n",
                        info->postcopy_blocktime);
diff --git a/migration/migration.c b/migration/migration.c
index a3791900fd..a4dcaa3c91 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -64,6 +64,7 @@
 #include "yank_functions.h"
 #include "sysemu/qtest.h"
 #include "options.h"
+#include "sysemu/dirtylimit.h"
 
 static NotifierList migration_state_notifiers =
     NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
@@ -974,6 +975,15 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
         info->ram->dirty_pages_rate =
            stat64_get(&mig_stats.dirty_pages_rate);
     }
+
+    if (migrate_dirty_limit() && dirtylimit_in_service()) {
+        info->has_dirty_limit_throttle_time_per_round = true;
+        info->dirty_limit_throttle_time_per_round =
+                            dirtylimit_throttle_time_per_round();
+
+        info->has_dirty_limit_ring_full_time = true;
+        info->dirty_limit_ring_full_time = dirtylimit_ring_full_time();
+    }
 }
 
 static void populate_disk_info(MigrationInfo *info)
diff --git a/qapi/migration.json b/qapi/migration.json
index 031832cde5..97f7d0fd3c 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -250,6 +250,18 @@
 #     blocked.  Present and non-empty when migration is blocked.
 #     (since 6.0)
 #
+# @dirty-limit-throttle-time-per-round: Maximum throttle time
+#     (in microseconds) of virtual CPUs each dirty ring full round,
+#     which shows how MigrationCapability dirty-limit affects the
+#     guest during live migration. (since 8.1)
+#
+# @dirty-limit-ring-full-time: Estimated average dirty ring full
+#     time (in microseconds) each dirty ring full round. The value
+#     equals dirty ring memory size divided by average dirty page
+#     rate of the virtual CPU, which can be used to observe the
+#     average memory load of the virtual CPU indirectly. Note that
+#     zero means guest doesn't dirty memory (since 8.1)
+#
 # Since: 0.14
 ##
 { 'struct': 'MigrationInfo',
@@ -267,7 +279,9 @@
            '*postcopy-blocktime' : 'uint32',
            '*postcopy-vcpu-blocktime': ['uint32'],
            '*compression': 'CompressionStats',
-           '*socket-address': ['SocketAddress'] } }
+           '*socket-address': ['SocketAddress'],
+           '*dirty-limit-throttle-time-per-round': 'uint64',
+           '*dirty-limit-ring-full-time': 'uint64'} }
 
 ##
 # @query-migrate:
diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
index 5134296667..a0686323e5 100644
--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -565,6 +565,45 @@ out:
     hmp_handle_error(mon, err);
 }
 
+/* Return the max throttle time of each virtual CPU */
+uint64_t dirtylimit_throttle_time_per_round(void)
+{
+    CPUState *cpu;
+    int64_t max = 0;
+
+    CPU_FOREACH(cpu) {
+        if (cpu->throttle_us_per_full > max) {
+            max = cpu->throttle_us_per_full;
+        }
+    }
+
+    return max;
+}
+
+/*
+ * Estimate average dirty ring full time of each virtaul CPU.
+ * Return 0 if guest doesn't dirty memory.
+ */
+uint64_t dirtylimit_ring_full_time(void)
+{
+    CPUState *cpu;
+    uint64_t curr_rate = 0;
+    int nvcpus = 0;
+
+    CPU_FOREACH(cpu) {
+        if (cpu->running) {
+            nvcpus++;
+            curr_rate += vcpu_dirty_rate_get(cpu->cpu_index);
+        }
+    }
+
+    if (!curr_rate || !nvcpus) {
+        return 0;
+    }
+
+    return dirtylimit_dirty_ring_full_time(curr_rate / nvcpus);
+}
+
 static struct DirtyLimitInfo *dirtylimit_query_vcpu(int cpu_index)
 {
     DirtyLimitInfo *info = NULL;
-- 
2.38.5



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH QEMU v8 9/9] tests: Add migration dirty-limit capability test
  2023-07-07  4:10 [PATCH QEMU v8 0/9] migration: introduce dirtylimit capability ~hyman
                   ` (6 preceding siblings ...)
  2023-06-07 16:21 ` [PATCH QEMU v8 8/9] migration: Extend query-migrate to provide dirty page limit info ~hyman
@ 2023-06-07 16:46 ` ~hyman
  2023-06-15 13:29 ` [PATCH QEMU v8 6/9] migration: Put the detection logic before auto-converge checking ~hyman
  8 siblings, 0 replies; 20+ messages in thread
From: ~hyman @ 2023-06-07 16:46 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Xu, Paolo Bonzini, Juan Quintela, Dr. David Alan Gilbert,
	Eric Blake, Markus Armbruster, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé,
	Hyman Huang(黄勇)

From: Hyman Huang(黄勇) <yong.huang@smartx.com>

Add migration dirty-limit capability test if kernel support
dirty ring.

Migration dirty-limit capability introduce dirty limit
capability, two parameters: x-vcpu-dirty-limit-period and
vcpu-dirty-limit are introduced to implement the live
migration with dirty limit.

The test case does the following things:
1. start src, dst vm and enable dirty-limit capability
2. start migrate and set cancel it to check if dirty limit
   stop working.
3. restart dst vm
4. start migrate and enable dirty-limit capability
5. check if migration satisfy the convergence condition
   during pre-switchover phase.

Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
---
 tests/qtest/migration-test.c | 155 +++++++++++++++++++++++++++++++++++
 1 file changed, 155 insertions(+)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index b9cc194100..f55f95c9b0 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2636,6 +2636,159 @@ static void test_vcpu_dirty_limit(void)
     dirtylimit_stop_vm(vm);
 }
 
+static void migrate_dirty_limit_wait_showup(QTestState *from,
+                                            const int64_t period,
+                                            const int64_t value)
+{
+    /* Enable dirty limit capability */
+    migrate_set_capability(from, "dirty-limit", true);
+
+    /* Set dirty limit parameters */
+    migrate_set_parameter_int(from, "x-vcpu-dirty-limit-period", period);
+    migrate_set_parameter_int(from, "vcpu-dirty-limit", value);
+
+    /* Make sure migrate can't converge */
+    migrate_ensure_non_converge(from);
+
+    /* To check limit rate after precopy */
+    migrate_set_capability(from, "pause-before-switchover", true);
+
+    /* Wait for the serial output from the source */
+    wait_for_serial("src_serial");
+}
+
+/*
+ * This test does:
+ *  source               target
+ *                       migrate_incoming
+ *     migrate
+ *     migrate_cancel
+ *                       restart target
+ *     migrate
+ *
+ *  And see that if dirty limit works correctly
+ */
+static void test_migrate_dirty_limit(void)
+{
+    g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
+    QTestState *from, *to;
+    int64_t remaining;
+    uint64_t throttle_us_per_full;
+    /*
+     * We want the test to be stable and as fast as possible.
+     * E.g., with 1Gb/s bandwith migration may pass without dirty limit,
+     * so we need to decrease a bandwidth.
+     */
+    const int64_t dirtylimit_period = 1000, dirtylimit_value = 50;
+    const int64_t max_bandwidth = 400000000; /* ~400Mb/s */
+    const int64_t downtime_limit = 250; /* 250ms */
+    /*
+     * We migrate through unix-socket (> 500Mb/s).
+     * Thus, expected migration speed ~= bandwidth limit (< 500Mb/s).
+     * So, we can predict expected_threshold
+     */
+    const int64_t expected_threshold = max_bandwidth * downtime_limit / 1000;
+    int max_try_count = 10;
+    MigrateCommon args = {
+        .start = {
+            .hide_stderr = true,
+            .use_dirty_ring = true,
+        },
+        .listen_uri = uri,
+        .connect_uri = uri,
+    };
+
+    /* Start src, dst vm */
+    if (test_migrate_start(&from, &to, args.listen_uri, &args.start)) {
+        return;
+    }
+
+    /* Prepare for dirty limit migration and wait src vm show up */
+    migrate_dirty_limit_wait_showup(from, dirtylimit_period, dirtylimit_value);
+
+    /* Start migrate */
+    migrate_qmp(from, uri, "{}");
+
+    /* Wait for dirty limit throttle begin */
+    throttle_us_per_full = 0;
+    while (throttle_us_per_full == 0) {
+        throttle_us_per_full =
+        read_migrate_property_int(from, "dirty-limit-throttle-time-per-round");
+        usleep(100);
+        g_assert_false(got_src_stop);
+    }
+
+    /* Now cancel migrate and wait for dirty limit throttle switch off */
+    migrate_cancel(from);
+    wait_for_migration_status(from, "cancelled", NULL);
+
+    /* Check if dirty limit throttle switched off, set timeout 1ms */
+    do {
+        throttle_us_per_full =
+        read_migrate_property_int(from, "dirty-limit-throttle-time-per-round");
+        usleep(100);
+        g_assert_false(got_src_stop);
+    } while (throttle_us_per_full != 0 && --max_try_count);
+
+    /* Assert dirty limit is not in service */
+    g_assert_cmpint(throttle_us_per_full, ==, 0);
+
+    args = (MigrateCommon) {
+        .start = {
+            .only_target = true,
+            .use_dirty_ring = true,
+        },
+        .listen_uri = uri,
+        .connect_uri = uri,
+    };
+
+    /* Restart dst vm, src vm already show up so we needn't wait anymore */
+    if (test_migrate_start(&from, &to, args.listen_uri, &args.start)) {
+        return;
+    }
+
+    /* Start migrate */
+    migrate_qmp(from, uri, "{}");
+
+    /* Wait for dirty limit throttle begin */
+    throttle_us_per_full = 0;
+    while (throttle_us_per_full == 0) {
+        throttle_us_per_full =
+        read_migrate_property_int(from, "dirty-limit-throttle-time-per-round");
+        usleep(100);
+        g_assert_false(got_src_stop);
+    }
+
+    /*
+     * The dirty limit rate should equals the return value of
+     * query-vcpu-dirty-limit if dirty limit cap set
+     */
+    g_assert_cmpint(dirtylimit_value, ==, get_limit_rate(from));
+
+    /* Now, we have tested if dirty limit works, let it converge */
+    migrate_set_parameter_int(from, "downtime-limit", downtime_limit);
+    migrate_set_parameter_int(from, "max-bandwidth", max_bandwidth);
+
+    /*
+     * Wait for pre-switchover status to check if migration
+     * satisfy the convergence condition
+     */
+    wait_for_migration_status(from, "pre-switchover", NULL);
+
+    remaining = read_ram_property_int(from, "remaining");
+    g_assert_cmpint(remaining, <,
+                    (expected_threshold + expected_threshold / 100));
+
+    migrate_continue(from, "pre-switchover");
+
+    qtest_qmp_eventwait(to, "RESUME");
+
+    wait_for_serial("dest_serial");
+    wait_for_migration_complete(from);
+
+    test_migrate_end(from, to, true);
+}
+
 static bool kvm_dirty_ring_supported(void)
 {
 #if defined(__linux__) && defined(HOST_X86_64)
@@ -2847,6 +3000,8 @@ int main(int argc, char **argv)
                        test_precopy_unix_dirty_ring);
         qtest_add_func("/migration/vcpu_dirty_limit",
                        test_vcpu_dirty_limit);
+        qtest_add_func("/migration/dirty_limit",
+                       test_migrate_dirty_limit);
     }
 
     ret = g_test_run();
-- 
2.38.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH QEMU v8 6/9] migration: Put the detection logic before auto-converge checking
  2023-07-07  4:10 [PATCH QEMU v8 0/9] migration: introduce dirtylimit capability ~hyman
                   ` (7 preceding siblings ...)
  2023-06-07 16:46 ` [PATCH QEMU v8 9/9] tests: Add migration dirty-limit capability test ~hyman
@ 2023-06-15 13:29 ` ~hyman
  8 siblings, 0 replies; 20+ messages in thread
From: ~hyman @ 2023-06-15 13:29 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Xu, Paolo Bonzini, Juan Quintela, Dr. David Alan Gilbert,
	Eric Blake, Markus Armbruster, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé,
	Hyman Huang(黄勇)

From: Hyman Huang(黄勇) <yong.huang@smartx.com>

This commit is prepared for the implementation of dirty-limit
convergence algo.

The detection logic of throttling condition can apply to both
auto-converge and dirty-limit algo, putting it's position
before the checking logic for auto-converge feature.

Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 78746849b5..b6559f9312 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -999,17 +999,18 @@ static void migration_trigger_throttle(RAMState *rs)
         return;
     }
 
-    if (migrate_auto_converge()) {
-        /* The following detection logic can be refined later. For now:
-           Check to see if the ratio between dirtied bytes and the approx.
-           amount of bytes that just got transferred since the last time
-           we were in this routine reaches the threshold. If that happens
-           twice, start or increase throttling. */
-
-        if ((bytes_dirty_period > bytes_dirty_threshold) &&
-            (++rs->dirty_rate_high_cnt >= 2)) {
+    /*
+     * The following detection logic can be refined later. For now:
+     * Check to see if the ratio between dirtied bytes and the approx.
+     * amount of bytes that just got transferred since the last time
+     * we were in this routine reaches the threshold. If that happens
+     * twice, start or increase throttling.
+     */
+    if ((bytes_dirty_period > bytes_dirty_threshold) &&
+        (++rs->dirty_rate_high_cnt >= 2)) {
+        rs->dirty_rate_high_cnt = 0;
+        if (migrate_auto_converge()) {
             trace_migration_throttle();
-            rs->dirty_rate_high_cnt = 0;
             mig_throttle_guest_down(bytes_dirty_period,
                                     bytes_dirty_threshold);
         }
-- 
2.38.5



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH QEMU v8 0/9] migration: introduce dirtylimit capability
@ 2023-07-07  4:10 ~hyman
  2022-11-18  2:08 ` [PATCH QEMU v8 1/9] softmmu/dirtylimit: Add parameter check for hmp "set_vcpu_dirty_limit" ~hyman
                   ` (8 more replies)
  0 siblings, 9 replies; 20+ messages in thread
From: ~hyman @ 2023-07-07  4:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Xu, Paolo Bonzini, Juan Quintela, Dr. David Alan Gilbert,
	Eric Blake, Markus Armbruster, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé,
	Hyman Huang(黄勇)

Hi, Juan and Markus, thanks for reviewing the previous
versions and please review the latest version if you have time :)

Yong

v8:
1. Rebase on master and refactor the docs suggested by Markus

v7:
1. Rebase on master and fix conflicts

v6:
1. Rebase on master
2. Split the commit "Implement dirty-limit convergence algo" into two as
    Juan suggested as the following:
    a. Put the detection logic before auto-converge checking
    b. Implement dirty-limit convergence algo
3. Put the detection logic before auto-converge checking
4. Sort the migrate_dirty_limit function in commit
    "Introduce dirty-limit capability" suggested by Juan
5. Substitute the the int64_t to uint64_t in the last 2 commits
6. Fix the comments spell mistake
7. Add helper function in the commit
    "Implement dirty-limit convergence algo" suggested by Juan

v5:
1. Rebase on master and enrich the comment for "dirty-limit" capability,
    suggesting by Markus.
2. Drop commits that have already been merged.

v4:
1. Polish the docs and update the release version suggested by Markus
2. Rename the migrate exported info "dirty-limit-throttle-time-per-
round"
   to "dirty-limit-throttle-time-per-full".

v3(resend):
- fix the syntax error of the topic.

v3:
This version make some modifications inspired by Peter and Markus
as following:
1. Do the code clean up in [PATCH v2 02/11] suggested by Markus
2. Replace the [PATCH v2 03/11] with a much simpler patch posted by
   Peter to fix the following bug:
   https://bugzilla.redhat.com/show_bug.cgi?id=2124756
3. Fix the error path of migrate_params_check in [PATCH v2 04/11]
   pointed out by Markus. Enrich the commit message to explain why
   x-vcpu-dirty-limit-period an unstable parameter.
4. Refactor the dirty-limit convergence algo in [PATCH v2 07/11]
   suggested by Peter:
   a. apply blk_mig_bulk_active check before enable dirty-limit
   b. drop the unhelpful check function before enable dirty-limit
   c. change the migration_cancel logic, just cancel dirty-limit
      only if dirty-limit capability turned on.
   d. abstract a code clean commit [PATCH v3 07/10] to adjust
      the check order before enable auto-converge
5. Change the name of observing indexes during dirty-limit live
   migration to make them more easy-understanding. Use the
   maximum throttle time of vpus as "dirty-limit-throttle-time-per-full"
6. Fix some grammatical and spelling errors pointed out by Markus
   and enrich the document about the dirty-limit live migration
   observing indexes "dirty-limit-ring-full-time"
   and "dirty-limit-throttle-time-per-full"
7. Change the default value of x-vcpu-dirty-limit-period to 1000ms,
   which is optimal value pointed out in cover letter in that
   testing environment.
8. Drop the 2 guestperf test commits [PATCH v2 10/11],
   [PATCH v2 11/11] and post them with a standalone series in the
   future.

v2:
This version make a little bit modifications comparing with
version 1 as following:
1. fix the overflow issue reported by Peter Maydell
2. add parameter check for hmp "set_vcpu_dirty_limit" command
3. fix the racing issue between dirty ring reaper thread and
   Qemu main thread.
4. add migrate parameter check for x-vcpu-dirty-limit-period
   and vcpu-dirty-limit.
5. add the logic to forbid hmp/qmp commands set_vcpu_dirty_limit,
   cancel_vcpu_dirty_limit during dirty-limit live migration when
   implement dirty-limit convergence algo.
6. add capability check to ensure auto-converge and dirty-limit
   are mutually exclusive.
7. pre-check if kvm dirty ring size is configured before setting
   dirty-limit migrate parameter

Hyman Huang(黄勇) (9):
  softmmu/dirtylimit: Add parameter check for hmp "set_vcpu_dirty_limit"
  qapi/migration: Introduce x-vcpu-dirty-limit-period parameter
  qapi/migration: Introduce vcpu-dirty-limit parameters
  migration: Introduce dirty-limit capability
  migration: Refactor auto-converge capability logic
  migration: Put the detection logic before auto-converge checking
  migration: Implement dirty-limit convergence algo
  migration: Extend query-migrate to provide dirty page limit info
  tests: Add migration dirty-limit capability test

 include/sysemu/dirtylimit.h    |   2 +
 migration/migration-hmp-cmds.c |  26 ++++++
 migration/migration.c          |  13 +++
 migration/options.c            |  73 ++++++++++++++++
 migration/options.h            |   1 +
 migration/ram.c                |  61 ++++++++++---
 migration/trace-events         |   1 +
 qapi/migration.json            |  75 ++++++++++++++--
 softmmu/dirtylimit.c           |  91 +++++++++++++++++--
 tests/qtest/migration-test.c   | 155 +++++++++++++++++++++++++++++++++
 10 files changed, 473 insertions(+), 25 deletions(-)

-- 
2.38.5


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH QEMU v8 2/9] qapi/migration: Introduce x-vcpu-dirty-limit-period parameter
  2023-06-07 13:32 ` [PATCH QEMU v8 2/9] qapi/migration: Introduce x-vcpu-dirty-limit-period parameter ~hyman
@ 2023-07-13 12:32   ` Markus Armbruster
  0 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2023-07-13 12:32 UTC (permalink / raw)
  To: ~hyman
  Cc: qemu-devel, ~hyman, Peter Xu, Paolo Bonzini, Juan Quintela,
	Dr. David Alan Gilbert, Eric Blake, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé

~hyman <hyman@git.sr.ht> writes:

> From: Hyman Huang(黄勇) <yong.huang@smartx.com>
>
> Introduce "x-vcpu-dirty-limit-period" migration experimental
> parameter, which is in the range of 1 to 1000ms and used to
> make dirtyrate calculation period configurable.

dirty rate

>
> Currently with the "x-vcpu-dirty-limit-period" varies, the

Currently, as the

> total time of live migration changes, test results show the

Suggest period instead of comma.

> optimal value of "x-vcpu-dirty-limit-period" ranges from
> 500ms to 1000 ms. "x-vcpu-dirty-limit-period" should be made
> stable once it proves best value can not be determined with
> developer's experiments.
>
> Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>

[...]

> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -789,9 +789,14 @@
>  #     Nodes are mapped to their block device name if there is one, and
>  #     to their node name otherwise.  (Since 5.2)
>  #
> +# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty
> +#     limit during live migration. Should be in the range 1 to 1000ms,
> +#     defaults to 1000ms. (Since 8.1)

Two spaces after sentence-ending punctuation, please:

   # @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty
   #     limit during live migration.  Should be in the range 1 to
   #     1000ms, defaults to 1000ms.  (Since 8.1)

> +#
>  # Features:
>  #
> -# @unstable: Member @x-checkpoint-delay is experimental.
> +# @unstable: Members @x-checkpoint-delay and
> +#     @x-vcpu-dirty-limit-period are experimental.
>  #
>  # Since: 2.4
>  ##
> @@ -809,8 +814,10 @@
>             'multifd-channels',
>             'xbzrle-cache-size', 'max-postcopy-bandwidth',
>             'max-cpu-throttle', 'multifd-compression',
> -           'multifd-zlib-level' ,'multifd-zstd-level',
> -           'block-bitmap-mapping' ] }
> +           'multifd-zlib-level', 'multifd-zstd-level',
> +           'block-bitmap-mapping',
> +           { 'name': 'x-vcpu-dirty-limit-period',
> +             'features': ['unstable'] } ] }
>  
>  ##
>  # @MigrateSetParameters:
> @@ -945,9 +952,14 @@
>  #     Nodes are mapped to their block device name if there is one, and
>  #     to their node name otherwise.  (Since 5.2)
>  #
> +# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty
> +#     limit during live migration. Should be in the range 1 to 1000ms,
> +#     defaults to 1000ms. (Since 8.1)

Likewise.

> +#
>  # Features:
>  #
> -# @unstable: Member @x-checkpoint-delay is experimental.
> +# @unstable: Members @x-checkpoint-delay and
> +#     @x-vcpu-dirty-limit-period are experimental.
>  #
>  # TODO: either fuse back into MigrationParameters, or make
>  #     MigrationParameters members mandatory
> @@ -982,7 +994,9 @@
>              '*multifd-compression': 'MultiFDCompression',
>              '*multifd-zlib-level': 'uint8',
>              '*multifd-zstd-level': 'uint8',
> -            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> +            '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
> +                                            'features': [ 'unstable' ] } } }
>  
>  ##
>  # @migrate-set-parameters:
> @@ -1137,9 +1151,14 @@
>  #     Nodes are mapped to their block device name if there is one, and
>  #     to their node name otherwise.  (Since 5.2)
>  #
> +# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty
> +#     limit during live migration. Should be in the range 1 to 1000ms,
> +#     defaults to 1000ms. (Since 8.1)

Likewise.

> +#
>  # Features:
>  #
> -# @unstable: Member @x-checkpoint-delay is experimental.
> +# @unstable: Members @x-checkpoint-delay and
> +#     @x-vcpu-dirty-limit-period are experimental.
>  #
>  # Since: 2.4
>  ##
> @@ -1171,7 +1190,9 @@
>              '*multifd-compression': 'MultiFDCompression',
>              '*multifd-zlib-level': 'uint8',
>              '*multifd-zstd-level': 'uint8',
> -            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> +            '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
> +                                            'features': [ 'unstable' ] } } }
>  
>  ##
>  # @query-migrate-parameters:

The issues I'm pointing out don't justfy yet another respin.  But if you
need to respin the series for some other reason, plase take care of them.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH QEMU v8 4/9] migration: Introduce dirty-limit capability
  2023-06-07 15:30 ` [PATCH QEMU v8 4/9] migration: Introduce dirty-limit capability ~hyman
@ 2023-07-13 12:44   ` Markus Armbruster
  2023-07-18  1:42     ` Yong Huang
  0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2023-07-13 12:44 UTC (permalink / raw)
  To: ~hyman
  Cc: qemu-devel, ~hyman, Peter Xu, Paolo Bonzini, Juan Quintela,
	Dr. David Alan Gilbert, Eric Blake, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé

~hyman <hyman@git.sr.ht> writes:

> From: Hyman Huang(黄勇) <yong.huang@smartx.com>
>
> Introduce migration dirty-limit capability, which can
> be turned on before live migration and limit dirty
> page rate durty live migration.
>
> Introduce migrate_dirty_limit function to help check
> if dirty-limit capability enabled during live migration.
>
> Meanwhile, refactor vcpu_dirty_rate_stat_collect
> so that period can be configured instead of hardcoded.
>
> dirty-limit capability is kind of like auto-converge
> but using dirty limit instead of traditional cpu-throttle
> to throttle guest down. To enable this feature, turn on
> the dirty-limit capability before live migration using
> migrate-set-capabilities, and set the parameters
> "x-vcpu-dirty-limit-period", "vcpu-dirty-limit" suitably
> to speed up convergence.
>
> Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
> Acked-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>

[...]

> diff --git a/qapi/migration.json b/qapi/migration.json
> index e43371955a..031832cde5 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -497,6 +497,15 @@
>  #     are present.  'return-path' capability must be enabled to use
>  #     it.  (since 8.1)
>  #
> +# @dirty-limit: If enabled, migration will use the dirty-limit
> +#     algorithm to throttle down guest instead of auto-converge
> +#     algorithm. This algorithm only works when vCPU's dirtyrate

Two spaces after sentence-ending punctuation, please.

"dirty rate" with a space, because that's how we spell it elsewhere.

> +#     greater than 'vcpu-dirty-limit', read processes in guest os
> +#     aren't penalized any more, so the algorithm can improve
> +#     performance of vCPU during live migration. This is an optional
> +#     performance feature and should not affect the correctness of the
> +#     existing auto-converge algorithm. (since 8.1)
> +#

I'm still confused.

The text suggests there are two separate algorithms "to throttle down
guest": "auto converge" and "dirty limit", and we get to pick one.
Correct?

If it is correct, then the last sentence feels redundant: picking
another algorithm can't affect the algorithm we're *not* using.  What
are you trying to express here?

When do we use "auto converge", and when do we use "dirty limit"?

What does the user really need to know about these algorithms?  Enough
to pick one, I guess.  That means advantages and disadvantages of the
two algorithms.  Which are?

>  # Features:
>  #
>  # @unstable: Members @x-colo and @x-ignore-shared are experimental.
> @@ -512,7 +521,8 @@
>             'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
>             { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
>             'validate-uuid', 'background-snapshot',
> -           'zero-copy-send', 'postcopy-preempt', 'switchover-ack'] }
> +           'zero-copy-send', 'postcopy-preempt', 'switchover-ack',
> +           'dirty-limit'] }
>  
>  ##
>  # @MigrationCapabilityStatus:

[...]



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH QEMU v8 8/9] migration: Extend query-migrate to provide dirty page limit info
  2023-06-07 16:21 ` [PATCH QEMU v8 8/9] migration: Extend query-migrate to provide dirty page limit info ~hyman
@ 2023-07-13 12:45   ` Markus Armbruster
  0 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2023-07-13 12:45 UTC (permalink / raw)
  To: ~hyman
  Cc: qemu-devel, ~hyman, Peter Xu, Paolo Bonzini, Juan Quintela,
	Dr. David Alan Gilbert, Eric Blake, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé

~hyman <hyman@git.sr.ht> writes:

> From: Hyman Huang(黄勇) <yong.huang@smartx.com>
>
> Extend query-migrate to provide throttle time and estimated
> ring full time with dirty-limit capability enabled, through which
> we can observe if dirty limit take effect during live migration.
>
> Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>

[...]

> diff --git a/qapi/migration.json b/qapi/migration.json
> index 031832cde5..97f7d0fd3c 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -250,6 +250,18 @@
>  #     blocked.  Present and non-empty when migration is blocked.
>  #     (since 6.0)
>  #
> +# @dirty-limit-throttle-time-per-round: Maximum throttle time
> +#     (in microseconds) of virtual CPUs each dirty ring full round,
> +#     which shows how MigrationCapability dirty-limit affects the
> +#     guest during live migration. (since 8.1)

Two spaces after sentence-ending punctuation, please:

   #     guest during live migration.  (since 8.1)

Same below.

> +#
> +# @dirty-limit-ring-full-time: Estimated average dirty ring full
> +#     time (in microseconds) each dirty ring full round. The value
> +#     equals dirty ring memory size divided by average dirty page
> +#     rate of the virtual CPU, which can be used to observe the
> +#     average memory load of the virtual CPU indirectly. Note that
> +#     zero means guest doesn't dirty memory (since 8.1)

Last sentence should end with with a period.

Together:

   # @dirty-limit-throttle-time-per-round: Maximum throttle time (in
   #     microseconds) of virtual CPUs each dirty ring full round, which
   #     shows how MigrationCapability dirty-limit affects the guest
   #     during live migration.  (Since 8.1)
   #
   # @dirty-limit-ring-full-time: Estimated average dirty ring full time
   #     (in microseconds) each dirty ring full round.  The value equals
   #     dirty ring memory size divided by average dirty page rate of the
   #     virtual CPU, which can be used to observe the average memory
   #     load of the virtual CPU indirectly.  Note that zero means guest
   #     doesn't dirty memory.  (Since 8.1)

> +#
>  # Since: 0.14
>  ##
>  { 'struct': 'MigrationInfo',
> @@ -267,7 +279,9 @@
>             '*postcopy-blocktime' : 'uint32',
>             '*postcopy-vcpu-blocktime': ['uint32'],
>             '*compression': 'CompressionStats',
> -           '*socket-address': ['SocketAddress'] } }
> +           '*socket-address': ['SocketAddress'],
> +           '*dirty-limit-throttle-time-per-round': 'uint64',
> +           '*dirty-limit-ring-full-time': 'uint64'} }
>  
>  ##
>  # @query-migrate:

[...]

The issues I'm pointing out don't justfy yet another respin.  But if you
need to respin the series for some other reason, plase take care of them.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH QEMU v8 4/9] migration: Introduce dirty-limit capability
  2023-07-13 12:44   ` Markus Armbruster
@ 2023-07-18  1:42     ` Yong Huang
  2023-07-18 11:04       ` Markus Armbruster
  0 siblings, 1 reply; 20+ messages in thread
From: Yong Huang @ 2023-07-18  1:42 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: ~hyman, qemu-devel, Peter Xu, Paolo Bonzini, Juan Quintela,
	Dr. David Alan Gilbert, Eric Blake, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé

[-- Attachment #1: Type: text/plain, Size: 4440 bytes --]

On Thu, Jul 13, 2023 at 8:44 PM Markus Armbruster <armbru@redhat.com> wrote:

> ~hyman <hyman@git.sr.ht> writes:
>
> > From: Hyman Huang(黄勇) <yong.huang@smartx.com>
> >
> > Introduce migration dirty-limit capability, which can
> > be turned on before live migration and limit dirty
> > page rate durty live migration.
> >
> > Introduce migrate_dirty_limit function to help check
> > if dirty-limit capability enabled during live migration.
> >
> > Meanwhile, refactor vcpu_dirty_rate_stat_collect
> > so that period can be configured instead of hardcoded.
> >
> > dirty-limit capability is kind of like auto-converge
> > but using dirty limit instead of traditional cpu-throttle
> > to throttle guest down. To enable this feature, turn on
> > the dirty-limit capability before live migration using
> > migrate-set-capabilities, and set the parameters
> > "x-vcpu-dirty-limit-period", "vcpu-dirty-limit" suitably
> > to speed up convergence.
> >
> > Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
> > Acked-by: Peter Xu <peterx@redhat.com>
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
>
> [...]
>
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index e43371955a..031832cde5 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -497,6 +497,15 @@
> >  #     are present.  'return-path' capability must be enabled to use
> >  #     it.  (since 8.1)
> >  #
> > +# @dirty-limit: If enabled, migration will use the dirty-limit
> > +#     algorithm to throttle down guest instead of auto-converge
> > +#     algorithm. This algorithm only works when vCPU's dirtyrate
>
> Two spaces after sentence-ending punctuation, please.
>
> "dirty rate" with a space, because that's how we spell it elsewhere.
>
> > +#     greater than 'vcpu-dirty-limit', read processes in guest os
> > +#     aren't penalized any more, so the algorithm can improve
> > +#     performance of vCPU during live migration. This is an optional
> > +#     performance feature and should not affect the correctness of the
> > +#     existing auto-converge algorithm. (since 8.1)
> > +#
>
> I'm still confused.
>
> The text suggests there are two separate algorithms "to throttle down
> guest": "auto converge" and "dirty limit", and we get to pick one.
> Correct?
>
Yes, indeed !

>
> If it is correct, then the last sentence feels redundant: picking
> another algorithm can't affect the algorithm we're *not* using.  What
> are you trying to express here?
>
What i want to express is that the new algorithm implementation does
not affect the original algorithm, leaving it in the comments seems
redundant indeed.  I'll drop this in the next version.

>
> When do we use "auto converge", and when do we use "dirty limit"?
>
> What does the user really need to know about these algorithms?  Enough
> to pick one, I guess.  That means advantages and disadvantages of the
> two algorithms.  Which are?

1. The implementation of dirty-limit is based on dirty-ring, which is
qualified
   to big systems with huge memories and can improve huge guest VM
    responsiveness remarkably during live migration. As a consequence,
dirty-limit
    is recommended on platforms with huge guest VMs as is the way with
dirty-ring.
2. dirty-limit convergence algorithm does not affect the "read-process" in
guest
   VM, so guest VM gains the equal read performance nearly as it runs on
host
   during the live migration. As a result, dirty-limit is recommended if
the guest
    VM requires a stable read performance.
The above explanation is about the recommendation of dirty-limit, please
review,
if it's ok, i'll place it in the comment of the dirty-limit capability.

>
> >  # Features:
> >  #
> >  # @unstable: Members @x-colo and @x-ignore-shared are experimental.
> > @@ -512,7 +521,8 @@
> >             'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
> >             { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
> >             'validate-uuid', 'background-snapshot',
> > -           'zero-copy-send', 'postcopy-preempt', 'switchover-ack'] }
> > +           'zero-copy-send', 'postcopy-preempt', 'switchover-ack',
> > +           'dirty-limit'] }
> >
> >  ##
> >  # @MigrationCapabilityStatus:
>
> [...]
>
> Thank Markus again for the attention to this patchset. :)
Yong
-- 
Best regards

[-- Attachment #2: Type: text/html, Size: 8851 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH QEMU v8 4/9] migration: Introduce dirty-limit capability
  2023-07-18  1:42     ` Yong Huang
@ 2023-07-18 11:04       ` Markus Armbruster
  2023-07-19  4:10         ` Yong Huang
  0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2023-07-18 11:04 UTC (permalink / raw)
  To: Yong Huang
  Cc: ~hyman, qemu-devel, Peter Xu, Paolo Bonzini, Juan Quintela,
	Dr. David Alan Gilbert, Eric Blake, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé

Yong Huang <yong.huang@smartx.com> writes:

> On Thu, Jul 13, 2023 at 8:44 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> ~hyman <hyman@git.sr.ht> writes:
>>
>> > From: Hyman Huang(黄勇) <yong.huang@smartx.com>
>> >
>> > Introduce migration dirty-limit capability, which can
>> > be turned on before live migration and limit dirty
>> > page rate durty live migration.
>> >
>> > Introduce migrate_dirty_limit function to help check
>> > if dirty-limit capability enabled during live migration.
>> >
>> > Meanwhile, refactor vcpu_dirty_rate_stat_collect
>> > so that period can be configured instead of hardcoded.
>> >
>> > dirty-limit capability is kind of like auto-converge
>> > but using dirty limit instead of traditional cpu-throttle
>> > to throttle guest down. To enable this feature, turn on
>> > the dirty-limit capability before live migration using
>> > migrate-set-capabilities, and set the parameters
>> > "x-vcpu-dirty-limit-period", "vcpu-dirty-limit" suitably
>> > to speed up convergence.
>> >
>> > Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
>> > Acked-by: Peter Xu <peterx@redhat.com>
>> > Reviewed-by: Juan Quintela <quintela@redhat.com>
>>
>> [...]
>>
>> > diff --git a/qapi/migration.json b/qapi/migration.json
>> > index e43371955a..031832cde5 100644
>> > --- a/qapi/migration.json
>> > +++ b/qapi/migration.json
>> > @@ -497,6 +497,15 @@
>> >  #     are present.  'return-path' capability must be enabled to use
>> >  #     it.  (since 8.1)
>> >  #
>> > +# @dirty-limit: If enabled, migration will use the dirty-limit
>> > +#     algorithm to throttle down guest instead of auto-converge
>> > +#     algorithm. This algorithm only works when vCPU's dirtyrate
>>
>> Two spaces after sentence-ending punctuation, please.
>>
>> "dirty rate" with a space, because that's how we spell it elsewhere.
>>
>> > +#     greater than 'vcpu-dirty-limit', read processes in guest os
>> > +#     aren't penalized any more, so the algorithm can improve
>> > +#     performance of vCPU during live migration. This is an optional
>> > +#     performance feature and should not affect the correctness of the
>> > +#     existing auto-converge algorithm. (since 8.1)
>> > +#
>>
>> I'm still confused.
>>
>> The text suggests there are two separate algorithms "to throttle down
>> guest": "auto converge" and "dirty limit", and we get to pick one.
>> Correct?
>>
> Yes, indeed !
>
>>
>> If it is correct, then the last sentence feels redundant: picking
>> another algorithm can't affect the algorithm we're *not* using.  What
>> are you trying to express here?
>>
> What i want to express is that the new algorithm implementation does
> not affect the original algorithm, leaving it in the comments seems
> redundant indeed.  I'll drop this in the next version.

Works for me.

>> When do we use "auto converge", and when do we use "dirty limit"?
>>
>> What does the user really need to know about these algorithms?  Enough
>> to pick one, I guess.  That means advantages and disadvantages of the
>> two algorithms.  Which are?
>
> 1. The implementation of dirty-limit is based on dirty-ring, which is
> qualified
>    to big systems with huge memories and can improve huge guest VM
>     responsiveness remarkably during live migration. As a consequence,
> dirty-limit
>     is recommended on platforms with huge guest VMs as is the way with
> dirty-ring.
> 2. dirty-limit convergence algorithm does not affect the "read-process" in
> guest
>    VM, so guest VM gains the equal read performance nearly as it runs on
> host
>    during the live migration. As a result, dirty-limit is recommended if
> the guest
>     VM requires a stable read performance.
> The above explanation is about the recommendation of dirty-limit, please
> review,
> if it's ok, i'll place it in the comment of the dirty-limit capability.

Yes, please.  But before that, I have still more questions.  "This
algorithm only works when vCPU's dirtyrate greater than
'vcpu-dirty-limit'" is a condition: "FEATURE only works when CONDITION".
What happens when the condition is not met?  How can the user ensure the
condition is met?

[...]



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH QEMU v8 4/9] migration: Introduce dirty-limit capability
  2023-07-18 11:04       ` Markus Armbruster
@ 2023-07-19  4:10         ` Yong Huang
  2023-07-19  5:26           ` Markus Armbruster
  0 siblings, 1 reply; 20+ messages in thread
From: Yong Huang @ 2023-07-19  4:10 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: ~hyman, qemu-devel, Peter Xu, Paolo Bonzini, Juan Quintela,
	Dr. David Alan Gilbert, Eric Blake, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé

[-- Attachment #1: Type: text/plain, Size: 4824 bytes --]

On Tue, Jul 18, 2023 at 7:04 PM Markus Armbruster <armbru@redhat.com> wrote:

> Yong Huang <yong.huang@smartx.com> writes:
>
> > On Thu, Jul 13, 2023 at 8:44 PM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> ~hyman <hyman@git.sr.ht> writes:
> >>
> >> > From: Hyman Huang(黄勇) <yong.huang@smartx.com>
> >> >
> >> > Introduce migration dirty-limit capability, which can
> >> > be turned on before live migration and limit dirty
> >> > page rate durty live migration.
> >> >
> >> > Introduce migrate_dirty_limit function to help check
> >> > if dirty-limit capability enabled during live migration.
> >> >
> >> > Meanwhile, refactor vcpu_dirty_rate_stat_collect
> >> > so that period can be configured instead of hardcoded.
> >> >
> >> > dirty-limit capability is kind of like auto-converge
> >> > but using dirty limit instead of traditional cpu-throttle
> >> > to throttle guest down. To enable this feature, turn on
> >> > the dirty-limit capability before live migration using
> >> > migrate-set-capabilities, and set the parameters
> >> > "x-vcpu-dirty-limit-period", "vcpu-dirty-limit" suitably
> >> > to speed up convergence.
> >> >
> >> > Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
> >> > Acked-by: Peter Xu <peterx@redhat.com>
> >> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> >>
> >> [...]
> >>
> >> > diff --git a/qapi/migration.json b/qapi/migration.json
> >> > index e43371955a..031832cde5 100644
> >> > --- a/qapi/migration.json
> >> > +++ b/qapi/migration.json
> >> > @@ -497,6 +497,15 @@
> >> >  #     are present.  'return-path' capability must be enabled to use
> >> >  #     it.  (since 8.1)
> >> >  #
> >> > +# @dirty-limit: If enabled, migration will use the dirty-limit
> >> > +#     algorithm to throttle down guest instead of auto-converge
> >> > +#     algorithm. This algorithm only works when vCPU's dirtyrate
> >>
> >> Two spaces after sentence-ending punctuation, please.
> >>
> >> "dirty rate" with a space, because that's how we spell it elsewhere.
> >>
> >> > +#     greater than 'vcpu-dirty-limit', read processes in guest os
> >> > +#     aren't penalized any more, so the algorithm can improve
> >> > +#     performance of vCPU during live migration. This is an optional
> >> > +#     performance feature and should not affect the correctness of
> the
> >> > +#     existing auto-converge algorithm. (since 8.1)
> >> > +#
> >>
> >> I'm still confused.
> >>
> >> The text suggests there are two separate algorithms "to throttle down
> >> guest": "auto converge" and "dirty limit", and we get to pick one.
> >> Correct?
> >>
> > Yes, indeed !
> >
> >>
> >> If it is correct, then the last sentence feels redundant: picking
> >> another algorithm can't affect the algorithm we're *not* using.  What
> >> are you trying to express here?
> >>
> > What i want to express is that the new algorithm implementation does
> > not affect the original algorithm, leaving it in the comments seems
> > redundant indeed.  I'll drop this in the next version.
>
> Works for me.
>
> >> When do we use "auto converge", and when do we use "dirty limit"?
> >>
> >> What does the user really need to know about these algorithms?  Enough
> >> to pick one, I guess.  That means advantages and disadvantages of the
> >> two algorithms.  Which are?
> >
> > 1. The implementation of dirty-limit is based on dirty-ring, which is
> > qualified
> >    to big systems with huge memories and can improve huge guest VM
> >     responsiveness remarkably during live migration. As a consequence,
> > dirty-limit
> >     is recommended on platforms with huge guest VMs as is the way with
> > dirty-ring.
> > 2. dirty-limit convergence algorithm does not affect the "read-process"
> in
> > guest
> >    VM, so guest VM gains the equal read performance nearly as it runs on
> > host
> >    during the live migration. As a result, dirty-limit is recommended if
> > the guest
> >     VM requires a stable read performance.
> > The above explanation is about the recommendation of dirty-limit, please
> > review,
> > if it's ok, i'll place it in the comment of the dirty-limit capability.
>
> Yes, please.  But before that, I have still more questions.  "This
> algorithm only works when vCPU's dirtyrate greater than
> 'vcpu-dirty-limit'" is a condition: "FEATURE only works when CONDITION".
>
I failed to express my meaning again : ( .  "Throttle algo only works when
vCPU's  dirtyrate greater than 'vcpu-dirty-limit' " should change to
"vCPU throttle only works when vCPU's dirtyrate greater than
'vcpu-dirty-limit'".
Not the whole "algo" !

> What happens when the condition is not met?  How can the user ensure the
> condition is met?
>
> [...]
>
>

-- 
Best regards

[-- Attachment #2: Type: text/html, Size: 7499 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH QEMU v8 4/9] migration: Introduce dirty-limit capability
  2023-07-19  4:10         ` Yong Huang
@ 2023-07-19  5:26           ` Markus Armbruster
  2023-07-19  6:14             ` Yong Huang
  0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2023-07-19  5:26 UTC (permalink / raw)
  To: Yong Huang
  Cc: qemu-devel, Peter Xu, Paolo Bonzini, Juan Quintela,
	Dr. David Alan Gilbert, Eric Blake, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé

Yong Huang <yong.huang@smartx.com> writes:

> On Tue, Jul 18, 2023 at 7:04 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> Yong Huang <yong.huang@smartx.com> writes:
>>
>> > On Thu, Jul 13, 2023 at 8:44 PM Markus Armbruster <armbru@redhat.com>
>> wrote:
>> >
>> >> ~hyman <hyman@git.sr.ht> writes:
>> >>
>> >> > From: Hyman Huang(黄勇) <yong.huang@smartx.com>
>> >> >
>> >> > Introduce migration dirty-limit capability, which can
>> >> > be turned on before live migration and limit dirty
>> >> > page rate durty live migration.
>> >> >
>> >> > Introduce migrate_dirty_limit function to help check
>> >> > if dirty-limit capability enabled during live migration.
>> >> >
>> >> > Meanwhile, refactor vcpu_dirty_rate_stat_collect
>> >> > so that period can be configured instead of hardcoded.
>> >> >
>> >> > dirty-limit capability is kind of like auto-converge
>> >> > but using dirty limit instead of traditional cpu-throttle
>> >> > to throttle guest down. To enable this feature, turn on
>> >> > the dirty-limit capability before live migration using
>> >> > migrate-set-capabilities, and set the parameters
>> >> > "x-vcpu-dirty-limit-period", "vcpu-dirty-limit" suitably
>> >> > to speed up convergence.
>> >> >
>> >> > Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
>> >> > Acked-by: Peter Xu <peterx@redhat.com>
>> >> > Reviewed-by: Juan Quintela <quintela@redhat.com>
>> >>
>> >> [...]
>> >>
>> >> > diff --git a/qapi/migration.json b/qapi/migration.json
>> >> > index e43371955a..031832cde5 100644
>> >> > --- a/qapi/migration.json
>> >> > +++ b/qapi/migration.json
>> >> > @@ -497,6 +497,15 @@
>> >> >  #     are present.  'return-path' capability must be enabled to use
>> >> >  #     it.  (since 8.1)
>> >> >  #
>> >> > +# @dirty-limit: If enabled, migration will use the dirty-limit
>> >> > +#     algorithm to throttle down guest instead of auto-converge
>> >> > +#     algorithm. This algorithm only works when vCPU's dirtyrate
>> >>
>> >> Two spaces after sentence-ending punctuation, please.
>> >>
>> >> "dirty rate" with a space, because that's how we spell it elsewhere.
>> >>
>> >> > +#     greater than 'vcpu-dirty-limit', read processes in guest os
>> >> > +#     aren't penalized any more, so the algorithm can improve
>> >> > +#     performance of vCPU during live migration. This is an optional
>> >> > +#     performance feature and should not affect the correctness of
>> the
>> >> > +#     existing auto-converge algorithm. (since 8.1)
>> >> > +#
>> >>
>> >> I'm still confused.
>> >>
>> >> The text suggests there are two separate algorithms "to throttle down
>> >> guest": "auto converge" and "dirty limit", and we get to pick one.
>> >> Correct?
>> >>
>> > Yes, indeed !
>> >
>> >>
>> >> If it is correct, then the last sentence feels redundant: picking
>> >> another algorithm can't affect the algorithm we're *not* using.  What
>> >> are you trying to express here?
>> >>
>> > What i want to express is that the new algorithm implementation does
>> > not affect the original algorithm, leaving it in the comments seems
>> > redundant indeed.  I'll drop this in the next version.
>>
>> Works for me.
>>
>> >> When do we use "auto converge", and when do we use "dirty limit"?
>> >>
>> >> What does the user really need to know about these algorithms?  Enough
>> >> to pick one, I guess.  That means advantages and disadvantages of the
>> >> two algorithms.  Which are?
>> >
>> > 1. The implementation of dirty-limit is based on dirty-ring, which is
>> > qualified
>> >    to big systems with huge memories and can improve huge guest VM
>> >     responsiveness remarkably during live migration. As a consequence,
>> > dirty-limit
>> >     is recommended on platforms with huge guest VMs as is the way with
>> > dirty-ring.
>> > 2. dirty-limit convergence algorithm does not affect the "read-process"
>> in
>> > guest
>> >    VM, so guest VM gains the equal read performance nearly as it runs on
>> > host
>> >    during the live migration. As a result, dirty-limit is recommended if
>> > the guest
>> >     VM requires a stable read performance.
>> > The above explanation is about the recommendation of dirty-limit, please
>> > review,
>> > if it's ok, i'll place it in the comment of the dirty-limit capability.
>>
>> Yes, please.  But before that, I have still more questions.  "This
>> algorithm only works when vCPU's dirtyrate greater than
>> 'vcpu-dirty-limit'" is a condition: "FEATURE only works when CONDITION".
>>
> I failed to express my meaning again : ( .  "Throttle algo only works when
> vCPU's  dirtyrate greater than 'vcpu-dirty-limit' " should change to
> "vCPU throttle only works when vCPU's dirtyrate greater than
> 'vcpu-dirty-limit'".
> Not the whole "algo" !

Let me paraphrase to make sure I got it...  The vCPU is throttled as
needed to keep its dirty rate within the limit set with
set-vcpu-dirty-limit.  Correct?

What happens when I enable the dirty limit convergence algorithm without
setting a limit with set-vcpu-dirty-limit?

>> What happens when the condition is not met?  How can the user ensure the
>> condition is met?
>>
>> [...]
>>
>>



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH QEMU v8 4/9] migration: Introduce dirty-limit capability
  2023-07-19  5:26           ` Markus Armbruster
@ 2023-07-19  6:14             ` Yong Huang
  2023-07-19  9:03               ` Markus Armbruster
  0 siblings, 1 reply; 20+ messages in thread
From: Yong Huang @ 2023-07-19  6:14 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: qemu-devel, Peter Xu, Paolo Bonzini, Juan Quintela,
	Dr. David Alan Gilbert, Eric Blake, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé

[-- Attachment #1: Type: text/plain, Size: 6065 bytes --]

On Wed, Jul 19, 2023 at 1:26 PM Markus Armbruster <armbru@redhat.com> wrote:

> Yong Huang <yong.huang@smartx.com> writes:
>
> > On Tue, Jul 18, 2023 at 7:04 PM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> Yong Huang <yong.huang@smartx.com> writes:
> >>
> >> > On Thu, Jul 13, 2023 at 8:44 PM Markus Armbruster <armbru@redhat.com>
> >> wrote:
> >> >
> >> >> ~hyman <hyman@git.sr.ht> writes:
> >> >>
> >> >> > From: Hyman Huang(黄勇) <yong.huang@smartx.com>
> >> >> >
> >> >> > Introduce migration dirty-limit capability, which can
> >> >> > be turned on before live migration and limit dirty
> >> >> > page rate durty live migration.
> >> >> >
> >> >> > Introduce migrate_dirty_limit function to help check
> >> >> > if dirty-limit capability enabled during live migration.
> >> >> >
> >> >> > Meanwhile, refactor vcpu_dirty_rate_stat_collect
> >> >> > so that period can be configured instead of hardcoded.
> >> >> >
> >> >> > dirty-limit capability is kind of like auto-converge
> >> >> > but using dirty limit instead of traditional cpu-throttle
> >> >> > to throttle guest down. To enable this feature, turn on
> >> >> > the dirty-limit capability before live migration using
> >> >> > migrate-set-capabilities, and set the parameters
> >> >> > "x-vcpu-dirty-limit-period", "vcpu-dirty-limit" suitably
> >> >> > to speed up convergence.
> >> >> >
> >> >> > Signed-off-by: Hyman Huang(黄勇) <yong.huang@smartx.com>
> >> >> > Acked-by: Peter Xu <peterx@redhat.com>
> >> >> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> >> >>
> >> >> [...]
> >> >>
> >> >> > diff --git a/qapi/migration.json b/qapi/migration.json
> >> >> > index e43371955a..031832cde5 100644
> >> >> > --- a/qapi/migration.json
> >> >> > +++ b/qapi/migration.json
> >> >> > @@ -497,6 +497,15 @@
> >> >> >  #     are present.  'return-path' capability must be enabled to
> use
> >> >> >  #     it.  (since 8.1)
> >> >> >  #
> >> >> > +# @dirty-limit: If enabled, migration will use the dirty-limit
> >> >> > +#     algorithm to throttle down guest instead of auto-converge
> >> >> > +#     algorithm. This algorithm only works when vCPU's dirtyrate
> >> >>
> >> >> Two spaces after sentence-ending punctuation, please.
> >> >>
> >> >> "dirty rate" with a space, because that's how we spell it elsewhere.
> >> >>
> >> >> > +#     greater than 'vcpu-dirty-limit', read processes in guest os
> >> >> > +#     aren't penalized any more, so the algorithm can improve
> >> >> > +#     performance of vCPU during live migration. This is an
> optional
> >> >> > +#     performance feature and should not affect the correctness of
> >> the
> >> >> > +#     existing auto-converge algorithm. (since 8.1)
> >> >> > +#
> >> >>
> >> >> I'm still confused.
> >> >>
> >> >> The text suggests there are two separate algorithms "to throttle down
> >> >> guest": "auto converge" and "dirty limit", and we get to pick one.
> >> >> Correct?
> >> >>
> >> > Yes, indeed !
> >> >
> >> >>
> >> >> If it is correct, then the last sentence feels redundant: picking
> >> >> another algorithm can't affect the algorithm we're *not* using.  What
> >> >> are you trying to express here?
> >> >>
> >> > What i want to express is that the new algorithm implementation does
> >> > not affect the original algorithm, leaving it in the comments seems
> >> > redundant indeed.  I'll drop this in the next version.
> >>
> >> Works for me.
> >>
> >> >> When do we use "auto converge", and when do we use "dirty limit"?
> >> >>
> >> >> What does the user really need to know about these algorithms?
> Enough
> >> >> to pick one, I guess.  That means advantages and disadvantages of the
> >> >> two algorithms.  Which are?
> >> >
> >> > 1. The implementation of dirty-limit is based on dirty-ring, which is
> >> > qualified
> >> >    to big systems with huge memories and can improve huge guest VM
> >> >     responsiveness remarkably during live migration. As a consequence,
> >> > dirty-limit
> >> >     is recommended on platforms with huge guest VMs as is the way with
> >> > dirty-ring.
> >> > 2. dirty-limit convergence algorithm does not affect the
> "read-process"
> >> in
> >> > guest
> >> >    VM, so guest VM gains the equal read performance nearly as it runs
> on
> >> > host
> >> >    during the live migration. As a result, dirty-limit is recommended
> if
> >> > the guest
> >> >     VM requires a stable read performance.
> >> > The above explanation is about the recommendation of dirty-limit,
> please
> >> > review,
> >> > if it's ok, i'll place it in the comment of the dirty-limit
> capability.
> >>
> >> Yes, please.  But before that, I have still more questions.  "This
> >> algorithm only works when vCPU's dirtyrate greater than
> >> 'vcpu-dirty-limit'" is a condition: "FEATURE only works when CONDITION".
> >>
> > I failed to express my meaning again : ( .  "Throttle algo only works
> when
> > vCPU's  dirtyrate greater than 'vcpu-dirty-limit' " should change to
> > "vCPU throttle only works when vCPU's dirtyrate greater than
> > 'vcpu-dirty-limit'".
> > Not the whole "algo" !
>
> Let me paraphrase to make sure I got it...  The vCPU is throttled as
> needed to keep its dirty rate within the limit set with
> set-vcpu-dirty-limit.  Correct?
>
Yes. Actually set with the internal function qmp_set_vcpu_dirty_limit.

And a parameter called "vcpu-dirty-limit"  of migration provided by
dirty-limit
aims to be the argument of qmp_set_vcpu_dirty_limit.


> What happens when I enable the dirty limit convergence algorithm without
> setting a limit with set-vcpu-dirty-limit?
>
dirty-limit will use the default value which is defined
in migration/options.c:
#define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT            1       /* MB/s */

So the default of the dirty-limit is 1MB/s.

>
> >> What happens when the condition is not met?  How can the user ensure the
> >> condition is met?
> >>
> >> [...]
> >>
> >>
>
>

-- 
Best regards

[-- Attachment #2: Type: text/html, Size: 10226 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH QEMU v8 4/9] migration: Introduce dirty-limit capability
  2023-07-19  6:14             ` Yong Huang
@ 2023-07-19  9:03               ` Markus Armbruster
  2023-07-19  9:31                 ` Yong Huang
  0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2023-07-19  9:03 UTC (permalink / raw)
  To: Yong Huang
  Cc: qemu-devel, Peter Xu, Paolo Bonzini, Juan Quintela,
	Dr. David Alan Gilbert, Eric Blake, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé

Yong Huang <yong.huang@smartx.com> writes:

> On Wed, Jul 19, 2023 at 1:26 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> Yong Huang <yong.huang@smartx.com> writes:
>>
>> > On Tue, Jul 18, 2023 at 7:04 PM Markus Armbruster <armbru@redhat.com>
>> wrote:
>> >
>> >> Yong Huang <yong.huang@smartx.com> writes:
>> >>
>> >> > On Thu, Jul 13, 2023 at 8:44 PM Markus Armbruster <armbru@redhat.com> wrote:

[...]

>> >> Yes, please.  But before that, I have still more questions.  "This
>> >> algorithm only works when vCPU's dirtyrate greater than
>> >> 'vcpu-dirty-limit'" is a condition: "FEATURE only works when CONDITION".
>> >>
>> > I failed to express my meaning again : ( .  "Throttle algo only works when
>> > vCPU's  dirtyrate greater than 'vcpu-dirty-limit' " should change to
>> > "vCPU throttle only works when vCPU's dirtyrate greater than
>> > 'vcpu-dirty-limit'".
>> > Not the whole "algo" !
>>
>> Let me paraphrase to make sure I got it...  The vCPU is throttled as
>> needed to keep its dirty rate within the limit set with
>> set-vcpu-dirty-limit.  Correct?
>>
> Yes. Actually set with the internal function qmp_set_vcpu_dirty_limit.
>
> And a parameter called "vcpu-dirty-limit"  of migration provided by
> dirty-limit
> aims to be the argument of qmp_set_vcpu_dirty_limit.

Alright, let me try to craft some documentation:

  # @dirty-limit: If enabled, migration will throttle vCPUs as needed to
  #     keep their dirty page rate within @vcpu-dirty-limit.  This can
  #     improve responsiveness of large guests during live migration,
  #     and can result in more stable read performance.  Requires KVM
  #     with accelerator property "dirty-ring-size" set.  (Since 8.1)

What do you think?

>> What happens when I enable the dirty limit convergence algorithm without
>> setting a limit with set-vcpu-dirty-limit?
>>
> dirty-limit will use the default value which is defined
> in migration/options.c:
> #define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT            1       /* MB/s */
>
> So the default of the dirty-limit is 1MB/s.

Is this default documented in the QAPI schema?  Hmm, looks like it isn't
before this series, but PATCH 3 fixes it.  Okay.

>> >> What happens when the condition is not met?  How can the user ensure the
>> >> condition is met?
>> >>
>> >> [...]



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH QEMU v8 4/9] migration: Introduce dirty-limit capability
  2023-07-19  9:03               ` Markus Armbruster
@ 2023-07-19  9:31                 ` Yong Huang
  0 siblings, 0 replies; 20+ messages in thread
From: Yong Huang @ 2023-07-19  9:31 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: qemu-devel, Peter Xu, Paolo Bonzini, Juan Quintela,
	Dr. David Alan Gilbert, Eric Blake, Thomas Huth, Laurent Vivier,
	Richard Henderson, Philippe Mathieu-Daudé

[-- Attachment #1: Type: text/plain, Size: 2704 bytes --]

On Wed, Jul 19, 2023 at 5:03 PM Markus Armbruster <armbru@redhat.com> wrote:

> Yong Huang <yong.huang@smartx.com> writes:
>
> > On Wed, Jul 19, 2023 at 1:26 PM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> Yong Huang <yong.huang@smartx.com> writes:
> >>
> >> > On Tue, Jul 18, 2023 at 7:04 PM Markus Armbruster <armbru@redhat.com>
> >> wrote:
> >> >
> >> >> Yong Huang <yong.huang@smartx.com> writes:
> >> >>
> >> >> > On Thu, Jul 13, 2023 at 8:44 PM Markus Armbruster <
> armbru@redhat.com> wrote:
>
> [...]
>
> >> >> Yes, please.  But before that, I have still more questions.  "This
> >> >> algorithm only works when vCPU's dirtyrate greater than
> >> >> 'vcpu-dirty-limit'" is a condition: "FEATURE only works when
> CONDITION".
> >> >>
> >> > I failed to express my meaning again : ( .  "Throttle algo only works
> when
> >> > vCPU's  dirtyrate greater than 'vcpu-dirty-limit' " should change to
> >> > "vCPU throttle only works when vCPU's dirtyrate greater than
> >> > 'vcpu-dirty-limit'".
> >> > Not the whole "algo" !
> >>
> >> Let me paraphrase to make sure I got it...  The vCPU is throttled as
> >> needed to keep its dirty rate within the limit set with
> >> set-vcpu-dirty-limit.  Correct?
> >>
> > Yes. Actually set with the internal function qmp_set_vcpu_dirty_limit.
> >
> > And a parameter called "vcpu-dirty-limit"  of migration provided by
> > dirty-limit
> > aims to be the argument of qmp_set_vcpu_dirty_limit.
>
> Alright, let me try to craft some documentation:
>
>   # @dirty-limit: If enabled, migration will throttle vCPUs as needed to
>   #     keep their dirty page rate within @vcpu-dirty-limit.  This can
>   #     improve responsiveness of large guests during live migration,
>   #     and can result in more stable read performance.  Requires KVM
>   #     with accelerator property "dirty-ring-size" set.  (Since 8.1)
>
> What do you think?
>
I feel ok, it seems clear and concise.
I'll use this comment in the next version. Thanks a lot. :)

>
> >> What happens when I enable the dirty limit convergence algorithm without
> >> setting a limit with set-vcpu-dirty-limit?
> >>
> > dirty-limit will use the default value which is defined
> > in migration/options.c:
> > #define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT            1       /* MB/s */
> >
> > So the default of the dirty-limit is 1MB/s.
>
> Is this default documented in the QAPI schema?  Hmm, looks like it isn't
> before this series, but PATCH 3 fixes it.  Okay.
>
> >> >> What happens when the condition is not met?  How can the user ensure
> the
> >> >> condition is met?
> >> >>
> >> >> [...]
>
>

-- 
Best regards

[-- Attachment #2: Type: text/html, Size: 4619 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2023-07-19  9:34 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-07  4:10 [PATCH QEMU v8 0/9] migration: introduce dirtylimit capability ~hyman
2022-11-18  2:08 ` [PATCH QEMU v8 1/9] softmmu/dirtylimit: Add parameter check for hmp "set_vcpu_dirty_limit" ~hyman
2023-06-07 13:32 ` [PATCH QEMU v8 2/9] qapi/migration: Introduce x-vcpu-dirty-limit-period parameter ~hyman
2023-07-13 12:32   ` Markus Armbruster
2023-06-07 14:58 ` [PATCH QEMU v8 3/9] qapi/migration: Introduce vcpu-dirty-limit parameters ~hyman
2023-06-07 15:30 ` [PATCH QEMU v8 4/9] migration: Introduce dirty-limit capability ~hyman
2023-07-13 12:44   ` Markus Armbruster
2023-07-18  1:42     ` Yong Huang
2023-07-18 11:04       ` Markus Armbruster
2023-07-19  4:10         ` Yong Huang
2023-07-19  5:26           ` Markus Armbruster
2023-07-19  6:14             ` Yong Huang
2023-07-19  9:03               ` Markus Armbruster
2023-07-19  9:31                 ` Yong Huang
2023-06-07 15:32 ` [PATCH QEMU v8 5/9] migration: Refactor auto-converge capability logic ~hyman
2023-06-07 16:12 ` [PATCH QEMU v8 7/9] migration: Implement dirty-limit convergence algo ~hyman
2023-06-07 16:21 ` [PATCH QEMU v8 8/9] migration: Extend query-migrate to provide dirty page limit info ~hyman
2023-07-13 12:45   ` Markus Armbruster
2023-06-07 16:46 ` [PATCH QEMU v8 9/9] tests: Add migration dirty-limit capability test ~hyman
2023-06-15 13:29 ` [PATCH QEMU v8 6/9] migration: Put the detection logic before auto-converge checking ~hyman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).