Hey Ben,
Just noticed, when compiling with sparse, it issues the warnings like:
"""
lib/netdev-linux.c:76:1: warning: symbol 'counter_netdev_set_policing' was
not declared. Should it be static?
lib/netdev-linux.c:77:1: warning: symbol 'counter_netdev_arp_lookup' was
not declared. Should it be static?
lib/netdev-linux.c:78:1: warning: symbol 'counter_netdev_get_ifindex' was
not declared. Should it be static?
"""
On Mon, Aug 19, 2013 at 8:08 AM, Alex Wang <al...@nicira.com> wrote:
> 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
