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

Alex Wang alexw at nicira.com
Mon Jun 17 18:11:29 UTC 2013


Thanks Ben,


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
> 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.
>


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
compute the
per-minute and hourly rate. So far, I cannot find a very efficient way to
calculate
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.



> Thanks,
>
> Ben.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130617/a3805908/attachment-0003.html>


More information about the dev mailing list