Re: [Qemu-devel] [v5 12/12] migration: Add commands to set and query parameter

[Markus Armbruster]

Copying Eric for additional QAPI design expertise.

I'm going to review just the QAPI schema changes.

Liang Li <address@hidden> writes:

> Add the qmp and hmp commands to tune and query the parameters used in
> live migration.
>
> Signed-off-by: Liang Li <address@hidden>
> Signed-off-by: Yang Zhang <address@hidden>
[...]
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 0dfc4ce..5bf21fe 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -541,6 +541,92 @@
>  ##
>  { '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. Usually, decompression is at least 4 times as fast as
> +#          compression, so set the decompress-threads to the number about 1/4
> +#          of compress-threads is adequate.
> +#
> +# Since: 2.3
> +##
> +{ 'enum': 'MigrationParameter',
> +  'data': ['compress-level', 'compress-threads', 'decompress-threads'] }
> +##
> +# @MigrationParameterBase
> +#
> +# Migration parameter information
> +#
> +# @parameter: the parameter of migration
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'MigrationParameterBase',
> +  'data': {'parameter': 'MigrationParameter'} }
> +##
> +# @MigrationParameterInt
> +#
> +# Migration parameter information
> +#
> +# @value: parameter int
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'MigrationParameterInt',
> +  'data': {'value': 'int'} }
> +##
> +# @MigrationParameterStatus
> +#
> +# Migration parameter information
> +#
> +# @compress-level: compression level
> +#
> +# @compress-threads: compression thread count
> +#
> +# @decompress-threads: decompression thread count
> +#
> +# Since: 2.3
> +##
> +{ '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'] } }

The command takes a list of key-value pairs.  Looks like this (example
stolen from your patch to qmp-commands.hx):

    { "execute": "migrate-set-parameters",
      "arguments": { "parameters":
                     [ { "parameter": "compress-level", "value": 1 } ] } }

Awkward.  I'd very much prefer

    { "execute": "migrate-set-parameters",
      "arguments": { "compress-level", 1 } }

I.e. the command simply takes the parameters as optional arguments.
Simpler, and a natural use of the schema language.

> +##
> +# @query-migrate-parameters
> +#
> +# Returns information about the current migration parameters status
> +#
> +# Returns: @MigrationParametersStatus
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'query-migrate-parameters',
> +  'returns': ['MigrationParameterStatus'] }
> +##
>  ##
>  # @MouseInfo:
>  #

Produces a list of key-value pairs.  Looks like this (stolen from the
same place):

    {
       "return": [
          {
             "parameter": "compress-level",
             "value": 1
          },
          {
             "parameter": "compress-threads",
             "value": 8
          },
          {
             "parameter": "decompress-threads",
             "value": 2
          }
       ]
    }

I'd very much prefer a simple object instead:

    { "return": { "compress-level": 1,
                  "compress-threads": 8,
                  "decompress-threads": 2 } }

[...]