[Qemu-devel] [PATCH v6 0/2] block latency histogram

[Qemu-devel] [PATCH v6 0/2] block latency histogram

[Vladimir Sementsov-Ogievskiy]

v6:

Use correct header qapi/qapi-builtin-types.h, to fix build again.
Sorry for spam =(


v5:

Revert to v3 and just add qapi-types.h header.


v4:

Move block_latency_histogram_set from block/accounting.c to
blockdev.c, as it uses qapi type uint64List and this fact breaks
build.


v3:

- semantics, naming and wording changed a lot
- x prefixes added to new qapi names
- bug fixed about calculation of new_size (new_nbins now)
- drop g_renew
- in _clear() set nbinst to zero too

v2:

01: add block_latency_histogram_clear()
02: fix spelling (sorry =()
    some rewordings
    remove histogram if latency parameter unspecified

Vladimir Sementsov-Ogievskiy (2):
  block/accounting: introduce latency histogram
  qapi: add block latency histogram interface

 qapi/block-core.json       | 111 ++++++++++++++++++++++++++++++++++++++++++++-
 include/block/accounting.h |  35 ++++++++++++++
 block/accounting.c         |  91 +++++++++++++++++++++++++++++++++++++
 block/qapi.c               |  41 +++++++++++++++++
 blockdev.c                 |  43 ++++++++++++++++++
 5 files changed, 320 insertions(+), 1 deletion(-)

-- 
2.11.1

[Qemu-devel] [PATCH v6 1/2] block/accounting: introduce latency histogram

[Vladimir Sementsov-Ogievskiy]

Introduce latency histogram statics for block devices.
For each accounted operation type latency region [0, +inf) is
divided into subregions by several points. Then, calculate
hits for each subregion.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/accounting.h | 35 ++++++++++++++++++
 block/accounting.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)

diff --git a/include/block/accounting.h b/include/block/accounting.h
index b833d26d6c..d1f67b10dd 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -27,6 +27,7 @@
 
 #include "qemu/timed-average.h"
 #include "qemu/thread.h"
+#include "qapi/qapi-builtin-types.h"
 
 typedef struct BlockAcctTimedStats BlockAcctTimedStats;
 typedef struct BlockAcctStats BlockAcctStats;
@@ -45,6 +46,36 @@ struct BlockAcctTimedStats {
     QSLIST_ENTRY(BlockAcctTimedStats) entries;
 };
 
+typedef struct BlockLatencyHistogram {
+    /* The following histogram is represented like this:
+     *
+     * 5|           *
+     * 4|           *
+     * 3| *         *
+     * 2| *         *    *
+     * 1| *    *    *    *
+     *  +------------------
+     *      10   50   100
+     *
+     * BlockLatencyHistogram histogram = {
+     *     .nbins = 4,
+     *     .boundaries = {10, 50, 100},
+     *     .bins = {3, 1, 5, 2},
+     * };
+     *
+     * @boundaries array define histogram intervals as follows:
+     * [0, boundaries[0]), [boundaries[0], boundaries[1]), ...
+     * [boundaries[nbins-2], +inf)
+     *
+     * So, for example above, histogram intervals are:
+     * [0, 10), [10, 50), [50, 100), [100, +inf)
+     */
+    int nbins;
+    uint64_t *boundaries; /* @nbins-1 numbers here
+                             (all boundaries, except 0 and +inf) */
+    uint64_t *bins;
+} BlockLatencyHistogram;
+
 struct BlockAcctStats {
     QemuMutex lock;
     uint64_t nr_bytes[BLOCK_MAX_IOTYPE];
@@ -57,6 +88,7 @@ struct BlockAcctStats {
     QSLIST_HEAD(, BlockAcctTimedStats) intervals;
     bool account_invalid;
     bool account_failed;
+    BlockLatencyHistogram latency_histogram[BLOCK_MAX_IOTYPE];
 };
 
 typedef struct BlockAcctCookie {
@@ -82,5 +114,8 @@ void block_acct_merge_done(BlockAcctStats *stats, enum BlockAcctType type,
 int64_t block_acct_idle_time_ns(BlockAcctStats *stats);
 double block_acct_queue_depth(BlockAcctTimedStats *stats,
                               enum BlockAcctType type);
+int block_latency_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
+                                uint64List *boundaries);
+void block_latency_histograms_clear(BlockAcctStats *stats);
 
 #endif
diff --git a/block/accounting.c b/block/accounting.c
index 87ef5bbfaa..70a3d9a426 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -94,6 +94,94 @@ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
     cookie->type = type;
 }
 
+/* block_latency_histogram_compare_func:
+ * Compare @key with interval [@it[0], @it[1]).
+ * Return: -1 if @key < @it[0]
+ *          0 if @key in [@it[0], @it[1])
+ *         +1 if @key >= @it[1]
+ */
+static int block_latency_histogram_compare_func(const void *key, const void *it)
+{
+    uint64_t k = *(uint64_t *)key;
+    uint64_t a = ((uint64_t *)it)[0];
+    uint64_t b = ((uint64_t *)it)[1];
+
+    return k < a ? -1 : (k < b ? 0 : 1);
+}
+
+static void block_latency_histogram_account(BlockLatencyHistogram *hist,
+                                            int64_t latency_ns)
+{
+    uint64_t *pos;
+
+    if (hist->bins == NULL) {
+        /* histogram disabled */
+        return;
+    }
+
+
+    if (latency_ns < hist->boundaries[0]) {
+        hist->bins[0]++;
+        return;
+    }
+
+    if (latency_ns >= hist->boundaries[hist->nbins - 2]) {
+        hist->bins[hist->nbins - 1]++;
+        return;
+    }
+
+    pos = bsearch(&latency_ns, hist->boundaries, hist->nbins - 2,
+                  sizeof(hist->boundaries[0]),
+                  block_latency_histogram_compare_func);
+    assert(pos != NULL);
+
+    hist->bins[pos - hist->boundaries + 1]++;
+}
+
+int block_latency_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
+                                uint64List *boundaries)
+{
+    BlockLatencyHistogram *hist = &stats->latency_histogram[type];
+    uint64List *entry;
+    uint64_t *ptr;
+    uint64_t prev = 0;
+    int new_nbins = 1;
+
+    for (entry = boundaries; entry; entry = entry->next) {
+        if (entry->value <= prev) {
+            return -EINVAL;
+        }
+        new_nbins++;
+        prev = entry->value;
+    }
+
+    hist->nbins = new_nbins;
+    g_free(hist->boundaries);
+    hist->boundaries = g_new(uint64_t, hist->nbins - 1);
+    for (entry = boundaries, ptr = hist->boundaries; entry;
+         entry = entry->next, ptr++)
+    {
+        *ptr = entry->value;
+    }
+
+    g_free(hist->bins);
+    hist->bins = g_new0(uint64_t, hist->nbins);
+
+    return 0;
+}
+
+void block_latency_histograms_clear(BlockAcctStats *stats)
+{
+    int i;
+
+    for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
+        BlockLatencyHistogram *hist = &stats->latency_histogram[i];
+        g_free(hist->bins);
+        g_free(hist->boundaries);
+        memset(hist, 0, sizeof(*hist));
+    }
+}
+
 static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
                                  bool failed)
 {
@@ -116,6 +204,9 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
         stats->nr_ops[cookie->type]++;
     }
 
