Re: [Qemu-devel] [PATCH v2 3/3] qapi: make block histogram interface common

[Vladimir Sementsov-Ogievskiy]

08.07.2019 12:32, zhenwei pi wrote:
> Modify command 'block-latency-histogram-set' to 'block-histogram-set'
> and modify struct 'BlockLatencyHistogramInfo' to struct
> 'BlockHistogramInfo', this makes block histogram interface common to
> use.
> 
> Currently 'BlockHistogramType' supports 'latency', it works as same
> as the old command 'block-latency-histogram-set'. New enum 'size'
> allows QEMU to fill histogram by block I/O size in byte.
> 
> Signed-off-by: zhenwei pi <address@hidden>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>


Hi Zhenwei!

You should add "Reviewed-by" marks only after someone give them,
writing it in reply to your patch. If someones reply don't have
such mark, you should not add it.

> ---
>   block/qapi.c         |  11 +++++-
>   blockdev.c           |  33 +++++++++++++----
>   qapi/block-core.json | 101 
> +++++++++++++++++++++++++++++++--------------------
>   3 files changed, 95 insertions(+), 50 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index 9d9b2f1927..c778b2c376 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -416,11 +416,11 @@ static uint64List *uint64_list(uint64_t *list, int size)
>   }
>   
>   static void bdrv_histogram_stats(BlockHistogram *hist, bool *not_null,
> -                                 BlockLatencyHistogramInfo **info)
> +                                 BlockHistogramInfo **info)
>   {
>       *not_null = hist->bins != NULL;
>       if (*not_null) {
> -        *info = g_new0(BlockLatencyHistogramInfo, 1);
> +        *info = g_new0(BlockHistogramInfo, 1);
>   
>           (*info)->boundaries = uint64_list(hist->boundaries, hist->nbins - 
> 1);
>           (*info)->bins = uint64_list(hist->bins, hist->nbins);
> @@ -502,6 +502,13 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, 
> BlockBackend *blk)
>       bdrv_histogram_stats(&stats->latency_histogram[BLOCK_ACCT_FLUSH],
>                            &ds->has_flush_latency_histogram,
>                            &ds->flush_latency_histogram);
> +    bdrv_histogram_stats(&stats->size_histogram[BLOCK_ACCT_READ],
> +                         &ds->has_rd_size_histogram, &ds->rd_size_histogram);
> +    bdrv_histogram_stats(&stats->size_histogram[BLOCK_ACCT_WRITE],
> +                         &ds->has_wr_size_histogram, &ds->wr_size_histogram);
> +    bdrv_histogram_stats(&stats->size_histogram[BLOCK_ACCT_FLUSH],
> +                         &ds->has_flush_size_histogram,
> +                         &ds->flush_size_histogram);
>   }
>   
>   static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,
> diff --git a/blockdev.c b/blockdev.c
> index 5d6a13dea9..82240ae8c5 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -4507,8 +4507,9 @@ void qmp_x_blockdev_set_iothread(const char *node_name, 
> StrOrNull *iothread,
>       aio_context_release(old_context);
>   }
>   
> -void qmp_block_latency_histogram_set(
> +void qmp_block_histogram_set(
>       const char *id,
> +    BlockHistogramType type,
>       bool has_boundaries, uint64List *boundaries,
>       bool has_boundaries_read, uint64List *boundaries_read,
>       bool has_boundaries_write, uint64List *boundaries_write,
> @@ -4517,24 +4518,42 @@ void qmp_block_latency_histogram_set(
>   {
>       BlockBackend *blk = qmp_get_blk(NULL, id, errp);
>       BlockAcctStats *stats;
> +    void (*clear_func)(BlockAcctStats *stats);
> +    int (*set_func)(BlockAcctStats *stats, enum BlockAcctType type,
> +                    uint64List *boundaries);
>       int ret;
>   
>       if (!blk) {
>           return;
>       }
>   
> +    switch (type) {
> +    case BLOCK_HISTOGRAM_TYPE_LATENCY: {
> +        clear_func = block_latency_histograms_clear;
> +        set_func = block_latency_histogram_set;

Hmm, seems too tricky. This is because we have mapping (int type) -> (stats 
field name).

And these two functions are mostly duplication, the difference is that they 
works with different
fields.

So, I think better to combine both size_histogram and latency_histogram to one 
array field, where
index is histogram type. This will reduce duplication and we can drop this 
switch(){}.

> +        break;
> +    }
> +    case BLOCK_HISTOGRAM_TYPE_SIZE: {
> +        clear_func = block_size_histograms_clear;
> +        set_func = block_size_histogram_set;
> +        break;
> +    }
> +    default:
> +        error_setg(errp, "Unsupported block histogram type");
> +        return;
> +    }
> +
>       stats = blk_get_stats(blk);
>   
>       if (!has_boundaries && !has_boundaries_read && !has_boundaries_write &&
>           !has_boundaries_flush)
>       {
> -        block_latency_histograms_clear(stats);
> +        clear_func(stats);
>           return;
>       }
>   
>       if (has_boundaries || has_boundaries_read) {
> -        ret = block_latency_histogram_set(
> -            stats, BLOCK_ACCT_READ,
> +        ret = set_func(stats, BLOCK_ACCT_READ,
>               has_boundaries_read ? boundaries_read : boundaries);
>           if (ret) {
>               error_setg(errp, "Device '%s' set read boundaries fail", id);
> @@ -4543,8 +4562,7 @@ void qmp_block_latency_histogram_set(
>       }
>   
>       if (has_boundaries || has_boundaries_write) {
> -        ret = block_latency_histogram_set(
> -            stats, BLOCK_ACCT_WRITE,
> +        ret = set_func(stats, BLOCK_ACCT_WRITE,
>               has_boundaries_write ? boundaries_write : boundaries);
>           if (ret) {
>               error_setg(errp, "Device '%s' set write boundaries fail", id);
> @@ -4553,8 +4571,7 @@ void qmp_block_latency_histogram_set(
>       }
>   
>       if (has_boundaries || has_boundaries_flush) {
> -        ret = block_latency_histogram_set(
> -            stats, BLOCK_ACCT_FLUSH,
> +        ret = set_func(stats, BLOCK_ACCT_FLUSH,
>               has_boundaries_flush ? boundaries_flush : boundaries);
>           if (ret) {
>               error_setg(errp, "Device '%s' set flush boundaries fail", id);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0d43d4f37c..b3b8d714d3 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json

And about qapi part:

1. Eric, as I understand we should not drop old command but deprecate it and 
add a new one?
2. 4.1 -> 4.2

> @@ -533,12 +533,12 @@
>              'flags': ['Qcow2BitmapInfoFlags'] } }
>   
>   ##
> -# @BlockLatencyHistogramInfo:
> +# @BlockHistogramInfo:
>   #
> -# Block latency histogram.
> +# Block histogram.
>   #
> -# @boundaries: list of interval boundary values in nanoseconds, all greater
> -#              than zero and in ascending order.
> +# @boundaries: list of interval boundary values, all greater than zero and in
> +#              ascending order.
>   #              For example, the list [10, 50, 100] produces the following
>   #              histogram intervals: [0, 10), [10, 50), [50, 100), [100, 
> +inf).
>   #
> @@ -555,57 +555,72 @@
>   #         +------------------
>   #             10   50   100
>   #
> -# Since: 4.0
> +# Since: 4.1
>   ##
> -{ 'struct': 'BlockLatencyHistogramInfo',
> +{ 'struct': 'BlockHistogramInfo',
>     'data': {'boundaries': ['uint64'], 'bins': ['uint64'] } }
>   
>   ##
> -# @block-latency-histogram-set:
> +# @BlockHistogramType:
>   #
> -# Manage read, write and flush latency histograms for the device.
> +# An enumeration of block histogram type.
>   #
> -# If only @id parameter is specified, remove all present latency histograms
> -# for the device. Otherwise, add/reset some of (or all) latency histograms.
> +# @latency: The histogram is filled by block I/O latency in nanosecond.
> +#
> +# @size: The histogram is filled by block I/O size in byte.
> +#
> +# Since: 4.1
> +##
> +{ 'enum': 'BlockHistogramType', 'data': [ 'latency', 'size' ] }
> +
> +##
> +# @block-histogram-set:
> +#
> +# Manage read, write and flush @BlockHistogramType histograms for the device.
> +#
> +# If only @id and @type parameter are specified, remove all present @type
> +# histograms for the device. Otherwise, add/reset some of (or all) @type
> +# histograms.
>   #
>   # @id: The name or QOM path of the guest device.
>   #
> +# @type: The type @BlockHistogramType of histogram.
> +#
>   # @boundaries: list of interval boundary values (see description in
> -#              BlockLatencyHistogramInfo definition). If specified, all
> -#              latency histograms are removed, and empty ones created for all
> -#              io types with intervals corresponding to @boundaries (except 
> for
> -#              io types, for which specific boundaries are set through the
> -#              following parameters).
> +#              BlockHistogramInfo definition). If specified, all histograms
> +#              are removed, and empty ones created for all io types with
> +#              intervals corresponding to @boundaries (except for io types
> +#              for which specific boundaries are set through the following
> +#              parameters).
>   #
> -# @boundaries-read: list of interval boundary values for read latency
> -#                   histogram. If specified, old read latency histogram is
> -#                   removed, and empty one created with intervals
> -#                   corresponding to @boundaries-read. The parameter has 
> higher
> -#                   priority then @boundaries.
> +# @boundaries-read: list of interval boundary values for read histogram. If
> +#                   specified, old read 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 latency
> -#                    histogram.
> +# @boundaries-write: list of interval boundary values for write histogram.
>   #
> -# @boundaries-flush: list of interval boundary values for flush latency
> -#                    histogram.
> +# @boundaries-flush: list of interval boundary values for flush histogram.
>   #
>   # Returns: error if device is not found or any boundary arrays are invalid.
>   #
> -# Since: 4.0
> +# Since: 4.1
>   #
> -# Example: set new histograms for all io types with intervals
> +# Example: set new latency histograms for all io types with intervals
>   # [0, 10), [10, 50), [50, 100), [100, +inf):
>   #
> -# -> { "execute": "block-latency-histogram-set",
> +# -> { "execute": "block-histogram-set",
>   #      "arguments": { "id": "drive0",
> +#                     "type":"latency",
>   #                     "boundaries": [10, 50, 100] } }
>   # <- { "return": {} }
>   #
> -# Example: set new histogram only for write, other histograms will remain
> -# not changed (or not created):
> +# Example: set new latency histogram only for write, other histograms will
> +# remain not changed (or not created):
>   #
> -# -> { "execute": "block-latency-histogram-set",
> +# -> { "execute": "block-histogram-set",
>   #      "arguments": { "id": "drive0",
> +#                     "type":"latency",
>   #                     "boundaries-write": [10, 50, 100] } }
>   # <- { "return": {} }
>   #
> @@ -613,20 +628,23 @@
>   #   read, flush: [0, 10), [10, 50), [50, 100), [100, +inf)
>   #   write: [0, 1000), [1000, 5000), [5000, +inf)
>   #
> -# -> { "execute": "block-latency-histogram-set",
> +# -> { "execute": "block-histogram-set",
>   #      "arguments": { "id": "drive0",
> +#                     "type":"latency",
>   #                     "boundaries": [10, 50, 100],
>   #                     "boundaries-write": [1000, 5000] } }
>   # <- { "return": {} }
>   #
>   # Example: remove all latency histograms:
>   #
> -# -> { "execute": "block-latency-histogram-set",
> -#      "arguments": { "id": "drive0" } }
> +# -> { "execute": "block-histogram-set",
> +#      "arguments": { "id": "drive0"
> +#                     "type":"latency"} }
>   # <- { "return": {} }
>   ##
> -{ 'command': 'block-latency-histogram-set',
> +{ 'command': 'block-histogram-set',
>     'data': {'id': 'str',
> +           'type': 'BlockHistogramType',
>              '*boundaries': ['uint64'],
>              '*boundaries-read': ['uint64'],
>              '*boundaries-write': ['uint64'],
> @@ -912,11 +930,11 @@
>   # @timed_stats: Statistics specific to the set of previously defined
>   #               intervals of time (Since 2.5)
>   #
> -# @rd_latency_histogram: @BlockLatencyHistogramInfo. (Since 4.0)
> +# @rd_latency_histogram: @BlockHistogramInfo. (Since 4.1)
>   #
> -# @wr_latency_histogram: @BlockLatencyHistogramInfo. (Since 4.0)
> +# @wr_latency_histogram: @BlockHistogramInfo. (Since 4.1)
>   #
> -# @flush_latency_histogram: @BlockLatencyHistogramInfo. (Since 4.0)
> +# @flush_latency_histogram: @BlockHistogramInfo. (Since 4.1)
>   #
>   # Since: 0.14.0
>   ##
> @@ -931,9 +949,12 @@
>              'invalid_wr_operations': 'int', 'invalid_flush_operations': 
> 'int',
>              'account_invalid': 'bool', 'account_failed': 'bool',
>              'timed_stats': ['BlockDeviceTimedStats'],
> -           '*rd_latency_histogram': 'BlockLatencyHistogramInfo',
> -           '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
> -           '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
> +           '*rd_latency_histogram': 'BlockHistogramInfo',
> +           '*wr_latency_histogram': 'BlockHistogramInfo',
> +           '*flush_latency_histogram': 'BlockHistogramInfo',
> +           '*rd_size_histogram': 'BlockHistogramInfo',
> +           '*wr_size_histogram': 'BlockHistogramInfo',
> +           '*flush_size_histogram': 'BlockHistogramInfo' } }
>   
>   ##
>   # @BlockStats:
> 


-- 
Best regards,
Vladimir