[libvirt] [PATCH v3 3/7] qemu monitor: add multithread compress parameters accessors

[Jiri Denemark]

On Fri, Feb 05, 2016 at 18:01:30 +0300, Nikolay Shirokovskiy wrote:
> 
> 
> On 04.02.2016 21:11, Jiri Denemark wrote:
> > On Thu, Jan 28, 2016 at 10:04:29 +0300, Nikolay Shirokovskiy wrote:
> >> From: ShaoHe Feng <shaohe.feng at intel.com>
> >>
> >> Current compression does not use all range of parameter values
> >> so let's use some of them as 'unspecified' values. These
> >> values will be used to mark parameters that were not specified
> >> on migrate command line. Thus we check that qemu does not
> >> use these values when we read parameters.
> >>
> >> Signed-off-by: ShaoHe Feng <shaohe.feng at intel.com>
> >> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
> >> ---
> >>  src/qemu/qemu_monitor.c      |  29 +++++++++++++
> >>  src/qemu/qemu_monitor.h      |  16 +++++++
> >>  src/qemu/qemu_monitor_json.c |  87 +++++++++++++++++++++++++++++++++++++
> >>  src/qemu/qemu_monitor_json.h |   5 +++
> >>  src/qemu/qemu_monitor_text.c | 100 +++++++++++++++++++++++++++++++++++++++++++
> >>  src/qemu/qemu_monitor_text.h |   5 +++
> > 
> >>  
> >>  
> >>  int
> >> +qemuMonitorGetMigrationCompressParametersMT(qemuMonitorPtr mon,
> >> +                                            qemuMonitorMigrationMTParametersPtr params)
> > 
> > I think a more general qemuMonitor[GS]etMigrationParameters [gs]etting
> > all parameters at once would be a bit better. After all, it all boils
> > down to query-migrate-parameters and migrate-set-parameters and having
> > separate functions for each group of parameters would mean we'd have to
> > call the QMP commands several times.
> 
> AFAIK it is not so. We can't set(get) xbzrle options (only size now) and 
> multithread options togather. We neet "query-migrate-cache-size" for the
> first and "query-migrate-parameters" for the second.

Yes, that's true. I didn't explicitly say so, but I was thinking about
future additions to query-migrate-perameters. It's very likely that new
parameters will be added and I think we should cover them all in one
QMP call. Rather than calling several qemuMonitorGet* functions which
would all run query-migrate-perameters command, but each of them would
be interested only in a subset of parameters.

We did similar stuff in the past with some query-block... command which
we called for every single disk again and again even though it returned
all of them at once. It's cleaner to just call it once, fill in a
structure describing the whole reply and do the filtering one layer up
to avoid calling the same command and parsing the same reply several
times.

> >> +int qemuMonitorJSONSetMigrationCompressParametersMT(qemuMonitorPtr mon,
> >> +                                                    qemuMonitorMigrationMTParametersPtr params)
> >> +{
> >> +    int ret = -1;
> >> +    virJSONValuePtr cmd;
> >> +    virJSONValuePtr reply = NULL;
> >> +
> >> +    cmd = qemuMonitorJSONMakeCommand("migrate-set-parameters",
> >> +                                     "i:compress-level", params->level,
> >> +                                     "u:compress-threads", params->threads,
> >> +                                     "u:decompress-threads", params->dthreads,
> >> +                                     NULL);
> > 
> > Is passing an "undefined" value for any of these parameters allowed
> > (i.e., will QEMU use a default value) or do we always have to set all of
> > them?
> 
> QEMU is good enough to take partial parameters specs. In this case the other
> just stay untouched. But I never use this command in this mode and specify
> all parameters. I do it to make sure we don't get compression parameters
> from previous failed migration attempt. See patch number 5.

Yeah, I saw that and it was the reason I asked the question :-)

> Basically instead of reverting parameters on unsuccessful migration I
> use hardcoded default values for parameters that are not specified on
> migration.

Somehow I don't like the hardcoded default values. Either we should
force users to specify all parameters or we could check current
values from QEMU, change some of them as requested by a user, migrate,
and revert the values back to their original values. Or we can just let
another migration attempt reuse the previous values (as we do with other
parameters, I believe).

Jirka