Re: [PATCH] Fix a NO_IDLE_HZ timer bug

[Martin Schwidefsky]

On Thu, 2006-05-18 at 13:48 -0700, Zachary Amsden wrote:
> It also looks like s390 has another bug.  When compiling the 32-bit 
> kernel, doesn't this computation overflow:
> 
> arch/s390/kernel/time.c, stop_hz_timer:274
> 
>         /*
>          * This cpu is going really idle. Set up the clock comparator
>          * for the next event.
>          */
>         next = next_timer_interrupt();
>         do {
>                 seq = read_seqbegin_irqsave(&xtime_lock, flags);
>                 timer = (__u64)(next - jiffies) + jiffies_64;
>         } while (read_seqretry_irqrestore(&xtime_lock, seq, flags));
> 
> 
> Since jiffies can advance between next_timer_interrupt and the read 
> under xtime lock, next-jiffies could be negative.  I would think you 
> want to cast that to signed long instead of __u64, but I'm not totally 
> qualified to talk about s390.

Seems like you are qualified to talk about s390 in this case. The
extension of (next - jiffies) to a 64 bit value needs to be done as a
signed extension, follow by a cast to u64. Blech. I think to cast next
and jiffies to u64 before subtracting them is cleaner. It takes a few
more cycles because we now do two 64 bit adds/subtracts but the code is
used for going idle so it doesn't matter. Patch attached, thanks Zach.

-- 
blue skies,
  Martin.

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

"Reality continues to ruin my life." - Calvin.

--

From: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>

[patch] s390: next_timer_interrupt overflow in stop_hz_timer.

The 32 bit unsigned substraction (next - jiffies) in stop_hz_timer
can overflow if jiffies gets advanced between next_timer_interrupt
and the read under the xtime lock. The cast to a u64 then results
in a large value which causes the cpu to wait too long.
Fix this by casting next and jiffies independently to u64 before
subtracting them.

Signed-off-by: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
---

 arch/s390/kernel/time.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

diff -urpN linux-2.6/arch/s390/kernel/time.c linux-2.6-patched/arch/s390/kernel/time.c
--- linux-2.6/arch/s390/kernel/time.c	2006-05-16 09:44:29.000000000 +0200
+++ linux-2.6-patched/arch/s390/kernel/time.c	2006-05-19 11:04:04.000000000 +0200
@@ -272,7 +272,7 @@ static inline void stop_hz_timer(void)
 	next = next_timer_interrupt();
 	do {
 		seq = read_seqbegin_irqsave(&xtime_lock, flags);
-		timer = (__u64)(next - jiffies) + jiffies_64;
+		timer = (__u64 next) - (__u64 jiffies) + jiffies_64;
 	} while (read_seqretry_irqrestore(&xtime_lock, seq, flags));
 	todval = -1ULL;
 	/* Be careful about overflows. */


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/