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

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

[Vladimir Sementsov-Ogievskiy]

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       | 73 +++++++++++++++++++++++++++++++++-
 include/block/accounting.h |  9 +++++
 block/accounting.c         | 97 ++++++++++++++++++++++++++++++++++++++++++++++
 block/qapi.c               | 31 +++++++++++++++
 blockdev.c                 | 19 +++++++++
 5 files changed, 228 insertions(+), 1 deletion(-)

-- 
2.11.1

[Qemu-devel] [PATCH v2 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 |  9 +++++
 block/accounting.c         | 97 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+)

diff --git a/include/block/accounting.h b/include/block/accounting.h
index b833d26d6c..9679020f64 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -45,6 +45,12 @@ struct BlockAcctTimedStats {
     QSLIST_ENTRY(BlockAcctTimedStats) entries;
 };
 
+typedef struct BlockLatencyHistogram {
+    int size;
+    uint64_t *points; /* @size-1 points here (all points, except 0 and +inf) */
+    uint64_t *histogram[BLOCK_MAX_IOTYPE]; /* @size elements for each type */
+} BlockLatencyHistogram;
+
 struct BlockAcctStats {
     QemuMutex lock;
     uint64_t nr_bytes[BLOCK_MAX_IOTYPE];
@@ -57,6 +63,7 @@ struct BlockAcctStats {
     QSLIST_HEAD(, BlockAcctTimedStats) intervals;
     bool account_invalid;
     bool account_failed;
+    BlockLatencyHistogram latency_histogram;
 };
 
 typedef struct BlockAcctCookie {
@@ -82,5 +89,7 @@ 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, uint64List *latency);
+void block_latency_histogram_clear(BlockAcctStats *stats);
 
 #endif
diff --git a/block/accounting.c b/block/accounting.c
index 87ef5bbfaa..0051ff0c24 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -94,6 +94,100 @@ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
     cookie->type = type;
 }
 
+/* block_latency_histogram_compare_func
+ * Compare @key with interval [@el, @el+1), where @el+1 is a next array element
+ * after @el.
+ * Return: -1 if @key < @el
+ *          0 if @key in [@el, @el+1)
+ *         +1 if @key >= @el+1
+ */
+static int block_latency_histogram_compare_func(const void *key, const void *el)
+{
+    uint64_t k = *(uint64_t *)key;
+    uint64_t a = *(uint64_t *)el;
+    uint64_t b = *((uint64_t *)el + 1);
+
+    return k < a ? -1 : (k < b ? 0 : 1);
+}
+
+static void block_latency_histogram_account(BlockLatencyHistogram *hist,
+                                            enum BlockAcctType type,
+                                            int64_t latency_ns)
+{
+    uint64_t *data, *pos;
+
+    if (hist->points == NULL) {
+        /* histogram disabled */
+        return;
+    }
+
+    data = hist->histogram[type];
+
+    if (latency_ns < hist->points[0]) {
+        data[0]++;
+        return;
+    }
+
+    if (latency_ns >= hist->points[hist->size - 2]) {
+        data[hist->size - 1]++;
+        return;
+    }
+
+    pos = bsearch(&latency_ns, hist->points, hist->size - 2,
+                  sizeof(hist->points[0]),
+                  block_latency_histogram_compare_func);
+    assert(pos != NULL);
+
+    data[pos - hist->points + 1]++;
+}
+
+int block_latency_histogram_set(BlockAcctStats *stats, uint64List *latency)
+{
+    BlockLatencyHistogram *hist = &stats->latency_histogram;
+    uint64List *entry;
+    uint64_t *ptr;
+    int i;
+    uint64_t prev = 0;
+
+    hist->size = 1;
+
+    for (entry = latency; entry; entry = entry->next) {
+        if (entry->value <= prev) {
+            return -EINVAL;
+        }
+        hist->size++;
+        prev = entry->value;
+    }
+
+    hist->points = g_renew(uint64_t, hist->points, hist->size - 1);
+    for (entry = latency, ptr = hist->points; entry;
+         entry = entry->next, ptr++)
+    {
+        *ptr = entry->value;
+    }
+
+    for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
+        hist->histogram[i] = g_renew(uint64_t, hist->histogram[i], hist->size);
+        memset(hist->histogram[i], 0, hist->size * sizeof(uint64_t));
+    }
+
+    return 0;
+}
+
+void block_latency_histogram_clear(BlockAcctStats *stats)
+{
+    BlockLatencyHistogram *hist = &stats->latency_histogram;
+    int i;
+
+    g_free(hist->points);
+    hist->points = NULL;
+
+    for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
+        g_free(hist->histogram[i]);
+        hist->histogram[i] = NULL;
+    }
+}
+
 static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
                                  bool failed)
 {
@@ -116,6 +210,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 v2 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 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 block/qapi.c         | 31 ++++++++++++++++++++++
 blockdev.c           | 19 ++++++++++++++
 3 files changed, 122 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8225308904..74fe3fe9c4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -451,6 +451,74 @@
            'status': 'DirtyBitmapStatus'} }
 
 ##
