Thanks for the review, Andy I have adjust the code according to your comments,
On Thu, Aug 15, 2013 at 5:39 PM, Andy Zhou <az...@nicira.com> wrote: > Thanks for rebasing. > > > On Wed, Aug 14, 2013 at 5:04 PM, Alex Wang <al...@nicira.com> wrote: > >> This commit changes the "ovs-appclt 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> >> >> --- >> >> v2 -> v3: >> - rebase to current master. >> >> v1 -> v2: >> - abandon the wakeup timer and coverage_wait() function. >> - now the coverage_run() will run only when more than 5 seconds have >> passed >> since last run. >> >> --- >> lib/coverage-unixctl.man | 3 +- >> lib/coverage.c | 95 >> +++++++++++++++++++++++++++++++++++++++------- >> lib/coverage.h | 17 +++++++-- >> lib/timeval.c | 1 - >> vswitchd/ovs-vswitchd.c | 2 + >> 5 files changed, 99 insertions(+), 19 deletions(-) >> >> diff --git a/lib/coverage-unixctl.man b/lib/coverage-unixctl.man >> index 9718894..2d283dc 100644 >> --- a/lib/coverage-unixctl.man >> +++ b/lib/coverage-unixctl.man >> @@ -8,4 +8,5 @@ main loop takes unusually long to run. >> Coverage counters are useful mainly for performance analysis and >> debugging. >> .IP "\fBcoverage/show\fR" >> -Displays the values of all of the coverage counters. >> +Displays the per-second, per-minute and per-hour rates, and total count >> +of all of the coverage counters. >> diff --git a/lib/coverage.c b/lib/coverage.c >> index f152474..e3e7610 100644 >> --- a/lib/coverage.c >> +++ b/lib/coverage.c >> @@ -47,9 +47,21 @@ struct coverage_counter *coverage_counters[] = { >> #define n_coverage_counters ARRAY_SIZE(coverage_counters) >> #endif /* !USE_LINKER_SECTIONS */ >> >> -static unsigned int epoch; >> +/* Makes coverage_run run every 5 seconds. */ >> +#define COVERAGE_RUN_INTERVAL 5000 >> > Can we drive MIN_LEN from this number? it is hard coded to 12 now, and > only works if the interval is 5 seconds. > >> +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; >> + >> +/* Index counter used to compute the moving average array's index. */ >> +static unsigned int idx_count = 0; >> >> static void coverage_read(struct svec *); >> +static unsigned int coverage_array_sum(const unsigned int *arr, >> + const unsigned int len); >> + >> >> static void >> coverage_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED, >> @@ -170,7 +182,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; >> @@ -189,8 +201,16 @@ 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)); >> + /* The per second rate is calculated via averaging the counts in >> + * last 5-second interval. */ >> + svec_add_nocopy(lines, >> + xasprintf("%-24s %5.1f/sec %7u/min " >> + "%9u/hr total: %llu", >> + c->name, >> + c->sec_to_min[(min_idx + 11) % MIN_LEN] / 5.0 > > How about use (MIN_LEN -1) instead of 11? > + coverage_array_sum(c->sec_to_min, MIN_LEN), > >> + coverage_array_sum(c->min_to_hr, HR_LEN), >> + c->count + c->total)); >> } >> >> /* Adds coverage counter information to 'lines'. */ >> @@ -204,8 +224,8 @@ 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 (frequency), " >> + "hash=%08"PRIx32":", hash)); >> for (i = 0; i < n_coverage_counters; i++) { >> struct coverage_counter *c = coverage_counters[i]; >> if (c->count) { >> @@ -225,16 +245,65 @@ coverage_read(struct svec *lines) >> svec_add_nocopy(lines, xasprintf("%zu events never hit", >> n_never_hit)); >> } >> >> -/* Advances to the next epoch of coverage, resetting all the counters to >> 0. */ >> +/* Runs every second to update the coverage counters' >> > Every 5 seconds? > >> + * sec_to_min array and min_to_hr array. */ >> void >> -coverage_clear(void) >> +coverage_run(void) >> { >> + long long int cur_time = time_msec(); >> + >> + /* Initialize the coverage_run_time. */ >> + if (coverage_run_time == LLONG_MIN) { >> + coverage_run_time = time_msec(); >> + } >> + >> + if (cur_time >= coverage_run_time) { >> + size_t i, j; >> + /* Computes the number of 5-second slots. */ >> + int slots = (cur_time - coverage_run_time) / >> COVERAGE_RUN_INTERVAL >> + + 1; >> + >> + /* This loop is necessary, since it is possible that the actual >> + * run interval is multiple of 5 seconds. */ >> + for (i = 0; i < slots; i++) { >> + /* Updates the index variables. */ >> + /* The min_idx is increased from 0 to 11. Every time the >> + * min_idx finishes a cycle (a cycle is one minute), >> + * the hr_idx is incremented by 1. */ >> + min_idx = idx_count % MIN_LEN; >> + hr_idx = idx_count / MIN_LEN; >> + >> + for (j = 0; j < n_coverage_counters; j++) { >> + struct coverage_counter *c = coverage_counters[j]; >> + /* Calculates the portion to be added, which is the >> average >> + * of counts over the time. Note, the last loop will also >> + * include the remainder. */ >> > It would be nice to add comments about what the following logic is trying > to accomplish. > >> + int portion = c->count / slots + (i == slots - 1 ? >> + c->count % slots : 0); >> + >> + c->total += portion; >> + c->count = (i == slots -1 ? 0 : c->count) ; >> + c->sec_to_min[min_idx] = portion; >> + /* Note, only adds up the latest count portion here. */ >> + c->min_to_hr[hr_idx] = min_idx == 0 ? >> + portion : (c->min_to_hr[hr_idx] + portion); >> + } >> + /* This is to guarantee that mov_avg_hr_idx ranges from 0 to >> 59. */ >> + idx_count = (idx_count + 1) % 720; >> > How about MIN_LEN * HR_LEN instead of 720. > >> + /* Updates the run time. */ >> + coverage_run_time += COVERAGE_RUN_INTERVAL; >> + } >> + } >> +} >> + >> +static unsigned int >> +coverage_array_sum(const unsigned int *arr, const unsigned int len) >> +{ >> + unsigned int sum = 0; >> size_t i; >> >> - epoch++; >> - for (i = 0; i < n_coverage_counters; i++) { >> - struct coverage_counter *c = coverage_counters[i]; >> - c->total += c->count; >> - c->count = 0; >> + for (i = 0; i < len; i++) { >> + sum += arr[i]; >> } >> + return sum; >> } >> diff --git a/lib/coverage.h b/lib/coverage.h >> index 968c489..26837ed 100644 >> --- a/lib/coverage.h >> +++ b/lib/coverage.h >> @@ -29,11 +29,19 @@ >> >> #include "vlog.h" >> >> +/* Defines the moving average array length. */ >> +#define MIN_LEN 12 /* 12 * 5 seconds = 1 minute */ >> > Would be nice to derive the number using COVERAGE_RUN_INTERVAL. So that > it will be easier to change interval later on. > >> +#define HR_LEN 60 /* 60 * 1 minute = 1 hour */ >> + >> /* 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. */ >> + unsigned int count; /* Count within the current second. */ >> + unsigned long long int total; /* Total count over all time. */ >> + >> + /* The moving average arrays. */ >> + unsigned int sec_to_min[MIN_LEN]; >> + unsigned int min_to_hr[HR_LEN]; >> }; >> >> /* Defines COUNTER. There must be exactly one such definition at file >> scope >> @@ -57,11 +65,12 @@ struct coverage_counter { >> >> void coverage_init(void); >> void coverage_log(void); >> -void coverage_clear(void); >> +void coverage_run(void); >> >> /* Implementation detail. */ >> #define COVERAGE_DEFINE__(COUNTER) \ >> extern struct coverage_counter counter_##COUNTER; \ >> - struct coverage_counter counter_##COUNTER = { #COUNTER, 0, 0 } >> + struct coverage_counter counter_##COUNTER = { #COUNTER, 0, 0, \ >> + {0}, {0} } >> >> #endif /* coverage.h */ >> diff --git a/lib/timeval.c b/lib/timeval.c >> index faf8e7b..2380a0a 100644 >> --- a/lib/timeval.c >> +++ b/lib/timeval.c >> @@ -304,7 +304,6 @@ time_poll(struct pollfd *pollfds, int n_pollfds, long >> long int timeout_when, >> if (*last_wakeup) { >> log_poll_interval(*last_wakeup); >> } >> - coverage_clear(); >> start = time_msec(); >> blocked = false; >> >> diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c >> index bc45dac..a9db1fd 100644 >> --- a/vswitchd/ovs-vswitchd.c >> +++ b/vswitchd/ovs-vswitchd.c >> @@ -28,6 +28,7 @@ >> #include "bridge.h" >> #include "command-line.h" >> #include "compiler.h" >> +#include "coverage.h" >> #include "daemon.h" >> #include "dirs.h" >> #include "dpif.h" >> @@ -119,6 +120,7 @@ main(int argc, char *argv[]) >> bridge_run_fast(); >> unixctl_server_run(unixctl); >> netdev_run(); >> + coverage_run(); >> >> signal_wait(sighup); >> memory_wait(); >> -- >> 1.7.9.5 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev >> > >
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev