[Qemu-devel] [PATCH 0/3] Add block size histogram qapi interface

[Qemu-devel] [PATCH 0/3] Add block size histogram qapi interface

[zhenwei pi]

Set/Clear block size histograms through new command
x-block-size-histogram-set and show new statistics in
query-blockstats results.

This command has been tested for half year on QEMU-2.12, and we found
that 3K+ virtual machines write 25GB/s totally, the block size
histogram like following:
        0 ~ 8k: 58% ~ 62%
        8k ~ 32k: 10% ~ 12%
        32k ~ 128k: 2% ~ 3%
        128K ~ 512K: 24% ~ 26%
        512K ~ : ...

And the histogram data help us to optimise backend distributed
storage.

zhenwei pi (3):
  block/accounting: rename struct BlockLatencyHistogram
  block/accounting: introduce block size histogram
  qapi: add block size histogram interface

 block/accounting.c         |  55 ++++++++++++++++++------
 block/qapi.c               |  26 ++++++++++-
 blockdev.c                 |  56 ++++++++++++++++++++++++
 include/block/accounting.h |  12 ++++--
 qapi/block-core.json       | 105 ++++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 235 insertions(+), 19 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/3] block/accounting: rename struct BlockLatencyHistogram

[zhenwei pi]

Rename struct BlockLatencyHistogram to BlockHistogram, and rename
related functions name.
make this struct and functions be common, they can be used widely.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 block/accounting.c         | 31 ++++++++++++++++++-------------
 block/qapi.c               |  2 +-
 include/block/accounting.h |  8 ++++----
 3 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/block/accounting.c b/block/accounting.c
index 70a3d9a426..bb8148b6b1 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -94,13 +94,13 @@ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
     cookie->type = type;
 }
 
-/* block_latency_histogram_compare_func:
+/* block_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)
+static int block_histogram_compare_func(const void *key, const void *it)
 {
     uint64_t k = *(uint64_t *)key;
     uint64_t a = ((uint64_t *)it)[0];
@@ -109,8 +109,7 @@ static int block_latency_histogram_compare_func(const void *key, const void *it)
     return k < a ? -1 : (k < b ? 0 : 1);
 }
 
-static void block_latency_histogram_account(BlockLatencyHistogram *hist,
-                                            int64_t latency_ns)
+static void block_histogram_account(BlockHistogram *hist, int64_t val)
 {
     uint64_t *pos;
 
@@ -120,28 +119,26 @@ static void block_latency_histogram_account(BlockLatencyHistogram *hist,
     }
 
 
-    if (latency_ns < hist->boundaries[0]) {
+    if (val < hist->boundaries[0]) {
         hist->bins[0]++;
         return;
     }
 
-    if (latency_ns >= hist->boundaries[hist->nbins - 2]) {
+    if (val >= hist->boundaries[hist->nbins - 2]) {
         hist->bins[hist->nbins - 1]++;
         return;
     }
 
-    pos = bsearch(&latency_ns, hist->boundaries, hist->nbins - 2,
+    pos = bsearch(&val, hist->boundaries, hist->nbins - 2,
                   sizeof(hist->boundaries[0]),
-                  block_latency_histogram_compare_func);
+                  block_histogram_compare_func);
     assert(pos != NULL);
 
     hist->bins[pos - hist->boundaries + 1]++;
 }
 
-int block_latency_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
-                                uint64List *boundaries)
+static int block_histogram_set(BlockHistogram *hist, uint64List *boundaries)
 {
-    BlockLatencyHistogram *hist = &stats->latency_histogram[type];
     uint64List *entry;
     uint64_t *ptr;
     uint64_t prev = 0;
@@ -170,12 +167,20 @@ int block_latency_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
     return 0;
 }
 
+int block_latency_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
+                                uint64List *boundaries)
+{
+    BlockHistogram *hist = &stats->latency_histogram[type];
+
+    return block_histogram_set(hist, boundaries);
+}
+
 void block_latency_histograms_clear(BlockAcctStats *stats)
 {
     int i;
 
     for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
-        BlockLatencyHistogram *hist = &stats->latency_histogram[i];
+        BlockHistogram *hist = &stats->latency_histogram[i];
         g_free(hist->bins);
         g_free(hist->boundaries);
         memset(hist, 0, sizeof(*hist));
@@ -204,7 +209,7 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
         stats->nr_ops[cookie->type]++;
     }
 
-    block_latency_histogram_account(&stats->latency_histogram[cookie->type],
+    block_histogram_account(&stats->latency_histogram[cookie->type],
                                     latency_ns);
 
     if (!failed || stats->account_failed) {
diff --git a/block/qapi.c b/block/qapi.c
index 917435f022..f3a84f776e 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -415,7 +415,7 @@ static uint64List *uint64_list(uint64_t *list, int size)
     return out_list;
 }
 
-static void bdrv_latency_histogram_stats(BlockLatencyHistogram *hist,
+static void bdrv_latency_histogram_stats(BlockHistogram *hist,
                                          bool *not_null,
                                          BlockLatencyHistogramInfo **info)
 {
diff --git a/include/block/accounting.h b/include/block/accounting.h
index d1f67b10dd..270fddb69c 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -46,7 +46,7 @@ struct BlockAcctTimedStats {
     QSLIST_ENTRY(BlockAcctTimedStats) entries;
 };
 
-typedef struct BlockLatencyHistogram {
+typedef struct BlockHistogram {
     /* The following histogram is represented like this:
      *
      * 5|           *
@@ -57,7 +57,7 @@ typedef struct BlockLatencyHistogram {
      *  +------------------
      *      10   50   100
      *
-     * BlockLatencyHistogram histogram = {
+     * BlockHistogram histogram = {
      *     .nbins = 4,
      *     .boundaries = {10, 50, 100},
      *     .bins = {3, 1, 5, 2},
@@ -74,7 +74,7 @@ typedef struct BlockLatencyHistogram {
     uint64_t *boundaries; /* @nbins-1 numbers here
                              (all boundaries, except 0 and +inf) */
     uint64_t *bins;
