Re: [PATCH 1/6] migration: Add multi-thread compress method

[Zeyu Jin]

On 2020/11/30 16:35, Markus Armbruster wrote:
> Zeyu Jin <jinzeyu@huawei.com> writes:
> 
>> On 2020/11/27 17:48, Markus Armbruster wrote:
>>> Kevin, Max, suggest to skip right to Qcow2CompressionType.
>>>
>>> Zeyu Jin <jinzeyu@huawei.com> writes:
>>>
>>>> A multi-thread compress method parameter is added to hold the method we
>>>> are going to use. By default the 'zlib' method is used to maintain the
>>>> compatibility as before.
>>>>
>>>> Signed-off-by: Zeyu Jin <jinzeyu@huawei.com>
>>>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>>> [...]
>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>> index 3c75820527..2ed6a55b92 100644
>>>> --- a/qapi/migration.json
>>>> +++ b/qapi/migration.json
>>>> @@ -525,6 +525,19 @@
>>>>    'data': [ 'none', 'zlib',
>>>>              { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
>>>>  
>>>> +##
>>>> +# @CompressMethod:
>>>> +#
>>>> +# An enumeration of multi-thread compression methods.
>>>> +#
>>>> +# @zlib: use zlib compression method.
>>>> +#
>>>> +# Since: 6.0
>>>> +#
>>>> +##
>>>> +{ 'enum': 'CompressMethod',
>>>> +  'data': [ 'zlib' ] }
>>>> +
>>>>  ##
>>>>  # @BitmapMigrationBitmapAlias:
>>>>  #
>>>> @@ -599,6 +612,9 @@
>>>>  #                      compression, so set the decompress-threads to the 
>>>> number about 1/4
>>>>  #                      of compress-threads is adequate.
>>>>  #
>>>> +# @compress-method: Set compression method to use in multi-thread 
>>>> compression.
>>>> +#                   Defaults to zlib. (Since 6.0)
>>>
>>> We already have @multifd-compression.  Why do we need to control the two
>>> separately?  Can you give a use case for different settings?
>>>
>>
>> Generally, mulit-thread compression deals with the situation
>> where network bandwith is limited but cpu resource is adequate. Multifd
>> instead aims to situation where single fd cannot take full advantage of
>> network bandwith. So compression based on multifd cannot fully cover the
>> cases for multi-thread compression.
>>
>> For example, for migration with a bandwith limitation of 10M
>> bytes/second, single fd is enough for data delivery. This is the case
>> for multi-thread compression.
> 
> Let me rephrase my question.
> 
> According to query-migrate-parameters, we default to
> 
>     "compress-level": 1
>     "compress-threads": 8
>     "compress-wait-thread": true
>     "decompress-threads": 2
>     "multifd-channels": 2
>     "multifd-compression": "none"
>     "multifd-zlib-level": 1
>     "multifd-zstd-level": 1
> 
> Your patch adds
> 
>     "compress-method": "zlib"
> 
> I have several basic questions I can't answer from the documentation:
> 
> 1. We appear to have two distinct sets of compression parameters:
> 
>    * Traditional: compress-level, compress-threads,
>      compress-wait-thread, decompress-threads.
> 
>      These parameters all apply to the same compression.  Correct?
> 
>      What data is being compressed by it?
> 
>    * Multi-fd: multifd-channels, multifd-compression,
>      multifd-zlib-level, multifd-std-level
> 
>      These parameters all apply to the same compression.  Correct?
> 
>      What data is being compressed by it?
> 
>    * Why do we want *two*?  I understand why multi-fd is optional, but
>      why do we need the capability to compress differently there?  Use
>      case?
> 
>    All of these questions predate your patch.  David, Juan?
>

I see. The problem is that the parameter sets seem to be redundant and
maybe there is an overlap between these two compression capabilities.

As you said, the questions predate my patch, so maybe we can have a
discussion here. What do you think, David, Juan?

> 2. Does compress-method belong to "traditional"?
>

Yes.

>>> If we do want two parameters: the names @compress-method and
>>> @multifd-compression are inconsistent.  According to your comment,
>>> @compress-method applies only to multi-thread compression.  That leads
>>> me to suggest renaming it to @multi-thread-compression.
>>>
>>
>> For the names, my original idea is to make 'CompressMethod' consistent
>> with other multi-thread compression parameters, like 'compress-threads'
>> and 'compress-level'. There is truly some inconsistency here, between
>> multifd-compression params and old multi-thread compression params.
> 
> I see.
> 
>> For now, I agree with you that 'multi-thread-compression' is better. It
>> is more specific and clear. I will rename the params in next version.
>> Thanks for the suggestion.
> 
> Please wait until we've sorted out the documentation mess.
> 
>>> After PATCH 4, CompressMethod is almost the same as MultiFDCompression:
>>>
>>>    { 'enum': 'CompressMethod',
>>>      'data': [ 'zlib' ] }
>>>      'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
>>>
>>>    { 'enum': 'MultiFDCompression',
>>>      'data': [ 'none', 'zlib',
>>>                { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
>>>
>>> The difference is member 'none'.  Why does compression 'none' make sense
>>> for multi-fd, but not for multi-thread?
>>>
>>
>> When you set 'none'in multi-fd compression, you would not use the
>> compression capability in multi-fd.
>>
>> In comparison, once you turn on multi-thread compression capability, you
>> have already admitted to use compression. In this case, member 'none' is
>> meaningless.
> 
> Let me rephrase my question:
> 
> How do you select zlib, zstd and no compression for "traditional"?
> 

By parameter "compress-method". And there is no need to support "no
compression" in "traditional" compression.

> If zlib, how do you set the compression level (0 = none, 1 = fastest
> compression, 9 = best compression)?
> 
> If zstd, how do you set the compression level (0 = none, 1 = fastest
> compression, 20 = best compression)?
>

In "traditional", zlib and zstd use the same parameter "compress_level".
For each compression method, we will use different parameter check to
make sure the level number is correct.

> How do you select zlib, zstd and no compression for "multi-fd"?
> 

By parameter "multifd-compression".

> If zlib, how do you set the compression level (0 = none, 1 = fastest
> compression, 9 = best compression)?
> 
> If zstd, how do you set the compression level (0 = none, 1 = fastest
> compression, 20 = best compression)?
>

In "multi-fd", the compress level for each method is separated, which
means, you use "multifd-zlib-level" for zlib, "multifd-zstd-level" for
zstd.

The way to set level is different.

>>> If the difference is wanted: the names are inconsistent.  Less of an
>>> issue than member names, because type names are not externally visible.
>>> Let's make them consistent anyway.
>>>
>>> Hmm, there's also Qcow2CompressionType.  That's another conversation;
>>> I'll start a new thread for it.
>>>
>>> [...]
>>>
>>> .
>>>
> 
> .
>