| Message ID | 20200129115655.10414-8-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Multifd Migration Compression | expand |
Juan Quintela <quintela@redhat.com> writes: > Signed-off-by: Juan Quintela <quintela@redhat.com> Same confusion as in PATCH 4; my proposal there applies here, too. For QAPI: Acked-by: Markus Armbruster <armbru@redhat.com>
* 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. 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? Dave > params->has_xbzrle_cache_size = true; > params->xbzrle_cache_size = s->parameters.xbzrle_cache_size; > params->has_max_postcopy_bandwidth = true; > @@ -1219,6 +1223,13 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp) > return false; > } > > + if (params->has_multifd_zstd_level && > + (params->multifd_zstd_level > 20)) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zstd_level", > + "is invalid, it should be in the range of 0 to 20"); > + return false; > + } > + > if (params->has_xbzrle_cache_size && > (params->xbzrle_cache_size < qemu_target_page_size() || > !is_power_of_2(params->xbzrle_cache_size))) { > @@ -3559,6 +3570,9 @@ static Property migration_properties[] = { > DEFINE_PROP_UINT8("multifd-zlib-level", MigrationState, > parameters.multifd_zlib_level, > DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL), > + DEFINE_PROP_UINT8("multifd-zstd-level", MigrationState, > + parameters.multifd_zstd_level, > + DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL), > DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState, > parameters.xbzrle_cache_size, > DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE), > @@ -3651,6 +3665,7 @@ static void migration_instance_init(Object *obj) > params->has_multifd_channels = true; > params->has_multifd_method = true; > params->has_multifd_zlib_level = true; > + params->has_multifd_zstd_level = true; > params->has_xbzrle_cache_size = true; > params->has_max_postcopy_bandwidth = true; > params->has_max_cpu_throttle = true; > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index 7f11866446..87db07694b 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -1840,6 +1840,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > p->has_multifd_zlib_level = true; > visit_type_int(v, param, &p->multifd_zlib_level, &err); > break; > + case MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL: > + p->has_multifd_zstd_level = true; > + visit_type_int(v, param, &p->multifd_zstd_level, &err); > + break; > case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE: > p->has_xbzrle_cache_size = true; > visit_type_size(v, param, &cache_size, &err); > diff --git a/qapi/migration.json b/qapi/migration.json > index 032ee7d3e6..bb5cb6b4f4 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -610,6 +610,13 @@ > # will consume more CPU. > # Defaults to 1. (Since 5.0) > # > +# @multifd-zstd-level: Set the compression level to be used in live > +# migration, the compression level is an integer between 0 > +# and 20, where 0 means no compression, 1 means the best > +# compression speed, and 20 means best compression ratio which > +# will consume more CPU. > +# Defaults to 1. (Since 5.0) > +# > # Since: 2.4 > ## > { 'enum': 'MigrationParameter', > @@ -623,7 +630,7 @@ > 'multifd-channels', > 'xbzrle-cache-size', 'max-postcopy-bandwidth', > 'max-cpu-throttle', 'multifd-method', > - 'multifd-zlib-level' ] } > + 'multifd-zlib-level' ,'multifd-zstd-level' ] } > > ## > # @MigrateSetParameters: > @@ -723,6 +730,13 @@ > # will consume more CPU. > # Defaults to 1. (Since 5.0) > # > +# @multifd-zstd-level: Set the compression level to be used in live > +# migration, the compression level is an integer between 0 > +# and 20, where 0 means no compression, 1 means the best > +# compression speed, and 20 means best compression ratio which > +# will consume more CPU. > +# Defaults to 1. (Since 5.0) > +# > # Since: 2.4 > ## > # TODO either fuse back into MigrationParameters, or make > @@ -750,7 +764,8 @@ > '*max-postcopy-bandwidth': 'size', > '*max-cpu-throttle': 'int', > '*multifd-method': 'MultiFDMethod', > - '*multifd-zlib-level': 'int' } } > + '*multifd-zlib-level': 'int', > + '*multifd-zstd-level': 'int' } } > > ## > # @migrate-set-parameters: > @@ -870,6 +885,13 @@ > # will consume more CPU. > # Defaults to 1. (Since 5.0) > # > +# @multifd-zstd-level: Set the compression level to be used in live > +# migration, the compression level is an integer between 0 > +# and 20, where 0 means no compression, 1 means the best > +# compression speed, and 20 means best compression ratio which > +# will consume more CPU. > +# Defaults to 1. (Since 5.0) > +# > # Since: 2.4 > ## > { 'struct': 'MigrationParameters', > @@ -895,7 +917,8 @@ > '*max-postcopy-bandwidth': 'size', > '*max-cpu-throttle': 'uint8', > '*multifd-method': 'MultiFDMethod', > - '*multifd-zlib-level': 'uint8' } } > + '*multifd-zlib-level': 'uint8', > + '*multifd-zstd-level': 'uint8' } } > > ## > # @query-migrate-parameters: > -- > 2.24.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"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, multifd-zlib-level, and multifd-zstd-level apply "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? > 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. Documentation of admissible range will become a bit awkward, too. Too many migration parameters...
* 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, multifd-zlib-level, and multifd-zstd-level apply > "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? Right. > > 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. The tricky part I see is validating settings/parameters; when someone tries to set a parameter migrate_params_check gets called and has checks like: if (params->has_compress_level && (params->compress_level > 9)) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level", "is invalid, it should be in the range of 0 to 9"); return false; } now that's nice because you error when you set the parameter rather than later when you try and start a migration; the downside now though is you get more complex interaction between parameters and capabilities - so for example if you set the multifd-compression type parameter to 'zstd' and *then* set the single compression level it'll be fine taking '20' as a compresison level, but if you were to set the compression level to 20 and then set the type to 'zstd' it might error because with zlib you can't have 20. > Documentation of admissible range will become a bit awkward, too. > > Too many migration parameters... Nod; which why I was trying to make it 1. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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.
Juan Quintela <quintela@redhat.com> writes: > 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. The interface pretends the parameters are all independent: you get to set them one by one. They are in fact not independent, and this now leads to difficulties. To avoid them, the interface should let you specify a desired configuration all at once, and its implementation should then do what it takes to get from here to there. Example: current state is multifd compression method "zstd", compression level is 19. User wants to switch to method "zlib" level 9 the obvious way With old interface: step 1: set method to "zlib" step 2: set level to 9 Problem: after step 1, we have method "zlib" with invalid level 19. Workaround: swap the steps. Note that the workaround only works because the set of levels both methods support is non-empty. We might still come up with more complicated parameter combinations where that is not the case. With new interface: set compression to "zlib" level 9 The new interface require us to specify a QAPI type capable of holding the complete migration configuration. The stupid way is to throw all migration parameters into a struct, and make the ones optional that apply only when others have certain values. Thus, the "applies only when" bits are semantical, documented in comments, and enforced by C code. With a bit more care, we can make "applies only when" syntactical instead. Examples: @max-bandwidth and @downtime-limit always apply. They go straight into the struct. @tls-creds, @tls-hostname, @tls-authz apply only when TLS is enabled by setting @tls-creds. Have an optional member @tls, which is a struct with mandatory member @creds, optional members @hostname, @authz. @multifd-zlib-level applies when @multifd-method is "zlib", and @multifd-zstd-level applies when it's "zstd". Have a union @multifd-compression, cases "none", "zlib" and "zstd", where each case's members are the parameters applying in that case. Please note the purpose of these examples is to show how things can be done in the schema. I'm not trying to tell you how these specific things *should* be done. The resulting type is perfectly suited as return value of a query command. It's awkward as argument of a "specify desired configuration" command, because it requires the user to specify *complete* configuration. If we want to support omitting the parts of it we don't want to change, we have to make more members optional. Imprecise for the query, where we now have to specify "always present" in comments. Usually less bad than duplicating a complex type. >> 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. Hope this helps some.
* Markus Armbruster (armbru@redhat.com) wrote: > Juan Quintela <quintela@redhat.com> writes: > > > 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. > > The interface pretends the parameters are all independent: you get to > set them one by one. > > They are in fact not independent, and this now leads to difficulties. > > To avoid them, the interface should let you specify a desired > configuration all at once, and its implementation should then do what it > takes to get from here to there. > > Example: current state is multifd compression method "zstd", compression > level is 19. User wants to switch to method "zlib" level 9 the obvious > way > > With old interface: > step 1: set method to "zlib" > step 2: set level to 9 > Problem: after step 1, we have method "zlib" with invalid level 19. > Workaround: swap the steps. > > Note that the workaround only works because the set of levels both > methods support is non-empty. We might still come up with more > complicated parameter combinations where that is not the case. > > With new interface: > set compression to "zlib" level 9 > > The new interface require us to specify a QAPI type capable of holding > the complete migration configuration. > > The stupid way is to throw all migration parameters into a struct, and > make the ones optional that apply only when others have certain values. > Thus, the "applies only when" bits are semantical, documented in > comments, and enforced by C code. I realised we already have that stupid struct! It's just MigrationParameters - it has all the parameters as optional values, and QMP's MigrateSetParameters already takes that - so you can already provide both the type and the level at the same time; although there's no semantic correlation between them. Dave > With a bit more care, we can make "applies only when" syntactical > instead. Examples: > > @max-bandwidth and @downtime-limit always apply. They go straight > into the struct. > > @tls-creds, @tls-hostname, @tls-authz apply only when TLS is enabled > by setting @tls-creds. Have an optional member @tls, which is a > struct with mandatory member @creds, optional members @hostname, > @authz. > > @multifd-zlib-level applies when @multifd-method is "zlib", and > @multifd-zstd-level applies when it's "zstd". Have a union > @multifd-compression, cases "none", "zlib" and "zstd", where each > case's members are the parameters applying in that case. > > Please note the purpose of these examples is to show how things can be > done in the schema. I'm not trying to tell you how these specific > things *should* be done. > > The resulting type is perfectly suited as return value of a query > command. It's awkward as argument of a "specify desired configuration" > command, because it requires the user to specify *complete* > configuration. If we want to support omitting the parts of it we don't > want to change, we have to make more members optional. Imprecise for > the query, where we now have to specify "always present" in comments. > Usually less bad than duplicating a complex type. > > >> 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. > > Hope this helps some. > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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; params->has_xbzrle_cache_size = true; params->xbzrle_cache_size = s->parameters.xbzrle_cache_size; params->has_max_postcopy_bandwidth = true; @@ -1219,6 +1223,13 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp) return false; } + if (params->has_multifd_zstd_level && + (params->multifd_zstd_level > 20)) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "multifd_zstd_level", + "is invalid, it should be in the range of 0 to 20"); + return false; + } + if (params->has_xbzrle_cache_size && (params->xbzrle_cache_size < qemu_target_page_size() || !is_power_of_2(params->xbzrle_cache_size))) { @@ -3559,6 +3570,9 @@ static Property migration_properties[] = { DEFINE_PROP_UINT8("multifd-zlib-level", MigrationState, parameters.multifd_zlib_level, DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL), + DEFINE_PROP_UINT8("multifd-zstd-level", MigrationState, + parameters.multifd_zstd_level, + DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL), DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState, parameters.xbzrle_cache_size, DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE), @@ -3651,6 +3665,7 @@ static void migration_instance_init(Object *obj) params->has_multifd_channels = true; params->has_multifd_method = true; params->has_multifd_zlib_level = true; + params->has_multifd_zstd_level = true; params->has_xbzrle_cache_size = true; params->has_max_postcopy_bandwidth = true; params->has_max_cpu_throttle = true; diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 7f11866446..87db07694b 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1840,6 +1840,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) p->has_multifd_zlib_level = true; visit_type_int(v, param, &p->multifd_zlib_level, &err); break; + case MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL: + p->has_multifd_zstd_level = true; + visit_type_int(v, param, &p->multifd_zstd_level, &err); + break; case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE: p->has_xbzrle_cache_size = true; visit_type_size(v, param, &cache_size, &err); diff --git a/qapi/migration.json b/qapi/migration.json index 032ee7d3e6..bb5cb6b4f4 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -610,6 +610,13 @@ # will consume more CPU. # Defaults to 1. (Since 5.0) # +# @multifd-zstd-level: Set the compression level to be used in live +# migration, the compression level is an integer between 0 +# and 20, where 0 means no compression, 1 means the best +# compression speed, and 20 means best compression ratio which +# will consume more CPU. +# Defaults to 1. (Since 5.0) +# # Since: 2.4 ## { 'enum': 'MigrationParameter', @@ -623,7 +630,7 @@ 'multifd-channels', 'xbzrle-cache-size', 'max-postcopy-bandwidth', 'max-cpu-throttle', 'multifd-method', - 'multifd-zlib-level' ] } + 'multifd-zlib-level' ,'multifd-zstd-level' ] } ## # @MigrateSetParameters: @@ -723,6 +730,13 @@ # will consume more CPU. # Defaults to 1. (Since 5.0) # +# @multifd-zstd-level: Set the compression level to be used in live +# migration, the compression level is an integer between 0 +# and 20, where 0 means no compression, 1 means the best +# compression speed, and 20 means best compression ratio which +# will consume more CPU. +# Defaults to 1. (Since 5.0) +# # Since: 2.4 ## # TODO either fuse back into MigrationParameters, or make @@ -750,7 +764,8 @@ '*max-postcopy-bandwidth': 'size', '*max-cpu-throttle': 'int', '*multifd-method': 'MultiFDMethod', - '*multifd-zlib-level': 'int' } } + '*multifd-zlib-level': 'int', + '*multifd-zstd-level': 'int' } } ## # @migrate-set-parameters: @@ -870,6 +885,13 @@ # will consume more CPU. # Defaults to 1. (Since 5.0) # +# @multifd-zstd-level: Set the compression level to be used in live +# migration, the compression level is an integer between 0 +# and 20, where 0 means no compression, 1 means the best +# compression speed, and 20 means best compression ratio which +# will consume more CPU. +# Defaults to 1. (Since 5.0) +# # Since: 2.4 ## { 'struct': 'MigrationParameters', @@ -895,7 +917,8 @@ '*max-postcopy-bandwidth': 'size', '*max-cpu-throttle': 'uint8', '*multifd-method': 'MultiFDMethod', - '*multifd-zlib-level': 'uint8' } } + '*multifd-zlib-level': 'uint8', + '*multifd-zstd-level': 'uint8' } } ## # @query-migrate-parameters:
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(-)