[1/3] block/accounting: rename struct BlockLatencyHistogram

Commit Message

zhenwei pi June 20, 2019, 8:54 a.m. UTC

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 <[e-mail]>
---
 block/accounting.c         | 31 ++++++++++++++++++-------------
 block/qapi.c               |  2 +-
 include/block/accounting.h |  8 ++++----
 3 files changed, 23 insertions(+), 18 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy June 21, 2019, 9:45 a.m. UTC | #1

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 <[e-mail]>
> ---
>   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 {
>

Patch

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 {