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

[Vladimir Sementsov-Ogievskiy]

11.03.2018 06:20, Eric Blake wrote:
> On 03/09/2018 10:52 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 | 111 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >   block/qapi.c         |  41 +++++++++++++++++++
> >   blockdev.c           |  43 ++++++++++++++++++++
> >   3 files changed, 194 insertions(+), 1 deletion(-)
>
> > +# @boundaries: list of interval boundary values in nanoseconds, all 
> > greater
> > +#              than zero and in ascending order.
> > +#              For example, the list [10, 50, 100] produces the 
> > following
> > +#              histogram intervals: [0, 10), [10, 50), [50, 100), 
> > [100, +inf).
>
> 10 vs. 50 nanoseconds is a bit unrealistic; the example makes sense 
> but you may want to scale the boundaries into values that are more 
> likely to make sense during use.  Can be a followup.

I've just taken Paolo's suggestion), not realistic, but do not occupy so 
much space as mine. Be free to adjust it if you want

> > +#
> > +# @bins: list of io request counts corresponding to histogram 
> > intervals.
> > +#        len(@bins) = len(@boundaries) + 1
> > +#        For the example above, @bins may be something like [3, 1, 
> > 5, 2],
> > +#        and corresponding histogram looks like:
> > +#
> > +#        5|           *
> > +#        4|           *
> > +#        3| *         *
> > +#        2| *         *    *
> > +#        1| *    *    *    *
> > +#         +------------------
> > +#             10   50   100
> > +#
> > +# Since: 2.12
> > +##
> > +{ 'struct': 'XBlockLatencyHistogramInfo',
>
> Struct names have no impact to introspection; I see no reason to use a 
> leading X here.
>
> > +  'data': {'boundaries': ['uint64'], 'bins': ['uint64'] } }
> > +
> > +##
> > +# @x-block-latency-histogram-set:
>
> I guess you've decided that this should be experimental in 2.12? If 
> you want to change the name and promote it to a released interface, 
> you don't have much time left.

Yes. This is not random feature, it's required by our customers. And I 
understood (after I remake it to support different bins for different io 
types) that we are on the early stage of doing it, so it would be good 
to have it in 2.12 with x-.

> > +#
> > +# @boundaries-read: list of interval boundary values for read latency
> > +#                   histogram. If specified, old read latency 
> > histogram is
> > +#                   removed, and empty one created with interavals
>
> s/interavals/intervals/
>
> > +#                   corresponding to @boundaries-read. The parameter 
> > has higher
> > +#                   priority then @boundaries.
> > +#
> > +# @boundaries-write: list of interaval boundary values for write 
> > latency
>
> and again
>
> > +#                    histogram.
> > +#
> > +# @boundaries-flush: list of interaval boundary values for flush 
> > latency
>
> copy-paste strikes again :)
>
> > +#                    histogram.
> > +#
> > +# Returns: error if device is not found or @boundaries* arrays are 
> > invalid.
>
> s/@boundaries*/any boundary arrays/
>
> > +#
> > +# Since: 2.12
> > +#
> > +# Example: set new histograms for all io types with intervals
> > +# [0, 10), [10, 50), [50, 100), [100, +inf):
>
> Again, are these reasonable numbers in nanoseconds?  And spelling out 
> the time unit in the example documentation may be helpful.
>
> > @@ -730,6 +830,12 @@
> >   # @timed_stats: Statistics specific to the set of previously defined
> >   #               intervals of time (Since 2.5)
> >   #
> > +# @x_rd_latency_histogram: @BlockLatencyHistogramInfo. (Since 2.12)
>
> Here, the naming makes sense (the _ is consistent with this being an 
> older interface and already using it elsewhere, and the leading x 
> matches with your command being marked experimental).
>
> > +++ b/blockdev.c
> > @@ -4180,6 +4180,49 @@ void qmp_x_blockdev_set_iothread(const char 
> > *node_name, StrOrNull *iothread,
> >       aio_context_release(old_context);
> >   }
> >   +void qmp_x_block_latency_histogram_set(
> > +    const char *device,
> I'll fix up the trivial typos, but you may want a followup patch.

Thank you!

> Reviewed-by: Eric Blake <address@hidden>


--
Best regards,
Vladimir