* [parisc-linux] [PATCH] timer_interrupt and gettimeoffset.
@ 2006-08-30 4:48 Carlos O'Donell
2006-08-31 6:53 ` Grant Grundler
0 siblings, 1 reply; 14+ messages in thread
From: Carlos O'Donell @ 2006-08-30 4:48 UTC (permalink / raw)
To: parisc-linux, willy, James Bottomley
[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]
A review of 'arch/parisc/kernel/time.c' and in particular
'timer_interrupt' and 'gettimeoffset' show that this code has a
couple of design flaws.
1. cr16 is treated as signed.
2. No attempt is made to handle cr16 wrapping.
3. jiffies vs. wall_jiffies adjustment is incorrect.
I have rewritten 'timer_interrupt'.
A. cr16 is treated as an unsigned long.
B. The following 3 scenarios exist.
1. The timer interrupt fires, and 'now' comes after 'next_tick'
2. The timer interrupt fires, and 'now' has wrapped and is before 'next_tick'
3. The timer interrupt fires, and 'now' is *just* before 'next_tick'
In theory 99% of the time we should be in scenario 1. On occasion we
may miss a timer interrupt, end up close to wrapping, and we get
scenario 2. Scenario 3 is just plain wrong and for now I will BUG() if
the timer fires before we intended.
I have adjusted 'gettimeoffset' in the following fashion:
A. cr16 is treated as an unsigned long.
B. Never ever ever return a negative adjustment.
C. (jiffies - wall_jiffies) adjustment is always positive and added to usec.
D. The 3 timer scenarios (same as above), exist.
I assert that 'gettimeoffset' should never return a negative value. It
represents the postive adjustment accounting for the fact that we are
*part* of the way through a tick.
The patch is attached. Please review. I booted this on my a500 without
any problems. I don't really know how to look for problems so I ran
the glibc testsuite and didn't get any extra regressions.
The real test will be to run this on a 715/50 and look for Guy's
reported problems. Guy, would you mind patching your kernel with this
patch and testing again?
Cheers,
Carlos.
[-- Attachment #2: patch-2006-08-29-time.diff --]
[-- Type: text/x-patch, Size: 5421 bytes --]
diff --git a/arch/parisc/kernel/time.c b/arch/parisc/kernel/time.c
index 5facc9b..ce7dc5e 100644
--- a/arch/parisc/kernel/time.c
+++ b/arch/parisc/kernel/time.c
@@ -44,32 +44,57 @@ #endif
irqreturn_t timer_interrupt(int irq, void *dev_id, struct pt_regs *regs)
{
- long now;
- long next_tick;
- int nticks;
+ unsigned long now;
+ unsigned long next_tick;
+ unsigned long next_next_tick;
+ unsigned long difference;
+ unsigned long remainder;
+ int nticks = 0;
int cpu = smp_processor_id();
profile_tick(CPU_PROFILING, regs);
- now = mfctl(16);
- /* initialize next_tick to time at last clocktick */
+ /* Initialize next_tick to the expected tick time. */
next_tick = cpu_data[cpu].it_value;
+ next_next_tick = next_tick + clocktick;
- /* since time passes between the interrupt and the mfctl()
- * above, it is never true that last_tick + clocktick == now. If we
- * never miss a clocktick, we could set next_tick = last_tick + clocktick
- * but maybe we'll miss ticks, hence the loop.
- *
- * Variables are *signed*.
- */
+ /* Get current interval timer reading, reads as 64/32 bit
+ value depending if you have a 64/32 bit machine. */
+ now = mfctl(16);
- nticks = 0;
- while((next_tick - now) < halftick) {
- next_tick += clocktick;
+ /* Determine how much time elapsed. */
+ if (now > next_tick) {
+ /* Scenario 1: "now" is late. */
+ nticks = (now - next_tick) / clocktick;
+ remainder = (now - next_tick) % clocktick;
+ }
+ else {
+ /* "now" is either early or cr16 wrapped. */
+ if (next_next_tick < next_tick) {
+ /* Scenario 2: A missed clock tick would wrap. */
+ difference = ~0UL - (next_tick - now);
+ nticks = difference / clocktick;
+ remainder = difference % clocktick;
+ } else {
+ /* Scenario 3: We didn't miss a clock tick.
+ "now" is early? */
+ printk ("timer_interrupt: Now is early, now = %lx, next_tick = %lx, clocktick = %lx\n", now, next_tick, clocktick);
+ BUG ();
+ }
+ }
+
+ /* Check the remainder to see how far we are into the next tick? */
+ if (remainder > halftick) {
+ /* More than 1/2 way in? Count a tick. */
nticks++;
}
- mtctl(next_tick, 16);
- cpu_data[cpu].it_value = next_tick;
+
+ /* Add any full ticks that have elapsed. */
+ next_next_tick += nticks * clocktick;
+
+ /* Only bottom 32-bits of next_tick are written to cr16. */
+ mtctl(next_next_tick, 16);
+ cpu_data[cpu].it_value = next_next_tick;
while (nticks--) {
#ifdef CONFIG_SMP
@@ -124,21 +149,47 @@ #ifndef CONFIG_SMP
* Once parisc-linux learns the cr16 difference between processors,
* this could be made to work.
*/
- long last_tick;
- long elapsed_cycles;
+ unsigned long now;
+ unsigned long last_tick;
+ unsigned long next_tick;
+ unsigned long next_next_tick;
+ unsigned long elapsed_cycles;
+ unsigned long usec;
+ unsigned long lost;
/* it_value is the intended time of the next tick */
- last_tick = cpu_data[smp_processor_id()].it_value;
+ next_tick = cpu_data[smp_processor_id()].it_value;
+ next_next_tick = next_tick + clocktick;
- /* Subtract one tick and account for possible difference between
- * when we expected the tick and when it actually arrived.
- * (aka wall vs real)
- */
- last_tick -= clocktick * (jiffies - wall_jiffies + 1);
- elapsed_cycles = mfctl(16) - last_tick;
+ /* This represents lost jiffies. */
+ lost = clocktick * (jiffies - wall_jiffies);
+
+ /* We roll back 1 tick. */
+ last_tick = next_tick - clocktick;
+
+ /* Read the hardware interval timer. */
+ now = mfctl(16);
- /* the precision of this math could be improved */
- return elapsed_cycles / (PAGE0->mem_10msec / 10000);
+ if (now > last_tick) {
+ /* Scenario 1: "now" is later than last_tick. */
+ elapsed_cycles = now - last_tick;
+ } else {
+ /* "now" is either early or cr16 wrapped. */
+ if (next_next_tick < last_tick) {
+ /* Scenario 2: A missed clock tick would wrap. */
+ elapsed_cycles = ~0UL - (last_tick - now);
+ } else {
+ /* Scenario 3: We didn't miss a clock tick.
+ "now" is early? */
+ printk ("gettimeoffset: Now is early, now = %lx, last_tick = %lx, next_tick = %lx, clocktick = %lx\n", now, last_tick, next_tick, clocktick);
+ BUG ();
+ }
+ }
+
+ /* FIXME: Improve precision. */
+ usec = elapsed_cycles * 10000 / PAGE0->mem_10msec;
+ usec += lost;
+ return usec;
#else
return 0;
#endif
@@ -149,6 +200,7 @@ do_gettimeofday (struct timeval *tv)
{
unsigned long flags, seq, usec, sec;
+ /* Hold xtime_lock and adjust timeval. */
do {
seq = read_seqbegin_irqsave(&xtime_lock, flags);
usec = gettimeoffset();
@@ -156,25 +208,13 @@ do_gettimeofday (struct timeval *tv)
usec += (xtime.tv_nsec / 1000);
} while (read_seqretry_irqrestore(&xtime_lock, seq, flags));
- if (unlikely(usec > LONG_MAX)) {
- /* This can happen if the gettimeoffset adjustment is
- * negative and xtime.tv_nsec is smaller than the
- * adjustment */
- printk(KERN_ERR "do_gettimeofday() spurious xtime.tv_nsec of %ld\n", usec);
- usec += USEC_PER_SEC;
- --sec;
- /* This should never happen, it means the negative
- * time adjustment was more than a second, so there's
- * something seriously wrong */
- BUG_ON(usec > LONG_MAX);
- }
-
-
+ /* Move adjusted usec's into sec's. */
while (usec >= USEC_PER_SEC) {
usec -= USEC_PER_SEC;
++sec;
}
+ /* Return adjusted result. */
tv->tv_sec = sec;
tv->tv_usec = usec;
}
[-- Attachment #3: Type: text/plain, Size: 169 bytes --]
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [parisc-linux] [PATCH] timer_interrupt and gettimeoffset.
2006-08-30 4:48 [parisc-linux] [PATCH] timer_interrupt and gettimeoffset Carlos O'Donell
@ 2006-08-31 6:53 ` Grant Grundler
2006-08-31 18:52 ` Carlos O'Donell
2006-09-01 22:48 ` Grant Grundler
0 siblings, 2 replies; 14+ messages in thread
From: Grant Grundler @ 2006-08-31 6:53 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: willy, James Bottomley, parisc-linux
On Wed, Aug 30, 2006 at 12:48:43AM -0400, Carlos O'Donell wrote:
> A review of 'arch/parisc/kernel/time.c' and in particular
> 'timer_interrupt' and 'gettimeoffset' show that this code has a
> couple of design flaws.
>
> 1. cr16 is treated as signed.
This was by design. Paul Bame (IIRC) wrote this code and
while I was never that comfortable with it, I could never
find anything wrong with it either. Randolph Chung and I
stared at timer_interrupt() _alot_.
> 2. No attempt is made to handle cr16 wrapping.
The signed math was supposed to handle it.
I believe it did but forgot details since looking at
it a few years ago.
I personally prefer to NOT use signed math in this case.
But that's honestly not a great reason to change this code.
I'd really prefer some evidence the kernel code is wrong.
> 3. jiffies vs. wall_jiffies adjustment is incorrect.
>
> I have rewritten 'timer_interrupt'.
And I've hacked your version some more.
diff is below.
I've included your changes to gettimeoffset() in my patch below
and will review those next.
And something is certainly wrong with my version of the code.
It seems to be running the system clock at ~ 1/2 speed now. :)
odine:~# while : ; do sleep 10 ; ntpdate pool.ntp.org ; done
30 Aug 23:51:13 ntpdate[14051]: step time server 203.217.30.156 offset 60.165510 sec
30 Aug 23:51:39 ntpdate[14053]: step time server 203.217.30.156 offset 13.065388 sec
30 Aug 23:52:04 ntpdate[14055]: step time server 195.234.188.26 offset 11.578574 sec
30 Aug 23:52:31 ntpdate[14058]: step time server 203.217.30.156 offset 13.954358 sec
...
> A. cr16 is treated as an unsigned long.
> B. The following 3 scenarios exist.
>
> 1. The timer interrupt fires, and 'now' comes after 'next_tick'
This is the "normal" case I think. 'now' should always lag behind
'next_tick' and how much depends on how long interrupts are blocked
on that CPU.
> 2. The timer interrupt fires, and 'now' has wrapped and is before
> 'next_tick'
> 3. The timer interrupt fires, and 'now' is *just* before 'next_tick'
>
> In theory 99% of the time we should be in scenario 1. On occasion we
> may miss a timer interrupt, end up close to wrapping, and we get
> scenario 2.
I'll assert we don't need to miss a timer interrupt to hit Scenario 2.
All we need is a timer interrupt that scheduled near 4GB (but below)
and 'normal' lag in interrupt delivery/handling will allow the
CR16 to wrap.
> Scenario 3 is just plain wrong and for now I will BUG() if
> the timer fires before we intended.
I agree.
...
> I assert that 'gettimeoffset' should never return a negative value. It
> represents the postive adjustment accounting for the fact that we are
> *part* of the way through a tick.
I have to think about this more. But I wonder if how "halftick" is
handled in timer_interrupt does not play well with this concept.
> The patch is attached. Please review. I booted this on my a500 without
> any problems. I don't really know how to look for problems so I ran
> the glibc testsuite and didn't get any extra regressions.
I've only booted on a500 (64-bit kernel) and will try on b2600
(32-bit kernel) next.
Please review and see if I added any new bugs.
Note that I replaced the "~0UL - XXX" with "~ XXX".
I'll be off by one cycle. I don't care.
thanks,
grant
diff --git a/arch/parisc/kernel/time.c b/arch/parisc/kernel/time.c
index 5facc9b..9f6cf58 100644
--- a/arch/parisc/kernel/time.c
+++ b/arch/parisc/kernel/time.c
@@ -44,34 +44,58 @@ #endif
irqreturn_t timer_interrupt(int irq, void *dev_id, struct pt_regs *regs)
{
- long now;
- long next_tick;
- int nticks;
+ unsigned long now;
+ unsigned long next_tick;
+ unsigned long remainder;
+ unsigned long ticks_elapsed = 0;
int cpu = smp_processor_id();
profile_tick(CPU_PROFILING, regs);
- now = mfctl(16);
- /* initialize next_tick to time at last clocktick */
+ /* Initialize next_tick to the expected tick time. */
next_tick = cpu_data[cpu].it_value;
- /* since time passes between the interrupt and the mfctl()
- * above, it is never true that last_tick + clocktick == now. If we
- * never miss a clocktick, we could set next_tick = last_tick + clocktick
- * but maybe we'll miss ticks, hence the loop.
- *
- * Variables are *signed*.
+ /* Get current interval timer.
+ * CR16 reads as 64 or 32 bit value depending CPU wide/narrow mode.
*/
+ now = mfctl(16);
+
+ /* Determine how much time elapsed. */
+ if (now > next_tick) {
+ /* Scenario 1: "now" is late. A bit late is normal. */
+ ticks_elapsed = (now - next_tick) / clocktick;
+ remainder = now - (ticks_elapsed * clocktick);
+ }
+ else {
+ /* "now" is either early or cr16 wrapped. */
+ if (~next_tick < clocktick) {
+ unsigned long difference;
+
+ /* Scenario 2: A missed clock tick would wrap. */
+ difference = ~0UL - (next_tick - now);
+ ticks_elapsed = difference / clocktick;
+ remainder = difference % clocktick;
+ } else {
+ /* Scenario 3: We didn't miss a clock tick.
+ "now" is early? */
+ printk ("timer_interrupt: Now is early, now = %lx, next_tick = %lx, clocktick = %lx\n", now, next_tick, clocktick);
+ BUG ();
+ }
+ }
- nticks = 0;
- while((next_tick - now) < halftick) {
- next_tick += clocktick;
- nticks++;
+ if (remainder > halftick) {
+ /* More than 1/2 way into the next tick. It counts. */
+ ticks_elapsed++;
}
+
+ /* Add any full ticks that have elapsed plus the one we want next. */
+ next_tick += (ticks_elapsed + 1) * clocktick;
+
+ /* Only bottom 32-bits of next_tick are written to cr16. */
mtctl(next_tick, 16);
cpu_data[cpu].it_value = next_tick;
- while (nticks--) {
+ while (ticks_elapsed--) {
#ifdef CONFIG_SMP
smp_do_timer(regs);
#else
@@ -124,21 +148,47 @@ #ifndef CONFIG_SMP
* Once parisc-linux learns the cr16 difference between processors,
* this could be made to work.
*/
- long last_tick;
- long elapsed_cycles;
+ unsigned long now;
+ unsigned long last_tick;
+ unsigned long next_tick;
+ unsigned long next_next_tick;
+ unsigned long elapsed_cycles;
+ unsigned long usec;
+ unsigned long lost;
/* it_value is the intended time of the next tick */
- last_tick = cpu_data[smp_processor_id()].it_value;
+ next_tick = cpu_data[smp_processor_id()].it_value;
+ next_next_tick = next_tick + clocktick;
- /* Subtract one tick and account for possible difference between
- * when we expected the tick and when it actually arrived.
- * (aka wall vs real)
- */
- last_tick -= clocktick * (jiffies - wall_jiffies + 1);
- elapsed_cycles = mfctl(16) - last_tick;
+ /* This represents lost jiffies. */
+ lost = clocktick * (jiffies - wall_jiffies);
+
+ /* We roll back 1 tick. */
+ last_tick = next_tick - clocktick;
+
+ /* Read the hardware interval timer. */
+ now = mfctl(16);
+
+ if (now > last_tick) {
+ /* Scenario 1: "now" is later than last_tick. */
+ elapsed_cycles = now - last_tick;
+ } else {
+ /* "now" is either early or cr16 wrapped. */
+ if (next_next_tick < last_tick) {
+ /* Scenario 2: A missed clock tick would wrap. */
+ elapsed_cycles = ~0UL - (last_tick - now);
+ } else {
+ /* Scenario 3: We didn't miss a clock tick.
+ "now" is early? */
+ printk ("gettimeoffset: Now is early, now = %lx, last_tick = %lx, next_tick = %lx, clocktick = %lx\n", now, last_tick, next_tick, clocktick);
+ BUG ();
+ }
+ }
- /* the precision of this math could be improved */
- return elapsed_cycles / (PAGE0->mem_10msec / 10000);
+ /* FIXME: Improve precision. */
+ usec = elapsed_cycles * 10000 / PAGE0->mem_10msec;
+ usec += lost;
+ return usec;
#else
return 0;
#endif
@@ -149,6 +199,7 @@ do_gettimeofday (struct timeval *tv)
{
unsigned long flags, seq, usec, sec;
+ /* Hold xtime_lock and adjust timeval. */
do {
seq = read_seqbegin_irqsave(&xtime_lock, flags);
usec = gettimeoffset();
@@ -156,25 +207,13 @@ do_gettimeofday (struct timeval *tv)
usec += (xtime.tv_nsec / 1000);
} while (read_seqretry_irqrestore(&xtime_lock, seq, flags));
- if (unlikely(usec > LONG_MAX)) {
- /* This can happen if the gettimeoffset adjustment is
- * negative and xtime.tv_nsec is smaller than the
- * adjustment */
- printk(KERN_ERR "do_gettimeofday() spurious xtime.tv_nsec of %ld\n", usec);
- usec += USEC_PER_SEC;
- --sec;
- /* This should never happen, it means the negative
- * time adjustment was more than a second, so there's
- * something seriously wrong */
- BUG_ON(usec > LONG_MAX);
- }
-
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [parisc-linux] [PATCH] timer_interrupt and gettimeoffset.
2006-08-31 6:53 ` Grant Grundler
@ 2006-08-31 18:52 ` Carlos O'Donell
2006-08-31 21:46 ` Grant Grundler
2006-09-01 22:48 ` Grant Grundler
1 sibling, 1 reply; 14+ messages in thread
From: Carlos O'Donell @ 2006-08-31 18:52 UTC (permalink / raw)
To: Grant Grundler; +Cc: willy, James Bottomley, parisc-linux
On 8/31/06, Grant Grundler <grundler@parisc-linux.org> wrote:
> > 1. cr16 is treated as signed.
>
> This was by design. Paul Bame (IIRC) wrote this code and
> while I was never that comfortable with it, I could never
> find anything wrong with it either. Randolph Chung and I
> stared at timer_interrupt() _alot_.
It's twice as smart as I am, therefore I am four times too dumb to
maintain this code. My aim was ti simplify this code, because when the
new timer infrastructure goes into place it will be crucial that we
handle the wrapping right.
> > 2. No attempt is made to handle cr16 wrapping.
>
> The signed math was supposed to handle it.
> I believe it did but forgot details since looking at
> it a few years ago.
That is *even* worse, signed arithmetic overflow is compiler
implementation dependant. It's also therefore not very nice C code.
The overflow case is in the middle of the 32-bit range now, where you
had a very large positive number which wraps to a very large negative
number. The gap is almost the entire 32-bit range, or 27 seconds on a
160Mhz box as we have shown already.
> I personally prefer to NOT use signed math in this case.
> But that's honestly not a great reason to change this code.
> I'd really prefer some evidence the kernel code is wrong.
I agree, this patch was an attempt to provide an alternate
implementation that made sense and was maintainer friendly. I believe
we should be able to get this working better and handle the failures
seen on the slower boxes.
> And I've hacked your version some more.
> diff is below.
Thanks.
> I've included your changes to gettimeoffset() in my patch below
> and will review those next.
Ok.
> And something is certainly wrong with my version of the code.
> It seems to be running the system clock at ~ 1/2 speed now. :)
>
> odine:~# while : ; do sleep 10 ; ntpdate pool.ntp.org ; done
> 30 Aug 23:51:13 ntpdate[14051]: step time server 203.217.30.156 offset 60.165510 sec
> 30 Aug 23:51:39 ntpdate[14053]: step time server 203.217.30.156 offset 13.065388 sec
> 30 Aug 23:52:04 ntpdate[14055]: step time server 195.234.188.26 offset 11.578574 sec
> 30 Aug 23:52:31 ntpdate[14058]: step time server 203.217.30.156 offset 13.954358 sec
> ...
Technically it is implementation dependant, and can be as low as 1/2
the peak instruction rate. In correcting timer_interrupt, I remember
removing what I thought was a spurious 'clocktick' addition in the
implementation. Does cr16 *actuall* count at the instruction rate, or
does it count at 1/2 rate?
> > A. cr16 is treated as an unsigned long.
> > B. The following 3 scenarios exist.
> >
> > 1. The timer interrupt fires, and 'now' comes after 'next_tick'
>
> This is the "normal" case I think. 'now' should always lag behind
> 'next_tick' and how much depends on how long interrupts are blocked
> on that CPU.
Sounds good.
> > 2. The timer interrupt fires, and 'now' has wrapped and is before
> > 'next_tick'
> > 3. The timer interrupt fires, and 'now' is *just* before 'next_tick'
> >
> > In theory 99% of the time we should be in scenario 1. On occasion we
> > may miss a timer interrupt, end up close to wrapping, and we get
> > scenario 2.
>
> I'll assert we don't need to miss a timer interrupt to hit Scenario 2.
> All we need is a timer interrupt that scheduled near 4GB (but below)
> and 'normal' lag in interrupt delivery/handling will allow the
> CR16 to wrap.
A timer interrupt that is scheduled near the 4GB boundary is what I
meant by "end up close to wrapping." You are right that interrupt
delivery/handling lag might mean cr16 would wrap without causing a
missed interrupt.
> > Scenario 3 is just plain wrong and for now I will BUG() if
> > the timer fires before we intended.
>
> I agree.
I think the detection of Scenario 3 is wrong in your patch, I'll
epxlain further down in the code.
> ...
> > I assert that 'gettimeoffset' should never return a negative value. It
> > represents the postive adjustment accounting for the fact that we are
> > *part* of the way through a tick.
>
> I have to think about this more. But I wonder if how "halftick" is
> handled in timer_interrupt does not play well with this concept.
There is a bug here because of this, you are right to be warry of the
halftick adjustment. It is also needed in gettimeoffset, or "now" will
appear before "next_tick" without wrapping.
> Please review and see if I added any new bugs.
There is 1 addtional bug.
> Note that I replaced the "~0UL - XXX" with "~ XXX".
> I'll be off by one cycle. I don't care.
>
> thanks,
> grant
>
>
> diff --git a/arch/parisc/kernel/time.c b/arch/parisc/kernel/time.c
> index 5facc9b..9f6cf58 100644
> --- a/arch/parisc/kernel/time.c
> +++ b/arch/parisc/kernel/time.c
> @@ -44,34 +44,58 @@ #endif
>
> irqreturn_t timer_interrupt(int irq, void *dev_id, struct pt_regs *regs)
> {
> - long now;
> - long next_tick;
> - int nticks;
> + unsigned long now;
> + unsigned long next_tick;
> + unsigned long remainder;
> + unsigned long ticks_elapsed = 0;
> int cpu = smp_processor_id();
>
> profile_tick(CPU_PROFILING, regs);
>
> - now = mfctl(16);
> - /* initialize next_tick to time at last clocktick */
> + /* Initialize next_tick to the expected tick time. */
> next_tick = cpu_data[cpu].it_value;
>
> - /* since time passes between the interrupt and the mfctl()
> - * above, it is never true that last_tick + clocktick == now. If we
> - * never miss a clocktick, we could set next_tick = last_tick + clocktick
> - * but maybe we'll miss ticks, hence the loop.
> - *
> - * Variables are *signed*.
> + /* Get current interval timer.
> + * CR16 reads as 64 or 32 bit value depending CPU wide/narrow mode.
> */
> + now = mfctl(16);
> +
> + /* Determine how much time elapsed. */
> + if (now > next_tick) {
> + /* Scenario 1: "now" is late. A bit late is normal. */
> + ticks_elapsed = (now - next_tick) / clocktick;
> + remainder = now - (ticks_elapsed * clocktick);
What have you got against modulo? The correct math is as follows.
remainder = now - next_tick - ticks_elapsed * clocktick;
> + }
> + else {
> + /* "now" is either early or cr16 wrapped. */
> + if (~next_tick < clocktick) {
Too clever for me :)
> + unsigned long difference;
> +
> + /* Scenario 2: A missed clock tick would wrap. */
> + difference = ~0UL - (next_tick - now);
> + ticks_elapsed = difference / clocktick;
> + remainder = difference % clocktick;
> + } else {
> + /* Scenario 3: We didn't miss a clock tick.
> + "now" is early? */
> + printk ("timer_interrupt: Now is early, now = %lx, next_tick = %lx, clocktick = %lx\n", now, next_tick, clocktick);
> + BUG ();
> + }
> + }
>
> - nticks = 0;
> - while((next_tick - now) < halftick) {
> - next_tick += clocktick;
> - nticks++;
> + if (remainder > halftick) {
> + /* More than 1/2 way into the next tick. It counts. */
> + ticks_elapsed++;
> }
> +
> + /* Add any full ticks that have elapsed plus the one we want next. */
> + next_tick += (ticks_elapsed + 1) * clocktick;
> +
> + /* Only bottom 32-bits of next_tick are written to cr16. */
> mtctl(next_tick, 16);
> cpu_data[cpu].it_value = next_tick;
>
> - while (nticks--) {
> + while (ticks_elapsed--) {
> #ifdef CONFIG_SMP
> smp_do_timer(regs);
> #else
> @@ -124,21 +148,47 @@ #ifndef CONFIG_SMP
> * Once parisc-linux learns the cr16 difference between processors,
> * this could be made to work.
> */
> - long last_tick;
> - long elapsed_cycles;
> + unsigned long now;
> + unsigned long last_tick;
> + unsigned long next_tick;
> + unsigned long next_next_tick;
> + unsigned long elapsed_cycles;
> + unsigned long usec;
> + unsigned long lost;
>
> /* it_value is the intended time of the next tick */
> - last_tick = cpu_data[smp_processor_id()].it_value;
> + next_tick = cpu_data[smp_processor_id()].it_value;
> + next_next_tick = next_tick + clocktick;
>
> - /* Subtract one tick and account for possible difference between
> - * when we expected the tick and when it actually arrived.
> - * (aka wall vs real)
> - */
> - last_tick -= clocktick * (jiffies - wall_jiffies + 1);
> - elapsed_cycles = mfctl(16) - last_tick;
> + /* This represents lost jiffies. */
> + lost = clocktick * (jiffies - wall_jiffies);
> +
> + /* We roll back 1 tick. */
> + last_tick = next_tick - clocktick;
> +
> + /* Read the hardware interval timer. */
> + now = mfctl(16);
> +
> + if (now > last_tick) {
> + /* Scenario 1: "now" is later than last_tick. */
> + elapsed_cycles = now - last_tick;
> + } else {
> + /* "now" is either early or cr16 wrapped. */
> + if (next_next_tick < last_tick) {
This could also use your clever "~next_tick < clocktick" trick.
Remember this is only a heuristic to catch wrapping.
> + /* Scenario 2: A missed clock tick would wrap. */
> + elapsed_cycles = ~0UL - (last_tick - now);
> + } else {
> + /* Scenario 3: We didn't miss a clock tick.
> + "now" is early? */
> + printk ("gettimeoffset: Now is early, now = %lx, last_tick = %lx, next_tick = %lx, clocktick = %lx\n", now, last_tick, next_tick, clocktick);
> + BUG ();
> + }
> + }
Somwhere in thie function we need to makeup for the halftick...
> - /* the precision of this math could be improved */
> - return elapsed_cycles / (PAGE0->mem_10msec / 10000);
> + /* FIXME: Improve precision. */
> + usec = elapsed_cycles * 10000 / PAGE0->mem_10msec;
> + usec += lost;
> + return usec;
> #else
> return 0;
> #endif
Joel has already seen a BUG, which IMO is caused by gettimeoffset not
taking into account the halftick.
Cheers,
Carlos.
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [parisc-linux] [PATCH] timer_interrupt and gettimeoffset.
2006-08-31 18:52 ` Carlos O'Donell
@ 2006-08-31 21:46 ` Grant Grundler
2006-09-03 14:54 ` Carlos O'Donell
0 siblings, 1 reply; 14+ messages in thread
From: Grant Grundler @ 2006-08-31 21:46 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: parisc-linux
On Thu, Aug 31, 2006 at 02:52:44PM -0400, Carlos O'Donell wrote:
...
> >> 2. No attempt is made to handle cr16 wrapping.
> >
> >The signed math was supposed to handle it.
> >I believe it did but forgot details since looking at
> >it a few years ago.
>
> That is *even* worse, signed arithmetic overflow is compiler
> implementation dependant. It's also therefore not very nice C code.
> The overflow case is in the middle of the 32-bit range now, where you
> had a very large positive number which wraps to a very large negative
> number. The gap is almost the entire 32-bit range, or 27 seconds on a
> 160Mhz box as we have shown already.
I think it's arbitrary where the overflow occurs.
Maybe unsigned math overflow is easier to visualize/understand.
At least it is for me.
> >I personally prefer to NOT use signed math in this case.
> >But that's honestly not a great reason to change this code.
> >I'd really prefer some evidence the kernel code is wrong.
>
> I agree, this patch was an attempt to provide an alternate
> implementation that made sense and was maintainer friendly. I believe
> we should be able to get this working better and handle the failures
> seen on the slower boxes.
Yes. I'm now pretty sure the fix for that will be in gettimeoffset()
and not in timer_interrupt(). But I agree the two functions are very
similar but have to handle slightly different corner cases.
> >13.065388 sec
> >30 Aug 23:52:04 ntpdate[14055]: step time server 195.234.188.26 offset
> >11.578574 sec
> >30 Aug 23:52:31 ntpdate[14058]: step time server 203.217.30.156 offset
> >13.954358 sec
> >...
>
> Technically it is implementation dependant, and can be as low as 1/2
> the peak instruction rate. In correcting timer_interrupt, I remember
> removing what I thought was a spurious 'clocktick' addition in the
> implementation.
I was thinking the opposite: I'm missing a "tick_elapsed" count
on each timer interrupt.
> Does cr16 *actuall* count at the instruction rate, or
> does it count at 1/2 rate?
I _believe_ all implementations count at the actual rate.
At least I'm not aware of any implementations that don't
count at the advertized clock speed.
...
> I think the detection of Scenario 3 is wrong in your patch, I'll
> epxlain further down in the code.
...
>
> >...
> >> I assert that 'gettimeoffset' should never return a negative value. It
> >> represents the postive adjustment accounting for the fact that we are
> >> *part* of the way through a tick.
> >
> >I have to think about this more. But I wonder if how "halftick" is
> >handled in timer_interrupt does not play well with this concept.
>
> There is a bug here because of this, you are right to be warry of the
> halftick adjustment. It is also needed in gettimeoffset, or "now" will
> appear before "next_tick" without wrapping.
Sorry - this didn't make sense. Can you provide an example?
For simplicity assume a 5 bit counter and pick the values
that illustrate your case.
> >Please review and see if I added any new bugs.
>
> There is 1 addtional bug.
...
> >+ /* Determine how much time elapsed. */
> >+ if (now > next_tick) {
> >+ /* Scenario 1: "now" is late. A bit late is normal. */
> >+ ticks_elapsed = (now - next_tick) / clocktick;
> >+ remainder = now - (ticks_elapsed * clocktick);
>
> What have you got against modulo?
Modulo is a division.
Division is much more expensive than integer multiplication.
Is there a way to do the division once _and_ get the remainder?
My assumption that integer multiplication is "cheaper"
could be wrong.
> The correct math is as follows.
>
> remainder = now - next_tick - ticks_elapsed * clocktick;
Erm, no. ticks_elapsed was calculated here:
ticks_elapsed = (now - next_tick) / clocktick;
(the line above.)
I'm taking advantage of truncation since this is integer division.
That make more sense?
> >+ /* "now" is either early or cr16 wrapped. */
> >+ if (~next_tick < clocktick) {
>
> Too clever for me :)
Now I'm worried. :)
I've already removed this code in the version I'm working on now.
...
> >+ } else {
> >+ /* "now" is either early or cr16 wrapped. */
> >+ if (next_next_tick < last_tick) {
>
> This could also use your clever "~next_tick < clocktick" trick.
> Remember this is only a heuristic to catch wrapping.
Yup - I'll post a new version that boots 64-bit correctly in a bit.
I've still not got the "wrapping" rules correct.
...
> Somwhere in thie function we need to makeup for the halftick...
Hrm. I need to think more about that. You might be right.
> Joel has already seen a BUG, which IMO is caused by gettimeoffset not
> taking into account the halftick.
ok. I was just looking at that.
thanks,
grant
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [parisc-linux] [PATCH] timer_interrupt and gettimeoffset.
2006-08-31 21:46 ` Grant Grundler
@ 2006-09-03 14:54 ` Carlos O'Donell
2006-09-03 19:55 ` Grant Grundler
0 siblings, 1 reply; 14+ messages in thread
From: Carlos O'Donell @ 2006-09-03 14:54 UTC (permalink / raw)
To: Grant Grundler; +Cc: parisc-linux
On 8/31/06, Grant Grundler <grundler@parisc-linux.org> wrote:
> I think it's arbitrary where the overflow occurs.
> Maybe unsigned math overflow is easier to visualize/understand.
> At least it is for me.
I agree with you, unsigned long code is easier to maintain.
Signed math overflow is implementation defined behaviour, it should
not be relied upon.
> Yes. I'm now pretty sure the fix for that will be in gettimeoffset()
> and not in timer_interrupt(). But I agree the two functions are very
> similar but have to handle slightly different corner cases.
I still disagree, if you see James most recent patch which disabled
IRQ's while we handle the timer interrupt, you will see that both
functions are closer to being similar :)
I will assert that at the end of the day we will be writing
"cr16_offset" which is a generic function that can be called with
interrupts disabled and *that* will handle all the corner cases and be
called by "timer_interrupt" and "gettimeoffset".
> I was thinking the opposite: I'm missing a "tick_elapsed" count
> on each timer interrupt.
That might also be the case.
> > Does cr16 *actuall* count at the instruction rate, or
> > does it count at 1/2 rate?
>
> I _believe_ all implementations count at the actual rate.
> At least I'm not aware of any implementations that don't
> count at the advertized clock speed.
OK.
> > There is a bug here because of this, you are right to be warry of the
> > halftick adjustment. It is also needed in gettimeoffset, or "now" will
> > appear before "next_tick" without wrapping.
>
> Sorry - this didn't make sense. Can you provide an example?
> For simplicity assume a 5 bit counter and pick the values
> that illustrate your case.
5 bit counter.
it_value = 5
cr16 = ... free running.
clocktick = 5
halftick = 2
a. Counter goes off at 8. Expected to go off at 5.
b. Since (8 - 5) == 3 > 2 or a halftick, we push forward 2 ticks
c. Set it_value to 15.
1. User calls gettimeofday.
2. cr16 says time is 9
3. The it_value is 15, subtract a clocktick and you get 10.
*boom*
The value of cr16 is < the value of a rolled back clocktick.
In counting the halftick, we need to rollback the halftick in gettimeoffset.
> > What have you got against modulo?
>
> Modulo is a division.
> Division is much more expensive than integer multiplication.
> Is there a way to do the division once _and_ get the remainder?
>
> My assumption that integer multiplication is "cheaper"
> could be wrong.
I timed this for some reasonable values of dividend and divisor.
The division and modulo operation is ~156 cycles.
The integer multiplication is ~73 cycles.
This is the average cr16 tick over 1000 runs.
The integer multiplication is "cheaper"
> > The correct math is as follows.
> >
> > remainder = now - next_tick - ticks_elapsed * clocktick;
>
> Erm, no. ticks_elapsed was calculated here:
> ticks_elapsed = (now - next_tick) / clocktick;
>
> (the line above.)
> I'm taking advantage of truncation since this is integer division.
> That make more sense?
Let us do some math.
You say the following equations are correct:
ticks_elapsed = (now - next_tick) / clocktick;
remainder = now - (ticks_elapsed * clocktick);
The ticks_elapsed equation *is* correct.
The remainder equation is wrong. The value of "now" could be 4GB, the
value of "next_tick" could be "4GB - clocktick", the ticks_elapsed
value is 1. This is correct. However, the remainder is going to be
"4GB - 1 * clocktick" which is incorrect. The remainder in this case
is zero.
To get the remainder you *must* do:
remainder = (now - next_tick) - (ticks_elapsed * clocktick)
This gives you the left over ticks you didn't count. I have added
brackets here to clarify the math.
> > >+ /* "now" is either early or cr16 wrapped. */
> > >+ if (~next_tick < clocktick) {
> >
> > Too clever for me :)
>
> Now I'm worried. :)
> I've already removed this code in the version I'm working on now.
I was only joking here, but a comment is absolutely required when you
use clever 1's compliment tricks to approximate more compilcated
mathematics. Including a comment saying that it's 1 off.
> > Somwhere in thie function we need to makeup for the halftick...
>
> Hrm. I need to think more about that. You might be right.
I have come to the conclusion that adding the halftick is conceptually wrong.
Follow me on this.
1. By adding the halftick, as a real tick, the system time moves into
the future.
2. A system with future time means we need negative gettimeoffset adjustments.
3. Negative gettimeoffset adjustment complicate things needlessly.
4. Halfticks should not be counted, and should be ignored.
5. Ignoring halfticks is not a problem, when the late timer interrupt
arrives we will accrue all full ticks anyway.
What do you think?
Cheers,
Carlos.
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [parisc-linux] [PATCH] timer_interrupt and gettimeoffset.
2006-09-03 14:54 ` Carlos O'Donell
@ 2006-09-03 19:55 ` Grant Grundler
2006-09-03 20:29 ` James Bottomley
2006-09-03 20:30 ` Carlos O'Donell
0 siblings, 2 replies; 14+ messages in thread
From: Grant Grundler @ 2006-09-03 19:55 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: parisc-linux
On Sun, Sep 03, 2006 at 10:54:18AM -0400, Carlos O'Donell wrote:
> >Yes. I'm now pretty sure the fix for that will be in gettimeoffset()
> >and not in timer_interrupt(). But I agree the two functions are very
> >similar but have to handle slightly different corner cases.
>
> I still disagree, if you see James most recent patch which disabled
> IRQ's while we handle the timer interrupt, you will see that both
> functions are closer to being similar :)
I'm not sure about that. James might be wrong in this case.
See do_cpu_irq_mask().
It clears EIEM and thus disables all external interrupts
including the interval timer. The comment is wrong in that
we disable all EIEM bits, not just TIMER and IPI.
> I will assert that at the end of the day we will be writing
> "cr16_offset" which is a generic function that can be called with
> interrupts disabled and *that* will handle all the corner cases and be
> called by "timer_interrupt" and "gettimeoffset".
hrm ok. I'll keep that in mind. I'm not convinced of that
yet since those are both performance critical piecves of code.
...
> >Sorry - this didn't make sense. Can you provide an example?
> >For simplicity assume a 5 bit counter and pick the values
> >that illustrate your case.
>
> 5 bit counter.
> it_value = 5
> cr16 = ... free running.
> clocktick = 5
> halftick = 2
>
> a. Counter goes off at 8. Expected to go off at 5.
Itimer will interrupt at 5 if it was expect to go off at 5.
timer_interrupt() will never get invoked until 8 becuase of
interrupt handling overhead.
> b. Since (8 - 5) == 3 > 2 or a halftick, we push forward 2 ticks
> c. Set it_value to 15.
Ok. I really don't like the business with halftick pushing
system time "into the future". Maybe we can just drop one tick
on the floor. Ie skip it and handle it on the next "full" tick?
> 1. User calls gettimeofday.
> 2. cr16 says time is 9
> 3. The it_value is 15, subtract a clocktick and you get 10.
>
> *boom*
>
> The value of cr16 is < the value of a rolled back clocktick.
> In counting the halftick, we need to rollback the halftick in gettimeoffset.
No. We need to just drop the next tick and not count it until
the next round.
> >Division is much more expensive than integer multiplication.
> >Is there a way to do the division once _and_ get the remainder?
> >
> >My assumption that integer multiplication is "cheaper"
> >could be wrong.
>
> I timed this for some reasonable values of dividend and divisor.
> The division and modulo operation is ~156 cycles.
> The integer multiplication is ~73 cycles.
> This is the average cr16 tick over 1000 runs.
> The integer multiplication is "cheaper"
ok - many thanks for doing that.
...
> You say the following equations are correct:
>
> ticks_elapsed = (now - next_tick) / clocktick;
> remainder = now - (ticks_elapsed * clocktick);
>
> The ticks_elapsed equation *is* correct.
>
> The remainder equation is wrong. The value of "now" could be 4GB, the
> value of "next_tick" could be "4GB - clocktick", the ticks_elapsed
> value is 1. This is correct. However, the remainder is going to be
> "4GB - 1 * clocktick" which is incorrect. The remainder in this case
> is zero.
>
> To get the remainder you *must* do:
>
> remainder = (now - next_tick) - (ticks_elapsed * clocktick)
>
> This gives you the left over ticks you didn't count. I have added
> brackets here to clarify the math.
Yes - got it now. thanks again!
> I have come to the conclusion that adding the halftick is conceptually
> wrong.
>
> Follow me on this.
>
> 1. By adding the halftick, as a real tick, the system time moves into
> the future.
I'm already convinced this is wrong and leads to too many complications.
> 2. A system with future time means we need negative gettimeoffset
> adjustments.
> 3. Negative gettimeoffset adjustment complicate things needlessly.
> 4. Halfticks should not be counted, and should be ignored.
> 5. Ignoring halfticks is not a problem, when the late timer interrupt
> arrives we will accrue all full ticks anyway.
>
> What do you think?
total agreement.
I'll rework my code later today to try to match the discussion so far.
thanks,
grant
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [parisc-linux] [PATCH] timer_interrupt and gettimeoffset.
2006-09-03 19:55 ` Grant Grundler
@ 2006-09-03 20:29 ` James Bottomley
2006-09-03 20:30 ` Carlos O'Donell
1 sibling, 0 replies; 14+ messages in thread
From: James Bottomley @ 2006-09-03 20:29 UTC (permalink / raw)
To: Grant Grundler; +Cc: parisc-linux
On Sun, 2006-09-03 at 13:55 -0600, Grant Grundler wrote:
> I'm not sure about that. James might be wrong in this case.
> See do_cpu_irq_mask().
> It clears EIEM and thus disables all external interrupts
> including the interval timer. The comment is wrong in that
> we disable all EIEM bits, not just TIMER and IPI.
Yes ... I thought I fixed that the last time I pulled this code apart,
but apparently not.
I'll concentrate on fixing this, since I suspect it is the root cause of
the horrible interrupt latencies causing the ticks to back up.
James
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [parisc-linux] [PATCH] timer_interrupt and gettimeoffset.
2006-09-03 19:55 ` Grant Grundler
2006-09-03 20:29 ` James Bottomley
@ 2006-09-03 20:30 ` Carlos O'Donell
2006-09-03 21:22 ` John David Anglin
1 sibling, 1 reply; 14+ messages in thread
From: Carlos O'Donell @ 2006-09-03 20:30 UTC (permalink / raw)
To: Grant Grundler; +Cc: parisc-linux
On 9/3/06, Grant Grundler <grundler@parisc-linux.org> wrote:
> I'll rework my code later today to try to match the discussion so far.
Original code:
58 now = mfctl(16);
60 next_tick = cpu_data[cpu].it_value;
70 nticks = 0;
71 while((next_tick - now) < halftick) {
72 next_tick += clocktick;
73 nticks++;
74 }
75 mtctl(next_tick, 16);
76 cpu_data[cpu].it_value = next_tick;
1. Let us assume a 32-bit CPU with a 32-bit counter and a 32-bit
trigger register.
2. Assume that it_value is *near* the signed long boundary (0x7fffff)
3. The counter and trigger match, raising an interrupt.
4. During the interrupt delivery cr16 wraps into negative signed long,
but only a little bit.
Let us say cr16 is ~(0x7fffffff + 50)
5. The value "next_tick - now" is equivalent to:
"next_tick - (-now)" since now is negative.
"next_tick + now"
6. The signed long math wraps, and the solution is a small negative
number, or small positive number, in this case it is +48.
7. The while loop adds clockticks until we are positive and bigger
than halftick. Adding 1 clocktick.
The signed math works correctly as an approximation (off by 2) and I
do admit I find no fault in the math, but it is still confusing.
However, as James points out, if the *real* cr16 gets ahead of the
newly set it_value, by delaying this function due to other interrupts,
then we will have to wait for a full wrap to have it trigger another
timer interrupt.
Cheers,
Carlos.
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [parisc-linux] [PATCH] timer_interrupt and gettimeoffset.
2006-09-03 20:30 ` Carlos O'Donell
@ 2006-09-03 21:22 ` John David Anglin
0 siblings, 0 replies; 14+ messages in thread
From: John David Anglin @ 2006-09-03 21:22 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: parisc-linux
> The signed math works correctly as an approximation (off by 2) and I
> do admit I find no fault in the math, but it is still confusing.
The signed math works as long as the latency in delivering the
interruption is not larger than about half a cycle. Once
next_tick - now >= halftick, then we stop updating next_tick
until the counter wraps. Probably, this could be extended
using unsigned math but you still have to handle the wrap.
The other issue is to ensure that the compare value for the
next interrupt is set before the counter passes this count.
We could have an interruption between the read and write
of cr16 even with interrupts disabled. So, I think we should
minimize memory accesses. The enclosed change eliminates
the memory accesses.
It may be we don't need the save/restore if interrupts are disabled.
Dave
--
J. David Anglin dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada (613) 990-0752 (FAX: 952-6602)
Index: time.c
===================================================================
RCS file: /var/cvs/linux-2.6/arch/parisc/kernel/time.c,v
retrieving revision 1.16
diff -u -p -u -r1.16 time.c
--- time.c 24 Jun 2006 16:05:18 -0000 1.16
+++ time.c 3 Sep 2006 21:17:25 -0000
@@ -47,29 +47,36 @@ irqreturn_t timer_interrupt(int irq, voi
{
long now;
long next_tick;
- int nticks;
+ unsigned long nticks = 0;
+ long ct = clocktick;
+ long ht = halftick;
int cpu = smp_processor_id();
+ long flags;
profile_tick(CPU_PROFILING, regs);
- now = mfctl(16);
- /* initialize next_tick to time at last clocktick */
+ /* Initialize next_tick to time at last clocktick */
next_tick = cpu_data[cpu].it_value;
- /* since time passes between the interrupt and the mfctl()
- * above, it is never true that last_tick + clocktick == now. If we
- * never miss a clocktick, we could set next_tick = last_tick + clocktick
- * but maybe we'll miss ticks, hence the loop.
+ /* Since time passes between the interrupt and the mfctl(),
+ * it is never true that last_tick + clocktick == now.
+ * If we never miss a clocktick, we could set
+ * next_tick = last_tick + clocktick, * but maybe we'll miss
+ * ticks, hence the loop.
*
* Variables are *signed*.
*/
- nticks = 0;
- while((next_tick - now) < halftick) {
- next_tick += clocktick;
+ /* Don't want to be interrupted while calculating
+ * the time for the next tick. */
+ local_irq_save(flags);
+ now = mfctl(16);
+ while((next_tick - now) < ht) {
+ next_tick += ct;
nticks++;
}
mtctl(next_tick, 16);
+ local_irq_restore(flags);
cpu_data[cpu].it_value = next_tick;
while (nticks--) {
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [parisc-linux] [PATCH] timer_interrupt and gettimeoffset.
2006-08-31 6:53 ` Grant Grundler
2006-08-31 18:52 ` Carlos O'Donell
@ 2006-09-01 22:48 ` Grant Grundler
2006-09-01 22:59 ` Grant Grundler
2006-09-06 6:29 ` [parisc-linux] [PATCH] timer_interrupt #5 Grant Grundler
1 sibling, 2 replies; 14+ messages in thread
From: Grant Grundler @ 2006-09-01 22:48 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: parisc-linux
On Thu, Aug 31, 2006 at 12:53:27AM -0600, Grant Grundler wrote:
> And I've hacked your version some more.
> diff is below.
version #3: seems to be working fine on 64-bit SMP.
32-bit UP kernel is still under construction.
That means gettimeoffset() hasn't been tested.
And I'm getting the following warnings on my b2600:
...
Setting the system clock..
timer_interrupt(CPU 0): delayed! run ntpdate ticks 859 cycles FFF51A1C rem 40E59C next/now FFFE7A9D/96080
gettimeoffset(CPU -119762782): missing ticks!cycles FFC87AE4 prev/now/next 6EBEA41/14C624/4C4B40 clock 0
gettimeoffset(CPU -177625053): missing ticks!cycles FFC87AE4 prev/now/next A5ED2C0/14C624/4C4B40 clock 12
gettimeoffset(CPU -238800592): missing ticks!cycles FFC87AE4 prev/now/next E0449B3/14C624/4C4B40 clock 12
gettimeoffset(CPU -302979574): missing ticks!cycles FFC87AE4 prev/now/next 11D794D9/14C624/4C4B40 clock 0
Sysgettimeoffset(CPU -361861366): missing ticks!cycles FFC87AE4 prev/now/next 155A0BD9/14C624/4C4B40 clock 12
tem Clock set. Local time: Fri Sep 1 22:42:09 UTC 2006.
gettimeoffset printk is busticated. I'll fix that in the next version.
But it looks like wrapped values are causing problems.
The timer_interrupt warning might be reasonable given we are making
a call to firmware to set the clock and block interrupts for
the duration.
In addition to time.c, two more files are touched:
smp.c I couldn't see where slave CPUs would start
the interval timer and added the call here.
param.h Use CONFIG_HZ for interval timer.
We were ignored this before.
I just realized one small change to arch/parisc/kernel/processor.c
is missin from this diff. We don't want processor_probe() to clobber
CPU0's cpu_data. "it_value" has already been initialized.
enjoy!
grant
diff --git a/arch/parisc/kernel/smp.c b/arch/parisc/kernel/smp.c
index 98e4095..f33e8de 100644
--- a/arch/parisc/kernel/smp.c
+++ b/arch/parisc/kernel/smp.c
@@ -430,8 +430,9 @@ smp_do_timer(struct pt_regs *regs)
static void __init
smp_cpu_init(int cpunum)
{
- extern int init_per_cpu(int); /* arch/parisc/kernel/setup.c */
+ extern int init_per_cpu(int); /* arch/parisc/kernel/processor.c */
extern void init_IRQ(void); /* arch/parisc/kernel/irq.c */
+ extern void start_cpu_itimer(void); /* arch/parisc/kernel/time.c */
/* Set modes and Enable floating point coprocessor */
(void) init_per_cpu(cpunum);
@@ -457,6 +458,7 @@ smp_cpu_init(int cpunum)
enter_lazy_tlb(&init_mm, current);
init_IRQ(); /* make sure no IRQ's are enabled or pending */
+ start_cpu_itimer();
}
diff --git a/arch/parisc/kernel/time.c b/arch/parisc/kernel/time.c
index 5facc9b..337c4ec 100644
--- a/arch/parisc/kernel/time.c
+++ b/arch/parisc/kernel/time.c
@@ -35,8 +35,8 @@ #include <linux/timex.h>
/* xtime and wall_jiffies keep wall-clock time */
extern unsigned long wall_jiffies;
-static long clocktick __read_mostly; /* timer cycles per tick */
-static long halftick __read_mostly;
+static unsigned long clocktick __read_mostly; /* timer cycles per tick */
+static unsigned long halftick __read_mostly;
#ifdef CONFIG_SMP
extern void smp_do_timer(struct pt_regs *regs);
@@ -44,34 +44,77 @@ #endif
irqreturn_t timer_interrupt(int irq, void *dev_id, struct pt_regs *regs)
{
- long now;
- long next_tick;
- int nticks;
+ unsigned long now;
+ unsigned long next_tick;
+ unsigned long cycles_elapsed;
+ unsigned long cycles_remainder;
+ unsigned long ticks_elapsed = 1; /* at least one elapsed */
int cpu = smp_processor_id();
profile_tick(CPU_PROFILING, regs);
- now = mfctl(16);
- /* initialize next_tick to time at last clocktick */
+ /* Initialize next_tick to the expected tick time. */
next_tick = cpu_data[cpu].it_value;
- /* since time passes between the interrupt and the mfctl()
- * above, it is never true that last_tick + clocktick == now. If we
- * never miss a clocktick, we could set next_tick = last_tick + clocktick
- * but maybe we'll miss ticks, hence the loop.
- *
- * Variables are *signed*.
+ /* Get current interval timer.
+ * CR16 reads as 64 bits in CPU wide mode.
+ * CR16 reads as 32 bits in CPU narrow mode.
*/
+ now = mfctl(16);
+
+ cycles_elapsed = now - next_tick;
+
+ /* Determine how much time elapsed. */
+ if (now < next_tick) {
+ /* Scenario 2: CR16 wrapped after clock tick.
+ * 1's complement will give us the "elapse cycles".
+ *
+ * This "cr16 wrapped" cruft is primarily for 32-bit kernels.
+ * So think "unsigned long is u32" when reading the code.
+ * And yes, of course 64-bit will someday wrap, but only
+ * every 198841 days on a 1GHz machine.
+ */
+ cycles_elapsed = ~cycles_elapsed; /* off by one cycle - don't care */
+ }
+
+ ticks_elapsed += cycles_elapsed / clocktick;
+ cycles_remainder = cycles_elapsed % clocktick;
- nticks = 0;
- while((next_tick - now) < halftick) {
- next_tick += clocktick;
- nticks++;
+ /* Can we differentiate between "early CR16" (aka Scenario 1) and
+ * "long delay" (aka Scenario 3)? I don't think so.
+ *
+ * We expected timer_interrupt to be delivered at least a few hundred
+ * cycles after the IT fires. But it's arbitrary how much time passes
+ * before we call it "late". I've picked one second.
+ */
+ if (ticks_elapsed > HZ) {
+ /* Scenario 3: very long delay? bad in any case */
+ printk (KERN_CRIT "timer_interrupt(CPU %d): delayed! run ntpdate"
+ " ticks %ld cycles %lX rem %lX"
+ " next/now %lX/%lX\n",
+ cpu,
+ ticks_elapsed, cycles_elapsed, cycles_remainder,
+ next_tick, now );
+
+ ticks_elapsed = 1; /* hack to limit damage in loop below */
}
+
+
+ /* Determine when (in CR16 cycles) next IT interrupt will fire.
+ * We want IT to fire modulo clocktick even if we miss/skip some.
+ * But those interrupts don't in fact get delivered that regularly.
+ */
+ next_tick = now + (clocktick - cycles_remainder);
+
+ /* Program the IT when to deliver the next interrupt. */
+ /* Only bottom 32-bits of next_tick are written to cr16. */
mtctl(next_tick, 16);
cpu_data[cpu].it_value = next_tick;
- while (nticks--) {
+ /* Now that we are done mucking with unreliable delivery of interrupts,
+ * go do system house keeping.
+ */
+ while (ticks_elapsed--) {
#ifdef CONFIG_SMP
smp_do_timer(regs);
#else
@@ -124,21 +167,40 @@ #ifndef CONFIG_SMP
* Once parisc-linux learns the cr16 difference between processors,
* this could be made to work.
*/
- long last_tick;
- long elapsed_cycles;
+ unsigned long now;
+ unsigned long prev_tick;
+ unsigned long next_tick;
+ unsigned long elapsed_cycles;
+ unsigned long usec;
- /* it_value is the intended time of the next tick */
- last_tick = cpu_data[smp_processor_id()].it_value;
+ next_tick = cpu_data[smp_processor_id()].it_value;
+ now = mfctl(16); /* Read the hardware interval timer. */
- /* Subtract one tick and account for possible difference between
- * when we expected the tick and when it actually arrived.
- * (aka wall vs real)
- */
- last_tick -= clocktick * (jiffies - wall_jiffies + 1);
- elapsed_cycles = mfctl(16) - last_tick;
+ prev_tick = next_tick - clocktick;
- /* the precision of this math could be improved */
- return elapsed_cycles / (PAGE0->mem_10msec / 10000);
+ /* Assume Scenario 1: "now" is later than prev_tick. */
+ elapsed_cycles = now - prev_tick;
+
+ if (now < prev_tick) {
+ /* Scenario 2: CR16 wrapped!
+ * 1's complement is close enough.
+ */
+ elapsed_cycles = ~elapsed_cycles;
+ }
+
+ if (elapsed_cycles > (HZ * clocktick)) {
+ /* Scenario 3: clock ticks are missing. */
+ printk (KERN_CRIT "gettimeoffset(CPU %d): missing ticks!"
+ "cycles %lX prev/now/next %lX/%lX/%lX clock %lX\n",
+ elapsed_cycles, prev_tick, now, next_tick, clocktick);
+ }
+
+ /* FIXME: Can we improve the precision? Not with PAGE0. */
+ usec = (elapsed_cycles * 10000) / PAGE0->mem_10msec;
+
+ /* add in "lost" jiffies */
+ usec += clocktick * (jiffies - wall_jiffies);
+ return usec;
#else
return 0;
#endif
@@ -149,6 +211,7 @@ do_gettimeofday (struct timeval *tv)
{
unsigned long flags, seq, usec, sec;
+ /* Hold xtime_lock and adjust timeval. */
do {
seq = read_seqbegin_irqsave(&xtime_lock, flags);
usec = gettimeoffset();
@@ -156,25 +219,13 @@ do_gettimeofday (struct timeval *tv)
usec += (xtime.tv_nsec / 1000);
} while (read_seqretry_irqrestore(&xtime_lock, seq, flags));
- if (unlikely(usec > LONG_MAX)) {
- /* This can happen if the gettimeoffset adjustment is
- * negative and xtime.tv_nsec is smaller than the
- * adjustment */
- printk(KERN_ERR "do_gettimeofday() spurious xtime.tv_nsec of %ld\n", usec);
- usec += USEC_PER_SEC;
- --sec;
- /* This should never happen, it means the negative
- * time adjustment was more than a second, so there's
- * something seriously wrong */
- BUG_ON(usec > LONG_MAX);
- }
-
-
+ /* Move adjusted usec's into sec's. */
while (usec >= USEC_PER_SEC) {
usec -= USEC_PER_SEC;
++sec;
}
+ /* Return adjusted result. */
tv->tv_sec = sec;
tv->tv_usec = usec;
}
@@ -226,22 +277,24 @@ unsigned long long sched_clock(void)
}
+void __init start_cpu_itimer(void)
+{
+ unsigned int cpu = smp_processor_id();
+ unsigned long next_tick = mfctl(16) + clocktick;
+
+ mtctl(next_tick, 16); /* kick off Interval Timer (CR16) */
+
+ cpu_data[cpu].it_value = next_tick;
+}
+
void __init time_init(void)
{
- unsigned long next_tick;
static struct pdc_tod tod_data;
clocktick = (100 * PAGE0->mem_10msec) / HZ;
halftick = clocktick / 2;
- /* Setup clock interrupt timing */
-
- next_tick = mfctl(16);
- next_tick += clocktick;
- cpu_data[smp_processor_id()].it_value = next_tick;
-
- /* kick off Itimer (CR16) */
- mtctl(next_tick, 16);
+ start_cpu_itimer(); /* get CPU 0 started */
if(pdc_tod_read(&tod_data) == 0) {
write_seqlock_irq(&xtime_lock);
diff --git a/include/asm-parisc/param.h b/include/asm-parisc/param.h
index 07cb9b9..32e03d8 100644
--- a/include/asm-parisc/param.h
+++ b/include/asm-parisc/param.h
@@ -2,13 +2,9 @@ #ifndef _ASMPARISC_PARAM_H
#define _ASMPARISC_PARAM_H
#ifdef __KERNEL__
-# ifdef CONFIG_PA20
-# define HZ 1000 /* Faster machines */
-# else
-# define HZ 100 /* Internal kernel timer frequency */
-# endif
-# define USER_HZ 100 /* .. some user interfaces are in "ticks" */
-# define CLOCKS_PER_SEC (USER_HZ) /* like times() */
+#define HZ CONFIG_HZ
+#define USER_HZ 100 /* some user API use "ticks" */
+#define CLOCKS_PER_SEC (USER_HZ) /* like times() */
#endif
#ifndef HZ
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [parisc-linux] [PATCH] timer_interrupt and gettimeoffset.
2006-09-01 22:48 ` Grant Grundler
@ 2006-09-01 22:59 ` Grant Grundler
2006-09-06 6:29 ` [parisc-linux] [PATCH] timer_interrupt #5 Grant Grundler
1 sibling, 0 replies; 14+ messages in thread
From: Grant Grundler @ 2006-09-01 22:59 UTC (permalink / raw)
To: Grant Grundler; +Cc: parisc-linux
On Fri, Sep 01, 2006 at 04:48:25PM -0600, Grant Grundler wrote:
> On Thu, Aug 31, 2006 at 12:53:27AM -0600, Grant Grundler wrote:
> > And I've hacked your version some more.
> > diff is below.
>
> version #3: seems to be working fine on 64-bit SMP.
> 32-bit UP kernel is still under construction.
...
> gettimeoffset printk is busticated. I'll fix that in the next version.
> But it looks like wrapped values are causing problems.
Looks like only the printk is broken.
The attached test program (based on Guy's description) seems
to be working fine on 32-bit UP/b2600:
...
Time : 1157151360 sec, 106770 usec
Time : 1157151361 sec, 116765 usec
Time : 1157151362 sec, 126770 usec
Time : 1157151363 sec, 136764 usec
...
Note there is ~10ms shift on each "Time" output.
I believe this is because the task will run on the next
tick _after_ the 1 second time has expired.
hth,
grant
#include <stdio.h>
#include <sys/time.h>
#include <time.h>
main()
{
struct timeval tv;
while(gettimeofday(&tv, NULL) == 0) {
printf("Time : %d sec, %d usec\n", tv.tv_sec, tv.tv_usec);
sleep(1);
}
}
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [parisc-linux] [PATCH] timer_interrupt #5
2006-09-01 22:48 ` Grant Grundler
2006-09-01 22:59 ` Grant Grundler
@ 2006-09-06 6:29 ` Grant Grundler
[not found] ` <44FF2A69.5040102@scarlet.be>
1 sibling, 1 reply; 14+ messages in thread
From: Grant Grundler @ 2006-09-06 6:29 UTC (permalink / raw)
To: parisc-linux
On Fri, Sep 01, 2006 at 04:48:25PM -0600, Grant Grundler wrote:
> version #3: seems to be working fine on 64-bit SMP.
> 32-bit UP kernel is still under construction.
I didn't bother posting version #4.
Here is version #5 (against current parisc git tree):
o makes all vars in timer_interrupt and gettimeoffset unsigned longs
o make a local var to store clocktick - gcc can optimize local vars
o completely remove halftick
o completely remove ticks_elapsed _and_ the loop that depended on it.
(James observed that x86 doesn't have such a loop...all the risc
arches seemed to have fallen behind on that.)
o skips a tick if we can't safely program CR16 for that tick
o Avoids div/mul math in nearly all cases. If timer_interrupt()
misses more than 32 ticks, division will be used in the
one modulo operation.
I'd like to commit this by this friday (or sooner) if other folks
have no objection.
thanks,
grant
Signed-off-by: Grant Grundler <grundler@parisc-linux.org>
diff --git a/arch/parisc/kernel/time.c b/arch/parisc/kernel/time.c
index e595b93..e61d603 100644
--- a/arch/parisc/kernel/time.c
+++ b/arch/parisc/kernel/time.c
@@ -35,8 +35,7 @@ #include <linux/timex.h>
/* xtime and wall_jiffies keep wall-clock time */
extern unsigned long wall_jiffies;
-static long clocktick __read_mostly; /* timer cycles per tick */
-static long halftick __read_mostly;
+static unsigned long clocktick __read_mostly; /* timer cycles per tick */
#ifdef CONFIG_SMP
extern void smp_do_timer(struct pt_regs *regs);
@@ -44,46 +43,106 @@ #endif
irqreturn_t timer_interrupt(int irq, void *dev_id, struct pt_regs *regs)
{
- long now;
- long next_tick;
- int nticks;
- int cpu = smp_processor_id();
+ unsigned long now;
+ unsigned long next_tick;
+ unsigned long cycles_elapsed;
+ unsigned long cycles_remainder;
+ unsigned int cpu = smp_processor_id();
+
+ /* gcc can optimize for "read-only" case with a local clocktick */
+ unsigned long cpt = clocktick;
profile_tick(CPU_PROFILING, regs);
- now = mfctl(16);
- /* initialize next_tick to time at last clocktick */
+ /* Initialize next_tick to the expected tick time. */
next_tick = cpu_data[cpu].it_value;
- /* since time passes between the interrupt and the mfctl()
- * above, it is never true that last_tick + clocktick == now. If we
- * never miss a clocktick, we could set next_tick = last_tick + clocktick
- * but maybe we'll miss ticks, hence the loop.
- *
- * Variables are *signed*.
+ /* Get current interval timer.
+ * CR16 reads as 64 bits in CPU wide mode.
+ * CR16 reads as 32 bits in CPU narrow mode.
*/
+ now = mfctl(16);
+
+ cycles_elapsed = now - next_tick;
+
+ if ((cycles_elapsed >> 5) < cpt) {
+ /* use "cheap" math (add/subtract) instead
+ * of the more expensive div/mul method
+ */
+ cycles_remainder = cycles_elapsed;
+ while (cycles_remainder > cpt) {
+ cycles_remainder -= cpt;
+ }
+ } else {
+ cycles_remainder = cycles_elapsed % cpt;
+ }
- nticks = 0;
- while((next_tick - now) < halftick) {
- next_tick += clocktick;
- nticks++;
+ /* Can we differentiate between "early CR16" (aka Scenario 1) and
+ * "long delay" (aka Scenario 3)? I don't think so.
+ *
+ * We expected timer_interrupt to be delivered at least a few hundred
+ * cycles after the IT fires. But it's arbitrary how much time passes
+ * before we call it "late". I've picked one second.
+ */
+/* aproximate HZ with shifts. Intended math is "(elapsed/clocktick) > HZ" */
+#if (HZ = 1000)
+ if (cycles_elapsed > (cpt << 10) )
+#elif (HZ = 250)
+ if (cycles_elapsed > (cpt << 8) )
+#elif (HZ = 100)
+ if (cycles_elapsed > (cpt << 7) )
+#elif
+#warn WTF is HZ set to anyway?
+ if (cycles_elapsed > (HZ * cpt) )
+#endif
+ {
+ /* Scenario 3: very long delay? bad in any case */
+ printk (KERN_CRIT "timer_interrupt(CPU %d): delayed!"
+ " cycles %lX rem %lX "
+ " next/now %lX/%lX\n",
+ cpu,
+ cycles_elapsed, cycles_remainder,
+ next_tick, now );
}
- mtctl(next_tick, 16);
+
+ /* convert from "division remainder" to "remainder of clock tick" */
+ cycles_remainder = cpt - cycles_remainder;
+
+ /* Determine when (in CR16 cycles) next IT interrupt will fire.
+ * We want IT to fire modulo clocktick even if we miss/skip some.
+ * But those interrupts don't in fact get delivered that regularly.
+ */
+ next_tick = now + cycles_remainder;
+
cpu_data[cpu].it_value = next_tick;
- while (nticks--) {
+ /* Skip one clocktick on purpose if we are likely to miss next_tick.
+ * We want to avoid the new next_tick being less than CR16.
+ * If that happened, itimer wouldn't fire until CR16 wrapped.
+ * We'll catch the tick we missed on the tick after that.
+ */
+ if (!(cycles_remainder >> 13))
+ next_tick += cpt;
+
+ /* Program the IT when to deliver the next interrupt. */
+ /* Only bottom 32-bits of next_tick are written to cr16. */
+ mtctl(next_tick, 16);
+
+
+ /* Done mucking with unreliable delivery of interrupts.
+ * Go do system house keeping.
+ */
#ifdef CONFIG_SMP
- smp_do_timer(regs);
+ smp_do_timer(regs);
#else
- update_process_times(user_mode(regs));
+ update_process_times(user_mode(regs));
#endif
- if (cpu == 0) {
- write_seqlock(&xtime_lock);
- do_timer(regs);
- write_sequnlock(&xtime_lock);
- }
+ if (cpu == 0) {
+ write_seqlock(&xtime_lock);
+ do_timer(regs);
+ write_sequnlock(&xtime_lock);
}
-
+
/* check soft power switch status */
if (cpu == 0 && !atomic_read(&power_tasklet.count))
tasklet_schedule(&power_tasklet);
@@ -109,14 +168,12 @@ #endif
EXPORT_SYMBOL(profile_pc);
-/*** converted from ia64 ***/
/*
* Return the number of micro-seconds that elapsed since the last
* update to wall time (aka xtime aka wall_jiffies). The xtime_lock
* must be at least read-locked when calling this routine.
*/
-static inline unsigned long
-gettimeoffset (void)
+static inline unsigned long gettimeoffset (void)
{
#ifndef CONFIG_SMP
/*
@@ -124,21 +181,47 @@ #ifndef CONFIG_SMP
* Once parisc-linux learns the cr16 difference between processors,
* this could be made to work.
*/
- long last_tick;
- long elapsed_cycles;
+ unsigned long now;
+ unsigned long prev_tick;
+ unsigned long next_tick;
+ unsigned long elapsed_cycles;
+ unsigned long usec;
+ unsigned long cpuid = smp_processor_id();
+ unsigned long cpt = clocktick;
+
+ next_tick = cpu_data[cpuid].it_value;
+ now = mfctl(16); /* Read the hardware interval timer. */
+
+ prev_tick = next_tick - cpt;
+
+ /* Assume Scenario 1: "now" is later than prev_tick. */
+ elapsed_cycles = now - prev_tick;
+
+/* aproximate HZ with shifts. Intended math is "(elapsed/clocktick) > HZ" */
+#if (HZ = 1000)
+ if (elapsed_cycles > (cpt << 10) )
+#elif (HZ = 250)
+ if (elapsed_cycles > (cpt << 8) )
+#elif (HZ = 100)
+ if (elapsed_cycles > (cpt << 7) )
+#elif
+#warn WTF is HZ set to anyway?
+ if (elapsed_cycles > (HZ * cpt) )
+#endif
+ {
+ /* Scenario 3: clock ticks are missing. */
+ printk (KERN_CRIT "gettimeoffset(CPU %ld): missing %ld ticks!"
+ " cycles %lX prev/now/next %lX/%lX/%lX clock %lX\n",
+ cpuid, elapsed_cycles / cpt,
+ elapsed_cycles, prev_tick, now, next_tick, cpt);
+ }
- /* it_value is the intended time of the next tick */
- last_tick = cpu_data[smp_processor_id()].it_value;
+ /* FIXME: Can we improve the precision? Not with PAGE0. */
+ usec = (elapsed_cycles * 10000) / PAGE0->mem_10msec;
- /* Subtract one tick and account for possible difference between
- * when we expected the tick and when it actually arrived.
- * (aka wall vs real)
- */
- last_tick -= clocktick * (jiffies - wall_jiffies + 1);
- elapsed_cycles = mfctl(16) - last_tick;
-
- /* the precision of this math could be improved */
- return elapsed_cycles / (PAGE0->mem_10msec / 10000);
+ /* add in "lost" jiffies */
+ usec += cpt * (jiffies - wall_jiffies);
+ return usec;
#else
return 0;
#endif
@@ -149,6 +232,7 @@ do_gettimeofday (struct timeval *tv)
{
unsigned long flags, seq, usec, sec;
+ /* Hold xtime_lock and adjust timeval. */
do {
seq = read_seqbegin_irqsave(&xtime_lock, flags);
usec = gettimeoffset();
@@ -156,25 +240,13 @@ do_gettimeofday (struct timeval *tv)
usec += (xtime.tv_nsec / 1000);
} while (read_seqretry_irqrestore(&xtime_lock, seq, flags));
- if (unlikely(usec > LONG_MAX)) {
- /* This can happen if the gettimeoffset adjustment is
- * negative and xtime.tv_nsec is smaller than the
- * adjustment */
- printk(KERN_ERR "do_gettimeofday() spurious xtime.tv_nsec of %ld\n", usec);
- usec += USEC_PER_SEC;
- --sec;
- /* This should never happen, it means the negative
- * time adjustment was more than a second, so there's
- * something seriously wrong */
- BUG_ON(usec > LONG_MAX);
- }
-
^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2006-09-23 0:41 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-08-30 4:48 [parisc-linux] [PATCH] timer_interrupt and gettimeoffset Carlos O'Donell
2006-08-31 6:53 ` Grant Grundler
2006-08-31 18:52 ` Carlos O'Donell
2006-08-31 21:46 ` Grant Grundler
2006-09-03 14:54 ` Carlos O'Donell
2006-09-03 19:55 ` Grant Grundler
2006-09-03 20:29 ` James Bottomley
2006-09-03 20:30 ` Carlos O'Donell
2006-09-03 21:22 ` John David Anglin
2006-09-01 22:48 ` Grant Grundler
2006-09-01 22:59 ` Grant Grundler
2006-09-06 6:29 ` [parisc-linux] [PATCH] timer_interrupt #5 Grant Grundler
[not found] ` <44FF2A69.5040102@scarlet.be>
[not found] ` <20060907001944.GA4407@colo.lackof.org>
[not found] ` <450188D8.5000501@scarlet.be>
2006-09-08 18:49 ` Grant Grundler
2006-09-23 0:41 ` John David Anglin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox