Re: [Qemu-devel] [v3 12/13] migration: Add command to set migration parameter

[Eric Blake <eblake@redhat.com>]

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

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   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]