[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