Re: [Qemu-devel] [External Email] Re: [PATCH 3/3] qapi: add block size histogram interface

[Vladimir Sementsov-Ogievskiy]

21.06.2019 4:52, zhenwei pi wrote:
> On 6/20/19 10:03 PM, Eric Blake wrote:
> 
>> On 6/20/19 3:54 AM, zhenwei pi wrote:
>>> Set/Clear block size histograms through new command
>>> x-block-size-histogram-set and show new statistics in
>>> query-blockstats results.
>>>
>> I'm guessing this is modeled after the existing
>> block-latency-histogram-set command?
> 
> zhenwei: Yes, it is.
> 
>>> Signed-off-by: zhenwei pi<address@hidden>
>>> ---
>>>   block/qapi.c         |  24 ++++++++++++
>>>   blockdev.c           |  56 +++++++++++++++++++++++++++
>>>   qapi/block-core.json | 105 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>   3 files changed, 184 insertions(+), 1 deletion(-)
>>> +++ b/qapi/block-core.json
>>> @@ -633,6 +633,100 @@
>>>              '*boundaries-flush': ['uint64'] } }
>>>   
>>>   ##
>>> +# @BlockSizeHistogramInfo:
>>> +#
>>> +# Block size histogram.
>>> +#
>>> +# @boundaries: list of interval boundary values in nanoseconds, all greater
>>> +#              than zero and in ascending order.
>>> +#              For example, the list [8193, 32769, 131073] produces the
>>> +#              following histogram intervals:
>>> +#              [0, 8193), [8193, 32769), [32769, 131073), [131073, +inf).
>>> +#
>>> +# @bins: list of io request counts corresponding to histogram intervals.
>>> +#        len(@bins) = len(@boundaries) + 1
>>> +#        For the example above, @bins may be something like [6, 3, 7, 9],
>>> +#        and corresponding histogram looks like:
>>> +#
>>> +# Since: 4.0
>> You've missed 4.0; the next release is 4.1.
> 
> zhenwei: OK, I will fix all the version info.
> 
>>> +##
>>> +{ 'struct': 'BlockSizeHistogramInfo',
>>> +  'data': {'boundaries': ['uint64'], 'bins': ['uint64'] } }
>> This is identical to struct BlockLatencyHistogramInfo; can we instead
>> rename the type (which does not affect API) and share it between both
>> implementations, instead of duplicating it?
>>
> zhenwei: Good idea. But I am confused about the compatibility of the
> structure BlockLatencyHistogramInfo. If I rename BlockLatencyHistogramInfo
> to BlockHistogramInfo, it will be common.
> 
>>> +
>>> +##
>>> +# @x-block-size-histogram-set:
>> Does this need to be experimental from the get-go? Or can it be stable
>> by dropping 'x-' since it matches the fact that
>> block-latency-histogram-set is stable?
> 
> zhenwei: OK, I will drop 'x-' prefix.
> 
>>> +#
>>> +# Manage read, write and flush size histograms for the device.
>>> +#
>>> +# If only @id parameter is specified, remove all present size histograms
>>> +# for the device. Otherwise, add/reset some of (or all) size histograms.
>>> +#
>>> +# @id: The name or QOM path of the guest device.
>>> +#
>>> +# @boundaries: list of interval boundary values (see description in
>>> +#              BlockSizeHistogramInfo definition). If specified, all
>>> +#              size histograms are removed, and empty ones created for all
>>> +#              io types with intervals corresponding to @boundaries 
>>> (except for
>>> +#              io types, for which specific boundaries are set through the
>>> +#              following parameters).
>>> +#
>>> +# @boundaries-read: list of interval boundary values for read size
>>> +#                   histogram. If specified, old read size histogram is
>>> +#                   removed, and empty one created with intervals
>>> +#                   corresponding to @boundaries-read. The parameter has 
>>> higher
>>> +#                   priority then @boundaries.
>>> +#
>>> +# @boundaries-write: list of interval boundary values for write size
>>> +#                    histogram.
>>> +#
>>> +# @boundaries-flush: list of interval boundary values for flush size
>>> +#                    histogram.
>>> +#
>>> +# Returns: error if device is not found or any boundary arrays are invalid.
>>> +#
>>> +# Since: 4.0
>> 4.1
>>
>>> +#
>>> +# Example: set new histograms for all io types with intervals
>>> +# [0, 8193), [8193, 32769), [32769, 131073), [131073, +inf):
>>> +#
>>> +# -> { "execute": "x-block-size-histogram-set",
>>> +#      "arguments": { "id": "drive0",
>>> +#                     "boundaries": [8193, 32769, 131073] } }
>>> +# <- { "return": {} }
>>> +#
>>> +# Example: set new histogram only for write, other histograms will remain
>>> +# not changed (or not created):
>>> +#
>>> +# -> { "execute": "x-block-size-histogram-set",
>>> +#      "arguments": { "id": "drive0",
>>> +#                     "boundaries-write": [8193, 32769, 131073] } }
>>> +# <- { "return": {} }
>>> +#
>>> +# Example: set new histograms with the following intervals:
>>> +#   read, flush: [0, 8193), [8193, 32769), [32769, 131073), [131073, +inf)
>>> +#   write: [0, 4097), [4097, 8193), [8193, 32769), [32769, +inf)
>>> +#
>>> +# -> { "execute": "x-block-size-histogram-set",
>>> +#      "arguments": { "id": "drive0",
>>> +#                     "boundaries": [8193, 32769, 131073],
>>> +#                     "boundaries-write": [4097, 8193, 32769] } }
>>> +# <- { "return": {} }
>>> +#
>>> +# Example: remove all size histograms:
>>> +#
>>> +# -> { "execute": "x-block-size-histogram-set",
>>> +#      "arguments": { "id": "drive0" } }
>>> +# <- { "return": {} }
>>> +##
>>> +{ 'command': 'x-block-size-histogram-set',
>>> +  'data': {'id': 'str',
>>> +           '*boundaries': ['uint64'],
>>> +           '*boundaries-read': ['uint64'],
>>> +           '*boundaries-write': ['uint64'],
>>> +           '*boundaries-flush': ['uint64'] } }
>> Again, this copies heavily from block-latency-histogram-set.  But
>> changing the command name is not API compatible.  Should we have a
>> single new command 'block-histogram-set' which takes an enum choosing
>> between 'latency' and 'size', and start the deprecation clock on
>> 'block-latency-histogram-set'?
>>   (and defaulting to 'latency' for back-compat
>>
> zhenwei: this actually copies from block-latency-histogram-set, because the
> two APIs have the similar logic but different structure
> BlockLatencyHistogramInfo and BlockSizeHistogramInfo.
> For further extension, a single new command 'block-histogram-set' with
> enum operation is better.
> So, should I remove 'block-latency-histogram-set' and add 
> 'block-histogram-set'?
> 

Hi Zhenwei!

Glad to see that my work is useful not only for my company! And sad that you 
didn't
propose it before dropping x-prefixes. But the interface is yang and I think 
it's
deprecation should not be a problem (for us at least, if I'm not mistaken, we 
only
use it for something near to internal purposes)..

Then, agree with Eric that it's better to make it common, avoiding duplication. 
Even
if we keep both commands (and anyway we have to, at least during deprecation 
period)
we can put their parameters to a separate structure to be shared between them, 
like
BlockDirtyBitmap is shared parameter structure for block-dirty-bitmap-clear,
block-dirty-bitmap-enable, etc.

Interesting, can we go further and create some generic histogram API, not only 
for
block subsystem? Hmm.. I don't see a compatible solution.. Histogram possible 
should
be a separate object with personal id, and than we bind it to our block stats, 
but
this definitely more complicated API than what we have..

-- 
Best regards,
Vladimir