Thanks Ben for the review,
> +static long long int coverage_run_time = LLONG_MIN; > > + > > +/* Defines the moving average array index variables. */ > > +static unsigned int min_idx = 0; > > +static unsigned int hr_idx = 0; > > It looks to me that min_idx and hr_idx are used only in > coverage_run(). You might consider moving them into that function. > > Makes sense, I'll adjust this. > > +/* Index counter used to compute the moving average array's index. */ > > +static unsigned int idx_count = 0; > > It looks to me like idx_count should be protected by coverage_mutex. I'm a bit confused here. Want to ask, that there will only be one thread going through the loop in ovs-vswitchd.c (which calls coverage_run()), right? If so, idx_count will only be accessed by that thread. Then do we still need to protect it? > > + coverage_array_sum(c[i]->min, MIN_AVG_LEN), > > + coverage_array_sum(c[i]->hr, HR_AVG_LEN), > > + totals[i])); > > + > > The indentation and parenthesization is not quite right here: > ((double) c[i]->min[(idx_count - 1) % > MIN_AVG_LEN]) > * 1000 / COVERAGE_RUN_INTERVAL, > It should be: > ((double) c[i]->min[(idx_count - 1) % > MIN_AVG_LEN] > * 1000 / COVERAGE_RUN_INTERVAL), > (I think that CodingStyle implies this--I intended that it should--but > it might require a close reading.) > > Also I might just use 1000.0 instead of a cast to double, because I am > not fond of casts: > (c[i]->min[(idx_count - 1) % MIN_AVG_LEN] > * 1000.0 / COVERAGE_RUN_INTERVAL), > > Thanks for pointing these out. I'll fix accordingly. > When I can, I like to use build assertions to enforce build-time > invariants in place of or in addition to comments. (This is level 9 > on Rusty Russell's "Hard To Misuse Positive Score List" at > http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html. Everyone > should read this.) > > In this case, I think that you can use BUILD_ASSERT_DECL. > Thanks for sharing this reference!
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