-} BlockLatencyHistogram;
+} BlockHistogram;
 
 struct BlockAcctStats {
     QemuMutex lock;
@@ -88,7 +88,7 @@ struct BlockAcctStats {
     QSLIST_HEAD(, BlockAcctTimedStats) intervals;
     bool account_invalid;
     bool account_failed;
-    BlockLatencyHistogram latency_histogram[BLOCK_MAX_IOTYPE];
+    BlockHistogram latency_histogram[BLOCK_MAX_IOTYPE];
 };
 
 typedef struct BlockAcctCookie {
-- 
2.11.0

[Qemu-devel] [PATCH 2/3] block/accounting: introduce block size histogram

[zhenwei pi]

Introduce block size histogram statics for block devices.

For read/write/flush operation type, the block size region
[0, +inf) is divided into subregions by several points.
It works like block latency histogram.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 block/accounting.c         | 24 ++++++++++++++++++++++++
 include/block/accounting.h |  4 ++++
 2 files changed, 28 insertions(+)

diff --git a/block/accounting.c b/block/accounting.c
index bb8148b6b1..94d5aa292e 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -187,6 +187,27 @@ void block_latency_histograms_clear(BlockAcctStats *stats)
     }
 }
 
+int block_size_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
+                                uint64List *boundaries)
+{
+    BlockHistogram *hist = &stats->size_histogram[type];
+
+    return block_histogram_set(hist, boundaries);
+}
+
+void block_size_histograms_clear(BlockAcctStats *stats)
+{
+    int i;
+
+    for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
+        BlockHistogram *hist = &stats->size_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)
 {
@@ -211,6 +232,9 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
 
     block_histogram_account(&stats->latency_histogram[cookie->type],
                                     latency_ns);
+    block_histogram_account(&stats->size_histogram[cookie->type],
+                                    cookie->bytes);
+
 
     if (!failed || stats->account_failed) {
         stats->total_time_ns[cookie->type] += latency_ns;
diff --git a/include/block/accounting.h b/include/block/accounting.h
index 270fddb69c..49d3a78f48 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -89,6 +89,7 @@ struct BlockAcctStats {
     bool account_invalid;
     bool account_failed;
     BlockHistogram latency_histogram[BLOCK_MAX_IOTYPE];
+    BlockHistogram size_histogram[BLOCK_MAX_IOTYPE];
 };
 
 typedef struct BlockAcctCookie {
@@ -117,5 +118,8 @@ double block_acct_queue_depth(BlockAcctTimedStats *stats,
 int block_latency_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
                                 uint64List *boundaries);
 void block_latency_histograms_clear(BlockAcctStats *stats);
+int block_size_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
+                                uint64List *boundaries);
+void block_size_histograms_clear(BlockAcctStats *stats);
 
 #endif
-- 
2.11.0

[Qemu-devel] [PATCH 3/3] qapi: add block size histogram interface

[zhenwei pi]

Set/Clear block size histograms through new command
x-block-size-histogram-set and show new statistics in
query-blockstats results.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 block/qapi.c         |  24 ++++++++++++
 blockdev.c           |  56 +++++++++++++++++++++++++++
 qapi/block-core.json | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 184 insertions(+), 1 deletion(-)

diff --git a/block/qapi.c b/block/qapi.c
index f3a84f776e..04edbd5243 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -428,6 +428,20 @@ static void bdrv_latency_histogram_stats(BlockHistogram *hist,
     }
 }
 
+static void bdrv_size_histogram_stats(BlockHistogram *hist,
+                                         bool *not_null,
+                                         BlockSizeHistogramInfo **info)
+{
+    *not_null = hist->bins != NULL;
+    if (*not_null) {
+        *info = g_new0(BlockSizeHistogramInfo, 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);
@@ -503,6 +517,16 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
     bdrv_latency_histogram_stats(&stats->latency_histogram[BLOCK_ACCT_FLUSH],
                                  &ds->has_flush_latency_histogram,
                                  &ds->flush_latency_histogram);
+
+    bdrv_size_histogram_stats(&stats->size_histogram[BLOCK_ACCT_READ],
+                                 &ds->has_x_rd_size_histogram,
+                                 &ds->x_rd_size_histogram);
+    bdrv_size_histogram_stats(&stats->size_histogram[BLOCK_ACCT_WRITE],
+                                 &ds->has_x_wr_size_histogram,
+                                 &ds->x_wr_size_histogram);
+    bdrv_size_histogram_stats(&stats->size_histogram[BLOCK_ACCT_FLUSH],
+                                 &ds->has_x_flush_size_histogram,
+                                 &ds->x_flush_size_histogram);
 }
 
 static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,
diff --git a/blockdev.c b/blockdev.c
index 5d6a13dea9..c3f893891d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4563,6 +4563,62 @@ void qmp_block_latency_histogram_set(
     }
 }
 
+void qmp_x_block_size_histogram_set(
+    const char *id,
+    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 = qmp_get_blk(NULL, id, errp);
+    BlockAcctStats *stats;
+    int ret;
+
+    if (!blk) {
+        return;
+    }
+
+    stats = blk_get_stats(blk);
+
+    if (!has_boundaries && !has_boundaries_read && !has_boundaries_write &&
+        !has_boundaries_flush)
+    {
+        block_size_histograms_clear(stats);
+        return;
+    }
+
+    if (has_boundaries || has_boundaries_read) {
+        ret = block_size_histogram_set(
+            stats, BLOCK_ACCT_READ,
+            has_boundaries_read ? boundaries_read : boundaries);
+        if (ret) {
+            error_setg(errp, "Device '%s' set read boundaries fail", id);
+            return;
+        }
+    }
+
+    if (has_boundaries || has_boundaries_write) {
+        ret = block_size_histogram_set(
+            stats, BLOCK_ACCT_WRITE,
+            has_boundaries_write ? boundaries_write : boundaries);
+        if (ret) {
+            error_setg(errp, "Device '%s' set write boundaries fail", id);
+            return;
+        }
+    }
+
+    if (has_boundaries || has_boundaries_flush) {
+        ret = block_size_histogram_set(
+            stats, BLOCK_ACCT_FLUSH,
+            has_boundaries_flush ? boundaries_flush : boundaries);
+        if (ret) {
+            error_setg(errp, "Device '%s' set flush boundaries fail", id);
+            return;
+        }
+    }
+}
+
 QemuOptsList qemu_common_drive_opts = {
     .name = "drive",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head),
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0d43d4f37c..cae45c9db5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -633,6 +633,100 @@
            '*boundaries-flush': ['uint64'] } }
 
 ##
