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 <ddiproie...@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?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
