Re: [libvirt] [PATCH v3 3/7] qemu monitor: add multithread compress parameters accessors
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(a)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(a)intel.com>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)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