* Markus Armbruster (arm...@redhat.com) wrote: > 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.
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 / dgilb...@redhat.com / Manchester, UK
