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

[Eric Blake]

On 02/06/2018 08:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> Set (and clear) histogram through new command
> block-latency-histogram-set and show new statistics in
> query-blockstats results.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>   qapi/block-core.json | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>   block/qapi.c         | 31 ++++++++++++++++++++++++++
>   blockdev.c           | 15 +++++++++++++
>   3 files changed, 107 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8225308904..4706a934d9 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -451,6 +451,63 @@
>              'status': 'DirtyBitmapStatus'} }
>   
>   ##
> +# @BlockLatencyHistogramInfo:
> +#
> +# Block latency histogram.
> +#
> +# @latency: list of latency points in microseconds. Equals to @latency 
> parameter
> +#           of last called block-latency-histogram-set.

Second sentence might read better as:

Matches the @latency parameter from the last call to 
block-latency-histogram-set for the given device.

> +#
> +# @read: list of read-request counts corresponding to latency region.
> +#        len(@read) = len(@latency) + 1
> +#        @read[0] corresponds to latency region [0, @latency[0])
> +#        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'] } }

Okay, I can see how this interface works.

> +
> +##
> +# @block-latency-histogram-set:
> +#
> +# Add latency histogram to block device. If latency histogram alredy exists for

s/latency/a latency/ (twice)
s/alredy/already/

> +# the device it will be removed and a new one created. Latency histogram may be

s/Latency/The latency/

> +# quered through query-blockstats.

s/quered/queried/

> +#
> +# @device: device name to set latency histogram for.
> +#
> +# @latency: list of latency points in microseconds. The sequcence must be

s/sequcence/sequence/

> +#           ascending, elements must be greater than zero. Histogram latency
> +#           regions would be
> +#           [0, @latency[0]), ..., address@hidden, @latency[i+1]), ...,
> +#           address@hidden, +inf)
> +#
> +# Returns: error if device is not found.
> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# -> { "execute": "block-latency-histogram-set",
> +#      "arguments": { "device": "drive0",
> +#                     "latency": [500000, 1000000, 2000000] } }
> +# <- { "return": {} }
> +##
> +{ 'command': 'block-latency-histogram-set',
> +  'data': {'device': 'str', 'latency': ['uint64'] } }

The commit message mentioned that you can set and clear histogram 
tracking; how does this interface let you clear things?  By passing a 
0-length latency array?  If you violate the constraint (pass 
non-ascending points, for example), does the previous latency map get 
wiped out or is it still preserved unchanged?

> +
> +##
>   # @BlockInfo:
>   #
>   # Block device information.  This structure describes a virtual device and
> @@ -730,6 +787,8 @@
>   # @timed_stats: Statistics specific to the set of previously defined
>   #               intervals of time (Since 2.5)
>   #
> +# @latency-histogram: @BlockLatencyHistogramInfo. (Since 2.12)

I'd mention that this only appears if block-latency-histogram-set has 
been called.

> +#
>   # Since: 0.14.0
>   ##
>   { 'struct': 'BlockDeviceStats',
> @@ -742,7 +801,8 @@
>              'failed_flush_operations': 'int', 'invalid_rd_operations': 'int',
>              'invalid_wr_operations': 'int', 'invalid_flush_operations': 'int',
>              'account_invalid': 'bool', 'account_failed': 'bool',
> -           'timed_stats': ['BlockDeviceTimedStats'] } }
> +           'timed_stats': ['BlockDeviceTimedStats'],
> +           '*latency-histogram': 'BlockLatencyHistogramInfo' } }
>   


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org