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

Wed Feb 24 08:48:18 UTC 2016

ping

On 17.02.2016 11:48, Nikolay Shirokovskiy wrote:
> 
> 
> 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.
>>
>>>>> +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
>>
> 
> Hi, Jiri.
> 
> I'm thinking on how should we pass compression parameters to migration. The problem
> is how to deal with partially specified parameters. The initial attempt was
> to set unspecified parameters to some hardcoded values. This is gross but
> have some good qualities nethertheless. You suggested next 3 options.
> 
> 1. Don't bother and just always pass all parameters.
> 
> The compression parameters are totally predictable. No problems except for that
> we need to specify too many options on command line.
> 
> 2. Don't bother and just set only that parameters that are specified.
> 
> Here we get unpredictable behaviour. If you don't specify value of a
> parameter its value can be default one, or one that was setted on
> previous unsuccessfull attempt or that was set during successfull migration
> from different host.
> 
> 3. Trying to be smarter.
> 
> Say read default values at good point in time and restore them after migration
> is done. This way we trying to keep the parameters equal to default values all 
> time except for the time of migration. Here we can fail for example in case of
> libvirt crash and restart during migration. Looks like current QEMU interface
> can't be used in this way reliably.
> 
> Thus only the first option looks viable to me. But I hesitate to implement
> it as this way we get too verbose. May be we should stick with hardcodes?
> 
> 
> Another question.
> 
> If we take hardcodes or 'set-them-all' approaches we don't really ever
> need to read this parameters from QEMU. Can we skip implementing
> monitor read command in this case? 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>