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 <al...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