+    block_latency_histogram_account(&stats->latency_histogram[cookie->type],
+                                    latency_ns);
+
     if (!failed || stats->account_failed) {
         stats->total_time_ns[cookie->type] += latency_ns;
         stats->last_access_time_ns = time_ns;
-- 
2.11.1

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

[Vladimir Sementsov-Ogievskiy]

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 <vsementsov@virtuozzo.com>
---
 qapi/block-core.json | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 block/qapi.c         |  41 +++++++++++++++++++
 blockdev.c           |  43 ++++++++++++++++++++
 3 files changed, 194 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 00475f08d4..efe8fe92ff 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -451,6 +451,106 @@
            'status': 'DirtyBitmapStatus'} }
 
 ##
+# @XBlockLatencyHistogramInfo:
+#
+# Block latency histogram.
+#
+# @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).
+#
+# @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',
+  'data': {'boundaries': ['uint64'], 'bins': ['uint64'] } }
+
+##
+# @x-block-latency-histogram-set:
+#
+# Manage read, write and flush latency histograms for the device.
+#
+# If only @device parameter is specified, remove all present latency histograms
+# for the device. Otherwise, add/reset some of (or all) latency histograms.
+#
+# @device: device name to set latency histogram for.
+#
+# @boundaries: list of interval boundary values (see description in
+#              XBlockLatencyHistogramInfo definition). If specified, all
+#              latency 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 latency
+#                   histogram. If specified, old read latency histogram is
+#                   removed, and empty one created with interavals
+#                   corresponding to @boundaries-read. The parameter has higher
+#                   priority then @boundaries.
+#
+# @boundaries-write: list of interaval boundary values for write latency
+#                    histogram.
+#
+# @boundaries-flush: list of interaval boundary values for flush latency
+#                    histogram.
+#
+# Returns: error if device is not found or @boundaries* arrays are invalid.
+#
+# Since: 2.12
+#
+# Example: set new histograms for all io types with intervals
+# [0, 10), [10, 50), [50, 100), [100, +inf):
+#
+# -> { "execute": "block-latency-histogram-set",
+#      "arguments": { "device": "drive0",
+#                     "boundaries": [10, 50, 100] } }
+# <- { "return": {} }
+#
+# Example: set new histogram only for write, other histograms will remain
+# not changed (or not created):
+#
+# -> { "execute": "block-latency-histogram-set",
+#      "arguments": { "device": "drive0",
+#                     "boundaries-write": [10, 50, 100] } }
+# <- { "return": {} }
+#
+# Example: set new histograms with the following intervals:
+#   read, flush: [0, 10), [10, 50), [50, 100), [100, +inf)
+#   write: [0, 1000), [1000, 5000), [5000, +inf)
+#
+# -> { "execute": "block-latency-histogram-set",
+#      "arguments": { "device": "drive0",
+#                     "boundaries": [10, 50, 100],
+#                     "boundaries-write": [1000, 5000] } }
+# <- { "return": {} }
+#
+# Example: remove all latency histograms:
+#
+# -> { "execute": "block-latency-histogram-set",
+#      "arguments": { "device": "drive0" } }
+# <- { "return": {} }
+##
+{ 'command': 'x-block-latency-histogram-set',
+  'data': {'device': 'str',
+           '*boundaries': ['uint64'],
+           '*boundaries-read': ['uint64'],
+           '*boundaries-write': ['uint64'],
+           '*boundaries-flush': ['uint64'] } }
+
+##
 # @BlockInfo:
 #
 # Block device information.  This structure describes a virtual device and
