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

[Nikolay Shirokovskiy]

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;
>> +}
>> +
>>