It appears the index beginning computation is not correct, the current
code does:

     i = (irqts->count & IRQ_TIMINGS_MASK) - 1

If irqts->count is equal to zero, we end up with an index equal to -1,
but that does not happen because the function checks against zero
before and returns in such case.

However, if irqts->count is a multiple of IRQ_TIMINGS_SIZE, the
resulting & bit op will be zero and leads also to a -1 index.

Re-introduce the iteration loop belonging to the previous variance
code which was correct.

Fixes: bbba0e7c5cda "genirq/timings: Add array suffix computation code"
Signed-off-by: Daniel Lezcano <daniel.lezc...@linaro.org>
---
 kernel/irq/timings.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/kernel/irq/timings.c b/kernel/irq/timings.c
index 60362aca4ca4..250bb00ccd85 100644
--- a/kernel/irq/timings.c
+++ b/kernel/irq/timings.c
@@ -267,6 +267,23 @@ void irq_timings_disable(void)
 #define PREDICTION_MAX         10 /* 2 ^ PREDICTION_MAX useconds */
 #define PREDICTION_BUFFER_SIZE 16 /* slots for EMAs, hardly more than 16 */
 
+/*
+ * Number of elements in the circular buffer: If it happens it was
+ * flushed before, then the number of elements could be smaller than
+ * IRQ_TIMINGS_SIZE, so the count is used, otherwise the array size is
+ * used as we wrapped. The index begins from zero when we did not
+ * wrap. That could be done in a nicer way with the proper circular
+ * array structure type but with the cost of extra computation in the
+ * interrupt handler hot path. We choose efficiency.
+ */
+#define for_each_irqts(i, irqts)                                       \
+       for (i = irqts->count < IRQ_TIMINGS_SIZE ?                      \
+                    0 : irqts->count & IRQ_TIMINGS_MASK,               \
+                    irqts->count = min(IRQ_TIMINGS_SIZE,               \
+                                       irqts->count);                  \
+            irqts->count > 0; irqts->count--,                          \
+                    i = (i + 1) & IRQ_TIMINGS_MASK)
+
 struct irqt_stat {
        u64     last_ts;
        u64     ema_time[PREDICTION_BUFFER_SIZE];
@@ -528,11 +545,7 @@ u64 irq_timings_next_event(u64 now)
         * model while decrementing the counter because we consume the
         * data from our circular buffer.
         */
-
-       i = (irqts->count & IRQ_TIMINGS_MASK) - 1;
-       irqts->count = min(IRQ_TIMINGS_SIZE, irqts->count);
-
-       for (; irqts->count > 0; irqts->count--, i = (i + 1) & 
IRQ_TIMINGS_MASK) {
+       for_each_irqts(i, irqts) {
                irq = irq_timing_decode(irqts->values[i], &ts);
                s = idr_find(&irqt_stats, irq);
                if (s)
-- 
2.17.1

Reply via email to