+# @BlockLatencyHistogramInfo:
+#
+# Block latency histogram.
+#
+# @latency: list of latency points in microseconds. 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
+#            [@latency[i-1], @latency[i])
+#        @read.last_element corresponds to latency region
+#            [@latency.last_element, +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'] } }
+
+##
+# @block-latency-histogram-set:
+#
+# Remove old latency histogram (if exist) and (optionally) create a new one.
+# Add a latency histogram to block device. If a latency histogram already
+# exists for the device it will be removed and a new one created. The latency
+# histogram may be queried through query-blockstats.
+# Set, restart or clear latency histogram.
+#
+# @device: device name to set latency histogram for.
+#
+# @latency: If unspecified, the latency histogram will be removed (if exists).
+#           Otherwise it should be list of latency points in microseconds. Old
+#           latency histogram will be removed (if exists) and a new one will be
+#           created. The sequence must be ascending, elements must be greater
+#           than zero. Histogram latency regions would be
+#           [0, @latency[0]), ..., [@latency[i], @latency[i+1]), ...,
+#           [@latency.last_element, +inf)
+#
+# Returns: error if device is not found or latency arrays is invalid.
+#
+# Since: 2.12
+#
+# Example (set new latency histogram):
+#
+# -> { "execute": "block-latency-histogram-set",
+#      "arguments": { "device": "drive0",
+#                     "latency": [500000, 1000000, 2000000] } }
+# <- { "return": {} }
+#
+# Example (remove latency histogram):
+#
+# -> { "execute": "block-latency-histogram-set",
+#      "arguments": { "device": "drive0" } }
+# <- { "return": {} }
+##
+{ 'command': 'block-latency-histogram-set',
+  'data': {'device': 'str', '*latency': ['uint64'] } }
+
+##
 # @BlockInfo:
 #
 # Block device information.  This structure describes a virtual device and
@@ -730,6 +798,8 @@
 # @timed_stats: Statistics specific to the set of previously defined
 #               intervals of time (Since 2.5)
 #
+# @latency-histogram: @BlockLatencyHistogramInfo. (Since 2.12)
+#
 # Since: 0.14.0
 ##
 { 'struct': 'BlockDeviceStats',
@@ -742,7 +812,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' } }
 
 ##
 # @BlockStats:
diff --git a/block/qapi.c b/block/qapi.c
index fc10f0a565..715ed17a6b 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -389,6 +389,24 @@ 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_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
 {
     BlockAcctStats *stats = blk_get_stats(blk);
@@ -454,6 +472,19 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
         dev_stats->avg_wr_queue_depth =
             block_acct_queue_depth(ts, BLOCK_ACCT_WRITE);
     }
+
+    ds->has_latency_histogram = stats->latency_histogram.points != NULL;
+    if (ds->has_latency_histogram) {
+        BlockLatencyHistogramInfo *info = g_new0(BlockLatencyHistogramInfo, 1);
+        BlockLatencyHistogram *h = &stats->latency_histogram;
+
+        ds->latency_histogram = info;
+
+        info->latency = uint64_list(h->points, h->size - 1);
+        info->read = uint64_list(h->histogram[BLOCK_ACCT_READ], h->size);
+        info->write = uint64_list(h->histogram[BLOCK_ACCT_WRITE], h->size);
+        info->flush = uint64_list(h->histogram[BLOCK_ACCT_FLUSH], h->size);
+    }
 }
 
 static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,
diff --git a/blockdev.c b/blockdev.c
index 8e977eef11..5fa3fafc8b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4176,6 +4176,25 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
     aio_context_release(old_context);
 }
 
+void qmp_block_latency_histogram_set(const char *device, bool has_latency,
+                                     uint64List *latency, Error **errp)
+{
+    BlockBackend *blk = blk_by_name(device);
+    if (!blk) {
+        error_setg(errp, "Device '%s' not found", device);
+        return;
+    }
+
+    if (has_latency) {
+        if (block_latency_histogram_set(blk_get_stats(blk), latency) < 0) {
+            error_setg(errp, "Invalid latency array");
+            return;
+        }
+    } else {
+        block_latency_histogram_clear(blk_get_stats(blk));
+    }
+}
+
 QemuOptsList qemu_common_drive_opts = {
     .name = "drive",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head),
-- 
2.11.1

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

[no-reply]

Hi,

This series failed build test on ppc host. Please find the details below.

Type: series
Subject: [Qemu-devel] [PATCH v2 0/2] block latency histogram
Message-id: 20180207125037.13510-1-vsementsov@virtuozzo.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e
echo "=== ENV ==="
env
echo "=== PACKAGES ==="
rpm -qa
echo "=== TEST BEGIN ==="
INSTALL=$PWD/install
BUILD=$PWD/build
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --prefix=$INSTALL
make -j100
# XXX: we need reliable clean up
# make check -j100 V=1
make install
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
error: RPC failed; result=18, HTTP code = 200
fatal: The remote end hung up unexpectedly
error: Could not fetch 3c8cf5a9c21ff8782164d1def7f44bd888713384
Traceback (most recent call last):
  File "/home/patchew/patchew/patchew-cli", line 504, in test_one
    git_clone_repo(clone, r["repo"], r["head"], logf)
  File "/home/patchew/patchew/patchew-cli", line 48, in git_clone_repo
    stdout=logf, stderr=logf)
  File "/usr/lib64/python3.4/subprocess.py", line 558, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'remote', 'add', '-f', '--mirror=fetch', '3c8cf5a9c21ff8782164d1def7f44bd888713384', 'https://github.com/patchew-project/qemu']' returned non-zero exit status 1



---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

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

[Vladimir Sementsov-Ogievskiy]

looks strange and unrelated.

07.02.2018 16:20, no-reply@patchew.org wrote:
> Hi,
>
> This series failed build test on ppc host. Please find the details below.
>
> Type: series
> Subject: [Qemu-devel] [PATCH v2 0/2] block latency histogram
> Message-id: 20180207125037.13510-1-vsementsov@virtuozzo.com
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> # Testing script will be invoked under the git checkout with
> # HEAD pointing to a commit that has the patches applied on top of "base"
> # branch
> set -e
> echo "=== ENV ==="
> env
> echo "=== PACKAGES ==="
> rpm -qa
> echo "=== TEST BEGIN ==="
> INSTALL=$PWD/install
> BUILD=$PWD/build
> mkdir -p $BUILD $INSTALL
> SRC=$PWD
> cd $BUILD
> $SRC/configure --prefix=$INSTALL
> make -j100
> # XXX: we need reliable clean up
> # make check -j100 V=1
> make install
> === TEST SCRIPT END ===
>
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> error: RPC failed; result=18, HTTP code = 200
> fatal: The remote end hung up unexpectedly
> error: Could not fetch 3c8cf5a9c21ff8782164d1def7f44bd888713384
> Traceback (most recent call last):
>    File "/home/patchew/patchew/patchew-cli", line 504, in test_one
>      git_clone_repo(clone, r["repo"], r["head"], logf)
>    File "/home/patchew/patchew/patchew-cli", line 48, in git_clone_repo
>      stdout=logf, stderr=logf)
>    File "/usr/lib64/python3.4/subprocess.py", line 558, in check_call
>      raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['git', 'remote', 'add', '-f', '--mirror=fetch', '3c8cf5a9c21ff8782164d1def7f44bd888713384', 'https://github.com/patchew-project/qemu']' returned non-zero exit status 1
>
>
>
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

ping
07.02.2018 15:50, Vladimir Sementsov-Ogievskiy wrote:
> 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       | 73 +++++++++++++++++++++++++++++++++-
>   include/block/accounting.h |  9 +++++
>   block/accounting.c         | 97 ++++++++++++++++++++++++++++++++++++++++++++++
>   block/qapi.c               | 31 +++++++++++++++
>   blockdev.c                 | 19 +++++++++
>   5 files changed, 228 insertions(+), 1 deletion(-)
>


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

ping
07.02.2018 15:50, Vladimir Sementsov-Ogievskiy wrote:
> 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       | 73 +++++++++++++++++++++++++++++++++-
>   include/block/accounting.h |  9 +++++
>   block/accounting.c         | 97 ++++++++++++++++++++++++++++++++++++++++++++++
>   block/qapi.c               | 31 +++++++++++++++
>   blockdev.c                 | 19 +++++++++
>   5 files changed, 228 insertions(+), 1 deletion(-)
>


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

07.02.2018 15:50, Vladimir Sementsov-Ogievskiy wrote:
> 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 |  9 +++++
>   block/accounting.c         | 97 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 106 insertions(+)
>
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index b833d26d6c..9679020f64 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -45,6 +45,12 @@ struct BlockAcctTimedStats {
>       QSLIST_ENTRY(BlockAcctTimedStats) entries;
>   };
>   
> +typedef struct BlockLatencyHistogram {
> +    int size;
> +    uint64_t *points; /* @size-1 points here (all points, except 0 and +inf) */
> +    uint64_t *histogram[BLOCK_MAX_IOTYPE]; /* @size elements for each type */
> +} BlockLatencyHistogram;
> +
>   struct BlockAcctStats {
>       QemuMutex lock;
>       uint64_t nr_bytes[BLOCK_MAX_IOTYPE];
> @@ -57,6 +63,7 @@ struct BlockAcctStats {
>       QSLIST_HEAD(, BlockAcctTimedStats) intervals;
>       bool account_invalid;
>       bool account_failed;
> +    BlockLatencyHistogram latency_histogram;
>   };
>   
>   typedef struct BlockAcctCookie {
> @@ -82,5 +89,7 @@ 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, uint64List *latency);
> +void block_latency_histogram_clear(BlockAcctStats *stats);
>   
>   #endif
> diff --git a/block/accounting.c b/block/accounting.c
> index 87ef5bbfaa..0051ff0c24 100644
> --- a/block/accounting.c
> +++ b/block/accounting.c
> @@ -94,6 +94,100 @@ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
>       cookie->type = type;
>   }
>   
> +/* block_latency_histogram_compare_func
> + * Compare @key with interval [@el, @el+1), where @el+1 is a next array element
> + * after @el.
> + * Return: -1 if @key < @el
> + *          0 if @key in [@el, @el+1)
> + *         +1 if @key >= @el+1
> + */
> +static int block_latency_histogram_compare_func(const void *key, const void *el)
> +{
> +    uint64_t k = *(uint64_t *)key;
> +    uint64_t a = *(uint64_t *)el;
> +    uint64_t b = *((uint64_t *)el + 1);
> +
> +    return k < a ? -1 : (k < b ? 0 : 1);
> +}
> +
> +static void block_latency_histogram_account(BlockLatencyHistogram *hist,
> +                                            enum BlockAcctType type,
> +                                            int64_t latency_ns)
> +{
> +    uint64_t *data, *pos;
> +
> +    if (hist->points == NULL) {
> +        /* histogram disabled */
> +        return;
> +    }
> +
> +    data = hist->histogram[type];
> +
> +    if (latency_ns < hist->points[0]) {
> +        data[0]++;
> +        return;
> +    }
> +
> +    if (latency_ns >= hist->points[hist->size - 2]) {
> +        data[hist->size - 1]++;
> +        return;
> +    }
> +
> +    pos = bsearch(&latency_ns, hist->points, hist->size - 2,
> +                  sizeof(hist->points[0]),
> +                  block_latency_histogram_compare_func);
> +    assert(pos != NULL);
> +
> +    data[pos - hist->points + 1]++;
> +}
> +
> +int block_latency_histogram_set(BlockAcctStats *stats, uint64List *latency)
> +{
> +    BlockLatencyHistogram *hist = &stats->latency_histogram;
> +    uint64List *entry;
> +    uint64_t *ptr;
> +    int i;
> +    uint64_t prev = 0;
> +
> +    hist->size = 1;

bug here. we can fail, so separate variable new_size is needed to 
calculate new size.

> +
> +    for (entry = latency; entry; entry = entry->next) {
> +        if (entry->value <= prev) {
> +            return -EINVAL;
> +        }
> +        hist->size++;
> +        prev = entry->value;
> +    }
> +
> +    hist->points = g_renew(uint64_t, hist->points, hist->size - 1);
> +    for (entry = latency, ptr = hist->points; entry;
> +         entry = entry->next, ptr++)
> +    {
> +        *ptr = entry->value;
> +    }
> +
> +    for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
> +        hist->histogram[i] = g_renew(uint64_t, hist->histogram[i], hist->size);
> +        memset(hist->histogram[i], 0, hist->size * sizeof(uint64_t));
> +    }
> +
> +    return 0;
> +}
> +
> +void block_latency_histogram_clear(BlockAcctStats *stats)
> +{
> +    BlockLatencyHistogram *hist = &stats->latency_histogram;
> +    int i;
> +
> +    g_free(hist->points);
> +    hist->points = NULL;
> +
> +    for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
> +        g_free(hist->histogram[i]);
> +        hist->histogram[i] = NULL;
> +    }
> +}
> +
>   static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
>                                    bool failed)
>   {
> @@ -116,6 +210,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;


-- 
Best regards,
Vladimir

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

[Stefan Hajnoczi]

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

On Wed, Feb 07, 2018 at 03:50:36PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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 |  9 +++++
>  block/accounting.c         | 97 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 106 insertions(+)
> 
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index b833d26d6c..9679020f64 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -45,6 +45,12 @@ struct BlockAcctTimedStats {
>      QSLIST_ENTRY(BlockAcctTimedStats) entries;
>  };
>  
> +typedef struct BlockLatencyHistogram {
> +    int size;
> +    uint64_t *points; /* @size-1 points here (all points, except 0 and +inf) */
> +    uint64_t *histogram[BLOCK_MAX_IOTYPE]; /* @size elements for each type */

The semantics of these fields is not obvious.  Please include a comment
with an example or ASCII-art diagram so anyone reading the code
understands the purpose of the fields without reading all the code.

According to Wikipedia and Mathworld, "intervals" and "bins" are
commonly used terms:
https://en.wikipedia.org/wiki/Histogram
http://mathworld.wolfram.com/Histogram.html

I suggest:

  typedef struct {
      /* The following histogram is represented like this:
       *
       * 5|           *
       * 4|           *
       * 3| *         *
       * 2| *         *    *
       * 1| *    *    *    *
       *  +------------------
       *      10   50   100
       *
       * BlockLatencyHistogram histogram = {
       *     .nbins = 4,
       *     .intervals = {10, 50, 100},
       *     .bins = {3, 1, 5, 2},
       * };
       */
      size_t nbins;

      /* Interval boundaries (there are @nbins - 1 elements) */
      uint64_t *intervals;

      /* Frequency data (there are @nbins elements) */
      uint64_t *bins[BLOCK_MAX_IOTYPE];
  } BlockLatencyHistogram;

> +/* block_latency_histogram_compare_func
> + * Compare @key with interval [@el, @el+1), where @el+1 is a next array element
> + * after @el.
> + * Return: -1 if @key < @el
> + *          0 if @key in [@el, @el+1)
> + *         +1 if @key >= @el+1

"@el+1" is confusing, usually x+1 means "the value of x plus 1", not
"y" (a different variable!).

I suggest:

  /* 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)

> +int block_latency_histogram_set(BlockAcctStats *stats, uint64List *latency)
> +{
> +    BlockLatencyHistogram *hist = &stats->latency_histogram;
> +    uint64List *entry;
> +    uint64_t *ptr;
> +    int i;
> +    uint64_t prev = 0;
> +
> +    hist->size = 1;
> +
> +    for (entry = latency; entry; entry = entry->next) {
> +        if (entry->value <= prev) {
> +            return -EINVAL;
> +        }
> +        hist->size++;
> +        prev = entry->value;
> +    }
> +
> +    hist->points = g_renew(uint64_t, hist->points, hist->size - 1);
> +    for (entry = latency, ptr = hist->points; entry;
> +         entry = entry->next, ptr++)
> +    {
> +        *ptr = entry->value;
> +    }
> +
> +    for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
> +        hist->histogram[i] = g_renew(uint64_t, hist->histogram[i], hist->size);
> +        memset(hist->histogram[i], 0, hist->size * sizeof(uint64_t));

Is there a reason for using g_renew() and then clearing everything?
This is more concise:

  g_free(hist->histogram[i]);
  hist->histogram[i] = g_new0(uint64_t, hist->size);

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

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

[Stefan Hajnoczi]

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

On Wed, Feb 07, 2018 at 03:50:37PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8225308904..74fe3fe9c4 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -451,6 +451,74 @@
>             'status': 'DirtyBitmapStatus'} }
>  
>  ##
> +# @BlockLatencyHistogramInfo:
> +#
> +# Block latency histogram.
> +#
> +# @latency: list of latency points in microseconds. Matches the @latency

The meaning of "point" is unclear here.  I think the term "interval
boundary" is clearer, but an example may help to illustrate this.  The
field could also be renamed to "intervals".

> +#           parameter from the last call to block-latency-histogram-set for the
> +#           given device.

The fastest NVMe devices are specified at 10 microsecond latency or
less.  The software stack adds latency on top of that, but using
microseconds as units here may not be future-proof.

I suggest using nanoseconds, just in case.

> +#
> +# @read: list of read-request counts corresponding to latency region.
> +#        len(@read) = len(@latency) + 1
> +#        @read[0] corresponds to latency region [0, @latency[0])

"latency region" == "bin" or "interval" in histogram terminology

> +#        for 0 < i < len(@latency): @read[i] corresponds to latency region
> +#            [@latency[i-1], @latency[i])
> +#        @read.last_element corresponds to latency region
> +#            [@latency.last_element, +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'] } }
> +
> +##
> +# @block-latency-histogram-set:
> +#
> +# Remove old latency histogram (if exist) and (optionally) create a new one.

s/if exist/if present/

> +# Add a latency histogram to block device. If a latency histogram already
> +# exists for the device it will be removed and a new one created. The latency
> +# histogram may be queried through query-blockstats.
> +# Set, restart or clear latency histogram.
> +#
> +# @device: device name to set latency histogram for.

query-blockstats also works on nodes, so this command must work not just
on devices but on named nodes too.

> +# @latency: If unspecified, the latency histogram will be removed (if exists).
> +#           Otherwise it should be list of latency points in microseconds. Old
> +#           latency histogram will be removed (if exists) and a new one will be
> +#           created. The sequence must be ascending, elements must be greater
> +#           than zero. Histogram latency regions would be
> +#           [0, @latency[0]), ..., [@latency[i], @latency[i+1]), ...,
> +#           [@latency.last_element, +inf)

Reworded:

  @intervals: An optional list of interval boundary values in
	      nanoseconds, all greater than zero and in ascending order.
	      The list [10, 50, 100] produces the following histogram
	      bins: [0, 10), [10, 50), [50, 100), [100, +inf).  If the
	      field is absent the existing latency histogram is removed
	      from the device (if present).

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

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

[Stefan Hajnoczi]

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

On Wed, Feb 07, 2018 at 03:50:35PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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       | 73 +++++++++++++++++++++++++++++++++-
>  include/block/accounting.h |  9 +++++
>  block/accounting.c         | 97 ++++++++++++++++++++++++++++++++++++++++++++++
>  block/qapi.c               | 31 +++++++++++++++
>  blockdev.c                 | 19 +++++++++
>  5 files changed, 228 insertions(+), 1 deletion(-)

The feature looks good.  I posted documentation and code readability
suggestions.

Stefan

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

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

[Emilio G. Cota]

On Tue, Mar 06, 2018 at 16:00:17 +0000, Stefan Hajnoczi wrote:
> On Wed, Feb 07, 2018 at 03:50:35PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > Vladimir Sementsov-Ogievskiy (2):
> >   block/accounting: introduce latency histogram
> >   qapi: add block latency histogram interface
(snip)
> >  5 files changed, 228 insertions(+), 1 deletion(-)
> 
> The feature looks good.  I posted documentation and code readability
> suggestions.

Hi Vladimir,

Did you consider using qdist? For reference, see commit bf3afd5f419
and/or grep 'qdist'.

Thanks,

		Emilio

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

[Vladimir Sementsov-Ogievskiy]

Hi Emilio!

Looked through qdist, if I understand correctly, it saves each added 
(with different value) element. It is not effective for disk io timing - 
we'll have too much elements. In my approach, histogram don't grow, it 
initially have several ranges and counts hits to each range.


06.03.2018 20:49, Emilio G. Cota wrote:
> On Tue, Mar 06, 2018 at 16:00:17 +0000, Stefan Hajnoczi wrote:
>> On Wed, Feb 07, 2018 at 03:50:35PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> Vladimir Sementsov-Ogievskiy (2):
>>>    block/accounting: introduce latency histogram
>>>    qapi: add block latency histogram interface
> (snip)
>>>   5 files changed, 228 insertions(+), 1 deletion(-)
>> The feature looks good.  I posted documentation and code readability
>> suggestions.
> Hi Vladimir,
>
> Did you consider using qdist? For reference, see commit bf3afd5f419
> and/or grep 'qdist'.
>
> Thanks,
>
> 		Emilio


-- 
Best regards,
Vladimir

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

[Eric Blake]

On 03/06/2018 09:32 AM, Stefan Hajnoczi wrote:
> On Wed, Feb 07, 2018 at 03:50:36PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 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>
>> ---

> According to Wikipedia and Mathworld, "intervals" and "bins" are
> commonly used terms:
> https://en.wikipedia.org/wiki/Histogram
> http://mathworld.wolfram.com/Histogram.html
> 
> I suggest:
> 
>    typedef struct {
>        /* The following histogram is represented like this:
>         *
>         * 5|           *
>         * 4|           *
>         * 3| *         *
>         * 2| *         *    *
>         * 1| *    *    *    *
>         *  +------------------
>         *      10   50   100
>         *
>         * BlockLatencyHistogram histogram = {
>         *     .nbins = 4,
>         *     .intervals = {10, 50, 100},
>         *     .bins = {3, 1, 5, 2},
>         * };

The name 'intervals' is still slightly ambiguous: does it hold the 
boundary point (0-10 for 10 slots, 10-50 for 40 slots, 50-100, for 50 
slots, then 100-INF) or is it the interval size of each slot (first bin 
is 10 slots for 0-10, next bin is 50 slots wide so 10-60, next bin is 
100 slots wide so 60-160, everything else is 160-INF).  But the 
ascii-art diagram plus the text is sufficient to resolve the intent if 
you keep that name (I don't have a suggestion for a better name).

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

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

[Eric Blake]

On 03/06/2018 10:00 AM, Stefan Hajnoczi wrote:
> On Wed, Feb 07, 2018 at 03:50:35PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 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       | 73 +++++++++++++++++++++++++++++++++-
>>   include/block/accounting.h |  9 +++++
>>   block/accounting.c         | 97 ++++++++++++++++++++++++++++++++++++++++++++++
>>   block/qapi.c               | 31 +++++++++++++++
>>   blockdev.c                 | 19 +++++++++
>>   5 files changed, 228 insertions(+), 1 deletion(-)
> 
> The feature looks good.  I posted documentation and code readability
> suggestions.
> 

I also think it makes sense; if a v3 hits the list in time, I will 
probably include it in my last QAPI pull before softfreeze.

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

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

[Vladimir Sementsov-Ogievskiy]

08.03.2018 20:31, Eric Blake wrote:
> On 03/06/2018 09:32 AM, Stefan Hajnoczi wrote:
>> On Wed, Feb 07, 2018 at 03:50:36PM +0300, Vladimir 
>> Sementsov-Ogievskiy wrote:
>>> 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>
>>> ---
>
>> According to Wikipedia and Mathworld, "intervals" and "bins" are
>> commonly used terms:
>> https://en.wikipedia.org/wiki/Histogram
>> http://mathworld.wolfram.com/Histogram.html
>>
>> I suggest:
>>
>>    typedef struct {
>>        /* The following histogram is represented like this:
>>         *
>>         * 5|           *
>>         * 4|           *
>>         * 3| *         *
>>         * 2| *         *    *
>>         * 1| *    *    *    *
>>         *  +------------------
>>         *      10   50   100
>>         *
>>         * BlockLatencyHistogram histogram = {
>>         *     .nbins = 4,
>>         *     .intervals = {10, 50, 100},
>>         *     .bins = {3, 1, 5, 2},
>>         * };
>
> The name 'intervals' is still slightly ambiguous: does it hold the 
> boundary point (0-10 for 10 slots, 10-50 for 40 slots, 50-100, for 50 
> slots, then 100-INF) or is it the interval size of each slot (first 
> bin is 10 slots for 0-10, next bin is 50 slots wide so 10-60, next bin 
> is 100 slots wide so 60-160, everything else is 160-INF).  But the 
> ascii-art diagram plus the text is sufficient to resolve the intent if 
> you keep that name (I don't have a suggestion for a better name).
>

Hm. these numbers are actually boundary points of histogram intervals, 
not intervals itself. And, wiki says "The bins are usually specified as 
consecutive, non-overlapping intervals of a variable.", so, intervals 
are bins.

So, what about:

1. interval_boundaries
2. bin_boundaries
3. boundaries

(and same names with s/_/-/ for qapi)


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

08.03.2018 21:14, Vladimir Sementsov-Ogievskiy wrote:
> 08.03.2018 20:31, Eric Blake wrote:
>> On 03/06/2018 09:32 AM, Stefan Hajnoczi wrote:
>>> On Wed, Feb 07, 2018 at 03:50:36PM +0300, Vladimir 
>>> Sementsov-Ogievskiy wrote:
>>>> 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>
>>>> ---
>>
>>> According to Wikipedia and Mathworld, "intervals" and "bins" are
>>> commonly used terms:
>>> https://en.wikipedia.org/wiki/Histogram
>>> http://mathworld.wolfram.com/Histogram.html
>>>
>>> I suggest:
>>>
>>>    typedef struct {
>>>        /* The following histogram is represented like this:
>>>         *
>>>         * 5|           *
>>>         * 4|           *
>>>         * 3| *         *
>>>         * 2| *         *    *
>>>         * 1| *    *    *    *
>>>         *  +------------------
>>>         *      10   50   100
>>>         *
>>>         * BlockLatencyHistogram histogram = {
>>>         *     .nbins = 4,
>>>         *     .intervals = {10, 50, 100},
>>>         *     .bins = {3, 1, 5, 2},
>>>         * };
>>
>> The name 'intervals' is still slightly ambiguous: does it hold the 
>> boundary point (0-10 for 10 slots, 10-50 for 40 slots, 50-100, for 50 
>> slots, then 100-INF) or is it the interval size of each slot (first 
>> bin is 10 slots for 0-10, next bin is 50 slots wide so 10-60, next 
>> bin is 100 slots wide so 60-160, everything else is 160-INF).  But 
>> the ascii-art diagram plus the text is sufficient to resolve the 
>> intent if you keep that name (I don't have a suggestion for a better 
>> name).
>>
>
> Hm. these numbers are actually boundary points of histogram intervals, 
> not intervals itself. And, wiki says "The bins are usually specified 
> as consecutive, non-overlapping intervals of a variable.", so, 
> intervals are bins.
>
> So, what about:
>
> 1. interval_boundaries
> 2. bin_boundaries
> 3. boundaries
>
> (and same names with s/_/-/ for qapi)
>
>


Also, now I doubt, is it a good idea to share same bin boundaries for 
each io type.

-- 
Best regards,
Vladimir

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

[Emilio G. Cota]

On Thu, Mar 08, 2018 at 14:42:29 +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi Emilio!
> 
> Looked through qdist, if I understand correctly, it saves each added (with
> different value) element. It is not effective for disk io timing - we'll
> have too much elements. In my approach, histogram don't grow, it initially
> have several ranges and counts hits to each range.

I thought about this use case, i.e. having a gazillion elements.
You should just do some pre-binning before inserting samples
into qdist, as pointed out in this comment in qdist.h:

/*
 * Samples with the same 'x value' end up in the same qdist_entry,
 * e.g. inc(0.1) and inc(0.1) end up as {x=0.1, count=2}.
 *
 * Binning happens only at print time, so that we retain the flexibility to
 * choose the binning. This might not be ideal for workloads that do not care
 * much about precision and insert many samples all with different x values;
 * in that case, pre-binning (e.g. entering both 0.115 and 0.097 as 0.1)
 * should be considered.
 */
struct qdist_entry {
    double x;
    unsigned long count;
};

Let me know if you need help with that.

Thanks,

		Emilio

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

[Vladimir Sementsov-Ogievskiy]

08.03.2018 21:21, Vladimir Sementsov-Ogievskiy wrote:
> 08.03.2018 21:14, Vladimir Sementsov-Ogievskiy wrote:
>> 08.03.2018 20:31, Eric Blake wrote:
>>> On 03/06/2018 09:32 AM, Stefan Hajnoczi wrote:
>>>> On Wed, Feb 07, 2018 at 03:50:36PM +0300, Vladimir 
>>>> Sementsov-Ogievskiy wrote:
>>>>> 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>
>>>>> ---
>>>
>>>> According to Wikipedia and Mathworld, "intervals" and "bins" are
>>>> commonly used terms:
>>>> https://en.wikipedia.org/wiki/Histogram
>>>> http://mathworld.wolfram.com/Histogram.html
>>>>
>>>> I suggest:
>>>>
>>>>    typedef struct {
>>>>        /* The following histogram is represented like this:
>>>>         *
>>>>         * 5|           *
>>>>         * 4|           *
>>>>         * 3| *         *
>>>>         * 2| *         *    *
>>>>         * 1| *    *    *    *
>>>>         *  +------------------
>>>>         *      10   50   100
>>>>         *
>>>>         * BlockLatencyHistogram histogram = {
>>>>         *     .nbins = 4,
>>>>         *     .intervals = {10, 50, 100},
>>>>         *     .bins = {3, 1, 5, 2},
>>>>         * };
>>>
>>> The name 'intervals' is still slightly ambiguous: does it hold the 
>>> boundary point (0-10 for 10 slots, 10-50 for 40 slots, 50-100, for 
>>> 50 slots, then 100-INF) or is it the interval size of each slot 
>>> (first bin is 10 slots for 0-10, next bin is 50 slots wide so 10-60, 
>>> next bin is 100 slots wide so 60-160, everything else is 160-INF).  
>>> But the ascii-art diagram plus the text is sufficient to resolve the 
>>> intent if you keep that name (I don't have a suggestion for a better 
>>> name).
>>>
>>
>> Hm. these numbers are actually boundary points of histogram 
>> intervals, not intervals itself. And, wiki says "The bins are usually 
>> specified as consecutive, non-overlapping intervals of a variable.", 
>> so, intervals are bins.
>>
>> So, what about:
>>
>> 1. interval_boundaries
>> 2. bin_boundaries
>> 3. boundaries
>>
>> (and same names with s/_/-/ for qapi)
>>
>>
>
>
> Also, now I doubt, is it a good idea to share same bin boundaries for 
> each io type.
>

so, for qmp command, what about:

boundaries - optional, default boundaries for all io operations
boundaries-read - boundaries for read
boundaries-write - boundaries for write
...

so, call without any boundaries: drop _all_ histograms
call with only specific boundaries-*: set or reset _only_ corresponding 
specific histograms
call with only boundaries parameter: set or reset _all_ histograms
call with boundaries parameter and some of (or all, but it is not 
useful) specific boundaries-*: set or reset _all_ histograms, some to 
default  and some to specific.

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

08.03.2018 21:56, Emilio G. Cota wrote:
> On Thu, Mar 08, 2018 at 14:42:29 +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi Emilio!
>>
>> Looked through qdist, if I understand correctly, it saves each added (with
>> different value) element. It is not effective for disk io timing - we'll
>> have too much elements. In my approach, histogram don't grow, it initially
>> have several ranges and counts hits to each range.
> I thought about this use case, i.e. having a gazillion elements.
> You should just do some pre-binning before inserting samples
> into qdist, as pointed out in this comment in qdist.h:
>
> /*
>   * Samples with the same 'x value' end up in the same qdist_entry,
>   * e.g. inc(0.1) and inc(0.1) end up as {x=0.1, count=2}.
>   *
>   * Binning happens only at print time, so that we retain the flexibility to
>   * choose the binning. This might not be ideal for workloads that do not care
>   * much about precision and insert many samples all with different x values;
>   * in that case, pre-binning (e.g. entering both 0.115 and 0.097 as 0.1)
>   * should be considered.
>   */
> struct qdist_entry {
>      double x;
>      unsigned long count;
> };
>
> Let me know if you need help with that.
>
> Thanks,
>
> 		Emilio

In this case, I'll have to do same bin search (and store same interval 
settings) as I already do, on my part, to calculate a parameter for 
qdist interface. And I'll have store almost all same data on my part. 
So, it doesn't really help. And I need nothing of qdist benefits: I 
don't need (and don't want) dynamic allocation of bins on adding an 
element or any type of visualization.

-- 
Best regards,
Vladimir

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

[Eric Blake]

On 03/08/2018 12:58 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Hm. these numbers are actually boundary points of histogram 
>>> intervals, not intervals itself. And, wiki says "The bins are usually 
>>> specified as consecutive, non-overlapping intervals of a variable.", 
>>> so, intervals are bins.
>>>
>>> So, what about:
>>>
>>> 1. interval_boundaries
>>> 2. bin_boundaries
>>> 3. boundaries
>>>
>>> (and same names with s/_/-/ for qapi)

Any of those works for me; 1 is probably most precise, while 3 is the 
shortest to type.

>>>
>>>
>>
>>
>> Also, now I doubt, is it a good idea to share same bin boundaries for 
>> each io type.
>>
> 
> so, for qmp command, what about:
> 
> boundaries - optional, default boundaries for all io operations
> boundaries-read - boundaries for read
> boundaries-write - boundaries for write
> ...
> 
> so, call without any boundaries: drop _all_ histograms
> call with only specific boundaries-*: set or reset _only_ corresponding 
> specific histograms
> call with only boundaries parameter: set or reset _all_ histograms
> call with boundaries parameter and some of (or all, but it is not 
> useful) specific boundaries-*: set or reset _all_ histograms, some to 
> default  and some to specific.

Seems reasonable, and if that makes the command more useful for your 
timing, I'm fine with it (your argument that different timing bins for 
read and write also makes sense, as there are some inherent differences 
in the amount of work done in the two directions).

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

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

