diff mbox

[v4,12/13] migration: Add command to set migration parameter

Message ID 1422875149-13198-13-git-send-email-liang.z.li@intel.com
State New
Headers show

Commit Message

Li, Liang Z Feb. 2, 2015, 11:05 a.m. UTC
Add the qmp and hmp commands to tune the parameters used in live
migration.

Signed-off-by: Liang Li <liang.z.li@intel.com>
Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
---
 hmp-commands.hx               | 15 ++++++++++
 hmp.c                         | 35 ++++++++++++++++++++++
 hmp.h                         |  3 ++
 include/migration/migration.h |  4 +--
 migration/migration.c         | 69 +++++++++++++++++++++++++++++++++++--------
 monitor.c                     | 18 +++++++++++
 qapi-schema.json              | 52 ++++++++++++++++++++++++++++++++
 qmp-commands.hx               | 25 ++++++++++++++++
 8 files changed, 206 insertions(+), 15 deletions(-)

Comments

Eric Blake Feb. 3, 2015, 11:28 p.m. UTC | #1
On 02/02/2015 04:05 AM, Liang Li wrote:
> Add the qmp and hmp commands to tune the parameters used in live
> migration.
> 
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> ---
>  hmp-commands.hx               | 15 ++++++++++
>  hmp.c                         | 35 ++++++++++++++++++++++
>  hmp.h                         |  3 ++
>  include/migration/migration.h |  4 +--
>  migration/migration.c         | 69 +++++++++++++++++++++++++++++++++++--------
>  monitor.c                     | 18 +++++++++++
>  qapi-schema.json              | 52 ++++++++++++++++++++++++++++++++
>  qmp-commands.hx               | 25 ++++++++++++++++
>  8 files changed, 206 insertions(+), 15 deletions(-)

> +++ b/migration/migration.c
> @@ -66,9 +66,12 @@ MigrationState *migrate_get_current(void)
>          .bandwidth_limit = MAX_THROTTLE,
>          .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
>          .mbps = -1,
> -        .compress_thread_count = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
> -        .decompress_thread_count = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
> -        .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
> +        .parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL] =
> +                DEFAULT_MIGRATE_COMPRESS_LEVEL,

Looks okay.

> +        .parameters[MIGRATION_PARAMETER_COMPRESS_THREADS] =
> +                DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
> +        .parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] =
> +                DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,

Hmm - do we really need two parameters here?  Remember, compress threads
is used only on the source, and decompress threads is used only on the
destination.  Having a single parameter, 'threads', which is set to
compression threads on source and decompression threads on destination,
and which need not be equal between the two machines, should still work,
right?

> +++ b/qapi-schema.json
> @@ -541,6 +541,58 @@
>  ##
>  { 'command': 'query-migrate-capabilities', 'returns':   ['MigrationCapabilityStatus']}
>  
> +# @MigrationParameter
> +#
> +# Migration parameters enumeration
> +#
> +# @compress-level: Set the compression level to be used in live migration,
> +#          the compression level is an integer between 0 and 9, where 0 means
> +#          no compression, 1 means the best compression speed, and 9 means best
> +#          compression ratio which will consume more CPU.
> +#
> +# @compress-threads: Set compression thread count to be used in live migration,
> +#          the compression thread count is an integer between 1 and 255.
> +#
> +# @decompress-threads: Set decompression thread count to be used in live migration,
> +#          the decompression thread count is an integer between 1 and 255.
> +#

Again, I think you could get by with just a single parameter
'compress-threads', and maybe document that the value is typically set
higher on destination than on source.

> +# Since: 2.3
> +##
> +{ 'enum': 'MigrationParameter',
> +  'data': ['compress-level', 'compress-threads', 'decompress-threads'] }
> +##
> +# @MigrationParameterStatus
> +#
> +# Migration parameter information
> +#
> +# @parameter: the parameter of migration
> +#
> +# @data: pointer to the parameter value
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'MigrationParameterBase',
> +  'data': {'parameter': 'MigrationParameter'} }
> +{ 'type': 'MigrationParameterInt',
> +  'data': {'value': 'int'} }
> +{ 'union': 'MigrationParameterStatus',

Is it worth having independent docs for each of these, rather than
cramming all three under one doc text?

> +  'base': 'MigrationParameterBase',
> +  'discriminator': 'parameter',
> +  'data': { 'compress-level': 'MigrationParameterInt',
> +            'compress-threads': 'MigrationParameterInt',
> +            'decompress-threads': 'MigrationParameterInt'} }
> +#
> +# @migrate-set-parameters
> +#
> +# Set the following migration parameters (like compress-level)
> +#
> +# @parameters: json array of parameter modifications to make
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'migrate-set-parameters',
> +  'data': { 'parameters': ['MigrationParameterStatus'] } }
> +##

