diff mbox

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

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

Commit Message

Li, Liang Z Dec. 12, 2014, 1:29 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                         | 32 +++++++++++++++++++++
 hmp.h                         |  3 ++
 include/migration/migration.h |  4 +--
 migration.c                   | 66 +++++++++++++++++++++++++++++++++++--------
 monitor.c                     | 18 ++++++++++++
 qapi-schema.json              | 44 +++++++++++++++++++++++++++++
 qmp-commands.hx               | 23 +++++++++++++++
 8 files changed, 190 insertions(+), 15 deletions(-)

Comments

Dr. David Alan Gilbert Jan. 23, 2015, 1:48 p.m. UTC | #1 
* Liang Li (liang.z.li@intel.com) wrote:
> Add the qmp and hmp commands to tune the parameters used in live
> migration.

If I understand correctly on the destination side we need to set the number of
decompression threads very early on an incoming migration - I'm not
clear how early that needs to be - especially if you're using fd: so it's
not waiting for a connect ?

Eric: How would libvirt do that?

> 
> 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                         | 32 +++++++++++++++++++++
>  hmp.h                         |  3 ++
>  include/migration/migration.h |  4 +--
>  migration.c                   | 66 +++++++++++++++++++++++++++++++++++--------
>  monitor.c                     | 18 ++++++++++++
>  qapi-schema.json              | 44 +++++++++++++++++++++++++++++
>  qmp-commands.hx               | 23 +++++++++++++++
>  8 files changed, 190 insertions(+), 15 deletions(-)
> 
> 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 63d7686..965c037 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1079,6 +1079,38 @@ 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));
> +    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->parameter = i;
> +            params->value->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..bd1b203 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.c b/migration.c
> index 9d1613d..d3d377e 100644
> --- a/migration.c
> +++ b/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,41 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>      }
>  }
>  
> +void qmp_migrate_set_parameters(MigrationParameterStatusList *params,
> +                                  Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +    MigrationParameterStatusList *p;
> +
> +    for (p = params; p; p = p->next) {
> +        switch (p->value->parameter) {
> +        case MIGRATION_PARAMETER_COMPRESS_LEVEL:
> +            if (p->value->value < 0 || p->value->value > 9) {
> +                error_set(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
> +                    "is invalied, it should be in the rang of 0 to 9");

Typo: 'rang'

> +                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;
> +            }
> +            if (p->value->value < 1 || p->value->value > 255) {
> +                error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> +                    "(de)compress_threads",
> +                    "is invalied, it should be in the rang of 1 to 255");

Typo: 'rang'

> +                return;
> +            }
> +            break;
> +        default:
> +           return;
> +        }
> +        s->parameters[p->value->parameter] = p->value->value;
> +    }
> +}

Do you need the same check here that is in qmp_migrate_set_capabilities;
to stop someone changing a parameter (e.g. the number of threads) while
it's running.

Dave

> +
>  /* shared migration helpers */
>  
>  static void migrate_set_state(MigrationState *s, int old_state, int new_state)
> @@ -386,9 +424,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));
> @@ -399,9 +439,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);
> @@ -589,7 +631,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)
> @@ -598,7 +640,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)
> @@ -607,7 +649,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 f1031a1..4cf62b6 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4544,6 +4544,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 d371af3..2caeccc 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -540,6 +540,50 @@
>  ##
>  { '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: parameter enum
> +#
> +# @value: parameter value int
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'MigrationParameterStatus',
> +  'data': { 'parameter' : 'MigrationParameter', 'value' : 'int' } }
> +##
> +# @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 718dd92..59d2643 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3225,6 +3225,29 @@ EQMP
>      },
>  
>  SQMP
> +migrate-set-parameters
> +----------------------
> +
> +Set migration parameters
> +
> +- "compress-leve": multiple compression thread support

Typo: 'compress-leve'

> +
> +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:i",
> +	.mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters,
> +    },
> +SQMP
>  query-balloon
>  -------------
>  
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Eric Blake Jan. 23, 2015, 3:39 p.m. UTC | #2 
On 12/11/2014 06:29 PM, 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                         | 32 +++++++++++++++++++++
>  hmp.h                         |  3 ++
>  include/migration/migration.h |  4 +--
>  migration.c                   | 66 +++++++++++++++++++++++++++++++++++--------
>  monitor.c                     | 18 ++++++++++++
>  qapi-schema.json              | 44 +++++++++++++++++++++++++++++
>  qmp-commands.hx               | 23 +++++++++++++++
>  8 files changed, 190 insertions(+), 15 deletions(-)
> 


> +++ 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);

Off-by-one on indentation.


> @@ -292,6 +295,41 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>      }
>  }
>  
> +void qmp_migrate_set_parameters(MigrationParameterStatusList *params,
> +                                  Error **errp)

Indentation is off.

