Re: [Qemu-devel] [PATCH 09/10] migration: Make xbzrle_cache_size a migration parameter

[Juan Quintela]

"Dr. David Alan Gilbert" <address@hidden> wrote:
> * Juan Quintela (address@hidden) wrote:
>> Right now it is a variable in MigrationState instead of a
>> MigrationParameter.  The change allows to set it as the rest of the
>> Migration parameters, from the command line, with
>> query_migration_paramters, set_migrate_parameters, etc.
>> 
>> Signed-off-by: Juan Quintela <address@hidden>

>>  static void migrate_params_apply(MigrateSetParameters *params)
>> @@ -939,6 +954,10 @@ static void migrate_params_apply(MigrateSetParameters 
>> *params)
>>      if (params->has_x_multifd_page_count) {
>>          s->parameters.x_multifd_page_count = params->x_multifd_page_count;
>>      }
>> +    if (params->has_xbzrle_cache_size) {
>> +        s->parameters.xbzrle_cache_size = params->xbzrle_cache_size;
>> +        xbzrle_cache_resize(params->xbzrle_cache_size, NULL);
>
> Not having the errp is a pain;   you've just moved all the error
> checking into xbzrle_cache_resize, so I think we really need an error
> pointer and to do something with it (even if it's just a local report).

ok.


>> @@ -474,6 +474,10 @@
>>  # @x-multifd-page-count: Number of pages sent together to a thread
>>  #                        The default value is 16 (since 2.11)
>>  #
>> +# @xbzrle-cache-size: cache size to be used by XBZRLE migration.  It
>> +#                     needs to be a multiple of the target page size
>
> and power of 2
>
>> +#                     (Since 2.11)
>> +#
>>  # Since: 2.4
>>  ##
>>  { 'enum': 'MigrationParameter',
>> @@ -481,7 +485,8 @@
>>             'cpu-throttle-initial', 'cpu-throttle-increment',
>>             'tls-creds', 'tls-hostname', 'max-bandwidth',
>>             'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
>> -           'x-multifd-channels', 'x-multifd-page-count' ] }
>> +           'x-multifd-channels', 'x-multifd-page-count',
>> +           'xbzrle-cache-size' ] }
>>  
>>  ##
>>  # @MigrateSetParameters:
>> @@ -545,6 +550,9 @@
>>  # @x-multifd-page-count: Number of pages sent together to a thread
>>  #                        The default value is 16 (since 2.11)
>>  #
>> +# @xbzrle-cache-size: cache size to be used by XBZRLE migration.  It
>> +#                     needs to be a multiple of the target page size
>> +#                     (Since 2.11)
>
> and power of 2
>
>>  # Since: 2.4
>>  ##
>>  # TODO either fuse back into MigrationParameters, or make
>> @@ -562,7 +570,8 @@
>>              '*x-checkpoint-delay': 'int',
>>              '*block-incremental': 'bool',
>>              '*x-multifd-channels': 'int',
>> -            '*x-multifd-page-count': 'int' } }
>> +            '*x-multifd-page-count': 'int',
>> +            '*xbzrle-cache-size': 'size' } }
>>  
>>  ##
>>  # @migrate-set-parameters:
>> @@ -641,6 +650,9 @@
>>  # @x-multifd-page-count: Number of pages sent together to a thread
>>  #                        The default value is 16 (since 2.11)
>>  #
>> +# @xbzrle-cache-size: cache size to be used by XBZRLE migration.  It
>> +#                     needs to be a multiple of the target page size
>> +#                     (Since 2.11)
>
> and power of 2 (wow this text is repeated a lot)

We repeat each parameter three times, and the text another three times.

Could we just put a pointer telling that documentation is in a single
place and call it a day?

I know that we need to declare the name in three places for historical
reasons, but at least the help can be done in a single place, or is the
text taken automagically?

Later, Juan.