[ovs-dev] [PATCH V1 1/3] coverage: Reimplement the "ovs-appclt coverage/show" command
alexw at nicira.com
Mon Jun 17 18:11:29 UTC 2013
On Wed, Jun 12, 2013 at 4:20 PM, Ben Pfaff <blp at nicira.com> wrote:
> 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
> > 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.
This is really important to know.
> 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.
Yes, this will achieve the per-second rate. But will not be very easy to
per-minute and hourly rate. So far, I cannot find a very efficient way to
these (maybe use long linked-lists that count to 1 minute and 1 hour?).
I'll put detailed comments in v2.
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.
Thanks for pointing this out.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the dev