11:09 a.m.
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