[ovs-dev] [RFC 05/10] ovs-thread-stats: New truly per-thread stats module

[Ben Pfaff]

On Wed, Oct 08, 2014 at 02:09:51PM -0700, Daniele Di Proietto wrote:
> Signed-off-by: Daniele Di Proietto <ddiproietto at vmware.com>

This looks very useful!  Thank you.

I am not sure that we need the "ovs" prefix on all the names.  I
included it in some ovsthread_*() names to provide a contrast and
analogy to pthread_*().  I included it in ovsrcu_*() because I wanted to
make sure there was a distinction from Linux and liburcu names.  But I
don't think there is an existing thread stats module that needs
distinguishing.  I think that "threadstats" might be a better prefix; it
is shorter, anyway.

We don't usually add these extern "C" blocks unless someone asks.
Do you have a use case?
> +#ifdef  __cplusplus
> +extern "C" {
> +#endif

I think that you could get atomic clearing by replacing the "clear" bool
by a uint32_t that is a sequence number.  Calling "clear" would just
increment the thread_stats's sequence number.  Calls to
ovsthread_stats_get_bucket() would compare the bucket's sequence number
against the thread_stats's sequence number and clear the data if they
differ.  Calls to ovsthread_stats_aggregate() could detect that the
stats were cleared during iteration by noticing that the thread_stats's
sequence number changed from the beginning to the end.

I am not a big fan of callback functions.  Is there a way to implement
ovsthread_stats_aggregate() as an iterator instead, so that a client
could iterate over the buckets with something like
OVSTHREAD_STATS_FOR_EACH_BUCKET instead of using a callback?

The documentation is good.  The formatting strikes me as odd:

 *Initialization*
 *    flow->statsid = ovsthread_stats_create_bucket();
 *
 *Deinitialization*
 *    ovsthread_stats_destroy_bucket(flow->statsid);
 *

I'd see something like the following as more in line with our usual
practice:

 * Initialization:
 *    flow->statsid = ovsthread_stats_create_bucket();
 *
 * Deinitialization:
 *    ovsthread_stats_destroy_bucket(flow->statsid);
 *

There's a lot of subtlety here.  I have not yet taken enough time to
make myself confident that everything is correct.  Even after I do, I
hope that Jarno will also look this code over.