[PATCH] next_timer_interrupt: simpler overflow handling

[PATCH] next_timer_interrupt: simpler overflow handling

[Keir Fraser]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

Having seen the patch applied to 2.6.17 to fix the overflowing 
comparison in next_timer_interrupt() it occurred to me that a much 
simpler fix is to not set hr_expires to MAX_JIFFY_OFFSET. It's way 
further out from jiffies than necessary, which is why it's caused 
problems. I instead propose that we initialise it to LONG_MAX>>1, just 
as we already do for the non-hr expires variable. This will allow safe 
comparison with any timer value in the range jiffies+/-(LONG_MAX>>1) 
which is plenty of range around jiffies (+/- 12 days if HZ=1000 and 
long is 32 bits).

The advantages are simpler code, and uniform initialisation of expires 
and hr_expires variables.

  -- Keir

Replace a fix for a comparison overflow in next_timer_interrupt() with 
a simpler alternative. We can be sure that the interesting range of 
timer values around jiffies is safe to compare with 
jiffies+(LONG_MAX>>1), unlike jiffies+MAX_JIFFY_OFFSET.

Signed-off-by: Keir Fraser <keir@xensource.com>


[-- Attachment #2: fix-next-timer-interrupt.patch --]
[-- Type: application/octet-stream, Size: 1281 bytes --]

diff -urp linux-2.6.18-rc1-git4/kernel/timer.c linux-2.6.18-rc1-git4-new/kernel/timer.c
--- linux-2.6.18-rc1-git4/kernel/timer.c	2006-07-12 11:24:21.287626294 +0100
+++ linux-2.6.18-rc1-git4-new/kernel/timer.c	2006-07-12 11:40:30.327700658 +0100
@@ -471,7 +471,7 @@ unsigned long next_timer_interrupt(void)
 	struct list_head *list;
 	struct timer_list *nte;
 	unsigned long expires;
-	unsigned long hr_expires = MAX_JIFFY_OFFSET;
+	unsigned long hr_expires = LONG_MAX >> 1;
 	ktime_t hr_delta;
 	tvec_t *varray[4];
 	int i, j;
@@ -537,22 +537,6 @@ found:
 	}
 	spin_unlock(&base->lock);
 
-	/*
-	 * It can happen that other CPUs service timer IRQs and increment
-	 * jiffies, but we have not yet got a local timer tick to process
-	 * the timer wheels.  In that case, the expiry time can be before
-	 * jiffies, but since the high-resolution timer here is relative to
-	 * jiffies, the default expression when high-resolution timers are
-	 * not active,
-	 *
-	 *   time_before(MAX_JIFFY_OFFSET + jiffies, expires)
-	 *
-	 * would falsely evaluate to true.  If that is the case, just
-	 * return jiffies so that we can immediately fire the local timer
-	 */
-	if (time_before(expires, jiffies))
-		return jiffies;
-
 	if (time_before(hr_expires, expires))
 		return hr_expires;
 

Re: [PATCH] next_timer_interrupt: simpler overflow handling

[Zachary Amsden]

Keir Fraser wrote:
> Having seen the patch applied to 2.6.17 to fix the overflowing 
> comparison in next_timer_interrupt() it occurred to me that a much 
> simpler fix is to not set hr_expires to MAX_JIFFY_OFFSET. It's way 
> further out from jiffies than necessary, which is why it's caused 
> problems. I instead propose that we initialise it to LONG_MAX>>1, just 
> as we already do for the non-hr expires variable. This will allow safe 
> comparison with any timer value in the range jiffies+/-(LONG_MAX>>1) 
> which is plenty of range around jiffies (+/- 12 days if HZ=1000 and 
> long is 32 bits).
>
> The advantages are simpler code, and uniform initialisation of expires 
> and hr_expires variables.

Even simpler would be to just make MAX_JIFFY_OFFSET be (LONG_MAX >> 1) 
and use this for both.  In fact, it appears it used to be, judging by 
the comment in jiffies.h:

 * The maximum jiffie value is (MAX_INT >> 1).  Here we translate that

But seeing as this could have unanticipated side effects, I like this 
fix better.

Acked-By: Zachary Amsden <zach@vmware.com>