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

Mon Oct 22 22:00:04 UTC 2018

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 at redhat.com>,
>>>> 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 at redhat.com>
>>>> ---
>>>>  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.

From: $QEMU.GIT/qapi/misc.json:

# @poll-max-ns: maximum polling time in ns, 0 means polling is disabled
#               (since 2.9)
#
# @poll-grow: how many ns will be added to polling time, 0 means that
it's not
#             configured (since 2.9)
#
# @poll-shrink: how many ns will be removed from polling time, 0 means that
#               it's not configured (since 2.9)
#

John

FWIW: Examples "in action"

# virsh domstats f18iothr --iothread
Domain: 'f18iothr'
  iothread.count=6
  iothread.1.poll-max-ns=32768
  iothread.1.poll-grow=0
  iothread.1.poll-shrink=0
  iothread.3.poll-max-ns=32768
  iothread.3.poll-grow=0
  iothread.3.poll-shrink=0
  iothread.4.poll-max-ns=32768
  iothread.4.poll-grow=0
  iothread.4.poll-shrink=0
  iothread.5.poll-max-ns=32768
  iothread.5.poll-grow=0
  iothread.5.poll-shrink=0
  iothread.6.poll-max-ns=32768
  iothread.6.poll-grow=0
  iothread.6.poll-shrink=0
  iothread.2.poll-max-ns=32768
  iothread.2.poll-grow=0
  iothread.2.poll-shrink=0

# virsh iothreadset f18iothr 3 --poll-grow 2

irsh domstats f18iothr --iothread
Domain: 'f18iothr'
  iothread.count=6
  iothread.1.poll-max-ns=32768
  iothread.1.poll-grow=0
  iothread.1.poll-shrink=0
  iothread.3.poll-max-ns=32768
  iothread.3.poll-grow=2
  iothread.3.poll-shrink=0
...

# virsh iothreadset f18iothr 3 --poll-max-ns 100000 2

# virsh domstats f18iothr --iothread
Domain: 'f18iothr'
  iothread.count=6
  iothread.1.poll-max-ns=32768
  iothread.1.poll-grow=0
  iothread.1.poll-shrink=0
  iothread.3.poll-max-ns=100000
  iothread.3.poll-grow=2
  iothread.3.poll-shrink=0
...

# virsh iothreadset f18iothr 3 --poll-max-ns 500000 2 --poll-shrink 4

# virsh domstats f18iothr --iothread
Domain: 'f18iothr'
  iothread.count=6
  iothread.1.poll-max-ns=32768
  iothread.1.poll-grow=0
  iothread.1.poll-shrink=0
  iothread.3.poll-max-ns=500000
  iothread.3.poll-grow=2
  iothread.3.poll-shrink=4
...