+# @BlockSizeHistogramInfo:
+#
+# Block size histogram.
+#
+# @boundaries: list of interval boundary values in nanoseconds, all greater
+#              than zero and in ascending order.
+#              For example, the list [8193, 32769, 131073] produces the
+#              following histogram intervals:
+#              [0, 8193), [8193, 32769), [32769, 131073), [131073, +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 [6, 3, 7, 9],
+#        and corresponding histogram looks like:
+#
+# Since: 4.0
+##
+{ 'struct': 'BlockSizeHistogramInfo',
+  'data': {'boundaries': ['uint64'], 'bins': ['uint64'] } }
+
+##
+# @x-block-size-histogram-set:
+#
+# Manage read, write and flush size histograms for the device.
+#
+# If only @id parameter is specified, remove all present size histograms
+# for the device. Otherwise, add/reset some of (or all) size histograms.
+#
+# @id: The name or QOM path of the guest device.
+#
+# @boundaries: list of interval boundary values (see description in
+#              BlockSizeHistogramInfo definition). If specified, all
+#              size 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 size
+#                   histogram. If specified, old read size histogram is
+#                   removed, and empty one created with intervals
+#                   corresponding to @boundaries-read. The parameter has higher
+#                   priority then @boundaries.
+#
+# @boundaries-write: list of interval boundary values for write size
+#                    histogram.
+#
+# @boundaries-flush: list of interval boundary values for flush size
+#                    histogram.
+#
+# Returns: error if device is not found or any boundary arrays are invalid.
+#
+# Since: 4.0
+#
+# Example: set new histograms for all io types with intervals
+# [0, 8193), [8193, 32769), [32769, 131073), [131073, +inf):
+#
+# -> { "execute": "x-block-size-histogram-set",
+#      "arguments": { "id": "drive0",
+#                     "boundaries": [8193, 32769, 131073] } }
+# <- { "return": {} }
+#
+# Example: set new histogram only for write, other histograms will remain
+# not changed (or not created):
+#
+# -> { "execute": "x-block-size-histogram-set",
+#      "arguments": { "id": "drive0",
+#                     "boundaries-write": [8193, 32769, 131073] } }
+# <- { "return": {} }
+#
+# Example: set new histograms with the following intervals:
+#   read, flush: [0, 8193), [8193, 32769), [32769, 131073), [131073, +inf)
+#   write: [0, 4097), [4097, 8193), [8193, 32769), [32769, +inf)
+#
+# -> { "execute": "x-block-size-histogram-set",
+#      "arguments": { "id": "drive0",
+#                     "boundaries": [8193, 32769, 131073],
+#                     "boundaries-write": [4097, 8193, 32769] } }
+# <- { "return": {} }
+#
+# Example: remove all size histograms:
+#
+# -> { "execute": "x-block-size-histogram-set",
+#      "arguments": { "id": "drive0" } }
+# <- { "return": {} }
+##
+{ 'command': 'x-block-size-histogram-set',
+  'data': {'id': 'str',
+           '*boundaries': ['uint64'],
+           '*boundaries-read': ['uint64'],
+           '*boundaries-write': ['uint64'],
+           '*boundaries-flush': ['uint64'] } }
+
+
+##
 # @BlockInfo:
 #
 # Block device information.  This structure describes a virtual device and
@@ -918,6 +1012,12 @@
 #
 # @flush_latency_histogram: @BlockLatencyHistogramInfo. (Since 4.0)
 #
+# @x_rd_size_histogram: @BlockSizeHistogramInfo. (Since 4.0)
+#
+# @x_wr_size_histogram: @BlockSizeHistogramInfo. (Since 4.0)
+#
+# @x_flush_size_histogram: @BlockSizeHistogramInfo. (Since 4.0)
+#
 # Since: 0.14.0
 ##
 { 'struct': 'BlockDeviceStats',
@@ -933,7 +1033,10 @@
            'timed_stats': ['BlockDeviceTimedStats'],
            '*rd_latency_histogram': 'BlockLatencyHistogramInfo',
            '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
-           '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
+           '*flush_latency_histogram': 'BlockLatencyHistogramInfo',
+           '*x_rd_size_histogram': 'BlockSizeHistogramInfo',
+           '*x_wr_size_histogram': 'BlockSizeHistogramInfo',
+           '*x_flush_size_histogram': 'BlockSizeHistogramInfo' } }
 
 ##
 # @BlockStats:
-- 
2.11.0

Re: [Qemu-devel] [PATCH 3/3] qapi: add block size histogram interface

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 6599 bytes --]

On 6/20/19 3:54 AM, zhenwei pi wrote:
> Set/Clear block size histograms through new command
> x-block-size-histogram-set and show new statistics in
> query-blockstats results.
> 

I'm guessing this is modeled after the existing
block-latency-histogram-set command?

> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  block/qapi.c         |  24 ++++++++++++
>  blockdev.c           |  56 +++++++++++++++++++++++++++
>  qapi/block-core.json | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 184 insertions(+), 1 deletion(-)

