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

[Dr. David Alan Gilbert]

* Zeyu Jin (jinzeyu@huawei.com) wrote:
> 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?

Yes it's true, they're redundant - it's the same settings duplicated
for the two systems, traditinoal and multifd.

Can I ask - have you compared the behaviour of multifd-zstd with plain
zstd?  I ask, because it's a shame to have two separate systems; and if
multifd-zstd worked well, then it would be good someday to deprecate the
non-multifd version of compression completely, and simplify a lot of
code that way.

Dave


> > 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.
> >>>
> >>> [...]
> >>>
> >>> .
> >>>
> > 
> > .
> > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK