Re: [Qemu-block] [Qemu-devel] [PATCH v2 2/2] qapi: add block latency histogram interface

[Stefan Hajnoczi]

On Wed, Feb 07, 2018 at 03:50:37PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8225308904..74fe3fe9c4 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -451,6 +451,74 @@
>             'status': 'DirtyBitmapStatus'} }
>  
>  ##
> +# @BlockLatencyHistogramInfo:
> +#
> +# Block latency histogram.
> +#
> +# @latency: list of latency points in microseconds. Matches the @latency

The meaning of "point" is unclear here.  I think the term "interval
boundary" is clearer, but an example may help to illustrate this.  The
field could also be renamed to "intervals".

> +#           parameter from the last call to block-latency-histogram-set for 
> the
> +#           given device.

The fastest NVMe devices are specified at 10 microsecond latency or
less.  The software stack adds latency on top of that, but using
microseconds as units here may not be future-proof.

I suggest using nanoseconds, just in case.

> +#
> +# @read: list of read-request counts corresponding to latency region.
> +#        len(@read) = len(@latency) + 1
> +#        @read[0] corresponds to latency region [0, @latency[0])

"latency region" == "bin" or "interval" in histogram terminology

> +#        for 0 < i < len(@latency): @read[i] corresponds to latency region
> +#            address@hidden, @latency[i])
> +#        @read.last_element corresponds to latency region
> +#            address@hidden, +inf)
> +#
> +# @write: list of write-request counts (see @read semantics)
> +#
> +# @flush: list of flush-request counts (see @read semantics)
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockLatencyHistogramInfo',
> +  'data': {'latency': ['uint64'],
> +           'read': ['uint64'],
> +           'write': ['uint64'],
> +           'flush': ['uint64'] } }
> +
> +##
> +# @block-latency-histogram-set:
> +#
> +# Remove old latency histogram (if exist) and (optionally) create a new one.

s/if exist/if present/

> +# Add a latency histogram to block device. If a latency histogram already
> +# exists for the device it will be removed and a new one created. The latency
> +# histogram may be queried through query-blockstats.
> +# Set, restart or clear latency histogram.
> +#
> +# @device: device name to set latency histogram for.

query-blockstats also works on nodes, so this command must work not just
on devices but on named nodes too.

> +# @latency: If unspecified, the latency histogram will be removed (if 
> exists).
> +#           Otherwise it should be list of latency points in microseconds. 
> Old
> +#           latency histogram will be removed (if exists) and a new one will 
> be
> +#           created. The sequence must be ascending, elements must be greater
> +#           than zero. Histogram latency regions would be
> +#           [0, @latency[0]), ..., address@hidden, @latency[i+1]), ...,
> +#           address@hidden, +inf)

Reworded:

  @intervals: An optional list of interval boundary values in
              nanoseconds, all greater than zero and in ascending order.
              The list [10, 50, 100] produces the following histogram
              bins: [0, 10), [10, 50), [50, 100), [100, +inf).  If the
              field is absent the existing latency histogram is removed
              from the device (if present).

signature.asc
Description: PGP signature