> +++ b/qapi/block-core.json
> @@ -633,6 +633,100 @@
>             '*boundaries-flush': ['uint64'] } }
>  
>  ##
> +# @BlockSizeHistogramInfo:
> +#
> +# Block size histogram.
> +#
> +# @boundaries: list of interval boundary values in nanoseconds, all greater
> +#              than zero and in ascending order.
> +#              For example, the list [8193, 32769, 131073] produces the
> +#              following histogram intervals:
> +#              [0, 8193), [8193, 32769), [32769, 131073), [131073, +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 [6, 3, 7, 9],
> +#        and corresponding histogram looks like:
> +#
> +# Since: 4.0

You've missed 4.0; the next release is 4.1.

> +##
> +{ 'struct': 'BlockSizeHistogramInfo',
> +  'data': {'boundaries': ['uint64'], 'bins': ['uint64'] } }

This is identical to struct BlockLatencyHistogramInfo; can we instead
rename the type (which does not affect API) and share it between both
implementations, instead of duplicating it?

> +
> +##
> +# @x-block-size-histogram-set:

Does this need to be experimental from the get-go? Or can it be stable
by dropping 'x-' since it matches the fact that
block-latency-histogram-set is stable?

> +#
> +# Manage read, write and flush size histograms for the device.
> +#
> +# If only @id parameter is specified, remove all present size histograms
> +# for the device. Otherwise, add/reset some of (or all) size histograms.
> +#
> +# @id: The name or QOM path of the guest device.
> +#
> +# @boundaries: list of interval boundary values (see description in
> +#              BlockSizeHistogramInfo definition). If specified, all
> +#              size 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 size
> +#                   histogram. If specified, old read size histogram is
> +#                   removed, and empty one created with intervals
> +#                   corresponding to @boundaries-read. The parameter has higher
> +#                   priority then @boundaries.
> +#
> +# @boundaries-write: list of interval boundary values for write size
> +#                    histogram.
> +#
> +# @boundaries-flush: list of interval boundary values for flush size
> +#                    histogram.
> +#
> +# Returns: error if device is not found or any boundary arrays are invalid.
> +#
> +# Since: 4.0

4.1

> +#
> +# Example: set new histograms for all io types with intervals
> +# [0, 8193), [8193, 32769), [32769, 131073), [131073, +inf):
> +#
> +# -> { "execute": "x-block-size-histogram-set",
> +#      "arguments": { "id": "drive0",
> +#                     "boundaries": [8193, 32769, 131073] } }
> +# <- { "return": {} }
> +#
> +# Example: set new histogram only for write, other histograms will remain
> +# not changed (or not created):
> +#
> +# -> { "execute": "x-block-size-histogram-set",
> +#      "arguments": { "id": "drive0",
> +#                     "boundaries-write": [8193, 32769, 131073] } }
> +# <- { "return": {} }
> +#
> +# Example: set new histograms with the following intervals:
> +#   read, flush: [0, 8193), [8193, 32769), [32769, 131073), [131073, +inf)
> +#   write: [0, 4097), [4097, 8193), [8193, 32769), [32769, +inf)
> +#
> +# -> { "execute": "x-block-size-histogram-set",
> +#      "arguments": { "id": "drive0",
> +#                     "boundaries": [8193, 32769, 131073],
> +#                     "boundaries-write": [4097, 8193, 32769] } }
> +# <- { "return": {} }
> +#
> +# Example: remove all size histograms:
> +#
> +# -> { "execute": "x-block-size-histogram-set",
> +#      "arguments": { "id": "drive0" } }
> +# <- { "return": {} }
> +##
> +{ 'command': 'x-block-size-histogram-set',
> +  'data': {'id': 'str',
> +           '*boundaries': ['uint64'],
> +           '*boundaries-read': ['uint64'],
> +           '*boundaries-write': ['uint64'],
> +           '*boundaries-flush': ['uint64'] } }

Again, this copies heavily from block-latency-histogram-set.  But
changing the command name is not API compatible.  Should we have a
single new command 'block-histogram-set' which takes an enum choosing
between 'latency' and 'size', and start the deprecation clock on
'block-latency-histogram-set'?
 (and defaulting to 'latency' for back-compat

> +
> +
> +##
>  # @BlockInfo:
>  #
>  # Block device information.  This structure describes a virtual device and
> @@ -918,6 +1012,12 @@
>  #
>  # @flush_latency_histogram: @BlockLatencyHistogramInfo. (Since 4.0)
>  #
> +# @x_rd_size_histogram: @BlockSizeHistogramInfo. (Since 4.0)
> +#
> +# @x_wr_size_histogram: @BlockSizeHistogramInfo. (Since 4.0)
> +#
> +# @x_flush_size_histogram: @BlockSizeHistogramInfo. (Since 4.0)

since 4.1 on all of these additions.

> +#
>  # Since: 0.14.0
>  ##
>  { 'struct': 'BlockDeviceStats',
> @@ -933,7 +1033,10 @@
>             'timed_stats': ['BlockDeviceTimedStats'],
>             '*rd_latency_histogram': 'BlockLatencyHistogramInfo',
>             '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
> -           '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
> +           '*flush_latency_histogram': 'BlockLatencyHistogramInfo',
> +           '*x_rd_size_histogram': 'BlockSizeHistogramInfo',
> +           '*x_wr_size_histogram': 'BlockSizeHistogramInfo',
> +           '*x_flush_size_histogram': 'BlockSizeHistogramInfo' } }
>  
>  ##
>  # @BlockStats:
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [External Email] Re: [PATCH 3/3] qapi: add block size histogram interface

[zhenwei pi]

On 6/20/19 10:03 PM, Eric Blake wrote:

