Re: [libvirt] [PATCH 06/11] qemu: Add monitor functions to set IOThread params

On 10/22/18 5:16 AM, Michal Prívozník wrote: > On 10/19/2018 03:09 PM, John Ferlan wrote: >> >> >> On 10/19/18 7:06 AM, Michal Privoznik wrote: >>> On 10/07/2018 03:00 PM, John Ferlan wrote: >>>> Add functions to set the IOThreadInfo param data for the live guest. >>>> >>>> Based on code originally posted by Pavel Hrdina <phrdina(a)redhat.com&gt;, >>>> but extracted into a separate patch. Note that qapi expects to receive >>>> integer parameters rather than unsigned long long or unsigned int's. >>>> QEMU does save the value in larger signed 64 bit values eventually. >>>> >>>> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >>>> --- >>>> src/qemu/qemu_monitor.c | 19 +++++++++++++++++++ >>>> src/qemu/qemu_monitor.h | 2 ++ >>>> src/qemu/qemu_monitor_json.c | 33 +++++++++++++++++++++++++++++++++ >>>> src/qemu/qemu_monitor_json.h | 4 ++++ >>>> 4 files changed, 58 insertions(+) >>>> >>>> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c >>>> index 7f7013e115..a65d638ab8 100644 >>>> --- a/src/qemu/qemu_monitor.c >>>> +++ b/src/qemu/qemu_monitor.c >>>> @@ -4135,6 +4135,25 @@ qemuMonitorGetIOThreads(qemuMonitorPtr mon, >>>> } >>>> >>>> >>>> +/** >>>> + * qemuMonitorSetIOThread: >>>> + * @mon: Pointer to the monitor >>>> + * @iothreadInfo: filled IOThread info with data >>>> + * >>>> + * Alter the specified IOThread's IOThreadInfo values. >>>> + */ >>>> +int >>>> +qemuMonitorSetIOThread(qemuMonitorPtr mon, >>>> + qemuMonitorIOThreadInfoPtr iothreadInfo) >>>> +{ >>>> + VIR_DEBUG("iothread=%p", iothreadInfo); >>>> + >>>> + QEMU_CHECK_MONITOR(mon); >>>> + >>>> + return qemuMonitorJSONSetIOThread(mon, iothreadInfo); >>>> +} >>>> + >>>> + >>>> /** >>>> * qemuMonitorGetMemoryDeviceInfo: >>>> * @mon: pointer to the monitor >>>> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h >>>> index c2991e2b16..ef71fc6448 100644 >>>> --- a/src/qemu/qemu_monitor.h >>>> +++ b/src/qemu/qemu_monitor.h >>>> @@ -1123,6 +1123,8 @@ struct _qemuMonitorIOThreadInfo { >>>> }; >>>> int qemuMonitorGetIOThreads(qemuMonitorPtr mon, >>>> qemuMonitorIOThreadInfoPtr **iothreads); >>>> +int qemuMonitorSetIOThread(qemuMonitorPtr mon, >>>> + qemuMonitorIOThreadInfoPtr iothreadInfo); >>>> >>>> typedef struct _qemuMonitorMemoryDeviceInfo qemuMonitorMemoryDeviceInfo; >>>> typedef qemuMonitorMemoryDeviceInfo *qemuMonitorMemoryDeviceInfoPtr; >>>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c >>>> index 2e92984b44..bb1d62b844 100644 >>>> --- a/src/qemu/qemu_monitor_json.c >>>> +++ b/src/qemu/qemu_monitor_json.c >>>> @@ -7474,6 +7474,39 @@ qemuMonitorJSONGetIOThreads(qemuMonitorPtr mon, >>>> } >>>> >>>> >>>> +int >>>> +qemuMonitorJSONSetIOThread(qemuMonitorPtr mon, >>>> + qemuMonitorIOThreadInfoPtr iothreadInfo) >>>> +{ >>>> + int ret = -1; >>>> + char *path = NULL; >>>> + qemuMonitorJSONObjectProperty prop; >>>> + >>>> + if (virAsprintf(&path, "/objects/iothread%u", >>>> + iothreadInfo->iothread_id) < 0) >>>> + goto cleanup; >>>> + >>>> +#define VIR_IOTHREAD_SET_PROP(propName, propVal) \ >>>> + memset(&prop, 0, sizeof(qemuMonitorJSONObjectProperty)); \ >>>> + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; \ >>>> + prop.val.iv = propVal; \ >>>> + if (qemuMonitorJSONSetObjectProperty(mon, path, propName, &prop) < 0) \ >>>> + goto cleanup; >>>> + >>>> + VIR_IOTHREAD_SET_PROP("poll-max-ns", iothreadInfo->poll_max_ns) >>>> + VIR_IOTHREAD_SET_PROP("poll-grow", iothreadInfo->poll_grow) >>>> + VIR_IOTHREAD_SET_PROP("poll-shrink", iothreadInfo->poll_shrink) >>> >>> So this is all or nothing approach. All values are set - even though >>> user might request through public API to change just one. I don't think >>> it is a good design. Either we need a monitor API that changes just one >>> value and call it for every typed parameter that user sends to us, or >>> public API implementation must copy the old values into this struct >>> (even though it would be ugly). >>> >>> Michal >>> >> >> Fair complaint - I tried to reuse as much as possible from the initial >> series so that I didn't "waste" time implementing something that in the >> long run wasn't desired. Originally there were lots of checks about what >> was or wasn't set - I just took the path of least resistance. >> >> It should be simple to add flag for each to determine which was set >> before setting them in the object path. >> >> IOW: >> >> #define VIR_IOTHREAD_SET_PROP(propName, propVal) \ >> if (iothreadInfo->set_##propVal) { \ > > This will fly iff zero value can't be set on the monitor. I have no idea > whether it can or can not. If it can be set, then we have to go with > something more clever. > > Michal > Not 100% sure what you meant... The point of the bools is to indicate when the value was set, doesn't matter if it's zero.