On 12/10/2015 06:34 PM, Nicolas Pitre wrote:
On Thu, 10 Dec 2015, Daniel Lezcano wrote:
Many IRQs are quiet most of the time, or they tend to come in bursts of fairly equal time intervals within each burst. It is therefore possible to detect those IRQs with stable intervals and guestimate when the next IRQ event is most likely to happen.
Examples of such IRQs may include audio related IRQs where the FIFO size and/or DMA descriptor size with the sample rate create stable intervals, block devices during large data transfers, etc. Even network streaming of multimedia content creates patterns of periodic network interface IRQs in some cases.
This patch adds code to track the mean interval and variance for each IRQ over a window of time intervals between IRQ events. Those statistics can be used to assist cpuidle in selecting the most appropriate sleep state by predicting the most likely time for the next interrupt.
Because the stats are gathered in interrupt context, the core computation is as light as possible.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
I'm rather disappointed by this patch.
So sorry for that.
Comments below.
[ ... ]
- if (diff > (1 << 20)) {
stats_reset(w->stats);w->predictable = 0;return;- }
- 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.
-- http://www.linaro.org/ Linaro.org │ Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro Facebook | http://twitter.com/#!/linaroorg Twitter | http://www.linaro.org/linaro-blog/ Blog