[ovs-dev] [PATCH V1 1/3] coverage: Reimplement the "ovs-appclt coverage/show" command

Ben Pfaff blp at nicira.com
Wed Jun 12 23:20:58 UTC 2013

On Wed, Jun 12, 2013 at 11:38:41AM -0700, Alex Wang 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 <alexw at nicira.com>
> ---
> Possible Issue:
> The "coverage_run" runs every second. And the number of addition operations is
> not very large. If it is still considered to be time-consuming, I'll adjust
> further.

I have two high-level design questions.

First is the cost, not of the periodic summations (I doubt that cost
matters) but of forcing a wakeup every second.  This probably doesn't
matter in ovs-vswitchd, but other daemons that don't wake up so
frequently also use the poll_loop library, and it would be a shame to
drive up system load unnecessarily.  So I would prefer, if possible,
for the coverage code not to use a timer but instead to just do work
in coverage_run() whenever the process happens to wake up.

Second is accuracy.  I believe that the current code considers any
interval greater than or equal to a second as exactly a second.
Because wakeups aren't precise, that tends to stretch out the
definition of a second to somewhat longer.  If you take my suggestion
about cost, above, that makes the problem worse.  So I think that it
may be worthwhile to measure the length of the actual interval for a
"second" and divide by its length.  For example, if there were 100
events of some type over 1.5 seconds, we would count that as 100 / 1.5
== 66.67 events per second, and use that in the average.  That might
require using "double"s for the average counters, or we could use
fixed-point arithmetic; I think either solution could be OK.

I didn't do a detailed review yet because I'd like to hear your
thoughts on these points first.  I did notice that
coverage_array_sum() is only used in coverage.c, so I would define it
there (and not "inline"), not in coverage.h.



More information about the dev mailing list