[ovs-dev] [PATCH 1/2] coverage: Reimplement the "ovs-appctl coverage/show" command

[Ben Pfaff]

On Wed, Aug 21, 2013 at 11:45:08AM -0700, Alex Wang wrote:
> This commit changes the "ovs-appctl coverage/show" command to show the
> per-second, per-minute and per-hour rates of function invocation.  More
> importantly, this makes using coverage counter an easy way to monitor
> the execution of specific functions.
> 
> Signed-off-by: Alex Wang <alexw at nicira.com>

Thanks for revising this.  From my recollection of the previous
versions, this version is simpler.  That's great, I like simple code.

> diff --git a/lib/coverage.c b/lib/coverage.c
> index 23e2997..153b6ac 100644
> --- a/lib/coverage.c
> +++ b/lib/coverage.c
> @@ -63,7 +63,17 @@ struct coverage_counter *coverage_counters[] = {
>  
>  static struct ovs_mutex coverage_mutex = OVS_MUTEX_INITIALIZER;
>  
> +static long long int coverage_run_time = LLONG_MIN;
> +
> +/* Defines the moving average array index variables. */
> +static unsigned int min_idx = 0;
> +static unsigned int hr_idx = 0;

It looks to me that min_idx and hr_idx are used only in
coverage_run().  You might consider moving them into that function.

> +/* Index counter used to compute the moving average array's index. */
> +static unsigned int idx_count = 0;

It looks to me like idx_count should be protected by coverage_mutex.

> @@ -220,15 +231,24 @@ coverage_read(struct svec *lines)
>      totals = xmalloc(n_coverage_counters * sizeof *totals);
>      ovs_mutex_lock(&coverage_mutex);
>      for (i = 0; i < n_coverage_counters; i++) {
> -        totals[i] = coverage_counters[i]->total;
> +        totals[i] = c[i]->total;
>      }
>      ovs_mutex_unlock(&coverage_mutex);
>  
>      for (i = 0; i < n_coverage_counters; i++) {
>          if (totals[i]) {
> -            svec_add_nocopy(lines, xasprintf("%-24s %9llu",
> -                                             coverage_counters[i]->name,
> -                                             totals[i]));
> +            /* The per second rate is calculated via averaging the count in
> +             * the most recent COVERAGE_RUN_INTERVAL interval. */
> +            svec_add_nocopy(lines,
> +                xasprintf("%-24s %5.1f/sec %7u/min "
> +                          "%9u/hr   total: %llu",
> +                          c[i]->name,
> +                          ((double) c[i]->min[(idx_count - 1) % MIN_AVG_LEN])
> +                             * 1000 / COVERAGE_RUN_INTERVAL,
> +                          coverage_array_sum(c[i]->min, MIN_AVG_LEN),
> +                          coverage_array_sum(c[i]->hr,  HR_AVG_LEN),
> +                          totals[i]));
> +

The indentation and parenthesization is not quite right here:
                          ((double) c[i]->min[(idx_count - 1) % MIN_AVG_LEN])
                             * 1000 / COVERAGE_RUN_INTERVAL,
It should be:
                          ((double) c[i]->min[(idx_count - 1) % MIN_AVG_LEN]
                           * 1000 / COVERAGE_RUN_INTERVAL),
(I think that CodingStyle implies this--I intended that it should--but
it might require a close reading.)

Also I might just use 1000.0 instead of a cast to double, because I am
not fond of casts:
                          (c[i]->min[(idx_count - 1) % MIN_AVG_LEN]
                           * 1000.0 / COVERAGE_RUN_INTERVAL),

> diff --git a/lib/coverage.h b/lib/coverage.h
> index 3d1a115..a01b2c1 100644
> --- a/lib/coverage.h
> +++ b/lib/coverage.h
> @@ -30,11 +30,24 @@
>  #include "ovs-thread.h"
>  #include "vlog.h"
>  
> +/* Makes coverage_run run every 5000 ms (5 seconds).
> + * If this value is redefined, the new value must
> + * divide 60000. */
> +#define COVERAGE_RUN_INTERVAL    5000

When I can, I like to use build assertions to enforce build-time
invariants in place of or in addition to comments.  (This is level 9
on Rusty Russell's "Hard To Misuse Positive Score List" at
http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html.  Everyone
should read this.)

In this case, I think that you can use BUILD_ASSERT_DECL.

Thanks,

Ben.