On 10/10/14, 9:35 AM, "Ben Pfaff" <[email protected]> wrote:
>On Wed, Oct 08, 2014 at 02:09:51PM -0700, Daniele Di Proietto wrote: >> Signed-off-by: Daniele Di Proietto <[email protected]> > >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. Oh, that's much better, I'll do it. > >We don't usually add these extern "C" blocks unless someone asks. >Do you have a use case? >> +#ifdef __cplusplus >> +extern "C" { >> +#endif I don't have a use case for it yet, so I removed it :-) > >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'm not sure I understand your suggestion. The problem that I see with clear() operation is that it sets the 'clear' flag on the threads' bucket (struct ovsthread_stats_bucket is thread local) one at a time. Therefore an "aggregator" might see some of the buckets with the 'clear' flags set and the others without the 'clear' flags set, because ovsthread_stats_clear() is in the process of setting the 'clear' flag on all the buckets. Are you suggesting to add a global (non per-thread), per-id counter? > >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? I think it will be much better to use OVSTHREAD_STATS_FOR_EACH_BUCKET. I'll try to implement it. > >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); > * It looks better, changed. > >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. Yes, the module uses atomic and RCU in a non trivial way. I'll spend some time trying to find possible race conditions and errors. Thanks, Daniele _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
