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

[Markus Armbruster <armbru@redhat.com>]

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.