All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/2] block latency histogram
@ 2018-02-06 14:07 Vladimir Sementsov-Ogievskiy
  2018-02-06 14:07 ` [Qemu-devel] [PATCH 1/2] block/accounting: introduce " Vladimir Sementsov-Ogievskiy
  2018-02-06 14:07 ` [Qemu-devel] [PATCH 2/2] qapi: add block latency histogram interface Vladimir Sementsov-Ogievskiy
  0 siblings, 2 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-02-06 14:07 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: eblake, armbru, mreitz, kwolf, den, nshirokovskiy, vsementsov

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

 qapi/block-core.json       | 62 +++++++++++++++++++++++++++++++++-
 include/block/accounting.h |  8 +++++
 block/accounting.c         | 83 ++++++++++++++++++++++++++++++++++++++++++++++
 block/qapi.c               | 31 +++++++++++++++++
 blockdev.c                 | 15 +++++++++
 5 files changed, 198 insertions(+), 1 deletion(-)

-- 
2.11.1

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH 1/2] block/accounting: introduce latency histogram
  2018-02-06 14:07 [Qemu-devel] [PATCH 0/2] block latency histogram Vladimir Sementsov-Ogievskiy
@ 2018-02-06 14:07 ` Vladimir Sementsov-Ogievskiy
  2018-02-06 14:07 ` [Qemu-devel] [PATCH 2/2] qapi: add block latency histogram interface Vladimir Sementsov-Ogievskiy
  1 sibling, 0 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-02-06 14:07 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: eblake, armbru, mreitz, kwolf, den, nshirokovskiy, vsementsov

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 |  8 +++++
 block/accounting.c         | 83 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+)

diff --git a/include/block/accounting.h b/include/block/accounting.h
index b833d26d6c..7fbfc86c43 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,6 @@ 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);
 
 #endif
diff --git a/block/accounting.c b/block/accounting.c
index 87ef5bbfaa..a34ef09015 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -94,6 +94,86 @@ 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;
+}
+
 static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
                                  bool failed)
 {
@@ -116,6 +196,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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH 2/2] qapi: add block latency histogram interface
  2018-02-06 14:07 [Qemu-devel] [PATCH 0/2] block latency histogram Vladimir Sementsov-Ogievskiy
  2018-02-06 14:07 ` [Qemu-devel] [PATCH 1/2] block/accounting: introduce " Vladimir Sementsov-Ogievskiy
@ 2018-02-06 14:07 ` Vladimir Sementsov-Ogievskiy
  2018-02-06 15:50   ` Eric Blake
  1 sibling, 1 reply; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-02-06 14:07 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: eblake, armbru, mreitz, kwolf, den, nshirokovskiy, vsementsov

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 | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 block/qapi.c         | 31 ++++++++++++++++++++++++++
 blockdev.c           | 15 +++++++++++++
 3 files changed, 107 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8225308904..4706a934d9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -451,6 +451,63 @@
            'status': 'DirtyBitmapStatus'} }
 
 ##
+# @BlockLatencyHistogramInfo:
+#
+# Block latency histogram.
+#
+# @latency: list of latency points in microseconds. Equals to @latency parameter
+#           of last called block-latency-histogram-set.
+#
+# @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:
+#
+# Add latency histogram to block device. If latency histogram alredy exists for
+# the device it will be removed and a new one created. Latency histogram may be
+# quered through query-blockstats.
+#
+# @device: device name to set latency histogram for.
+#
+# @latency: list of latency points in microseconds. The sequcence 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.
+#
+# Since: 2.12
+#
+# Example:
+#
+# -> { "execute": "block-latency-histogram-set",
+#      "arguments": { "device": "drive0",
+#                     "latency": [500000, 1000000, 2000000] } }
+# <- { "return": {} }
+##
+{ 'command': 'block-latency-histogram-set',
+  'data': {'device': 'str', 'latency': ['uint64'] } }
+
+##
 # @BlockInfo:
 #
 # Block device information.  This structure describes a virtual device and
@@ -730,6 +787,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 +801,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..dbf17aca0c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4176,6 +4176,21 @@ 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, uint64List *latency,
+                                     Error **errp)
+{
+    BlockBackend *blk = blk_by_name(device);
+    if (!blk) {
+        error_setg(errp, "Device '%s' not found", device);
+        return;
+    }
+
+    if (block_latency_histogram_set(blk_get_stats(blk), latency) < 0) {
+        error_setg(errp, "Invalid latency array");
+        return;
+    }
+}
+
 QemuOptsList qemu_common_drive_opts = {
     .name = "drive",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head),
-- 
2.11.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] qapi: add block latency histogram interface
  2018-02-06 14:07 ` [Qemu-devel] [PATCH 2/2] qapi: add block latency histogram interface Vladimir Sementsov-Ogievskiy
@ 2018-02-06 15:50   ` Eric Blake
  2018-02-06 18:06     ` Vladimir Sementsov-Ogievskiy
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Blake @ 2018-02-06 15:50 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
  Cc: armbru, mreitz, kwolf, den, nshirokovskiy

On 02/06/2018 08:07 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 | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>   block/qapi.c         | 31 ++++++++++++++++++++++++++
>   blockdev.c           | 15 +++++++++++++
>   3 files changed, 107 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8225308904..4706a934d9 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -451,6 +451,63 @@
>              'status': 'DirtyBitmapStatus'} }
>   
>   ##
> +# @BlockLatencyHistogramInfo:
> +#
> +# Block latency histogram.
> +#
> +# @latency: list of latency points in microseconds. Equals to @latency parameter
> +#           of last called block-latency-histogram-set.

Second sentence might read better as:

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'] } }

Okay, I can see how this interface works.

> +
> +##
> +# @block-latency-histogram-set:
> +#
> +# Add latency histogram to block device. If latency histogram alredy exists for

s/latency/a latency/ (twice)
s/alredy/already/

> +# the device it will be removed and a new one created. Latency histogram may be

s/Latency/The latency/

> +# quered through query-blockstats.

s/quered/queried/

> +#
> +# @device: device name to set latency histogram for.
> +#
> +# @latency: list of latency points in microseconds. The sequcence must be

s/sequcence/sequence/

> +#           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.
> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# -> { "execute": "block-latency-histogram-set",
> +#      "arguments": { "device": "drive0",
> +#                     "latency": [500000, 1000000, 2000000] } }
> +# <- { "return": {} }
> +##
> +{ 'command': 'block-latency-histogram-set',
> +  'data': {'device': 'str', 'latency': ['uint64'] } }

The commit message mentioned that you can set and clear histogram 
tracking; how does this interface let you clear things?  By passing a 
0-length latency array?  If you violate the constraint (pass 
non-ascending points, for example), does the previous latency map get 
wiped out or is it still preserved unchanged?

> +
> +##
>   # @BlockInfo:
>   #
>   # Block device information.  This structure describes a virtual device and
> @@ -730,6 +787,8 @@
>   # @timed_stats: Statistics specific to the set of previously defined
>   #               intervals of time (Since 2.5)
>   #
> +# @latency-histogram: @BlockLatencyHistogramInfo. (Since 2.12)

I'd mention that this only appears if block-latency-histogram-set has 
been called.

> +#
>   # Since: 0.14.0
>   ##
>   { 'struct': 'BlockDeviceStats',
> @@ -742,7 +801,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' } }
>   


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] qapi: add block latency histogram interface
  2018-02-06 15:50   ` Eric Blake
@ 2018-02-06 18:06     ` Vladimir Sementsov-Ogievskiy
  2018-02-06 19:02       ` Eric Blake
  0 siblings, 1 reply; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-02-06 18:06 UTC (permalink / raw)
  To: Eric Blake, qemu-devel, qemu-block
  Cc: armbru, mreitz, kwolf, den, nshirokovskiy

06.02.2018 18:50, Eric Blake wrote:
> On 02/06/2018 08:07 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 | 62 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   block/qapi.c         | 31 ++++++++++++++++++++++++++
>>   blockdev.c           | 15 +++++++++++++
>>   3 files changed, 107 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 8225308904..4706a934d9 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -451,6 +451,63 @@
>>              'status': 'DirtyBitmapStatus'} }
>>     ##
>> +# @BlockLatencyHistogramInfo:
>> +#
>> +# Block latency histogram.
>> +#
>> +# @latency: list of latency points in microseconds. Equals to 
>> @latency parameter
>> +#           of last called block-latency-histogram-set.
>
> Second sentence might read better as:
>
> 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'] } }
>
> Okay, I can see how this interface works.
>
>> +
>> +##
>> +# @block-latency-histogram-set:
>> +#
>> +# Add latency histogram to block device. If latency histogram alredy 
>> exists for
>
> s/latency/a latency/ (twice)
> s/alredy/already/
>
>> +# the device it will be removed and a new one created. Latency 
>> histogram may be
>
> s/Latency/The latency/
>
>> +# quered through query-blockstats.
>
> s/quered/queried/
>
>> +#
>> +# @device: device name to set latency histogram for.
>> +#
>> +# @latency: list of latency points in microseconds. The sequcence 
>> must be
>
> s/sequcence/sequence/
>
>> +#           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.
>> +#
>> +# Since: 2.12
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "block-latency-histogram-set",
>> +#      "arguments": { "device": "drive0",
>> +#                     "latency": [500000, 1000000, 2000000] } }
>> +# <- { "return": {} }
>> +##
>> +{ 'command': 'block-latency-histogram-set',
>> +  'data': {'device': 'str', 'latency': ['uint64'] } }
>
> The commit message mentioned that you can set and clear histogram 
> tracking; how does this interface let you clear things?  By passing a 
> 0-length latency array?  If you violate the constraint (pass 
> non-ascending points, for example), does the previous latency map get 
> wiped out or is it still preserved unchanged?

On error nothing is changed.

By "clear" I mean zeroing statistics, not removing the whole histogram. 
And to zero statistics you can call set with the same latency array.
There is no way to remove histogram at all.. We can add 
block-latency-histogram-unset later if needed.

>
>> +
>> +##
>>   # @BlockInfo:
>>   #
>>   # Block device information.  This structure describes a virtual 
>> device and
>> @@ -730,6 +787,8 @@
>>   # @timed_stats: Statistics specific to the set of previously defined
>>   #               intervals of time (Since 2.5)
>>   #
>> +# @latency-histogram: @BlockLatencyHistogramInfo. (Since 2.12)
>
> I'd mention that this only appears if block-latency-histogram-set has 
> been called.

yes

>
>> +#
>>   # Since: 0.14.0
>>   ##
>>   { 'struct': 'BlockDeviceStats',
>> @@ -742,7 +801,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' } }
>
>


-- 
Best regards,
Vladimir

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] qapi: add block latency histogram interface
  2018-02-06 18:06     ` Vladimir Sementsov-Ogievskiy
