This looks good to me,
The only issue may be that some comments need to be changed:
-/* Sorts coverage counters in descending order by count, within equal
counts
- * alphabetically by name. */
+/* Sorts coverage counters in descending order by total count,
+ * within equal total counts alphabetically by name. */
static int
compare_coverage_counters(const void *a_, const void *b_)
{
@@ -108,7 +108,7 @@ coverage_hash(void)
uint32_t hash = 0;
int n_groups, i;
- /* Sort coverage counters into groups with equal counts. */
+ /* Sort coverage counters into groups with equal total counts. */
c = xmalloc(n_coverage_counters * sizeof *c);
ovs_mutex_lock(&coverage_mutex);
for (i = 0; i < n_coverage_counters; i++) {
On Fri, Aug 16, 2013 at 3:47 PM, Ben Pfaff <b...@nicira.com> wrote:
> This makes each of the coverage counters per-thread. It abandons the
> idea of trying to keep track of the number of hits in the "current" poll
> loop, since there are many poll loops running, each in its own thread, as
> well as the idea of numbering epochs for the same reason. Instead, we
> just keep track of overall totals for the process for each coverage
> counter, accumulating per-thread counts into the global total each time
> a thread's main loop passes through poll_block().
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> lib/coverage.c | 73
> +++++++++++++++++++++++++++++++-------------------------
> lib/coverage.h | 41 ++++++++++++++++++++++---------
> 2 files changed, 71 insertions(+), 43 deletions(-)
>
> diff --git a/lib/coverage.c b/lib/coverage.c
> index f152474..ac5dd67 100644
> --- a/lib/coverage.c
> +++ b/lib/coverage.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
> + * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -35,7 +35,19 @@ extern struct coverage_counter *__stop_coverage[];
> #define coverage_counters __start_coverage
> #define n_coverage_counters (__stop_coverage - __start_coverage)
> #else /* !USE_LINKER_SECTIONS */
> -#define COVERAGE_COUNTER(NAME) COVERAGE_DEFINE__(NAME);
> +#define COVERAGE_COUNTER(COUNTER) \
> + DECLARE_EXTERN_PER_THREAD_DATA(unsigned int, \
> + counter_##COUNTER); \
> + DEFINE_EXTERN_PER_THREAD_DATA(counter_##COUNTER, 0); \
> + static unsigned int COUNTER##_count(void) \
> + { \
> + unsigned int *countp = counter_##COUNTER##_get(); \
> + unsigned int count = *countp; \
> + *countp = 0; \
> + return count; \
> + } \
> + struct coverage_counter counter_##COUNTER \
> + = { #COUNTER, COUNTER##_count, 0 };
> #include "coverage.def"
> #undef COVERAGE_COUNTER
>
> @@ -47,7 +59,7 @@ struct coverage_counter *coverage_counters[] = {
> #define n_coverage_counters ARRAY_SIZE(coverage_counters)
> #endif /* !USE_LINKER_SECTIONS */
>
> -static unsigned int epoch;
> +static struct ovs_mutex coverage_mutex = OVS_MUTEX_INITIALIZER;
>
> static void coverage_read(struct svec *);
>
> @@ -82,8 +94,8 @@ compare_coverage_counters(const void *a_, const void *b_)
> const struct coverage_counter *const *bp = b_;
> const struct coverage_counter *a = *ap;
> const struct coverage_counter *b = *bp;
> - if (a->count != b->count) {
> - return a->count < b->count ? 1 : -1;
> + if (a->total != b->total) {
> + return a->total < b->total ? 1 : -1;
> } else {
> return strcmp(a->name, b->name);
> }
> @@ -98,9 +110,11 @@ coverage_hash(void)
>
> /* Sort coverage counters into groups with equal counts. */
> c = xmalloc(n_coverage_counters * sizeof *c);
> + ovs_mutex_lock(&coverage_mutex);
> for (i = 0; i < n_coverage_counters; i++) {
> c[i] = coverage_counters[i];
> }
> + ovs_mutex_unlock(&coverage_mutex);
> qsort(c, n_coverage_counters, sizeof *c, compare_coverage_counters);
>
> /* Hash the names in each group along with the rank. */
> @@ -108,13 +122,13 @@ coverage_hash(void)
> for (i = 0; i < n_coverage_counters; ) {
> int j;
>
> - if (!c[i]->count) {
> + if (!c[i]->total) {
> break;
> }
> n_groups++;
> hash = hash_int(i, hash);
> for (j = i; j < n_coverage_counters; j++) {
> - if (c[j]->count != c[i]->count) {
> + if (c[j]->total != c[i]->total) {
> break;
> }
> hash = hash_string(c[j]->name, hash);
> @@ -170,7 +184,7 @@ coverage_log(void)
> uint32_t hash = coverage_hash();
> if (coverage_hit(hash)) {
> VLOG_INFO("Skipping details of duplicate event coverage for "
> - "hash=%08"PRIx32" in epoch %u", hash, epoch);
> + "hash=%08"PRIx32, hash);
> } else {
> struct svec lines;
> const char *line;
> @@ -186,17 +200,11 @@ coverage_log(void)
> }
> }
>
> -static void
> -coverage_read_counter(struct svec *lines, const struct coverage_counter
> *c)
> -{
> - svec_add_nocopy(lines, xasprintf("%-24s %5u / %9llu",
> - c->name, c->count, c->count +
> c->total));
> -}
> -
> /* Adds coverage counter information to 'lines'. */
> static void
> coverage_read(struct svec *lines)
> {
> + unsigned long long int *totals;
> size_t n_never_hit;
> uint32_t hash;
> size_t i;
> @@ -204,37 +212,38 @@ coverage_read(struct svec *lines)
> hash = coverage_hash();
>
> n_never_hit = 0;
> - svec_add_nocopy(lines, xasprintf("Event coverage (epoch %u/entire
> run), "
> - "hash=%08"PRIx32":", epoch, hash));
> + svec_add_nocopy(lines,
> + xasprintf("Event coverage, hash=%08"PRIx32":", hash));
> +
> + totals = xmalloc(n_coverage_counters * sizeof *totals);
> + ovs_mutex_lock(&coverage_mutex);
> for (i = 0; i < n_coverage_counters; i++) {
> - struct coverage_counter *c = coverage_counters[i];
> - if (c->count) {
> - coverage_read_counter(lines, c);
> - }
> + totals[i] = coverage_counters[i]->total;
> }
> + ovs_mutex_unlock(&coverage_mutex);
> +
> for (i = 0; i < n_coverage_counters; i++) {
> - struct coverage_counter *c = coverage_counters[i];
> - if (!c->count) {
> - if (c->total) {
> - coverage_read_counter(lines, c);
> - } else {
> - n_never_hit++;
> - }
> + if (totals[i]) {
> + svec_add_nocopy(lines, xasprintf("%-24s %9llu",
> + coverage_counters[i]->name,
> + totals[i]));
> + } else {
> + n_never_hit++;
> }
> }
> svec_add_nocopy(lines, xasprintf("%zu events never hit",
> n_never_hit));
> + free(totals);
> }
>
> -/* Advances to the next epoch of coverage, resetting all the counters to
> 0. */
> void
> coverage_clear(void)
> {
> size_t i;
>
> - epoch++;
> + ovs_mutex_lock(&coverage_mutex);
> for (i = 0; i < n_coverage_counters; i++) {
> struct coverage_counter *c = coverage_counters[i];
> - c->total += c->count;
> - c->count = 0;
> + c->total += c->count();
> }
> + ovs_mutex_unlock(&coverage_mutex);
> }
> diff --git a/lib/coverage.h b/lib/coverage.h
> index 968c489..73b027a 100644
> --- a/lib/coverage.h
> +++ b/lib/coverage.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
> + * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -27,33 +27,54 @@
> * for traditional coverage instrumentation with e.g. "gcov", but it is
> still
> * a useful debugging tool. */
>
> +#include "ovs-thread.h"
> #include "vlog.h"
>
> /* A coverage counter. */
> struct coverage_counter {
> - const char *name; /* Textual name. */
> - unsigned int count; /* Count within the current epoch. */
> - unsigned long long int total; /* Total count over all epochs. */
> + const char *const name; /* Textual name. */
> + unsigned int (*const count)(void); /* Gets, zeros this thread's
> count. */
> + unsigned long long int total; /* Total count. */
> };
>
> /* Defines COUNTER. There must be exactly one such definition at file
> scope
> * within a program. */
> #if USE_LINKER_SECTIONS
> #define COVERAGE_DEFINE(COUNTER) \
> - COVERAGE_DEFINE__(COUNTER); \
> + DEFINE_STATIC_PER_THREAD_DATA(unsigned int, \
> + counter_##COUNTER, 0); \
> + static unsigned int COUNTER##_count(void) \
> + { \
> + unsigned int *countp = counter_##COUNTER##_get(); \
> + unsigned int count = *countp; \
> + *countp = 0; \
> + return count; \
> + } \
> + static inline void COUNTER##_add(unsigned int n) \
> + { \
> + *counter_##COUNTER##_get() += n; \
> + } \
> + struct coverage_counter counter_##COUNTER \
> + = { #COUNTER, COUNTER##_count, 0 }; \
> extern struct coverage_counter *counter_ptr_##COUNTER; \
> struct coverage_counter *counter_ptr_##COUNTER \
> __attribute__((section("coverage"))) = &counter_##COUNTER
> #else
> -#define COVERAGE_DEFINE(MODULE) \
> - extern struct coverage_counter counter_##MODULE
> +#define COVERAGE_DEFINE(COUNTER) \
> + DECLARE_EXTERN_PER_THREAD_DATA(unsigned int, \
> + counter_##COUNTER); \
> + static inline void COUNTER##_add(unsigned int n) \
> + { \
> + *counter_##COUNTER##_get() += n; \
> + } \
> + extern struct coverage_counter counter_##COUNTER
> #endif
>
> /* Adds 1 to COUNTER. */
> -#define COVERAGE_INC(COUNTER) counter_##COUNTER.count++;
> +#define COVERAGE_INC(COUNTER) COVERAGE_ADD(COUNTER, 1)
>
> /* Adds AMOUNT to COUNTER. */
> -#define COVERAGE_ADD(COUNTER, AMOUNT) counter_##COUNTER.count += (AMOUNT);
> +#define COVERAGE_ADD(COUNTER, AMOUNT) COUNTER##_add(AMOUNT)
>
> void coverage_init(void);
> void coverage_log(void);
> @@ -61,7 +82,5 @@ void coverage_clear(void);
>
> /* Implementation detail. */
> #define COVERAGE_DEFINE__(COUNTER) \
> - extern struct coverage_counter counter_##COUNTER; \
> - struct coverage_counter counter_##COUNTER = { #COUNTER, 0, 0 }
>
> #endif /* coverage.h */
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