@@ -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)
+#
+# @x_wr_latency_histogram: @BlockLatencyHistogramInfo. (Since 2.12)
+#
+# @x_flush_latency_histogram: @BlockLatencyHistogramInfo. (Since 2.12)
+#
 # Since: 0.14.0
 ##
 { 'struct': 'BlockDeviceStats',
@@ -742,7 +848,10 @@
            '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'],
+           '*x_rd_latency_histogram': 'XBlockLatencyHistogramInfo',
+           '*x_wr_latency_histogram': 'XBlockLatencyHistogramInfo',
+           '*x_flush_latency_histogram': 'XBlockLatencyHistogramInfo' } }
 
 ##
 # @BlockStats:
diff --git a/block/qapi.c b/block/qapi.c
index 4c9923d262..cc4e3b0620 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -394,6 +394,37 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
     qapi_free_BlockInfo(info);
 }
 
+static uint64List *uint64_list(uint64_t *list, int size)
+{
+    int i;
+    uint64List *out_list = NULL;
+    uint64List **pout_list = &out_list;
+
+    for (i = 0; i < size; i++) {
+        uint64List *entry = g_new(uint64List, 1);
+        entry->value = list[i];
+        *pout_list = entry;
+        pout_list = &entry->next;
+    }
+
+    *pout_list = NULL;
+
+    return out_list;
+}
+
+static void bdrv_latency_histogram_stats(BlockLatencyHistogram *hist,
+                                         bool *not_null,
+                                         XBlockLatencyHistogramInfo **info)
+{
+    *not_null = hist->bins != NULL;
+    if (*not_null) {
+        *info = g_new0(XBlockLatencyHistogramInfo, 1);
+
+        (*info)->boundaries = uint64_list(hist->boundaries, hist->nbins - 1);
+        (*info)->bins = uint64_list(hist->bins, hist->nbins);
+    }
+}
+
 static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
 {
     BlockAcctStats *stats = blk_get_stats(blk);
@@ -459,6 +490,16 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
         dev_stats->avg_wr_queue_depth =
             block_acct_queue_depth(ts, BLOCK_ACCT_WRITE);
     }
+
+    bdrv_latency_histogram_stats(&stats->latency_histogram[BLOCK_ACCT_READ],
+                                 &ds->has_x_rd_latency_histogram,
+                                 &ds->x_rd_latency_histogram);
+    bdrv_latency_histogram_stats(&stats->latency_histogram[BLOCK_ACCT_WRITE],
+                                 &ds->has_x_wr_latency_histogram,
+                                 &ds->x_wr_latency_histogram);
+    bdrv_latency_histogram_stats(&stats->latency_histogram[BLOCK_ACCT_FLUSH],
+                                 &ds->has_x_flush_latency_histogram,
+                                 &ds->x_flush_latency_histogram);
 }
 
 static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,
