diff mbox series

[v5,7/8] multifd: Add multifd-zstd-level parameter

Message ID 20200129115655.10414-8-quintela@redhat.com (mailing list archive)
State New, archived
Headers show
Series Multifd Migration Compression | expand

Commit Message

Juan Quintela Jan. 29, 2020, 11:56 a.m. UTC
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 15 +++++++++++++++
 monitor/hmp-cmds.c    |  4 ++++
 qapi/migration.json   | 29 ++++++++++++++++++++++++++---
 3 files changed, 45 insertions(+), 3 deletions(-)

Comments

Markus Armbruster Jan. 30, 2020, 8:08 a.m. UTC | #1
Juan Quintela <quintela@redhat.com> writes:

> Signed-off-by: Juan Quintela <quintela@redhat.com>

Same confusion as in PATCH 4; my proposal there applies here, too.

For QAPI:
Acked-by: Markus Armbruster <armbru@redhat.com>
Dr. David Alan Gilbert Feb. 11, 2020, 6:47 p.m. UTC | #2
* Juan Quintela (quintela@redhat.com) wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration.c | 15 +++++++++++++++
>  monitor/hmp-cmds.c    |  4 ++++
>  qapi/migration.json   | 29 ++++++++++++++++++++++++++---
>  3 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 3b081e8147..b690500545 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -91,6 +91,8 @@
>  #define DEFAULT_MIGRATE_MULTIFD_METHOD MULTIFD_METHOD_NONE
>  /*0: means nocompress, 1: best speed, ... 9: best compress ratio */
>  #define DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL 1
> +/* 0: means nocompress, 1: best speed, ... 20: best compress ratio */
> +#define DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL 1
>  
>  /* Background transfer rate for postcopy, 0 means unlimited, note
>   * that page requests can still exceed this limit.
> @@ -805,6 +807,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->multifd_method = s->parameters.multifd_method;
>      params->has_multifd_zlib_level = true;
>      params->multifd_zlib_level = s->parameters.multifd_zlib_level;
> +    params->has_multifd_zstd_level = true;
> +    params->multifd_zstd_level = s->parameters.multifd_zstd_level;

Do we really want different 'multifd_...._level's or just one
'multifd_compress_level' - or even just reuse the existing
'compress-level' parameter.

The only tricky thing about combining them is how to handle
the difference in allowed ranges;  When would the right time be
to check it?

Markus/Eric: Any idea?

Dave

>      params->has_xbzrle_cache_size = true;
>      params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
>      params->has_max_postcopy_bandwidth = true;
> @@ -1219,6 +1223,13 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>          return false;
>      }
>  
> +    if (params->has_multifd_zstd_level &&
> +        (params->multifd_zstd_level > 20)) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zstd_level",
> +                   "is invalid, it should be in the range of 0 to 20");
> +        return false;
> +    }
> +
>      if (params->has_xbzrle_cache_size &&
>          (params->xbzrle_cache_size < qemu_target_page_size() ||
>           !is_power_of_2(params->xbzrle_cache_size))) {
> @@ -3559,6 +3570,9 @@ static Property migration_properties[] = {
>      DEFINE_PROP_UINT8("multifd-zlib-level", MigrationState,
>                        parameters.multifd_zlib_level,
>                        DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL),
> +    DEFINE_PROP_UINT8("multifd-zstd-level", MigrationState,
> +                      parameters.multifd_zstd_level,
> +                      DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL),
>      DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState,
>                        parameters.xbzrle_cache_size,
>                        DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE),
> @@ -3651,6 +3665,7 @@ static void migration_instance_init(Object *obj)
>      params->has_multifd_channels = true;
>      params->has_multifd_method = true;
>      params->has_multifd_zlib_level = true;
> +    params->has_multifd_zstd_level = true;
>      params->has_xbzrle_cache_size = true;
>      params->has_max_postcopy_bandwidth = true;
>      params->has_max_cpu_throttle = true;
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 7f11866446..87db07694b 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1840,6 +1840,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          p->has_multifd_zlib_level = true;
>          visit_type_int(v, param, &p->multifd_zlib_level, &err);
>          break;
> +    case MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL:
> +        p->has_multifd_zstd_level = true;
> +        visit_type_int(v, param, &p->multifd_zstd_level, &err);
> +        break;
>      case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
>          p->has_xbzrle_cache_size = true;
>          visit_type_size(v, param, &cache_size, &err);
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 032ee7d3e6..bb5cb6b4f4 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -610,6 +610,13 @@
>  #          will consume more CPU.
>  #          Defaults to 1. (Since 5.0)
>  #
> +# @multifd-zstd-level: Set the compression level to be used in live
> +#          migration, the compression level is an integer between 0
> +#          and 20, where 0 means no compression, 1 means the best
> +#          compression speed, and 20 means best compression ratio which
> +#          will consume more CPU.
> +#          Defaults to 1. (Since 5.0)
> +#
>  # Since: 2.4
>  ##
>  { 'enum': 'MigrationParameter',
> @@ -623,7 +630,7 @@
>             'multifd-channels',
>             'xbzrle-cache-size', 'max-postcopy-bandwidth',
>             'max-cpu-throttle', 'multifd-method',
> -           'multifd-zlib-level' ] }
> +           'multifd-zlib-level' ,'multifd-zstd-level' ] }
>  
>  ##
>  # @MigrateSetParameters:
> @@ -723,6 +730,13 @@
>  #          will consume more CPU.
>  #          Defaults to 1. (Since 5.0)
>  #
> +# @multifd-zstd-level: Set the compression level to be used in live
> +#          migration, the compression level is an integer between 0
> +#          and 20, where 0 means no compression, 1 means the best
> +#          compression speed, and 20 means best compression ratio which
> +#          will consume more CPU.
> +#          Defaults to 1. (Since 5.0)
> +#
>  # Since: 2.4
>  ##
>  # TODO either fuse back into MigrationParameters, or make
> @@ -750,7 +764,8 @@
>              '*max-postcopy-bandwidth': 'size',
>              '*max-cpu-throttle': 'int',
>              '*multifd-method': 'MultiFDMethod',
> -            '*multifd-zlib-level': 'int' } }
> +            '*multifd-zlib-level': 'int',
> +            '*multifd-zstd-level': 'int' } }
>  
>  ##
>  # @migrate-set-parameters:
> @@ -870,6 +885,13 @@
>  #          will consume more CPU.
>  #          Defaults to 1. (Since 5.0)
>  #
> +# @multifd-zstd-level: Set the compression level to be used in live
> +#          migration, the compression level is an integer between 0
> +#          and 20, where 0 means no compression, 1 means the best
> +#          compression speed, and 20 means best compression ratio which
> +#          will consume more CPU.
> +#          Defaults to 1. (Since 5.0)
> +#
>  # Since: 2.4
>  ##
>  { 'struct': 'MigrationParameters',
> @@ -895,7 +917,8 @@
>  	    '*max-postcopy-bandwidth': 'size',
>              '*max-cpu-throttle': 'uint8',
>              '*multifd-method': 'MultiFDMethod',
> -            '*multifd-zlib-level': 'uint8' } }
> +            '*multifd-zlib-level': 'uint8',
> +            '*multifd-zstd-level': 'uint8' } }
>  
>  ##
>  # @query-migrate-parameters:
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Markus Armbruster Feb. 13, 2020, 2:04 p.m. UTC | #3
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Juan Quintela (quintela@redhat.com) wrote:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/migration.c | 15 +++++++++++++++
>>  monitor/hmp-cmds.c    |  4 ++++
>>  qapi/migration.json   | 29 ++++++++++++++++++++++++++---
>>  3 files changed, 45 insertions(+), 3 deletions(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 3b081e8147..b690500545 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -91,6 +91,8 @@
>>  #define DEFAULT_MIGRATE_MULTIFD_METHOD MULTIFD_METHOD_NONE
>>  /*0: means nocompress, 1: best speed, ... 9: best compress ratio */
>>  #define DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL 1
>> +/* 0: means nocompress, 1: best speed, ... 20: best compress ratio */
>> +#define DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL 1
>>  
>>  /* Background transfer rate for postcopy, 0 means unlimited, note
>>   * that page requests can still exceed this limit.
>> @@ -805,6 +807,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>>      params->multifd_method = s->parameters.multifd_method;
>>      params->has_multifd_zlib_level = true;
>>      params->multifd_zlib_level = s->parameters.multifd_zlib_level;
>> +    params->has_multifd_zstd_level = true;
>> +    params->multifd_zstd_level = s->parameters.multifd_zstd_level;
>
> Do we really want different 'multifd_...._level's or just one
> 'multifd_compress_level' - or even just reuse the existing
> 'compress-level' parameter.

compress-level, multifd-zlib-level, and multifd-zstd-level apply
"normal" live migration compression, multifd zlib live migration
compression, and multifd zstd live migration compression, respectively.

Any live migration can only use one of the three compressions.

Correct?

> The only tricky thing about combining them is how to handle
> the difference in allowed ranges;  When would the right time be
> to check it?
>
> Markus/Eric: Any idea?

To have an informed opinion, I'd have to dig through the migration
code.

Documentation of admissible range will become a bit awkward, too.

Too many migration parameters...
Dr. David Alan Gilbert Feb. 13, 2020, 2:28 p.m. UTC | #4
* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Juan Quintela (quintela@redhat.com) wrote:
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  migration/migration.c | 15 +++++++++++++++
> >>  monitor/hmp-cmds.c    |  4 ++++
> >>  qapi/migration.json   | 29 ++++++++++++++++++++++++++---
> >>  3 files changed, 45 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index 3b081e8147..b690500545 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -91,6 +91,8 @@
> >>  #define DEFAULT_MIGRATE_MULTIFD_METHOD MULTIFD_METHOD_NONE
> >>  /*0: means nocompress, 1: best speed, ... 9: best compress ratio */
> >>  #define DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL 1
> >> +/* 0: means nocompress, 1: best speed, ... 20: best compress ratio */
> >> +#define DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL 1
> >>  
> >>  /* Background transfer rate for postcopy, 0 means unlimited, note
> >>   * that page requests can still exceed this limit.
> >> @@ -805,6 +807,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
> >>      params->multifd_method = s->parameters.multifd_method;
> >>      params->has_multifd_zlib_level = true;
> >>      params->multifd_zlib_level = s->parameters.multifd_zlib_level;
> >> +    params->has_multifd_zstd_level = true;
> >> +    params->multifd_zstd_level = s->parameters.multifd_zstd_level;
> >
> > Do we really want different 'multifd_...._level's or just one
> > 'multifd_compress_level' - or even just reuse the existing
> > 'compress-level' parameter.
> 
> compress-level, multifd-zlib-level, and multifd-zstd-level apply
> "normal" live migration compression, multifd zlib live migration
> compression, and multifd zstd live migration compression, respectively.
> 
> Any live migration can only use one of the three compressions.
> 
> Correct?

Right.

> > The only tricky thing about combining them is how to handle
> > the difference in allowed ranges;  When would the right time be
> > to check it?
> >
> > Markus/Eric: Any idea?
> 
> To have an informed opinion, I'd have to dig through the migration
> code.

The tricky part I see is validating settings/parameters;
when someone tries to set a parameter migrate_params_check gets called
and has checks like:

    if (params->has_compress_level &&
        (params->compress_level > 9)) {
        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
                   "is invalid, it should be in the range of 0 to 9");
        return false;
    }


now that's nice because you error when you set the parameter rather
than later when you try and start a migration; the downside now
though is you get more complex interaction between parameters and
capabilities - so for example if you set the multifd-compression type
parameter to 'zstd' and *then* set the single compression level
it'll be fine taking '20' as a compresison level, but if you were
to set the compression level to 20 and then set the type to 'zstd'
it might error because with zlib you can't have 20.


> Documentation of admissible range will become a bit awkward, too.
> 
> Too many migration parameters...

Nod; which why I was trying to make it 1.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Juan Quintela Feb. 13, 2020, 3:33 p.m. UTC | #5
Markus Armbruster <armbru@redhat.com> wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>
>> * Juan Quintela (quintela@redhat.com) wrote:
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>>  migration/migration.c | 15 +++++++++++++++
>>>  monitor/hmp-cmds.c    |  4 ++++
>>>  qapi/migration.json   | 29 ++++++++++++++++++++++++++---
>>>  3 files changed, 45 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 3b081e8147..b690500545 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -91,6 +91,8 @@
>>>  #define DEFAULT_MIGRATE_MULTIFD_METHOD MULTIFD_METHOD_NONE
>>>  /*0: means nocompress, 1: best speed, ... 9: best compress ratio */
>>>  #define DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL 1
>>> +/* 0: means nocompress, 1: best speed, ... 20: best compress ratio */
>>> +#define DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL 1
>>>  
>>>  /* Background transfer rate for postcopy, 0 means unlimited, note
>>>   * that page requests can still exceed this limit.
>>> @@ -805,6 +807,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>>>      params->multifd_method = s->parameters.multifd_method;
>>>      params->has_multifd_zlib_level = true;
>>>      params->multifd_zlib_level = s->parameters.multifd_zlib_level;
>>> +    params->has_multifd_zstd_level = true;
>>> +    params->multifd_zstd_level = s->parameters.multifd_zstd_level;
>>
>> Do we really want different 'multifd_...._level's or just one
>> 'multifd_compress_level' - or even just reuse the existing
>> 'compress-level' parameter.
>
> compress-level,

possible values: 1 to 9 deprecated

> multifd-zlib-level

possible values: 1 to 9, default 1
(zlib default is -1, let the lib decide)

, and multifd-zstd-level apply

possible values: 1 to 19, default 1
(zstd default is 3)

> "normal" live migration compression, multifd zlib live migration
> compression, and multifd zstd live migration compression, respectively.
>
> Any live migration can only use one of the three compressions.
>
> Correct?

Yeap.

>> The only tricky thing about combining them is how to handle
>> the difference in allowed ranges;  When would the right time be
>> to check it?
>>
>> Markus/Eric: Any idea?
>
> To have an informed opinion, I'd have to dig through the migration
> code.

Problem is _how_ to setup them.  if we setup zstd compression method,
put the value at 19, and then setup zlib compression method, what should
we do?

Truncate to 9?
Give one error?
Don't allow the zlib setup?

Too complicated.

> Documentation of admissible range will become a bit awkward, too.
>
> Too many migration parameters...

Sure, but the other option is taking a value and live with it.
I am all for leaving the library default and call it a day.

Later, Juan.
Markus Armbruster Feb. 14, 2020, 8:49 a.m. UTC | #6
Juan Quintela <quintela@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>>
>>> * Juan Quintela (quintela@redhat.com) wrote:
>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>> ---
>>>>  migration/migration.c | 15 +++++++++++++++
>>>>  monitor/hmp-cmds.c    |  4 ++++
>>>>  qapi/migration.json   | 29 ++++++++++++++++++++++++++---
>>>>  3 files changed, 45 insertions(+), 3 deletions(-)
>>>> 
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index 3b081e8147..b690500545 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -91,6 +91,8 @@
>>>>  #define DEFAULT_MIGRATE_MULTIFD_METHOD MULTIFD_METHOD_NONE
>>>>  /*0: means nocompress, 1: best speed, ... 9: best compress ratio */
>>>>  #define DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL 1
>>>> +/* 0: means nocompress, 1: best speed, ... 20: best compress ratio */
>>>> +#define DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL 1
>>>>  
>>>>  /* Background transfer rate for postcopy, 0 means unlimited, note
>>>>   * that page requests can still exceed this limit.
>>>> @@ -805,6 +807,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>>>>      params->multifd_method = s->parameters.multifd_method;
>>>>      params->has_multifd_zlib_level = true;
>>>>      params->multifd_zlib_level = s->parameters.multifd_zlib_level;
>>>> +    params->has_multifd_zstd_level = true;
>>>> +    params->multifd_zstd_level = s->parameters.multifd_zstd_level;
>>>
>>> Do we really want different 'multifd_...._level's or just one
>>> 'multifd_compress_level' - or even just reuse the existing
>>> 'compress-level' parameter.
>>
>> compress-level,
>
> possible values: 1 to 9 deprecated
>
>> multifd-zlib-level
>
> possible values: 1 to 9, default 1
> (zlib default is -1, let the lib decide)
>
> , and multifd-zstd-level apply
>
> possible values: 1 to 19, default 1
> (zstd default is 3)
>
>> "normal" live migration compression, multifd zlib live migration
>> compression, and multifd zstd live migration compression, respectively.
>>
>> Any live migration can only use one of the three compressions.
>>
>> Correct?
>
> Yeap.
>
>>> The only tricky thing about combining them is how to handle
>>> the difference in allowed ranges;  When would the right time be
>>> to check it?
>>>
>>> Markus/Eric: Any idea?
>>
>> To have an informed opinion, I'd have to dig through the migration
>> code.
>
> Problem is _how_ to setup them.  if we setup zstd compression method,
> put the value at 19, and then setup zlib compression method, what should
> we do?
>
> Truncate to 9?
> Give one error?
> Don't allow the zlib setup?
>
> Too complicated.

The interface pretends the parameters are all independent: you get to
set them one by one.

They are in fact not independent, and this now leads to difficulties.

To avoid them, the interface should let you specify a desired
configuration all at once, and its implementation should then do what it
takes to get from here to there.

Example: current state is multifd compression method "zstd", compression
level is 19.  User wants to switch to method "zlib" level 9 the obvious
way

    With old interface:
        step 1: set method to "zlib"
        step 2: set level to 9
    Problem: after step 1, we have method "zlib" with invalid level 19.
    Workaround: swap the steps.

    Note that the workaround only works because the set of levels both
    methods support is non-empty.  We might still come up with more
    complicated parameter combinations where that is not the case.

    With new interface:
        set compression to "zlib" level 9

The new interface require us to specify a QAPI type capable of holding
the complete migration configuration.

The stupid way is to throw all migration parameters into a struct, and
make the ones optional that apply only when others have certain values.
Thus, the "applies only when" bits are semantical, documented in
comments, and enforced by C code.

With a bit more care, we can make "applies only when" syntactical
instead.  Examples:

    @max-bandwidth and @downtime-limit always apply.  They go straight
    into the struct.

    @tls-creds, @tls-hostname, @tls-authz apply only when TLS is enabled
    by setting @tls-creds.  Have an optional member @tls, which is a
    struct with mandatory member @creds, optional members @hostname,
    @authz.

    @multifd-zlib-level applies when @multifd-method is "zlib", and
    @multifd-zstd-level applies when it's "zstd".  Have a union
    @multifd-compression, cases "none", "zlib" and "zstd", where each
    case's members are the parameters applying in that case.

Please note the purpose of these examples is to show how things can be
done in the schema.  I'm not trying to tell you how these specific
things *should* be done.

The resulting type is perfectly suited as return value of a query
command.  It's awkward as argument of a "specify desired configuration"
command, because it requires the user to specify *complete*
configuration.  If we want to support omitting the parts of it we don't
want to change, we have to make more members optional.  Imprecise for
the query, where we now have to specify "always present" in comments.
Usually less bad than duplicating a complex type.

>> Documentation of admissible range will become a bit awkward, too.
>>
>> Too many migration parameters...
>
> Sure, but the other option is taking a value and live with it.
> I am all for leaving the library default and call it a day.
>
> Later, Juan.

Hope this helps some.
Dr. David Alan Gilbert Feb. 14, 2020, 6:50 p.m. UTC | #7
* Markus Armbruster (armbru@redhat.com) wrote:
> Juan Quintela <quintela@redhat.com> writes:
> 
> > Markus Armbruster <armbru@redhat.com> wrote:
> >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> >>
> >>> * Juan Quintela (quintela@redhat.com) wrote:
> >>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >>>> ---
> >>>>  migration/migration.c | 15 +++++++++++++++
> >>>>  monitor/hmp-cmds.c    |  4 ++++
> >>>>  qapi/migration.json   | 29 ++++++++++++++++++++++++++---
> >>>>  3 files changed, 45 insertions(+), 3 deletions(-)
> >>>> 
> >>>> diff --git a/migration/migration.c b/migration/migration.c
> >>>> index 3b081e8147..b690500545 100644
> >>>> --- a/migration/migration.c
> >>>> +++ b/migration/migration.c
> >>>> @@ -91,6 +91,8 @@
> >>>>  #define DEFAULT_MIGRATE_MULTIFD_METHOD MULTIFD_METHOD_NONE
> >>>>  /*0: means nocompress, 1: best speed, ... 9: best compress ratio */
> >>>>  #define DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL 1
> >>>> +/* 0: means nocompress, 1: best speed, ... 20: best compress ratio */
> >>>> +#define DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL 1
> >>>>  
> >>>>  /* Background transfer rate for postcopy, 0 means unlimited, note
> >>>>   * that page requests can still exceed this limit.
> >>>> @@ -805,6 +807,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
> >>>>      params->multifd_method = s->parameters.multifd_method;
> >>>>      params->has_multifd_zlib_level = true;
> >>>>      params->multifd_zlib_level = s->parameters.multifd_zlib_level;
> >>>> +    params->has_multifd_zstd_level = true;
> >>>> +    params->multifd_zstd_level = s->parameters.multifd_zstd_level;
> >>>
> >>> Do we really want different 'multifd_...._level's or just one
> >>> 'multifd_compress_level' - or even just reuse the existing
> >>> 'compress-level' parameter.
> >>
> >> compress-level,
> >
> > possible values: 1 to 9 deprecated
> >
> >> multifd-zlib-level
> >
> > possible values: 1 to 9, default 1
> > (zlib default is -1, let the lib decide)
> >
> > , and multifd-zstd-level apply
> >
> > possible values: 1 to 19, default 1
> > (zstd default is 3)
> >
> >> "normal" live migration compression, multifd zlib live migration
> >> compression, and multifd zstd live migration compression, respectively.
> >>
> >> Any live migration can only use one of the three compressions.
> >>
> >> Correct?
> >
> > Yeap.
> >
> >>> The only tricky thing about combining them is how to handle
> >>> the difference in allowed ranges;  When would the right time be
> >>> to check it?
> >>>
> >>> Markus/Eric: Any idea?
> >>
> >> To have an informed opinion, I'd have to dig through the migration
> >> code.
> >
> > Problem is _how_ to setup them.  if we setup zstd compression method,
> > put the value at 19, and then setup zlib compression method, what should
> > we do?
> >
> > Truncate to 9?
> > Give one error?
> > Don't allow the zlib setup?
> >
> > Too complicated.
> 
> The interface pretends the parameters are all independent: you get to
> set them one by one.
> 
> They are in fact not independent, and this now leads to difficulties.
> 
> To avoid them, the interface should let you specify a desired
> configuration all at once, and its implementation should then do what it
> takes to get from here to there.
> 
> Example: current state is multifd compression method "zstd", compression
> level is 19.  User wants to switch to method "zlib" level 9 the obvious
> way
> 
>     With old interface:
>         step 1: set method to "zlib"
>         step 2: set level to 9
>     Problem: after step 1, we have method "zlib" with invalid level 19.
>     Workaround: swap the steps.
> 
>     Note that the workaround only works because the set of levels both
>     methods support is non-empty.  We might still come up with more
>     complicated parameter combinations where that is not the case.
> 
>     With new interface:
>         set compression to "zlib" level 9
> 
> The new interface require us to specify a QAPI type capable of holding
> the complete migration configuration.
> 
> The stupid way is to throw all migration parameters into a struct, and
> make the ones optional that apply only when others have certain values.
> Thus, the "applies only when" bits are semantical, documented in
> comments, and enforced by C code.

I realised we already have that stupid struct!  It's just
MigrationParameters - it has all the parameters as optional values,
and QMP's MigrateSetParameters already takes that - so you can already
provide both the type and the level at the same time; although there's
no semantic correlation between them.

Dave

> With a bit more care, we can make "applies only when" syntactical
> instead.  Examples:
> 
>     @max-bandwidth and @downtime-limit always apply.  They go straight
>     into the struct.
> 
>     @tls-creds, @tls-hostname, @tls-authz apply only when TLS is enabled
>     by setting @tls-creds.  Have an optional member @tls, which is a
>     struct with mandatory member @creds, optional members @hostname,
>     @authz.
> 
>     @multifd-zlib-level applies when @multifd-method is "zlib", and
>     @multifd-zstd-level applies when it's "zstd".  Have a union
>     @multifd-compression, cases "none", "zlib" and "zstd", where each
>     case's members are the parameters applying in that case.
> 
> Please note the purpose of these examples is to show how things can be
> done in the schema.  I'm not trying to tell you how these specific
> things *should* be done.
> 
> The resulting type is perfectly suited as return value of a query
> command.  It's awkward as argument of a "specify desired configuration"
> command, because it requires the user to specify *complete*
> configuration.  If we want to support omitting the parts of it we don't
> want to change, we have to make more members optional.  Imprecise for
> the query, where we now have to specify "always present" in comments.
> Usually less bad than duplicating a complex type.
> 
> >> Documentation of admissible range will become a bit awkward, too.
> >>
> >> Too many migration parameters...
> >
> > Sure, but the other option is taking a value and live with it.
> > I am all for leaving the library default and call it a day.
> >
> > Later, Juan.
> 
> Hope this helps some.
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox series

Patch

diff --git a/migration/migration.c b/migration/migration.c
index 3b081e8147..b690500545 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -91,6 +91,8 @@ 
 #define DEFAULT_MIGRATE_MULTIFD_METHOD MULTIFD_METHOD_NONE
 /*0: means nocompress, 1: best speed, ... 9: best compress ratio */
 #define DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL 1
