Hi Nico,


On 01/20/2016 06:46 PM, Nicolas Pitre wrote:

On Wed, 20 Jan 2016, Daniel Lezcano wrote:

quoted

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 <redacted>
---

[ ... ]

quoted

+struct stats {
+	u64           sum;                     /* sum of values */
+	u32           values[STATS_NR_VALUES]; /* array of values */
+	unsigned char w_ptr;                   /* current window pointer */

Why did you change this from an unsigned int?

This won't provide any memory space saving given that the structure has
to be padded up to the next 64-bit boundary.

Ok, I will change it back to unsigned int.

[ ... ]

quoted

+	for (i = 0; i < STATS_NR_VALUES; i++) {
+		s64 diff = s->values[i] - mean;
+		variance += (u64)diff * diff;
+	}

This is completely wrong.  Even more wrong than it used to be.  I must
have expressed myself badly about this last time.

To avoid any confusion, here's what the code should be:

	int i;
	u64 variance = 0;

	for (i = 0; i < STATS_NR_VALUES; i++) {
		s32 diff = s->values[i] - mean;
		variance += (s64)diff * diff;
	}

	[...]

Aah, ok :)

[ ... ]

quoted

+	if (diff > (1 << 20)) {

You could use the USEC_PER_SEC constant here. It is already widely used
and would make the code even more obvious.

Indeed.

[ ... ]

quoted

+		/*
+		 * There is no point attempting predictions on interrupts more
+		 * than 1 second apart. This has no benefit for sleep state
+		 * selection and increases the risk of overflowing our variance
+		 * computation. Reset all stats in that case.
+		 */

This comment is wrong.  It is relevant in sched_irq_timing_handler() but
not here.  Instead this should be something like:

		/*
		 * This interrupt last triggered more than a second ago.
		 * It is definitely not predictable for our purpose anymore.
		 */

Ok.

[ ... ]

quoted

+		interval = w->stats.values[w->stats.w_ptr];
+		if ((u64)((interval - mean) * (interval - mean)) > variance)

s/u64/s64/ please.

Noted.

Thanks Nico for the review.


   -- Daniel

-- 
  <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