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

[Michal Prívozník]

On 10/22/2018 11:00 PM, John Ferlan wrote:
> 
> 
> 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.

Ah sorry, I've misunderstood then. set_##propVal is a boolean not
integer holding the value we want to set. So you'd have two variables,
int A and bool set_A. Okay, this will work well. However, at this point
I wonder if having monitor API that would take attribute name and a
value would be simpler. Something like:


qemuMonitorSetIOThreadAttr(qemuMonitorPtr mon,
                           unsigned int thread_id,
                           const char *attrName,
                           int attrVal);


Then this function would be called like this:

static int
qemuDomainSetIOThreadParams(virDomainPtr dom,
                            unsigned int iothread_id,
                            virTypedParameterPtr params,
                            int nparams,
                            unsigned int flags)
{
    ...

    for (i = 0; i < nparams; i++) {
        qemuMonitorSetIOThreadAttr(mon, thread_id, param[i].str,
                                   param[i].val);
    }
    ...
}


Anyway, I'll leave it up to you.

Michal