[parisc-linux] [PATCH] timer_interrupt and gettimeoffset.

[James Bottomley]

On Wed, 2006-08-30 at 16:23 -0400, Carlos O'Donell wrote:
> It actaully turns out I don't think I ever booted this patch, my
> palo.conf was hosed and I was writing the wrong kernel. It was too
> good to be true :)
> 
> I'll have a go at testing this again tonight.

Actually, according to my analysis on ioz (pa8800) there seem to be some
hidden issues with our implementation (i.e. it's not the mathematics).

The first problem is that interrupts are re-entrant, so the timer
interrupt can get re-interrupted.  If this happens between the mfctl(16)
and the mtctl(), which is made much longer by the use of while loops,
then there's a small possibility that the interrupt caused us to miss
the next tick (i.e. cr16 moved beyond next_tick while in the interrupt).
I see this very occasionally on the pa8800 caused by flush IPIs (since
the cache is so huge) ... it's probably caused by SCSI interrupts on the
C3xxx that everyone else is testing with.  However, when this happens,
you have to wait for cr16 to wrap before you get another timer
interrupt, which I believe to be the source of the time jumps and
negative offsets in gettimeoffset().

My proposed fix for this is below.  However, we seem to have a few other
issues:

     1. On SMP, cr16 of the secondary processors (and next_tick) is
        never initialised ... we just wait for the timer to wrap and
        then pick up ticking from there.
     2. processor_probe() blows away all of the next_tick data when it's
        called (once for every CPU)
     3. We're regularly missing multiple ticks ... mainly below about
        30 .. there must be some cause for this but I can't immediately
        find it.
     4. we don't obey CONFIG_HZ at all the clock is always either 1000
        for pa2.0 or 100 for pa1.0

James

diff --git a/arch/parisc/kernel/time.c b/arch/parisc/kernel/time.c
index 5facc9b..93322a2 100644
--- a/arch/parisc/kernel/time.c
+++ b/arch/parisc/kernel/time.c
@@ -48,9 +48,13 @@ irqreturn_t timer_interrupt(int irq, voi
 	long next_tick;
 	int nticks;
 	int cpu = smp_processor_id();
+	unsigned long flags;
 
 	profile_tick(CPU_PROFILING, regs);
 
+	/* Don't want to be interrupted while calculating
+	 * time offsets */
+	local_irq_save(flags);
 	now = mfctl(16);
 	/* initialize next_tick to time at last clocktick */
 	next_tick = cpu_data[cpu].it_value;
@@ -63,13 +67,24 @@ irqreturn_t timer_interrupt(int irq, voi
 	 * Variables are *signed*.
 	 */
 
-	nticks = 0;
-	while((next_tick - now) < halftick) {
+	/* Don't do expensive mul and div for the likely case */
+	if (likely(now - next_tick < clocktick)) {
+		nticks = 1;
 		next_tick += clocktick;
+	} else {
+		nticks = ((now - next_tick)/clocktick) + 1;
+		next_tick += clocktick*nticks;
+	}
+	/* Don't interrupt too much.  If we only have half
+	 * the time to go to the next tick, push it out one
+	 * more tick */
+	if (unlikely(next_tick - now < halftick)) {
 		nticks++;
+		next_tick += clocktick;
 	}
 	mtctl(next_tick, 16);
 	cpu_data[cpu].it_value = next_tick;
+	local_irq_restore(flags);
 
 	while (nticks--) {
 #ifdef CONFIG_SMP