[libvirt] [PATCH] api,qemu: add block latency histogram

Vladimir Sementsov-Ogievskiy vsementsov at virtuozzo.com
Mon Sep 17 16:09:37 UTC 2018


11.09.2018 14:36, Vladimir Sementsov-Ogievskiy wrote:
> 04.09.2018 09:59, Nikolay Shirokovskiy wrote:
>> Hi, Peter. I have questions to several of your comments:
>>
>> On 03.09.2018 14:59, Peter Krempa wrote:
>>> On Mon, Sep 03, 2018 at 13:58:31 +0300, Nikolay Shirokovskiy wrote:
>>>> This patch adds option to configure/read latency histogram of
>>>> block device IO operations. The corresponding functionality
>>>> in qemu still has x- prefix AFAIK. So this patch should
>>>> help qemu functionality to become non experimental.
>>> This can be used as a proof of concept for this but commiting this
>>> feature will be possible only when qemu removes the experimental 
>>> prefix.
>>>
>>> Until then they are free to change the API and that would result in
>>> multiple implementations in libvirt or they can even drop it.
>>>
>>>> In short histogram is configured thru new API 
>>>> virDomainSetBlockLatencyHistogram and
>>>> histogram itself is available as new fields of 
>>>> virConnectGetAllDomainStats
>>>> output.
>>>>
>> ...
>>
>>>>   static int
>>>> +qemuDomainGetBlockLatency(virDomainStatsRecordPtr record,
>>>> +                          int *maxparams,
>>>> +                          size_t block_idx,
>>>> +                          const char *op,
>>>> +                          qemuBlockLatencyStatsPtr latency)
>>>> +{
>>>> +    char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
>>>> +    size_t i;
>>>> +    int ret = -1;
>>>> +
>>>> +    if (!latency->nbins)
>>>> +        return 0;
>>>> +
>>>> +    snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>>>> +             "block.%zu.latency.%s.bincount", block_idx, op);
>>>> +    if (virTypedParamsAddUInt(&record->params, &record->nparams, 
>>>> maxparams,
>>>> +                              param_name, latency->nbins) < 0)
>>>> +        goto cleanup;
>>>> +
>>>> +    for (i = 0; i < latency->nbins; i++) {
>>>> +        snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>>>> +                 "block.%zu.latency.%s.bin.%zu", block_idx, op, i);
>>>> +        if (virTypedParamsAddULLong(&record->params, 
>>>> &record->nparams, maxparams,
>>>> +                                    param_name, latency->bins[i]) 
>>>> < 0)
>>>> +            goto cleanup;
>>>> +    }
>>>> +
>>>> +    for (i = 0; i < latency->nbins - 1; i++) {
>>>> +        snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
>>>> +                 "block.%zu.latency.%s.boundary.%zu", block_idx, 
>>>> op, i);
>>>> +        if (virTypedParamsAddULLong(&record->params, 
>>>> &record->nparams, maxparams,
>>>> +                                    param_name, 
>>>> latency->boundaries[i]) < 0)
>>>> +            goto cleanup;
>>> The two loops can be merged, so that the bin and value are together.
>> These loops counts differ by 1. I can either add check inside loop to 
>> skip
>> last iteration for boundaries or add record for last bin outside of 
>> the loop.
>> What is preferable?
>>
>> ...
>>
>>>> +
>>>> +static
>>>> +int qemuDomainSetBlockLatencyHistogram(virDomainPtr dom,
>>>> +                                       const char *dev,
>>>> +                                       unsigned int op,
>>>> +                                       unsigned long long 
>>>> *boundaries,
>>>> +                                       int nboundaries,
>>>> +                                       unsigned int flags)
>>> The setting and getting impl should be separated.
>>>
>> This API is only for setting bonudaries. After setting boundaries
>> are available in output of virConnectGetAllDomainStats. Example output:
>>
>>> virsh block-set-latency-histogram vm sda "10000, 50000"
>>> virsh domstats vm | grep latency
>>    block.0.latency.rd.bincount=3
>>    block.0.latency.rd.bin.0=0
>>    block.0.latency.rd.bin.1=0
>>    block.0.latency.rd.bin.2=0
>>    block.0.latency.rd.boundary.0=10000
>>    block.0.latency.rd.boundary.1=50000
>>    block.0.latency.wr.bincount=3
>>    block.0.latency.wr.bin.0=0
>>    block.0.latency.wr.bin.1=0
>>    block.0.latency.wr.bin.2=0
>>    block.0.latency.wr.boundary.0=10000
>>    block.0.latency.wr.boundary.1=50000
>>    block.0.latency.fl.bincount=3
>>    block.0.latency.fl.bin.0=0
>>    block.0.latency.fl.bin.1=0
>>    block.0.latency.fl.bin.2=0
>>    block.0.latency.fl.boundary.0=10000
>>    block.0.latency.fl.boundary.1=50000
>> ...
>>
>>
>>>> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
>>>> index 48b142a..9295ecb 100644
>>>> --- a/src/qemu/qemu_monitor.h
>>>> +++ b/src/qemu/qemu_monitor.h
>>>> @@ -569,6 +569,14 @@ virHashTablePtr 
>>>> qemuMonitorGetBlockInfo(qemuMonitorPtr mon);
>>>>     virJSONValuePtr qemuMonitorQueryBlockstats(qemuMonitorPtr mon);
>>>>   +typedef struct _qemuBlockLatencyStats qemuBlockLatencyStats;
>>>> +typedef qemuBlockLatencyStats *qemuBlockLatencyStatsPtr;
>>>> +struct _qemuBlockLatencyStats {
>>>> +    unsigned long long *boundaries;
>>>> +    unsigned long long *bins;
>>> Since they are connected, you should use an array of structs containing
>>> both, so that they can't be interpreted otherwise.
>> Unfortunately these arrays sizes differ by 1. nboundaries = nbins + 1.
>> That's why I don't introduce another structure here, it would be 
>> inconvinient.
>>
>>>> +    unsigned int nbins;
>>>> +};
>>>> +
>> ...
>>
>>>> +static int
>>>> +qemuMonitorJSONGetBlockLatencyStats(virJSONValuePtr stats,
>>>> +                                    const char *name,
>>>> + qemuBlockLatencyStatsPtr latency)
>>>> +{
>>>> +    virJSONValuePtr latencyJSON;
>>>> +    virJSONValuePtr bins;
>>>> +    virJSONValuePtr boundaries;
>>>> +    size_t i;
>>>> +
>>>> +    if (!(latencyJSON = virJSONValueObjectGetObject(stats, name)))
>>>> +        return 0;
>>>> +
>>>> +    if (!(bins = virJSONValueObjectGetArray(latencyJSON, "bins"))) {
>>>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> +                       _("cannot read bins in latency %s"), name);
>>>> +        return -1;
>>>> +    }
>>>> +
>>>> +    if (!(boundaries = virJSONValueObjectGetArray(latencyJSON, 
>>>> "boundaries"))) {
>>>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> +                       _("cannot read boundaries in latency %s"), 
>>>> name);
>>>> +        return -1;
>>>> +    }
>>>> +
>>>> +    if (virJSONValueArraySize(bins) != 
>>>> virJSONValueArraySize(boundaries) + 1) {
>>>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> +                       _("bins and boundaries size mismatch in 
>>>> latency %s"), name);
>>>> +        return -1;
>>> Maybe the qemu API can be improved to return an array of objects rather
>>> than two separate arrays?
>> Volodya?
>
> Hmm, and what kind of objects?
>
> something like
>
> {
>    .prev_bound
>    .bin
> }
>
> ?
>
> Isn't it more weird than two different arrays?
>

Moreover, x- prefix was dropped for latest Virtuozzo release by mistake. 
So, we vote for already published interface without changes.

>
>>
>>>> +    }
>>>> +    latency->nbins = virJSONValueArraySize(bins);
>>>> +
>>>> +    if (VIR_ALLOC_N(latency->boundaries, latency->nbins - 1) < 0 ||
>>>> +        VIR_ALLOC_N(latency->bins, latency->nbins) < 0)
>>>> +        return -1;
>>>> +
>>>> +    for (i = 0; i < latency->nbins; i++) {
>>>> +        if (virJSONValueGetNumberUlong(virJSONValueArrayGet(bins, i),
>>>> + &latency->bins[i]) < 0) {
>>>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> +                           _("invalid bins in latency %s"), name);
>>>> +            return -1;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    for (i = 0; i < latency->nbins - 1; i++) {
>>>> +        if 
>>>> (virJSONValueGetNumberUlong(virJSONValueArrayGet(boundaries, i),
>>>> + &latency->boundaries[i]) < 0) {
>>>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> +                           _("invalid boundaries in latency %s"), 
>>>> name);
>>>> +            return -1;
>>>> +        }
>>> One loop ought to be enough.
>>>
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>
>
>


-- 
Best regards,
Vladimir




More information about the libvir-list mailing list