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

[Juan Quintela <quintela@redhat.com>]

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.