> On 6/20/19 3:54 AM, zhenwei pi wrote:
>> Set/Clear block size histograms through new command
>> x-block-size-histogram-set and show new statistics in
>> query-blockstats results.
>>
> I'm guessing this is modeled after the existing
> block-latency-histogram-set command?

zhenwei: Yes, it is.

>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
>> ---
>>   block/qapi.c         |  24 ++++++++++++
>>   blockdev.c           |  56 +++++++++++++++++++++++++++
>>   qapi/block-core.json | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   3 files changed, 184 insertions(+), 1 deletion(-)
>> +++ b/qapi/block-core.json
>> @@ -633,6 +633,100 @@
>>              '*boundaries-flush': ['uint64'] } }
>>   
>>   ##
>> +# @BlockSizeHistogramInfo:
>> +#
>> +# Block size histogram.
>> +#
>> +# @boundaries: list of interval boundary values in nanoseconds, all greater
>> +#              than zero and in ascending order.
>> +#              For example, the list [8193, 32769, 131073] produces the
>> +#              following histogram intervals:
>> +#              [0, 8193), [8193, 32769), [32769, 131073), [131073, +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 [6, 3, 7, 9],
>> +#        and corresponding histogram looks like:
>> +#
>> +# Since: 4.0
> You've missed 4.0; the next release is 4.1.

zhenwei: OK, I will fix all the version info.

>> +##
>> +{ 'struct': 'BlockSizeHistogramInfo',
>> +  'data': {'boundaries': ['uint64'], 'bins': ['uint64'] } }
> This is identical to struct BlockLatencyHistogramInfo; can we instead
> rename the type (which does not affect API) and share it between both
> implementations, instead of duplicating it?
>
zhenwei: Good idea. But I am confused about the compatibility of the
structure BlockLatencyHistogramInfo. If I rename BlockLatencyHistogramInfo
to BlockHistogramInfo, it will be common.

>> +
>> +##
>> +# @x-block-size-histogram-set:
> Does this need to be experimental from the get-go? Or can it be stable
> by dropping 'x-' since it matches the fact that
> block-latency-histogram-set is stable?

zhenwei: OK, I will drop 'x-' prefix.

>> +#
>> +# Manage read, write and flush size histograms for the device.
>> +#
>> +# If only @id parameter is specified, remove all present size histograms
>> +# for the device. Otherwise, add/reset some of (or all) size histograms.
>> +#
>> +# @id: The name or QOM path of the guest device.
>> +#
>> +# @boundaries: list of interval boundary values (see description in
>> +#              BlockSizeHistogramInfo definition). If specified, all
>> +#              size 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 size
>> +#                   histogram. If specified, old read size histogram is
>> +#                   removed, and empty one created with intervals
>> +#                   corresponding to @boundaries-read. The parameter has higher
>> +#                   priority then @boundaries.
>> +#
>> +# @boundaries-write: list of interval boundary values for write size
>> +#                    histogram.
>> +#
>> +# @boundaries-flush: list of interval boundary values for flush size
>> +#                    histogram.
>> +#
>> +# Returns: error if device is not found or any boundary arrays are invalid.
>> +#
>> +# Since: 4.0
> 4.1
>
>> +#
>> +# Example: set new histograms for all io types with intervals
>> +# [0, 8193), [8193, 32769), [32769, 131073), [131073, +inf):
>> +#
>> +# -> { "execute": "x-block-size-histogram-set",
>> +#      "arguments": { "id": "drive0",
>> +#                     "boundaries": [8193, 32769, 131073] } }
>> +# <- { "return": {} }
>> +#
>> +# Example: set new histogram only for write, other histograms will remain
>> +# not changed (or not created):
>> +#
>> +# -> { "execute": "x-block-size-histogram-set",
>> +#      "arguments": { "id": "drive0",
>> +#                     "boundaries-write": [8193, 32769, 131073] } }
>> +# <- { "return": {} }
>> +#
>> +# Example: set new histograms with the following intervals:
>> +#   read, flush: [0, 8193), [8193, 32769), [32769, 131073), [131073, +inf)
>> +#   write: [0, 4097), [4097, 8193), [8193, 32769), [32769, +inf)
>> +#
>> +# -> { "execute": "x-block-size-histogram-set",
>> +#      "arguments": { "id": "drive0",
>> +#                     "boundaries": [8193, 32769, 131073],
>> +#                     "boundaries-write": [4097, 8193, 32769] } }
>> +# <- { "return": {} }
>> +#
>> +# Example: remove all size histograms:
>> +#
>> +# -> { "execute": "x-block-size-histogram-set",
>> +#      "arguments": { "id": "drive0" } }
>> +# <- { "return": {} }
>> +##
>> +{ 'command': 'x-block-size-histogram-set',
>> +  'data': {'id': 'str',
>> +           '*boundaries': ['uint64'],
>> +           '*boundaries-read': ['uint64'],
>> +           '*boundaries-write': ['uint64'],
>> +           '*boundaries-flush': ['uint64'] } }
> Again, this copies heavily from block-latency-histogram-set.  But
> changing the command name is not API compatible.  Should we have a
> single new command 'block-histogram-set' which takes an enum choosing
> between 'latency' and 'size', and start the deprecation clock on
> 'block-latency-histogram-set'?
>   (and defaulting to 'latency' for back-compat
>
zhenwei: this actually copies from block-latency-histogram-set, because the
two APIs have the similar logic but different structure
BlockLatencyHistogramInfo and BlockSizeHistogramInfo.
For further extension, a single new command 'block-histogram-set' with
enum operation is better.
So, should I remove 'block-latency-histogram-set' and add 'block-histogram-set'?

>> +
>> +
>> +##
>>   # @BlockInfo:
>>   #
>>   # Block device information.  This structure describes a virtual device and
>> @@ -918,6 +1012,12 @@
>>   #
>>   # @flush_latency_histogram: @BlockLatencyHistogramInfo. (Since 4.0)
>>   #
>> +# @x_rd_size_histogram: @BlockSizeHistogramInfo. (Since 4.0)
>> +#
>> +# @x_wr_size_histogram: @BlockSizeHistogramInfo. (Since 4.0)
>> +#
>> +# @x_flush_size_histogram: @BlockSizeHistogramInfo. (Since 4.0)
> since 4.1 on all of these additions.
>
>> +#
>>   # Since: 0.14.0
>>   ##
>>   { 'struct': 'BlockDeviceStats',
>> @@ -933,7 +1033,10 @@
>>              'timed_stats': ['BlockDeviceTimedStats'],
>>              '*rd_latency_histogram': 'BlockLatencyHistogramInfo',
>>              '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
>> -           '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
>> +           '*flush_latency_histogram': 'BlockLatencyHistogramInfo',
>> +           '*x_rd_size_histogram': 'BlockSizeHistogramInfo',
>> +           '*x_wr_size_histogram': 'BlockSizeHistogramInfo',
>> +           '*x_flush_size_histogram': 'BlockSizeHistogramInfo' } }
>>   
>>   ##
>>   # @BlockStats:
>>