diff --git a/blockdev.c b/blockdev.c
index 1fbfd3a2c4..6e553096ff 100644
--- a/blockdev.c
+++ 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,
+    bool has_boundaries, uint64List *boundaries,
+    bool has_boundaries_read, uint64List *boundaries_read,
+    bool has_boundaries_write, uint64List *boundaries_write,
+    bool has_boundaries_flush, uint64List *boundaries_flush,
+    Error **errp)
+{
+    BlockBackend *blk = blk_by_name(device);
+    BlockAcctStats *stats;
+
+    if (!blk) {
+        error_setg(errp, "Device '%s' not found", device);
+        return;
+    }
+    stats = blk_get_stats(blk);
+
+    if (!has_boundaries && !has_boundaries_read && !has_boundaries_write &&
+        !has_boundaries_flush)
+    {
+        block_latency_histograms_clear(stats);
+        return;
+    }
+
+    if (has_boundaries || has_boundaries_read) {
+        block_latency_histogram_set(
+            stats, BLOCK_ACCT_READ,
+            has_boundaries_read ? boundaries_read : boundaries);
+    }
+
+    if (has_boundaries || has_boundaries_write) {
+        block_latency_histogram_set(
+            stats, BLOCK_ACCT_WRITE,
+            has_boundaries_write ? boundaries_write : boundaries);
+    }
+
+    if (has_boundaries || has_boundaries_flush) {
+        block_latency_histogram_set(
+            stats, BLOCK_ACCT_FLUSH,
+            has_boundaries_flush ? boundaries_flush : boundaries);
+    }
+}
+
 QemuOptsList qemu_common_drive_opts = {
     .name = "drive",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head),
-- 
2.11.1

Re: [Qemu-devel] [PATCH v6 1/2] block/accounting: introduce latency histogram

[Eric Blake]

On 03/09/2018 10:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> Introduce latency histogram statics for block devices.
> For each accounted operation type latency region [0, +inf) is

Hard to read; I suggest: s/type latency/type, the latency/

> divided into subregions by several points. Then, calculate
> hits for each subregion.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/accounting.h | 35 ++++++++++++++++++
>   block/accounting.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 126 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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

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

[Eric Blake]

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 <vsementsov@virtuozzo.com>
> ---
>   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.

> +#
> +# @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.


> +#
> +# @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.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [PATCH v6 0/2] block latency histogram

[Eric Blake]

On 03/09/2018 10:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> v6:
> 
> Use correct header qapi/qapi-builtin-types.h, to fix build again.
> Sorry for spam =(
> 
> 

> Vladimir Sementsov-Ogievskiy (2):
>    block/accounting: introduce latency histogram
>    qapi: add block latency histogram interface

This touches both block and QAPI, but I've gone ahead and queued it on 
the QAPI tree.  Not much time left if you want to drop the x- prefix and 
make this interface officially supported for 2.12.  Queued:

git://repo.or.cz/qemu/ericb.git qapi
http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi

I'll send a PULL request for my QAPI tree as my first act on Monday.

> 
>   qapi/block-core.json       | 111 ++++++++++++++++++++++++++++++++++++++++++++-
>   include/block/accounting.h |  35 ++++++++++++++
>   block/accounting.c         |  91 +++++++++++++++++++++++++++++++++++++
>   block/qapi.c               |  41 +++++++++++++++++
>   blockdev.c                 |  43 ++++++++++++++++++
>   5 files changed, 320 insertions(+), 1 deletion(-)
> 

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

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 <vsementsov@virtuozzo.com>
>> ---
>>   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 <eblake@redhat.com>
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v6 0/2] block latency histogram

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

On Fri, Mar 09, 2018 at 07:52:10PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> v6:
> 
> Use correct header qapi/qapi-builtin-types.h, to fix build again.
> Sorry for spam =(
> 
> 
> v5:
> 
> Revert to v3 and just add qapi-types.h header.
> 
> 
> v4:
> 
> Move block_latency_histogram_set from block/accounting.c to
> blockdev.c, as it uses qapi type uint64List and this fact breaks
> build.
> 
> 
> v3:
> 
> - semantics, naming and wording changed a lot
> - x prefixes added to new qapi names
> - bug fixed about calculation of new_size (new_nbins now)
> - drop g_renew
> - in _clear() set nbinst to zero too
> 
> v2:
> 
> 01: add block_latency_histogram_clear()
> 02: fix spelling (sorry =()
>     some rewordings
>     remove histogram if latency parameter unspecified
> 
> Vladimir Sementsov-Ogievskiy (2):
>   block/accounting: introduce latency histogram
>   qapi: add block latency histogram interface
> 
>  qapi/block-core.json       | 111 ++++++++++++++++++++++++++++++++++++++++++++-
>  include/block/accounting.h |  35 ++++++++++++++
>  block/accounting.c         |  91 +++++++++++++++++++++++++++++++++++++
>  block/qapi.c               |  41 +++++++++++++++++
>  blockdev.c                 |  43 ++++++++++++++++++
>  5 files changed, 320 insertions(+), 1 deletion(-)
> 
> -- 
> 2.11.1
> 
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]