[PATCH] loosing time with CPU counter timer

[PATCH] loosing time with CPU counter timer

[Jun Sun]

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


This patch fixes an ancient timer bug.

If one uses CPU counter as the system timer, it looses time
over the time.

Basically, timer_interrupt() does the following when it serves
an cpu counter interrupt (only relavent part shown);

0) interrupt happens
1) read cpu counter;
2) add it with cycles_per_jiffy, and set the value to compare
   register.

The problem is that cpu counter could increase between 0) and 1),
say by delta units.  Then the next timer interrupt is set to
t0 + delta + cycles_per_unit, instead of t0 + cycles_per_unit,
(where t0 is the current timer interrupt time)

Similar bug also exists in old-time.c, but anybody really cares? :)
Especially it has been there for quite a while ....

Jun

[-- Attachment #2: 030414.loosing-time-with-cpu-counter-timer.patch --]
[-- Type: text/plain, Size: 3851 bytes --]

diff -Nru linux/arch/mips/ddb5xxx/ddb5477/setup.c.orig linux/arch/mips/ddb5xxx/ddb5477/setup.c
--- linux/arch/mips/ddb5xxx/ddb5477/setup.c.orig	Mon Apr 14 15:28:57 2003
+++ linux/arch/mips/ddb5xxx/ddb5477/setup.c	Mon Apr 14 16:08:35 2003
@@ -43,7 +43,7 @@
 #include "lcd44780.h"
 
 
-// #define	USE_CPU_COUNTER_TIMER	/* whether we use cpu counter */
+#define	USE_CPU_COUNTER_TIMER	/* whether we use cpu counter */
 
 #define	SP_TIMER_BASE			DDB_SPT1CTRL_L
 #define	SP_TIMER_IRQ			VRC5477_IRQ_SPT1
@@ -154,10 +154,6 @@
         /* we are using the cpu counter for timer interrupts */
 	setup_irq(CPU_IRQ_BASE + 7, irq);
 
-        /* to generate the first timer interrupt */
-        count = read_c0_count();
-        write_c0_compare(count + 1000);
-
 #else
 
 	/* if we use Special purpose timer 1 */
diff -Nru linux/arch/mips/kernel/time.c.orig linux/arch/mips/kernel/time.c
--- linux/arch/mips/kernel/time.c.orig	Fri Apr 11 11:05:18 2003
+++ linux/arch/mips/kernel/time.c	Mon Apr 14 16:51:13 2003
@@ -140,6 +140,24 @@
 /* Cycle counter value at the previous timer interrupt.. */
 static unsigned int timerhi, timerlo;
 
+/* 
+ * Cycle counter value after which next timer interrupt is considered "missed".
+ * Suppose we are serving timer interrupt scheduled at time t, the theorectical 
+ * expiriing point for next interrupt is t + 2 * cycles_per_jiffy.
+ * In practice, we set it a little earlier, which is 
+ * 	t + 2 * cycles_per_jiffy - EXTRA_CUSHION_CYCLES
+ * just to make sure we still have some time to set registers after we
+ * decide whether a timer interrupt is missed.
+ */
+static unsigned int expirehi, expirelo;
+
+/* 
+ * extra "cushion" cycles used when we decide whether we have missed an
+ * timer interrupt (in the case of using cpu counter).  It should be long
+ * enough to cover at least 20 instructions.
+ */
+#define	EXTRA_CUSHION_CYCLES	50
+
 /* last time when xtime and rtc are sync'ed up */
 static long last_rtc_update;
 
@@ -330,6 +348,16 @@
  * high-level timer interrupt service routines.  This function
  * is set as irqaction->handler and is invoked through do_IRQ.
  */
+static inline int 
+64bit_compare(unsigned hi1, unsigned lo1, unsigned hi2, unsigned lo2)
+{
+	if (hi1 == hi2) {
+		return lo1 - lo2;
+	} else {
+		return hi1 - hi2;
+	}
+}
+
 void timer_interrupt(int irq, void *dev_id, struct pt_regs *regs)
 {
 	if (cpu_has_counter) {
@@ -343,14 +371,19 @@
 		timerhi += (count < timerlo);   /* Wrap around */
 		timerlo = count;
 
-		/*
-		 * set up for next timer interrupt - no harm if the machine
-		 * is using another timer interrupt source.
-		 * Note that writing to COMPARE register clears the interrupt
-		 */
-		write_c0_compare(
-					  count + cycles_per_jiffy);
-
+		/* check to see if we have missed a timer interrupt */
+		if (64bit_compare(timerhi, timerlo, expirehi, expirelo) < 0) {
+			unsigned int old_expirelo=expirelo;
+			expirelo += cycles_per_jiffy;
+			expirehi += (expirelo < old_expirelo);
+			write_c0_compare(old_expirelo + EXTRA_CUSHION_CYCLES);
+		} else {
+			// missed_timer_count ++;
+			/* we have missed at least one timer interrupt */
+			expirelo = timerlo + cycles_per_jiffy*2 - EXTRA_CUSHION_CYCLES;
+			expirehi = timerhi + (expirelo < timerlo);
+			write_c0_compare(timerlo + cycles_per_jiffy);
+		}
 	}
 
 	/*
@@ -504,8 +537,6 @@
 
 	/* caclulate cache parameters */
 	if (mips_counter_frequency) {
-		u32 count;
-
 		cycles_per_jiffy = mips_counter_frequency / HZ;
 
 		/* sll32_usecs_per_cycle = 10^6 * 2^32 / mips_counter_freq */
@@ -518,9 +549,9 @@
 		 * For those using cpu counter as timer,  this sets up the
 		 * first interrupt
 		 */
-		count = read_c0_count();
-		write_c0_compare(
-					  count + cycles_per_jiffy);
+		write_c0_compare(cycles_per_jiffy);
+		write_c0_count(0);
+		expirelo = cycles_per_jiffy * 2 - EXTRA_CUSHION_CYCLES;
 	}
 
 	/*

Re: [PATCH] loosing time with CPU counter timer

[Juan Quintela]

>>>>> "jun" == Jun Sun <jsun@mvista.com> writes:

Hi
        only aesthetical fixes:

jun> diff -Nru linux/arch/mips/ddb5xxx/ddb5477/setup.c.orig linux/arch/mips/ddb5xxx/ddb5477/setup.c
jun> --- linux/arch/mips/ddb5xxx/ddb5477/setup.c.orig	Mon Apr 14 15:28:57 2003
jun> +++ linux/arch/mips/ddb5xxx/ddb5477/setup.c	Mon Apr 14 16:08:35 2003
jun> @@ -330,6 +348,16 @@
jun> * high-level timer interrupt service routines.  This function
jun> * is set as irqaction->handler and is invoked through do_IRQ.
jun> */
jun> +static inline int 
jun> +64bit_compare(unsigned hi1, unsigned lo1, unsigned hi2, unsigned lo2)

Please, use "unsigned int" /me notices that file is not consistent, in
some places it uses "unsigned" and in other "unsigned int".

jun> +{
jun> +	if (hi1 == hi2) {
jun> +		return lo1 - lo2;
jun> +	} else {
jun> +		return hi1 - hi2;
jun> +	}
jun> +}

Ralf tax on braces apply here.

jun> +		/* check to see if we have missed a timer interrupt */
jun> +		if (64bit_compare(timerhi, timerlo, expirehi, expirelo) < 0) {
jun> +			unsigned int old_expirelo=expirelo;
jun> +			expirelo += cycles_per_jiffy;
jun> +			expirehi += (expirelo < old_expirelo);

Tax on parens :)

Later, Juan.

-- 
In theory, practice and theory are the same, but in practice they 
are different -- Larry McVoy

Re: [PATCH] loosing time with CPU counter timer

[Jun Sun]

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

On Mon, Apr 14, 2003 at 05:16:55PM -0700, Jun Sun wrote:
> 
> This patch fixes an ancient timer bug.
> 
> If one uses CPU counter as the system timer, it looses time
> over the time.
> 
> Basically, timer_interrupt() does the following when it serves
> an cpu counter interrupt (only relavent part shown);
> 
> 0) interrupt happens
> 1) read cpu counter;
> 2) add it with cycles_per_jiffy, and set the value to compare
>    register.
> 
> The problem is that cpu counter could increase between 0) and 1),
> say by delta units.  Then the next timer interrupt is set to
> t0 + delta + cycles_per_unit, instead of t0 + cycles_per_unit,
> (where t0 is the current timer interrupt time)
> 
> Similar bug also exists in old-time.c, but anybody really cares? :)
> Especially it has been there for quite a while ....
> 
<snip>