Interface looks reasonable to me (but I'm biased, as I suggested it).
Li, Liang Z Feb. 4, 2015, 1:26 a.m. UTC | #2
> > +++ b/migration/migration.c

> > @@ -66,9 +66,12 @@ MigrationState *migrate_get_current(void)

> >          .bandwidth_limit = MAX_THROTTLE,

> >          .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,

> >          .mbps = -1,

> > -        .compress_thread_count =

> DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,

> > -        .decompress_thread_count =

> DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,

> > -        .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,

> > +        .parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL] =

> > +                DEFAULT_MIGRATE_COMPRESS_LEVEL,

> 

> Looks okay.

> 

> > +        .parameters[MIGRATION_PARAMETER_COMPRESS_THREADS] =

> > +                DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,

> > +        .parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] =

> > +                DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,

> 

> Hmm - do we really need two parameters here?  Remember, compress

> threads is used only on the source, and decompress threads is used only on

> the destination.  Having a single parameter, 'threads', which is set to

> compression threads on source and decompression threads on destination,

> and which need not be equal between the two machines, should still work,

> right?

>


Yes, it works. The benefit of using one parameter instead of two can reduce the QMP 
command count, and the side effect of using the same thread count for compression
 and decompression is a little waste if the user just want to use the default settings,
you know, decompression is usually  about 4 times faster than compression.  Use more
decompression threads than needed will waste some RAM which used to save data 
structure related to the decompression thread, about 4K bytes RAM per thread, is it 
acceptable?

Liang
Eric Blake Feb. 4, 2015, 2:27 a.m. UTC | #3
On 02/03/2015 06:26 PM, Li, Liang Z wrote:

>> Hmm - do we really need two parameters here?  Remember, compress
>> threads is used only on the source, and decompress threads is used only on
>> the destination.  Having a single parameter, 'threads', which is set to
>> compression threads on source and decompression threads on destination,
>> and which need not be equal between the two machines, should still work,
>> right?
>>
> 
> Yes, it works. The benefit of using one parameter instead of two can reduce the QMP 
> command count, and the side effect of using the same thread count for compression
>  and decompression is a little waste if the user just want to use the default settings,
> you know, decompression is usually  about 4 times faster than compression.  Use more
> decompression threads than needed will waste some RAM which used to save data 
> structure related to the decompression thread, about 4K bytes RAM per thread, is it 
> acceptable?

The default setting is no compression.  The user already has to
configure things on both sides to get compression, so it is not a burden
to ask them to configure thread count on both sides correctly.
diff mbox

Patch

diff --git a/hmp-commands.hx b/hmp-commands.hx
index e37bc8b..535b5ba 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -985,6 +985,21 @@  Enable/Disable the usage of a capability @var{capability} for migration.
 ETEXI
 
     {
+        .name       = "migrate_set_parameter",
+        .args_type  = "parameter:s,value:i",
+        .params     = "parameter value",
+        .help       = "Set the parameter for migration",
+        .mhandler.cmd = hmp_migrate_set_parameter,
+        .command_completion = migrate_set_parameter_completion,
+    },
+
+STEXI
+@item migrate_set_parameter @var{parameter} @var{value}
+@findex migrate_set_parameter
+Set the parameter @var{parameter} for migration.
+ETEXI
+
+    {
         .name       = "client_migrate_info",
         .args_type  = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
         .params     = "protocol hostname port tls-port cert-subject",
diff --git a/hmp.c b/hmp.c
index 481be80..faab4b0 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1134,6 +1134,41 @@  void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict)
     }
 }
 
+void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
+{
+    const char *param = qdict_get_str(qdict, "parameter");
+    int value = qdict_get_int(qdict, "value");
+    Error *err = NULL;
+    MigrationParameterStatusList *params = g_malloc0(sizeof(*params));
+    MigrationParameterInt *data;
+    int i;
+
+    for (i = 0; i < MIGRATION_PARAMETER_MAX; i++) {
+        if (strcmp(param, MigrationParameter_lookup[i]) == 0) {
+            params->value = g_malloc0(sizeof(*params->value));
+            params->value->kind = i;
+            params->value->data = g_malloc0(sizeof(MigrationParameterInt));
+            data = (MigrationParameterInt *)params->value->data;
+            data->value = value;
+            params->next = NULL;
+            qmp_migrate_set_parameters(params, &err);
+            break;
+        }
+    }
+
+    if (i == MIGRATION_PARAMETER_MAX) {
+        error_set(&err, QERR_INVALID_PARAMETER, param);
+    }
+
+    qapi_free_MigrationParameterStatusList(params);
+
+    if (err) {
+        monitor_printf(mon, "migrate_set_parameter: %s\n",
+                       error_get_pretty(err));
+        error_free(err);
+    }
+}
+
 void hmp_set_password(Monitor *mon, const QDict *qdict)
 {
     const char *protocol  = qdict_get_str(qdict, "protocol");
diff --git a/hmp.h b/hmp.h
index 4bb5dca..429efea 100644
--- a/hmp.h
+++ b/hmp.h
@@ -63,6 +63,7 @@  void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict);
+void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict);
 void hmp_set_password(Monitor *mon, const QDict *qdict);
 void hmp_expire_password(Monitor *mon, const QDict *qdict);