-- 
Thanks and Best Regards,
zhenwei pi

Re: [Qemu-devel] [External Email] Re: [PATCH 3/3] qapi: add block size histogram interface

[Vladimir Sementsov-Ogievskiy]

21.06.2019 4:52, zhenwei pi wrote:
> On 6/20/19 10:03 PM, Eric Blake wrote:
> 
>> On 6/20/19 3:54 AM, zhenwei pi wrote:
>>> Set/Clear block size histograms through new command
>>> x-block-size-histogram-set and show new statistics in
>>> query-blockstats results.
>>>
>> I'm guessing this is modeled after the existing
>> block-latency-histogram-set command?
> 
> zhenwei: Yes, it is.
> 
>>> Signed-off-by: zhenwei pi<pizhenwei@bytedance.com>
>>> ---
>>>   block/qapi.c         |  24 ++++++++++++
>>>   blockdev.c           |  56 +++++++++++++++++++++++++++
>>>   qapi/block-core.json | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>   3 files changed, 184 insertions(+), 1 deletion(-)
>>> +++ b/qapi/block-core.json
>>> @@ -633,6 +633,100 @@
>>>              '*boundaries-flush': ['uint64'] } }
>>>   
>>>   ##
>>> +# @BlockSizeHistogramInfo:
>>> +#
>>> +# Block size histogram.
>>> +#
>>> +# @boundaries: list of interval boundary values in nanoseconds, all greater
>>> +#              than zero and in ascending order.
>>> +#              For example, the list [8193, 32769, 131073] produces the
>>> +#              following histogram intervals:
>>> +#              [0, 8193), [8193, 32769), [32769, 131073), [131073, +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 [6, 3, 7, 9],
>>> +#        and corresponding histogram looks like:
>>> +#
>>> +# Since: 4.0
>> You've missed 4.0; the next release is 4.1.
> 
> zhenwei: OK, I will fix all the version info.
> 
>>> +##
>>> +{ 'struct': 'BlockSizeHistogramInfo',
>>> +  'data': {'boundaries': ['uint64'], 'bins': ['uint64'] } }
>> This is identical to struct BlockLatencyHistogramInfo; can we instead
>> rename the type (which does not affect API) and share it between both
>> implementations, instead of duplicating it?
>>
> zhenwei: Good idea. But I am confused about the compatibility of the
> structure BlockLatencyHistogramInfo. If I rename BlockLatencyHistogramInfo
> to BlockHistogramInfo, it will be common.
> 
>>> +
>>> +##
>>> +# @x-block-size-histogram-set:
>> Does this need to be experimental from the get-go? Or can it be stable
>> by dropping 'x-' since it matches the fact that
>> block-latency-histogram-set is stable?
> 
> zhenwei: OK, I will drop 'x-' prefix.
> 
>>> +#
>>> +# Manage read, write and flush size histograms for the device.
>>> +#
>>> +# If only @id parameter is specified, remove all present size histograms
>>> +# for the device. Otherwise, add/reset some of (or all) size histograms.
>>> +#
>>> +# @id: The name or QOM path of the guest device.
>>> +#
>>> +# @boundaries: list of interval boundary values (see description in
>>> +#              BlockSizeHistogramInfo definition). If specified, all
>>> +#              size 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 size
>>> +#                   histogram. If specified, old read size histogram is
>>> +#                   removed, and empty one created with intervals
>>> +#                   corresponding to @boundaries-read. The parameter has higher
>>> +#                   priority then @boundaries.
>>> +#
>>> +# @boundaries-write: list of interval boundary values for write size
>>> +#                    histogram.
>>> +#
>>> +# @boundaries-flush: list of interval boundary values for flush size
>>> +#                    histogram.
>>> +#
>>> +# Returns: error if device is not found or any boundary arrays are invalid.
>>> +#
>>> +# Since: 4.0
>> 4.1
>>
>>> +#
>>> +# Example: set new histograms for all io types with intervals
>>> +# [0, 8193), [8193, 32769), [32769, 131073), [131073, +inf):
>>> +#
>>> +# -> { "execute": "x-block-size-histogram-set",
>>> +#      "arguments": { "id": "drive0",
>>> +#                     "boundaries": [8193, 32769, 131073] } }
>>> +# <- { "return": {} }
>>> +#
>>> +# Example: set new histogram only for write, other histograms will remain
>>> +# not changed (or not created):
>>> +#
>>> +# -> { "execute": "x-block-size-histogram-set",
>>> +#      "arguments": { "id": "drive0",
>>> +#                     "boundaries-write": [8193, 32769, 131073] } }
>>> +# <- { "return": {} }
>>> +#
>>> +# Example: set new histograms with the following intervals:
>>> +#   read, flush: [0, 8193), [8193, 32769), [32769, 131073), [131073, +inf)
>>> +#   write: [0, 4097), [4097, 8193), [8193, 32769), [32769, +inf)
>>> +#
>>> +# -> { "execute": "x-block-size-histogram-set",
>>> +#      "arguments": { "id": "drive0",
>>> +#                     "boundaries": [8193, 32769, 131073],
>>> +#                     "boundaries-write": [4097, 8193, 32769] } }
>>> +# <- { "return": {} }
>>> +#
>>> +# Example: remove all size histograms:
>>> +#
>>> +# -> { "execute": "x-block-size-histogram-set",
>>> +#      "arguments": { "id": "drive0" } }
>>> +# <- { "return": {} }
>>> +##
>>> +{ 'command': 'x-block-size-histogram-set',
>>> +  'data': {'id': 'str',
>>> +           '*boundaries': ['uint64'],
>>> +           '*boundaries-read': ['uint64'],
>>> +           '*boundaries-write': ['uint64'],
>>> +           '*boundaries-flush': ['uint64'] } }
>> Again, this copies heavily from block-latency-histogram-set.  But
>> changing the command name is not API compatible.  Should we have a
>> single new command 'block-histogram-set' which takes an enum choosing
>> between 'latency' and 'size', and start the deprecation clock on
>> 'block-latency-histogram-set'?
>>   (and defaulting to 'latency' for back-compat
>>
> zhenwei: this actually copies from block-latency-histogram-set, because the
> two APIs have the similar logic but different structure
> BlockLatencyHistogramInfo and BlockSizeHistogramInfo.
> For further extension, a single new command 'block-histogram-set' with
> enum operation is better.
> So, should I remove 'block-latency-histogram-set' and add 'block-histogram-set'?
> 

Hi Zhenwei!

Glad to see that my work is useful not only for my company! And sad that you didn't
propose it before dropping x-prefixes. But the interface is yang and I think it's
deprecation should not be a problem (for us at least, if I'm not mistaken, we only
use it for something near to internal purposes)..

Then, agree with Eric that it's better to make it common, avoiding duplication. Even
if we keep both commands (and anyway we have to, at least during deprecation period)
we can put their parameters to a separate structure to be shared between them, like
BlockDirtyBitmap is shared parameter structure for block-dirty-bitmap-clear,
block-dirty-bitmap-enable, etc.

Interesting, can we go further and create some generic histogram API, not only for
block subsystem? Hmm.. I don't see a compatible solution.. Histogram possible should
be a separate object with personal id, and than we bind it to our block stats, but
this definitely more complicated API than what we have..

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 1/3] block/accounting: rename struct BlockLatencyHistogram