After a refreshing weekend I realize there exists a much more
elegant fix for the problem.  :) See the patch attached.

Unfortuately I have already checked in the not-so-elegant fix.
So the actual patch to check in this time will be diff between 
this one and the previous patch.

Comments?

Jun


[-- Attachment #2: junk1 --]
[-- Type: text/plain, Size: 2028 bytes --]

diff -u arch/mips/kernel/time.c ./arch/mips/kernel/time.c
--- arch/mips/kernel/time.c	17 Apr 2003 22:50:37 -0000
+++ ./arch/mips/kernel/time.c	Mon Apr 21 14:02:06 2003
@@ -140,6 +140,9 @@
 /* Cycle counter value at the previous timer interrupt.. */
 static unsigned int timerhi, timerlo;
 
+/* expirelo is the count value for next CPU timer interrupt */
+static unsigned int expirelo;
+
 /* last time when xtime and rtc are sync'ed up */
 static long last_rtc_update;
 
@@ -335,22 +338,21 @@
 	if (cpu_has_counter) {
 		unsigned int count;
 
-		/*
-		 * The cycle counter is only 32 bit which is good for about
-		 * a minute at current count rates of upto 150MHz or so.
-		 */
+		/* ack timer interrupt, and try to set next interrupt */
+		expirelo += cycles_per_jiffy;
+		write_c0_compare(expirelo);
 		count = read_c0_count();
-		timerhi += (count < timerlo);   /* Wrap around */
-		timerlo = count;
 
-		/*
-		 * set up for next timer interrupt - no harm if the machine
-		 * is using another timer interrupt source.
-		 * Note that writing to COMPARE register clears the interrupt
-		 */
-		write_c0_compare(
-					  count + cycles_per_jiffy);
+		/* check to see if we have missed any timer interrupts */
+		if ((count - expirelo) < 0x7fffffff) {
+			/* missed_timer_count ++; */
+			expirelo = count + cycles_per_jiffy;
+			write_c0_compare(expirelo);
+		}
 
+		/* Update timerhi/timerlo for intra-jiffy calibration. */
+		timerhi += count < timerlo;	/* Wrap around */
+		timerlo = count;
 	}
 
 	/*
@@ -504,8 +506,6 @@
 
 	/* caclulate cache parameters */
 	if (mips_counter_frequency) {
-		u32 count;
-
 		cycles_per_jiffy = mips_counter_frequency / HZ;
 
 		/* sll32_usecs_per_cycle = 10^6 * 2^32 / mips_counter_freq */
@@ -518,9 +518,9 @@
 		 * For those using cpu counter as timer,  this sets up the
 		 * first interrupt
 		 */
-		count = read_c0_count();
-		write_c0_compare(
-					  count + cycles_per_jiffy);
+		write_c0_compare(cycles_per_jiffy);
+		write_c0_count(0);
+		expirelo = cycles_per_jiffy;
 	}
 
 	/*