+/* 0: means nocompress, 1: best speed, ... 20: best compress ratio */
+#define DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL 1
 
 /* Background transfer rate for postcopy, 0 means unlimited, note
  * that page requests can still exceed this limit.
@@ -805,6 +807,8 @@  MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->multifd_method = s->parameters.multifd_method;
     params->has_multifd_zlib_level = true;
     params->multifd_zlib_level = s->parameters.multifd_zlib_level;
+    params->has_multifd_zstd_level = true;
+    params->multifd_zstd_level = s->parameters.multifd_zstd_level;
     params->has_xbzrle_cache_size = true;
     params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
     params->has_max_postcopy_bandwidth = true;
@@ -1219,6 +1223,13 @@  static bool migrate_params_check(MigrationParameters *params, Error **errp)
         return false;
     }
 
+    if (params->has_multifd_zstd_level &&
+        (params->multifd_zstd_level > 20)) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zstd_level",
+                   "is invalid, it should be in the range of 0 to 20");
+        return false;
+    }
+
     if (params->has_xbzrle_cache_size &&
         (params->xbzrle_cache_size < qemu_target_page_size() ||
          !is_power_of_2(params->xbzrle_cache_size))) {
@@ -3559,6 +3570,9 @@  static Property migration_properties[] = {
     DEFINE_PROP_UINT8("multifd-zlib-level", MigrationState,
                       parameters.multifd_zlib_level,
                       DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL),
+    DEFINE_PROP_UINT8("multifd-zstd-level", MigrationState,
+                      parameters.multifd_zstd_level,
+                      DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL),
     DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState,
                       parameters.xbzrle_cache_size,
                       DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE),
@@ -3651,6 +3665,7 @@  static void migration_instance_init(Object *obj)
     params->has_multifd_channels = true;
     params->has_multifd_method = true;
     params->has_multifd_zlib_level = true;
+    params->has_multifd_zstd_level = true;
     params->has_xbzrle_cache_size = true;
     params->has_max_postcopy_bandwidth = true;
     params->has_max_cpu_throttle = true;
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 7f11866446..87db07694b 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1840,6 +1840,10 @@  void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         p->has_multifd_zlib_level = true;
         visit_type_int(v, param, &p->multifd_zlib_level, &err);
         break;
+    case MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL:
+        p->has_multifd_zstd_level = true;
+        visit_type_int(v, param, &p->multifd_zstd_level, &err);
+        break;
     case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
         p->has_xbzrle_cache_size = true;
         visit_type_size(v, param, &cache_size, &err);
diff --git a/qapi/migration.json b/qapi/migration.json
index 032ee7d3e6..bb5cb6b4f4 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -610,6 +610,13 @@ 
 #          will consume more CPU.
 #          Defaults to 1. (Since 5.0)
 #
+# @multifd-zstd-level: Set the compression level to be used in live
+#          migration, the compression level is an integer between 0
+#          and 20, where 0 means no compression, 1 means the best
+#          compression speed, and 20 means best compression ratio which
+#          will consume more CPU.
+#          Defaults to 1. (Since 5.0)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
@@ -623,7 +630,7 @@ 
            'multifd-channels',
            'xbzrle-cache-size', 'max-postcopy-bandwidth',
            'max-cpu-throttle', 'multifd-method',
-           'multifd-zlib-level' ] }
+           'multifd-zlib-level' ,'multifd-zstd-level' ] }
 
 ##
 # @MigrateSetParameters:
@@ -723,6 +730,13 @@ 
 #          will consume more CPU.
 #          Defaults to 1. (Since 5.0)
 #
+# @multifd-zstd-level: Set the compression level to be used in live
+#          migration, the compression level is an integer between 0
+#          and 20, where 0 means no compression, 1 means the best
+#          compression speed, and 20 means best compression ratio which
+#          will consume more CPU.
+#          Defaults to 1. (Since 5.0)
+#
 # Since: 2.4
 ##
 # TODO either fuse back into MigrationParameters, or make
@@ -750,7 +764,8 @@ 
             '*max-postcopy-bandwidth': 'size',
             '*max-cpu-throttle': 'int',
             '*multifd-method': 'MultiFDMethod',
-            '*multifd-zlib-level': 'int' } }
+            '*multifd-zlib-level': 'int',
+            '*multifd-zstd-level': 'int' } }
 
 ##
 # @migrate-set-parameters:
@@ -870,6 +885,13 @@ 
 #          will consume more CPU.
 #          Defaults to 1. (Since 5.0)
 #
+# @multifd-zstd-level: Set the compression level to be used in live
+#          migration, the compression level is an integer between 0
+#          and 20, where 0 means no compression, 1 means the best
+#          compression speed, and 20 means best compression ratio which
+#          will consume more CPU.
+#          Defaults to 1. (Since 5.0)
+#
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -895,7 +917,8 @@ 
 	    '*max-postcopy-bandwidth': 'size',
             '*max-cpu-throttle': 'uint8',
             '*multifd-method': 'MultiFDMethod',
-            '*multifd-zlib-level': 'uint8' } }
+            '*multifd-zlib-level': 'uint8',
+            '*multifd-zstd-level': 'uint8' } }
 
 ##
 # @query-migrate-parameters: