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