1:59 a.m.
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?
> + }
> + 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;
> +}
> +
>