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

Reply via email to