Re: [Qemu-block] [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 <address@hidden>
> ---
>   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