> +{
> +    MigrationState *s = migrate_get_current();
> +    MigrationParameterStatusList *p;
> +
> +    for (p = params; p; p = p->next) {
> +        switch (p->value->parameter) {
> +        case MIGRATION_PARAMETER_COMPRESS_LEVEL:
> +            if (p->value->value < 0 || p->value->value > 9) {
> +                error_set(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
> +                    "is invalied, it should be in the rang of 0 to 9");

s/invalied/invalid/; s/rang/range/


> +            if (p->value->value < 1 || p->value->value > 255) {
> +                error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> +                    "(de)compress_threads",
> +                    "is invalied, it should be in the rang of 1 to 255");

and again


> +++ b/monitor.c
> @@ -4544,6 +4544,24 @@ void migrate_set_capability_completion(ReadLineState *rs, int nb_args,
>      }
>  }
>  
> +void migrate_set_parameter_completion(ReadLineState *rs, int nb_args,
> +                                       const char *str)

Indentation is off.


> +++ b/qapi-schema.json
> @@ -540,6 +540,50 @@
>  ##
>  { 'command': 'query-migrate-capabilities', 'returns':   ['MigrationCapabilityStatus']}
>  
> +# @MigrationParameter
> +#
> +# Migration parameters enumeration
> +#
> +# @compress-level:Set the compression level to be used in live migration,

s/:/: /

> +#          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: parameter enum
> +#
> +# @value: parameter value int
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'MigrationParameterStatus',
> +  'data': { 'parameter' : 'MigrationParameter', 'value' : 'int' } }

Doesn't allow for non-integer parameters.  Not necessarily fatal for
input only (I think a flat union could be utilized with a
MigrationParameter as the discriminator to allow non-int values later on)...

> +##
> +# @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'] } }

...but on output, we might confuse callers by outputting non-int values
unless we plan _from the start_ to support them.  That is, I think we want:

{ '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' } }

to make it obvious to callers that they must be prepared to accept
non-int values down the road if we ever extend the union to add another
parameter with a different type.


>  SQMP
> +migrate-set-parameters
> +----------------------
> +
> +Set migration parameters
> +
> +- "compress-leve": multiple compression thread support

s/leve/level/

Missing two of the three defined parameters.

I hate write-only interfaces.  I'm hoping you add the query of
parameters in a later patch - but I'd prefer you squash it into one patch.
Eric Blake Jan. 23, 2015, 3:42 p.m. UTC | #3 
On 01/23/2015 06:48 AM, Dr. David Alan Gilbert wrote:
> * Liang Li (liang.z.li@intel.com) wrote:
>> Add the qmp and hmp commands to tune the parameters used in live
>> migration.
> 
> If I understand correctly on the destination side we need to set the number of
> decompression threads very early on an incoming migration - I'm not
> clear how early that needs to be - especially if you're using fd: so it's
> not waiting for a connect ?
> 
> Eric: How would libvirt do that?

Libvirt does some handshaking before starting migration.  The source
would advertise that "I'm capable of threaded migration; and plan to use
X send and Y receive threads"; the destination would either reply that
threads are unsupported or set the receive thread count at that point,
then the source would know if it can enable threads and set the send
thread count, before letting migration start.  I don't see any problems
with the current design of starting things up.
Dr. David Alan Gilbert Jan. 23, 2015, 3:59 p.m. UTC | #4 
* Eric Blake (eblake@redhat.com) wrote:
> On 01/23/2015 06:48 AM, Dr. David Alan Gilbert wrote:
> > * Liang Li (liang.z.li@intel.com) wrote:
> >> Add the qmp and hmp commands to tune the parameters used in live
> >> migration.
> > 
> > If I understand correctly on the destination side we need to set the number of
> > decompression threads very early on an incoming migration - I'm not
> > clear how early that needs to be - especially if you're using fd: so it's
> > not waiting for a connect ?
> > 
> > Eric: How would libvirt do that?
> 
> Libvirt does some handshaking before starting migration.  The source
> would advertise that "I'm capable of threaded migration; and plan to use
> X send and Y receive threads"; the destination would either reply that
> threads are unsupported or set the receive thread count at that point,
> then the source would know if it can enable threads and set the send
> thread count, before letting migration start.  I don't see any problems
> with the current design of starting things up.

Patch 3 calls migrate_decompress_threads_create from process_incoming_migration_co,
if you're using fd based incoming migration then I think that gets called
before libvirt would have a chance to connect to the monitor and
call the parameter setting to say how many threads it wants.
(A tcp incoming migration wouldn't have that problem because it
waits until the TCP accept before calling process_incoming_migration_co)

Dave

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Eric Blake Jan. 23, 2015, 4:06 p.m. UTC | #5 
On 01/23/2015 08:59 AM, Dr. David Alan Gilbert wrote:
> * Eric Blake (eblake@redhat.com) wrote:
>> On 01/23/2015 06:48 AM, Dr. David Alan Gilbert wrote:
>>> * Liang Li (liang.z.li@intel.com) wrote:
>>>> Add the qmp and hmp commands to tune the parameters used in live
>>>> migration.
>>>
>>> If I understand correctly on the destination side we need to set the number of
>>> decompression threads very early on an incoming migration - I'm not
>>> clear how early that needs to be - especially if you're using fd: so it's
>>> not waiting for a connect ?
>>>
>>> Eric: How would libvirt do that?
>>
>> Libvirt does some handshaking before starting migration.  The source
>> would advertise that "I'm capable of threaded migration; and plan to use
>> X send and Y receive threads"; the destination would either reply that
>> threads are unsupported or set the receive thread count at that point,
>> then the source would know if it can enable threads and set the send
>> thread count, before letting migration start.  I don't see any problems
>> with the current design of starting things up.
> 
> Patch 3 calls migrate_decompress_threads_create from process_incoming_migration_co,
> if you're using fd based incoming migration then I think that gets called
> before libvirt would have a chance to connect to the monitor and
> call the parameter setting to say how many threads it wants.
> (A tcp incoming migration wouldn't have that problem because it
> waits until the TCP accept before calling process_incoming_migration_co)

Then it probably needs to be exposed through a command-line parameter,
so that -incoming gains the ability to specify parameters as part of
starting up qemu.
Li, Liang Z Jan. 24, 2015, 2:14 p.m. UTC | #6 
> * Liang Li (liang.z.li@intel.com) wrote:
> > Add the qmp and hmp commands to tune the parameters used in live
> > migration.
> 
> If I understand correctly on the destination side we need to set the number
> of decompression threads very early on an incoming migration - I'm not clear
> how early that needs to be - especially if you're using fd: so it's not waiting
> for a connect ?

The decompression threads can be set after QEMU started (with -incomming
options) and before the TCP accept.
Dr. David Alan Gilbert Jan. 26, 2015, 9:22 a.m. UTC | #7 
* Li, Liang Z (liang.z.li@intel.com) wrote:
>  
> > * Liang Li (liang.z.li@intel.com) wrote:
> > > Add the qmp and hmp commands to tune the parameters used in live
> > > migration.
> > 
> > If I understand correctly on the destination side we need to set the number
> > of decompression threads very early on an incoming migration - I'm not clear
> > how early that needs to be - especially if you're using fd: so it's not waiting
> > for a connect ?
> 
> The decompression threads can be set after QEMU started (with -incomming
> options) and before the TCP accept.

But that doesn't work for an fd: incoming migration.

Dave

> 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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 63d7686..965c037 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1079,6 +1079,38 @@  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));
+    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->parameter = i;
+            params->value->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..bd1b203 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.c b/migration.c
index 9d1613d..d3d377e 100644
--- a/migration.c
+++ b/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,41 @@  void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
     }
 }
 
+void qmp_migrate_set_parameters(MigrationParameterStatusList *params,
+                                  Error **errp)
+{
+    MigrationState *s = migrate_get_current();
+    MigrationParameterStatusList *p;
+
+    for (p = params; p; p = p->next) {
+        switch (p->value->parameter) {
+        case MIGRATION_PARAMETER_COMPRESS_LEVEL:
+            if (p->value->value < 0 || p->value->value > 9) {
+                error_set(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
+                    "is invalied, it should be in the rang 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;
+            }
+            if (p->value->value < 1 || p->value->value > 255) {
+                error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                    "(de)compress_threads",
+                    "is invalied, it should be in the rang of 1 to 255");
+                return;
+            }
+            break;
+        default:
+           return;
+        }
+        s->parameters[p->value->parameter] = p->value->value;
+    }
+}
+
 /* shared migration helpers */
 
 static void migrate_set_state(MigrationState *s, int old_state, int new_state)
@@ -386,9 +424,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));
@@ -399,9 +439,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);
@@ -589,7 +631,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)
@@ -598,7 +640,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)
@@ -607,7 +649,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 f1031a1..4cf62b6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4544,6 +4544,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 d371af3..2caeccc 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -540,6 +540,50 @@ 
 ##
 { '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: parameter enum
+#
+# @value: parameter value int
+#
+# Since: 2.3
+##
+{ 'type': 'MigrationParameterStatus',
+  'data': { 'parameter' : 'MigrationParameter', 'value' : 'int' } }
+##
+# @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 718dd92..59d2643 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3225,6 +3225,29 @@  EQMP
     },
 
 SQMP
+migrate-set-parameters
+----------------------
+
+Set migration parameters
+
+- "compress-leve": multiple compression thread support
+
+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:i",
+	.mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters,
+    },
+SQMP
 query-balloon
 -------------