On Thu, 10 Dec 2015, Daniel Lezcano wrote:
On 12/10/2015 06:34 PM, Nicolas Pitre wrote:
On Thu, 10 Dec 2015, Daniel Lezcano wrote:
- mean = stats_mean(w->stats);
- stddev = stats_stddev(w->stats);
After my review of your stat lib code, you must realize that you've inserted the two most expensive operations of that lib right there with those two calls. And if you fix those flaws that I highlighted earlier, it'll become even more expensive.
Yet, I provided you with equivalent code already that was highly optimized to be acceptable in interrupt context. Quoting the commit message that came with my patch
Because the stats are gathered in interrupt context, the core computation is as light as possible, turning into 3 subs, 3 adds and 6 mults where 4 of those mults involve a small compile-time constant.Here with your version you literally increased the cost for the same computations by an order of magnitude if not more... especially on ARM32. I don't think this is ever going to be accepted upstream as it is. Given tglx's propension for flaming people submitting suboptimal code, I'd refrain from posting this if I were you.
I agree.
I will change the code to have this computation done when searching the next wakeup event and probably replace the stddev by the optimization you did by comparing squared mean values with the variance instead of the mean with the square root of the variance. That will save us the int_sqrt and will reduce the number of operations in the irq handler.
Thanks for the review.
I still have to ask: why did you not use my code?
I do see that you borrowed some of its structure, but you completely ripped out all the code that made this computation extremely efficient.
Nicolas