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&gt; >>> >>> 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&gt; >>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; >>> --- >>> 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