Juan Quintela <quint...@redhat.com> writes: > Markus Armbruster <arm...@redhat.com> wrote: >> "Dr. David Alan Gilbert" <dgilb...@redhat.com> writes: >> >>> * Juan Quintela (quint...@redhat.com) wrote: >>>> Signed-off-by: Juan Quintela <quint...@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.