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

[Ben Pfaff]

On Fri, Oct 10, 2014 at 09:35:52AM -0700, Ben Pfaff wrote:
> 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.

I thought of another question.  I think that there's a hard limit, with
this mechanism, on the maximum size of the client's statistics data
structure.  Without looking at the patch again, I think it's 64 - 8 = 56
bytes.  Is there anything in the code to make it hard for the client to
accidentally try to store more data than that?