@ 2018-02-06 19:02       ` Eric Blake
  2018-02-07 10:32         ` Vladimir Sementsov-Ogievskiy
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Blake @ 2018-02-06 19:02 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
  Cc: armbru, mreitz, kwolf, den, nshirokovskiy

On 02/06/2018 12:06 PM, Vladimir Sementsov-Ogievskiy wrote:
> 06.02.2018 18:50, Eric Blake wrote:
>> On 02/06/2018 08:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Set (and clear) histogram through new command
>>> block-latency-histogram-set and show new statistics in
>>> query-blockstats results.
>>>
>>
>> The commit message mentioned that you can set and clear histogram 
>> tracking; how does this interface let you clear things?  By passing a 
>> 0-length latency array?  If you violate the constraint (pass 
>> non-ascending points, for example), does the previous latency map get 
>> wiped out or is it still preserved unchanged?
> 
> On error nothing is changed.
> 
> By "clear" I mean zeroing statistics, not removing the whole histogram. 
> And to zero statistics you can call set with the same latency array.
> There is no way to remove histogram at all.. We can add 
> block-latency-histogram-unset later if needed.

Maybe "set (or restart) histogram collection points" might read better? 
I also don't think we need a new command; if you make 'latency' 
optional, then omitting it can serve to stop collecting statistics 
altogether, without needing a new command (then again, if you do that, 
now the command is used to "set, restart, and clear histogram collection").

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] qapi: add block latency histogram interface
  2018-02-06 19:02       ` Eric Blake
@ 2018-02-07 10:32         ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-02-07 10:32 UTC (permalink / raw)
  To: Eric Blake, qemu-devel, qemu-block
  Cc: armbru, mreitz, kwolf, den, nshirokovskiy

06.02.2018 22:02, Eric Blake wrote:
> On 02/06/2018 12:06 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 06.02.2018 18:50, Eric Blake wrote:
>>> On 02/06/2018 08:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Set (and clear) histogram through new command
>>>> block-latency-histogram-set and show new statistics in
>>>> query-blockstats results.
>>>>
>>>
>>> The commit message mentioned that you can set and clear histogram 
>>> tracking; how does this interface let you clear things?  By passing 
>>> a 0-length latency array?  If you violate the constraint (pass 
>>> non-ascending points, for example), does the previous latency map 
>>> get wiped out or is it still preserved unchanged?
>>
>> On error nothing is changed.
>>
>> By "clear" I mean zeroing statistics, not removing the whole 
>> histogram. And to zero statistics you can call set with the same 
>> latency array.
>> There is no way to remove histogram at all.. We can add 
>> block-latency-histogram-unset later if needed.
>
> Maybe "set (or restart) histogram collection points" might read 
> better? I also don't think we need a new command; if you make 
> 'latency' optional, then omitting it can serve to stop collecting 
> statistics altogether, without needing a new command (then again, if 
> you do that, now the command is used to "set, restart, and clear 
> histogram collection").
>

I like the idea, will do.

-- 
Best regards,
Vladimir

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-02-07 10:33 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-06 14:07 [Qemu-devel] [PATCH 0/2] block latency histogram Vladimir Sementsov-Ogievskiy
2018-02-06 14:07 ` [Qemu-devel] [PATCH 1/2] block/accounting: introduce " Vladimir Sementsov-Ogievskiy
2018-02-06 14:07 ` [Qemu-devel] [PATCH 2/2] qapi: add block latency histogram interface Vladimir Sementsov-Ogievskiy
2018-02-06 15:50   ` Eric Blake
2018-02-06 18:06     ` Vladimir Sementsov-Ogievskiy
2018-02-06 19:02       ` Eric Blake
2018-02-07 10:32         ` Vladimir Sementsov-Ogievskiy

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.