Re: Bug fix for PR 26470 for review

[Joerg Sonnenberger <joerg%britannica.bec.de@localhost>]

On Sun, May 25, 2008 at 11:28:28AM -0700, Dennis Ferguson wrote:
> Your change also changes the behaviour of the code when the time is
> stepped backwards.  What I've been pointing out is that this problem
> is fixed at no cost whatsoever by changing the time base used for
> interval timers to uptime (i.e. change the calls to getmicrotime()
> to getmicrouptime()).

Well, setitimer is supposed to change with the real time and I would
argue that settimeofday modifies the real time.

Attached is a patch that handles the easy case (non-overflow) without
64bit arithmetic. That is, if the current time is between the old
time and the new time, it will just use that. In case of overflow, you
need to do more work if you want to provide the overflow count, so it is
not worth optimising that a lot.

Joerg

Index: kern_time.c
===================================================================
RCS file: /data/repo/netbsd/src/sys/kern/kern_time.c,v
retrieving revision 1.147
diff -u -p -r1.147 kern_time.c
--- kern_time.c 8 May 2008 18:56:58 -0000       1.147
+++ kern_time.c 12 Jan 2009 06:30:55 -0000
@@ -923,8 +923,10 @@ sys_timer_getoverrun(struct lwp *l, cons
 void
 realtimerexpire(void *arg)
 {
-       struct timeval now;
+       uint64_t last_val, next_val, interval, now_ms;
+       struct timeval now, next;
        struct ptimer *pt;
+       int backwards;
 
        pt = arg;
 
@@ -936,24 +938,39 @@ realtimerexpire(void *arg)
                mutex_spin_exit(&timer_lock);
                return;
        }
-       for (;;) {
-               timeradd(&pt->pt_time.it_value,
-                   &pt->pt_time.it_interval, &pt->pt_time.it_value);
-               getmicrotime(&now);
-               if (timercmp(&pt->pt_time.it_value, &now, >)) {
-                       /*
-                        * Don't need to check hzto() return value, here.
-                        * callout_reset() does it for us.
-                        */
-                       callout_reset(&pt->pt_ch, hzto(&pt->pt_time.it_value),
-                           realtimerexpire, pt);
-                       mutex_spin_exit(&timer_lock);
-                       return;
-               }
-               mutex_spin_exit(&timer_lock);
-               pt->pt_overruns++;
-               mutex_spin_enter(&timer_lock);
+
+       getmicrotime(&now);
+       backwards = (timercmp(&pt->pt_time.it_value, &now, >));
+       timeradd(&pt->pt_time.it_value, &pt->pt_time.it_interval, &next);
+       /* Handle the easy case of non-overflown timers first. */
+       if (!backwards && timercmp(&next, &now, >)) {
+               pt->pt_time.it_value = next;
+       } else {
+#define TV2MS(x) (((uint64_t)(x)->tv_sec) * 1000000 + (x)->tv_usec)
+               now_ms = TV2MS(&now);
+               last_val = TV2MS(&pt->pt_time.it_value);
+               interval = TV2MS(&pt->pt_time.it_interval);
+#undef TV2MS
+
+               next_val = now_ms +
+                   (now_ms - last_val + interval - 1) % interval;
+
+               if (backwards)
+                       next_val += interval;
+               else
+                       pt->pt_overruns += (now_ms - last_val) / interval;
+
+               pt->pt_time.it_value.tv_sec = next_val / 1000000;
+               pt->pt_time.it_value.tv_usec = next_val % 1000000;
        }
+
+       /*
+        * Don't need to check hzto() return value, here.
+        * callout_reset() does it for us.
+        */
+       callout_reset(&pt->pt_ch, hzto(&pt->pt_time.it_value),
+           realtimerexpire, pt);
+       mutex_spin_exit(&timer_lock);
 }
 
 /* BSD routine to get the value of an interval timer. */