| Message ID | 20201127074116.2061-1-jinzeyu@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Multi-thread compression method support | expand |
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? 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. 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? 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. [...]
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. > > 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. 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. > 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. > 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. > > [...] >> .
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. > 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. 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. > 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. > 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. > > [...] > > . >
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. >> >> [...] >> >> . >>
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. >>> >>> [...] >>> >>> . >>> > > . >
* 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. > >>> > >>> [...] > >>> > >>> . > >>> > > > > . > > >
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index 9d80a07d26..a582721a7b 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -663,6 +663,17 @@ const PropertyInfo qdev_prop_multifd_compression = { .set_default_value = qdev_propinfo_set_default_value_enum, }; +/* --- CompressMethod --- */ +const PropertyInfo qdev_prop_compress_method = { + .name = "CompressMethod", + .description = "multi-thread compression method, " + "zlib", + .enum_table = &CompressMethod_lookup, + .get = qdev_propinfo_get_enum, + .set = qdev_propinfo_set_enum, + .set_default_value = qdev_propinfo_set_default_value_enum, +}; + /* --- Reserved Region --- */ /* diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h index 4437450065..4a943f7e80 100644 --- a/include/hw/qdev-properties.h +++ b/include/hw/qdev-properties.h @@ -23,6 +23,7 @@ extern const PropertyInfo qdev_prop_macaddr; extern const PropertyInfo qdev_prop_reserved_region; extern const PropertyInfo qdev_prop_on_off_auto; extern const PropertyInfo qdev_prop_multifd_compression; +extern const PropertyInfo qdev_prop_compress_method; extern const PropertyInfo qdev_prop_losttickpolicy; extern const PropertyInfo qdev_prop_blockdev_on_error; extern const PropertyInfo qdev_prop_bios_chs_trans; @@ -193,6 +194,9 @@ extern const PropertyInfo qdev_prop_pcie_link_width; #define DEFINE_PROP_MULTIFD_COMPRESSION(_n, _s, _f, _d) \ DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_multifd_compression, \ MultiFDCompression) +#define DEFINE_PROP_COMPRESS_METHOD(_n, _s, _f, _d) \ + DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_compress_method, \ + CompressMethod) #define DEFINE_PROP_LOSTTICKPOLICY(_n, _s, _f, _d) \ DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_losttickpolicy, \ LostTickPolicy) diff --git a/migration/migration.c b/migration/migration.c index 87a9b59f83..bfbe48cc74 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -83,6 +83,7 @@ #define DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT 2 /*0: means nocompress, 1: best speed, ... 9: best compress ratio */ #define DEFAULT_MIGRATE_COMPRESS_LEVEL 1 +#define DEFAULT_MIGRATE_COMPRESS_METHOD COMPRESS_METHOD_ZLIB /* Define default autoconverge cpu throttle migration parameters */ #define DEFAULT_MIGRATE_THROTTLE_TRIGGER_THRESHOLD 50 #define DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL 20 @@ -843,6 +844,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp) params->compress_wait_thread = s->parameters.compress_wait_thread; params->has_decompress_threads = true; params->decompress_threads = s->parameters.decompress_threads; + params->has_compress_method = true; + params->compress_method = s->parameters.compress_method; params->has_throttle_trigger_threshold = true; params->throttle_trigger_threshold = s->parameters.throttle_trigger_threshold; params->has_cpu_throttle_initial = true; @@ -1407,6 +1410,10 @@ static void migrate_params_test_apply(MigrateSetParameters *params, dest->decompress_threads = params->decompress_threads; } + if (params->has_compress_method) { + dest->compress_method = params->compress_method; + } + if (params->has_throttle_trigger_threshold) { dest->throttle_trigger_threshold = params->throttle_trigger_threshold; } @@ -1504,6 +1511,10 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) s->parameters.decompress_threads = params->decompress_threads; } + if (params->has_compress_method) { + s->parameters.compress_method = params->compress_method; + } + if (params->has_throttle_trigger_threshold) { s->parameters.throttle_trigger_threshold = params->throttle_trigger_threshold; } @@ -3717,6 +3728,9 @@ static Property migration_properties[] = { DEFINE_PROP_UINT8("x-decompress-threads", MigrationState, parameters.decompress_threads, DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT), + DEFINE_PROP_COMPRESS_METHOD("compress-method", MigrationState, + parameters.compress_method, + DEFAULT_MIGRATE_COMPRESS_METHOD), DEFINE_PROP_UINT8("x-throttle-trigger-threshold", MigrationState, parameters.throttle_trigger_threshold, DEFAULT_MIGRATE_THROTTLE_TRIGGER_THRESHOLD), @@ -3831,6 +3845,7 @@ static void migration_instance_init(Object *obj) params->has_compress_level = true; params->has_compress_threads = true; params->has_decompress_threads = true; + params->has_compress_method = true; params->has_throttle_trigger_threshold = true; params->has_cpu_throttle_initial = true; params->has_cpu_throttle_increment = true; diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 65d8ff4849..a86574048c 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -420,6 +420,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) MigrationParameter_str(MIGRATION_PARAMETER_DECOMPRESS_THREADS), params->decompress_threads); assert(params->has_throttle_trigger_threshold); + monitor_printf(mon, "%s: %s\n", + MigrationParameter_str(MIGRATION_PARAMETER_COMPRESS_METHOD), + CompressMethod_str(params->compress_method)); monitor_printf(mon, "%s: %u\n", MigrationParameter_str(MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD), params->throttle_trigger_threshold); @@ -1282,6 +1285,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) MigrateSetParameters *p = g_new0(MigrateSetParameters, 1); uint64_t valuebw = 0; uint64_t cache_size; + CompressMethod compress_method; Error *err = NULL; int val, ret; @@ -1307,6 +1311,14 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) p->has_decompress_threads = true; visit_type_int(v, param, &p->decompress_threads, &err); break; + case MIGRATION_PARAMETER_COMPRESS_METHOD: + p->has_compress_method = true; + visit_type_CompressMethod(v, param, &compress_method, &err); + if (err) { + break; + } + p->compress_method = compress_method; + break; case MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD: p->has_throttle_trigger_threshold = true; visit_type_int(v, param, &p->throttle_trigger_threshold, &err); 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) +# # @throttle-trigger-threshold: The ratio of bytes_dirty_period and bytes_xfer_period # to trigger throttling. It is expressed as percentage. # The default value is 50. (Since 5.0) @@ -722,7 +738,7 @@ 'data': ['announce-initial', 'announce-max', 'announce-rounds', 'announce-step', 'compress-level', 'compress-threads', 'decompress-threads', - 'compress-wait-thread', 'throttle-trigger-threshold', + 'compress-wait-thread', 'compress-method', 'throttle-trigger-threshold', 'cpu-throttle-initial', 'cpu-throttle-increment', 'cpu-throttle-tailslow', 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', @@ -759,6 +775,9 @@ # # @decompress-threads: decompression thread count # +# @compress-method: Which multi-thread compression method to use. +# Defaults to zlib. (Since 6.0) +# # @throttle-trigger-threshold: The ratio of bytes_dirty_period and bytes_xfer_period # to trigger throttling. It is expressed as percentage. # The default value is 50. (Since 5.0) @@ -889,6 +908,7 @@ '*compress-threads': 'int', '*compress-wait-thread': 'bool', '*decompress-threads': 'int', + '*compress-method': 'CompressMethod', '*throttle-trigger-threshold': 'int', '*cpu-throttle-initial': 'int', '*cpu-throttle-increment': 'int', @@ -953,6 +973,9 @@ # # @decompress-threads: decompression thread count # +# @compress-method: Which multi-thread compression method to use. +# Defaults to zlib. (Since 6.0) +# # @throttle-trigger-threshold: The ratio of bytes_dirty_period and bytes_xfer_period # to trigger throttling. It is expressed as percentage. # The default value is 50. (Since 5.0) @@ -1083,6 +1106,7 @@ '*compress-threads': 'uint8', '*compress-wait-thread': 'bool', '*decompress-threads': 'uint8', + '*compress-method': 'CompressMethod', '*throttle-trigger-threshold': 'uint8', '*cpu-throttle-initial': 'uint8', '*cpu-throttle-increment': 'uint8',