@@ -111,6 +112,8 @@  void watchdog_action_completion(ReadLineState *rs, int nb_args,
                                 const char *str);
 void migrate_set_capability_completion(ReadLineState *rs, int nb_args,
                                        const char *str);
+void migrate_set_parameter_completion(ReadLineState *rs, int nb_args,
+                                      const char *str);
 void host_net_add_completion(ReadLineState *rs, int nb_args, const char *str);
 void host_net_remove_completion(ReadLineState *rs, int nb_args,
                                 const char *str);
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 0c4f21c..8e09b42 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -50,9 +50,7 @@  struct MigrationState
     QEMUBH *cleanup_bh;
     QEMUFile *file;
     QemuThread *compress_thread;
-    int compress_thread_count;
-    int decompress_thread_count;
-    int compress_level;
+    int parameters[MIGRATION_PARAMETER_MAX];
 
     int state;
     MigrationParams params;
diff --git a/migration/migration.c b/migration/migration.c
index cbbd455..b5055dc 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -66,9 +66,12 @@  MigrationState *migrate_get_current(void)
         .bandwidth_limit = MAX_THROTTLE,
         .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
         .mbps = -1,
-        .compress_thread_count = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
-        .decompress_thread_count = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
-        .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
+        .parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL] =
+                DEFAULT_MIGRATE_COMPRESS_LEVEL,
+        .parameters[MIGRATION_PARAMETER_COMPRESS_THREADS] =
+                DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
+        .parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] =
+                DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
     };
 
     return &current_migration;
@@ -292,6 +295,44 @@  void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
     }
 }
 
+void qmp_migrate_set_parameters(MigrationParameterStatusList *params,
+                                Error **errp)
+{
+    MigrationState *s = migrate_get_current();
+    MigrationParameterStatusList *p;
+    MigrationParameterInt *data;
+
+    for (p = params; p; p = p->next) {
+        switch (p->value->kind) {
+        case MIGRATION_PARAMETER_COMPRESS_LEVEL:
+            data = (MigrationParameterInt *)p->value->data;
+            if (data->value < 0 || data->value > 9) {
+                error_set(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
+                          "is invalid, it should be in the range of 0 to 9");
+                return;
+            }
+            break;
+        case MIGRATION_PARAMETER_COMPRESS_THREADS:
+        case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
+            if (s->state == MIG_STATE_ACTIVE || s->state == MIG_STATE_SETUP) {
+                error_set(errp, QERR_MIGRATION_ACTIVE);
+                return;
+            }
+            data = (MigrationParameterInt *)p->value->data;
+            if (data->value < 1 || data->value > 255) {
+                error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                          "(de)compress_threads",
+                          "is invalid, it should be in the range of 1 to 255");
+                return;
+            }
+            break;
+        default:
+           return;
+        }
+        s->parameters[p->value->kind] = data->value;
+    }
+}
+
 /* shared migration helpers */
 
 static void migrate_set_state(MigrationState *s, int old_state, int new_state)
@@ -398,9 +439,11 @@  static MigrationState *migrate_init(const MigrationParams *params)
     int64_t bandwidth_limit = s->bandwidth_limit;
     bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
     int64_t xbzrle_cache_size = s->xbzrle_cache_size;
-    int compress_level = s->compress_level;
-    int compress_thread_count = s->compress_thread_count;
-    int decompress_thread_count = s->decompress_thread_count;
+    int compress_level = s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL];
+    int compress_thread_count =
+            s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS];
+    int decompress_thread_count =
+            s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS];
 
     memcpy(enabled_capabilities, s->enabled_capabilities,
            sizeof(enabled_capabilities));
@@ -411,9 +454,11 @@  static MigrationState *migrate_init(const MigrationParams *params)
            sizeof(enabled_capabilities));
     s->xbzrle_cache_size = xbzrle_cache_size;
 
-    s->compress_level = compress_level;
-    s->compress_thread_count = compress_thread_count;
-    s->decompress_thread_count = decompress_thread_count;
+    s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL] = compress_level;
+    s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS] =
+               compress_thread_count;
+    s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] =
+               decompress_thread_count;
     s->bandwidth_limit = bandwidth_limit;
     s->state = MIG_STATE_SETUP;
     trace_migrate_set_state(MIG_STATE_SETUP);
@@ -601,7 +646,7 @@  int migrate_compress_level(void)
 
     s = migrate_get_current();
 
-    return s->compress_level;
+    return s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL];
 }
 
 int migrate_compress_threads(void)
@@ -610,7 +655,7 @@  int migrate_compress_threads(void)
 
     s = migrate_get_current();
 
-    return s->compress_thread_count;
+    return s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS];
 }
 
 int migrate_decompress_threads(void)
@@ -619,7 +664,7 @@  int migrate_decompress_threads(void)
 
     s = migrate_get_current();
 
-    return s->decompress_thread_count;
+    return s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS];
 }
 
 int migrate_use_xbzrle(void)
diff --git a/monitor.c b/monitor.c
index 7e4f605..fa8ebde 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4554,6 +4554,24 @@  void migrate_set_capability_completion(ReadLineState *rs, int nb_args,
     }
 }
 
+void migrate_set_parameter_completion(ReadLineState *rs, int nb_args,
+                                      const char *str)
+{
+    size_t len;
+
+    len = strlen(str);
+    readline_set_completion_index(rs, len);
+    if (nb_args == 2) {
+        int i;
+        for (i = 0; i < MIGRATION_PARAMETER_MAX; i++) {
+            const char *name = MigrationParameter_lookup[i];
+            if (!strncmp(str, name, len)) {
+                readline_add_completion(rs, name);
+            }
+        }
+    }
+}
+
 void host_net_add_completion(ReadLineState *rs, int nb_args, const char *str)
 {
     int i;
diff --git a/qapi-schema.json b/qapi-schema.json
index 0dfc4ce..273f991 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -541,6 +541,58 @@ 
 ##
 { 'command': 'query-migrate-capabilities', 'returns':   ['MigrationCapabilityStatus']}
 
+# @MigrationParameter
+#
+# Migration parameters enumeration
+#
+# @compress-level: Set the compression level to be used in live migration,
+#          the compression level is an integer between 0 and 9, where 0 means
+#          no compression, 1 means the best compression speed, and 9 means best
+#          compression ratio which will consume more CPU.
+#
+# @compress-threads: Set compression thread count to be used in live migration,
+#          the compression thread count is an integer between 1 and 255.
+#
+# @decompress-threads: Set decompression thread count to be used in live migration,
+#          the decompression thread count is an integer between 1 and 255.
+#
+# Since: 2.3
+##
+{ 'enum': 'MigrationParameter',
+  'data': ['compress-level', 'compress-threads', 'decompress-threads'] }
+##
+# @MigrationParameterStatus
+#
+# Migration parameter information
+#
+# @parameter: the parameter of migration
+#
+# @data: pointer to the parameter value
+#
+# Since: 2.3
+##
+{ 'type': 'MigrationParameterBase',
+  'data': {'parameter': 'MigrationParameter'} }
+{ 'type': 'MigrationParameterInt',
+  'data': {'value': 'int'} }
+{ 'union': 'MigrationParameterStatus',
+  'base': 'MigrationParameterBase',
+  'discriminator': 'parameter',
+  'data': { 'compress-level': 'MigrationParameterInt',
+            'compress-threads': 'MigrationParameterInt',
+            'decompress-threads': 'MigrationParameterInt'} }
+#
+# @migrate-set-parameters
+#
+# Set the following migration parameters (like compress-level)
+#
+# @parameters: json array of parameter modifications to make
+#
+# Since: 2.3
+##
+{ 'command': 'migrate-set-parameters',
+  'data': { 'parameters': ['MigrationParameterStatus'] } }
+##
 ##
 # @MouseInfo:
 #
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c5f16dd..9d16386 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3278,6 +3278,31 @@  EQMP
     },
 
 SQMP
+migrate-set-parameters
+----------------------
+
+Set migration parameters
+
+- "compress-level": set compression level during migration
+- "compress-threads": set compression thread count for migration
+- "decompress-threads": set decompression thread count for migration
+
+Arguments:
+
+Example:
+
+-> { "execute": "migrate-set-parameters" , "arguments":
+     { "parameters": [ { "parameter": "compress-level", "value": 1 } ] } }
+
+EQMP
+
+    {
+        .name       = "migrate-set-parameters",
+        .args_type  = "parameters:O",
+        .params     = "parameter:s,value:O",
+	.mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters,
+    },
+SQMP
 query-balloon
 -------------