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

[Nikolay Shirokovskiy]

On 05.02.2016 18:24, Jiri Denemark wrote:
> 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.

I see. So it basically about naming. I'll use 'migrate parameters' without
specifying explicitly current set of multihreaded in name.

> 
>>>> +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).
> 

I don't like hardcodes here either. I cannot find a reference
but I wrote the series with а explicit requirement in mind that migration command
should be reproducible. I recollect that you or Daniel formulated
it in the discussion of previous versions of series. If it so then reusing
options from previous migration is not an option. Do we really have
such a goal or it just some glitch in my head?)

> Jirka
>