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

[Markus Armbruster]

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?

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

>> 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"?

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)?

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

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)?

>> 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.
>> 
>> [...]
>> 
>> .
>>