[Emilio G. Cota]

On Thu, Mar 08, 2018 at 22:07:35 +0300, Vladimir Sementsov-Ogievskiy wrote:
> 08.03.2018 21:56, Emilio G. Cota wrote:
> >  * Binning happens only at print time, so that we retain the flexibility to
> >  * choose the binning. This might not be ideal for workloads that do not care
> >  * much about precision and insert many samples all with different x values;
> >  * in that case, pre-binning (e.g. entering both 0.115 and 0.097 as 0.1)
> >  * should be considered.
(snip)
> In this case, I'll have to do same bin search (and store same interval
> settings) as I already do, on my part, to calculate a parameter for qdist
> interface. And I'll have store almost all same data on my part. So, it
> doesn't really help. And I need nothing of qdist benefits: I don't need (and
> don't want) dynamic allocation of bins on adding an element or any type of
> visualization.

I see. You require a couple of features that qdist doesn't yet support:

- Arbitrarily-sized, pre-defined bins.
- Support for querying the data programmatically instead of just
  printing it out.

We could circumvent the first missing feature with pre-binning,
but in that case we'd do a bsearch twice as you point out (BTW
your concern about memory allocation wouldn't apply though).

The second missing feature should be easy to add to qdist.

That said, given that you want this in for 2.12, I'd go with your
approach for now. In the future we should look into supporting
your use case in qdist, since it is likely that there will be
more users with a similar need.

Thanks,

		Emilio