6:36 a.m.
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?
>> + }
>> + 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