Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/2] block/accounting: introduce latency histogram

[Stefan Hajnoczi]

On Wed, Feb 07, 2018 at 03:50:36PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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 <address@hidden>
> ---
>  include/block/accounting.h |  9 +++++
>  block/accounting.c         | 97 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 106 insertions(+)
> 
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index b833d26d6c..9679020f64 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 */

The semantics of these fields is not obvious.  Please include a comment
with an example or ASCII-art diagram so anyone reading the code
understands the purpose of the fields without reading all the code.

According to Wikipedia and Mathworld, "intervals" and "bins" are
commonly used terms:
https://en.wikipedia.org/wiki/Histogram
http://mathworld.wolfram.com/Histogram.html

I suggest:

  typedef struct {
      /* The following histogram is represented like this:
       *
       * 5|           *
       * 4|           *
       * 3| *         *
       * 2| *         *    *
       * 1| *    *    *    *
       *  +------------------
       *      10   50   100
       *
       * BlockLatencyHistogram histogram = {
       *     .nbins = 4,
       *     .intervals = {10, 50, 100},
       *     .bins = {3, 1, 5, 2},
       * };
       */
      size_t nbins;

      /* Interval boundaries (there are @nbins - 1 elements) */
      uint64_t *intervals;

      /* Frequency data (there are @nbins elements) */
      uint64_t *bins[BLOCK_MAX_IOTYPE];
  } BlockLatencyHistogram;

> +/* block_latency_histogram_compare_func
> + * Compare @key with interval address@hidden, @el+1), where @el+1 is a next 
> array element
> + * after @el.
> + * Return: -1 if @key < @el
> + *          0 if @key in address@hidden, @el+1)
> + *         +1 if @key >= @el+1

"@el+1" is confusing, usually x+1 means "the value of x plus 1", not
"y" (a different variable!).

I suggest:

  /* block_latency_histogram_compare_func:
   * Compare @key with interval address@hidden, @it[1]).
   * Return: -1 if @key < @it[0]
   *          0 if @key in address@hidden, @it[1])
   *         +1 if @key >= @it[1]
   */
  static int block_latency_histogram_compare_func(const void *key, const void 
*it)

> +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));

Is there a reason for using g_renew() and then clearing everything?
This is more concise:

  g_free(hist->histogram[i]);
  hist->histogram[i] = g_new0(uint64_t, hist->size);

signature.asc
Description: PGP signature