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

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(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 > 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list