[Vladimir Sementsov-Ogievskiy]

20.06.2019 11:54, zhenwei pi wrote:
> Rename struct BlockLatencyHistogram to BlockHistogram, and rename
> related functions name.
> make this struct and functions be common, they can be used widely.

Hmm, we can go further, and make it just Histogram and move to separate file,
as there is nothing actually about block-layer inside it. But it may be done
later of course, I'm OK with this too.

> 
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>   block/accounting.c         | 31 ++++++++++++++++++-------------
>   block/qapi.c               |  2 +-
>   include/block/accounting.h |  8 ++++----
>   3 files changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/block/accounting.c b/block/accounting.c
> index 70a3d9a426..bb8148b6b1 100644
> --- a/block/accounting.c
> +++ b/block/accounting.c
> @@ -94,13 +94,13 @@ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
>       cookie->type = type;
>   }
>   
> -/* block_latency_histogram_compare_func:
> +/* block_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)
> +static int block_histogram_compare_func(const void *key, const void *it)
>   {
>       uint64_t k = *(uint64_t *)key;
>       uint64_t a = ((uint64_t *)it)[0];
> @@ -109,8 +109,7 @@ static int block_latency_histogram_compare_func(const void *key, const void *it)
>       return k < a ? -1 : (k < b ? 0 : 1);
>   }
>   
> -static void block_latency_histogram_account(BlockLatencyHistogram *hist,
> -                                            int64_t latency_ns)
> +static void block_histogram_account(BlockHistogram *hist, int64_t val)
>   {
>       uint64_t *pos;
>   
> @@ -120,28 +119,26 @@ static void block_latency_histogram_account(BlockLatencyHistogram *hist,
>       }
>   
>   
> -    if (latency_ns < hist->boundaries[0]) {
> +    if (val < hist->boundaries[0]) {
>           hist->bins[0]++;
>           return;
>       }
>   
> -    if (latency_ns >= hist->boundaries[hist->nbins - 2]) {
> +    if (val >= hist->boundaries[hist->nbins - 2]) {
>           hist->bins[hist->nbins - 1]++;
>           return;
>       }
>   
> -    pos = bsearch(&latency_ns, hist->boundaries, hist->nbins - 2,
> +    pos = bsearch(&val, hist->boundaries, hist->nbins - 2,
>                     sizeof(hist->boundaries[0]),
> -                  block_latency_histogram_compare_func);
> +                  block_histogram_compare_func);
>       assert(pos != NULL);
>   
>       hist->bins[pos - hist->boundaries + 1]++;
>   }
>   
> -int block_latency_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
> -                                uint64List *boundaries)
> +static int block_histogram_set(BlockHistogram *hist, uint64List *boundaries)
>   {
> -    BlockLatencyHistogram *hist = &stats->latency_histogram[type];
>       uint64List *entry;
>       uint64_t *ptr;
>       uint64_t prev = 0;
> @@ -170,12 +167,20 @@ int block_latency_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
>       return 0;
>   }
>   
> +int block_latency_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
> +                                uint64List *boundaries)
> +{
> +    BlockHistogram *hist = &stats->latency_histogram[type];
> +
> +    return block_histogram_set(hist, boundaries);
> +}
> +
>   void block_latency_histograms_clear(BlockAcctStats *stats)
>   {
>       int i;
>   
>       for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
> -        BlockLatencyHistogram *hist = &stats->latency_histogram[i];
> +        BlockHistogram *hist = &stats->latency_histogram[i];
>           g_free(hist->bins);
>           g_free(hist->boundaries);

I think this should be moved to separate block_histogram_clear, to not duplicate this code
in 02.

>           memset(hist, 0, sizeof(*hist));
> @@ -204,7 +209,7 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
>           stats->nr_ops[cookie->type]++;
>       }
>   
> -    block_latency_histogram_account(&stats->latency_histogram[cookie->type],
> +    block_histogram_account(&stats->latency_histogram[cookie->type],
>                                       latency_ns);

indentation

>   
>       if (!failed || stats->account_failed) {
> diff --git a/block/qapi.c b/block/qapi.c
> index 917435f022..f3a84f776e 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -415,7 +415,7 @@ static uint64List *uint64_list(uint64_t *list, int size)
>       return out_list;
>   }
>   
> -static void bdrv_latency_histogram_stats(BlockLatencyHistogram *hist,
> +static void bdrv_latency_histogram_stats(BlockHistogram *hist,
>                                            bool *not_null,
>                                            BlockLatencyHistogramInfo **info)
>   {

Why not rename this function too, and not duplicate it in 03?

> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index d1f67b10dd..270fddb69c 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -46,7 +46,7 @@ struct BlockAcctTimedStats {
>       QSLIST_ENTRY(BlockAcctTimedStats) entries;
>   };
>   
> -typedef struct BlockLatencyHistogram {
> +typedef struct BlockHistogram {
>       /* The following histogram is represented like this:
>        *
>        * 5|           *
> @@ -57,7 +57,7 @@ typedef struct BlockLatencyHistogram {
>        *  +------------------
>        *      10   50   100
>        *
> -     * BlockLatencyHistogram histogram = {
> +     * BlockHistogram histogram = {
>        *     .nbins = 4,
>        *     .boundaries = {10, 50, 100},
>        *     .bins = {3, 1, 5, 2},
> @@ -74,7 +74,7 @@ typedef struct BlockLatencyHistogram {
>       uint64_t *boundaries; /* @nbins-1 numbers here
>                                (all boundaries, except 0 and +inf) */
>       uint64_t *bins;
> -} BlockLatencyHistogram;
> +} BlockHistogram;
>   
>   struct BlockAcctStats {
>       QemuMutex lock;
> @@ -88,7 +88,7 @@ struct BlockAcctStats {
>       QSLIST_HEAD(, BlockAcctTimedStats) intervals;
>       bool account_invalid;
>       bool account_failed;
> -    BlockLatencyHistogram latency_histogram[BLOCK_MAX_IOTYPE];
> +    BlockHistogram latency_histogram[BLOCK_MAX_IOTYPE];
>   };
>   
>   typedef struct BlockAcctCookie {
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/3] block/accounting: introduce block size histogram

[Vladimir Sementsov-Ogievskiy]

20.06.2019 11:54, zhenwei pi wrote:
> Introduce block size histogram statics for block devices.
> 
> For read/write/flush operation type, the block size region
> [0, +inf) is divided into subregions by several points.
> It works like block latency histogram.
> 
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>   block/accounting.c         | 24 ++++++++++++++++++++++++
>   include/block/accounting.h |  4 ++++
>   2 files changed, 28 insertions(+)
> 
> diff --git a/block/accounting.c b/block/accounting.c
> index bb8148b6b1..94d5aa292e 100644
> --- a/block/accounting.c
> +++ b/block/accounting.c
> @@ -187,6 +187,27 @@ void block_latency_histograms_clear(BlockAcctStats *stats)
>       }
>   }
>   
> +int block_size_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
> +                                uint64List *boundaries)
> +{
> +    BlockHistogram *hist = &stats->size_histogram[type];
> +
> +    return block_histogram_set(hist, boundaries);
> +}
> +
> +void block_size_histograms_clear(BlockAcctStats *stats)
> +{
> +    int i;
> +
> +    for (i = 0; i < BLOCK_MAX_IOTYPE; i++) {
> +        BlockHistogram *hist = &stats->size_histogram[i];
> +        g_free(hist->bins);
> +        g_free(hist->boundaries);
> +        memset(hist, 0, sizeof(*hist));

And this is the duplication I've told about in 01

> +    }
> +}
> +
> +
>   static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
>                                    bool failed)
>   {
> @@ -211,6 +232,9 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
>   
>       block_histogram_account(&stats->latency_histogram[cookie->type],
>                                       latency_ns);
> +    block_histogram_account(&stats->size_histogram[cookie->type],
> +                                    cookie->bytes);
> +
>   
>       if (!failed || stats->account_failed) {
>           stats->total_time_ns[cookie->type] += latency_ns;
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index 270fddb69c..49d3a78f48 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -89,6 +89,7 @@ struct BlockAcctStats {
>       bool account_invalid;
>       bool account_failed;
>       BlockHistogram latency_histogram[BLOCK_MAX_IOTYPE];
> +    BlockHistogram size_histogram[BLOCK_MAX_IOTYPE];
>   };
>   
>   typedef struct BlockAcctCookie {
> @@ -117,5 +118,8 @@ double block_acct_queue_depth(BlockAcctTimedStats *stats,
>   int block_latency_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
>                                   uint64List *boundaries);
>   void block_latency_histograms_clear(BlockAcctStats *stats);
> +int block_size_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
> +                                uint64List *boundaries);

Hmm, uncommon indentation you use. In Qemu the second line of parameters should start
at the position of first symbol after '(', like this:

int block_size_histogram_set(BlockAcctStats *stats, enum BlockAcctType type,
                              uint64List *boundaries);

> +void block_size_histograms_clear(BlockAcctStats *stats);
>   
>   #endif
> 


-- 
Best regards,
Vladimir