qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-devel] [PATCH v6 2/2] qapi: add block latency histogram interface
Date: Mon, 12 Mar 2018 11:18:46 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]