From owner-freebsd-bugs Sun Sep 22 20:44:19 1996 Return-Path: owner-bugs Received: (from root@localhost) by freefall.freebsd.org (8.7.5/8.7.3) id UAA12719 for bugs-outgoing; Sun, 22 Sep 1996 20:44:19 -0700 (PDT) Received: from godzilla.zeta.org.au (godzilla.zeta.org.au [203.2.228.19]) by freefall.freebsd.org (8.7.5/8.7.3) with ESMTP id UAA12658 for ; Sun, 22 Sep 1996 20:44:12 -0700 (PDT) Received: (from bde@localhost) by godzilla.zeta.org.au (8.7.6/8.6.9) id NAA02998; Mon, 23 Sep 1996 13:41:58 +1000 Date: Mon, 23 Sep 1996 13:41:58 +1000 From: Bruce Evans Message-Id: <199609230341.NAA02998@godzilla.zeta.org.au> To: bde@zeta.org.au, julian@whistle.com Subject: Re: kern/1652: Version of itimer patch for -current Cc: freebsd-bugs@freefall.freebsd.org Sender: owner-bugs@FreeBSD.ORG X-Loop: FreeBSD.org Precedence: bulk >> >Also SPL must be considered if the number of processes is large.. >> >> Yes, walking the queue at splclock() defeats the micro-optimized spl >> placement in settimeofday(). It think atomic update is only >> necessary for each itimer, not for all of them together. > >hmm do you think we need any particular locking then? >will we break SMP stuff? splsoftclock() is enough (at least in the !SMP case). It inhibits realitimexpire() so that the itimers don't change underneath you, and in contrast to the itimer initializations in setitimer(), splclock() isn't required for adjusting the individual itimers because `time' isn't accessed. BTW, the micro-delatentized (sp?) splclock() section in hzto() is mostly a waste of time. hzto() is called at splclock() for all callers except one in in_rmx.c. OTOH, it has the wrong interface. It should take a timeval arg instead of always using the global `time'. Callers could then use it for any timeval and if they want the current time they can reduce latency by copying the current time and leaving the splclock() section before calling hzto(). >do we need to lock the process list for SMP ops? Don't know. If you do, then the SMP people must have had to lock hundreds or thousands of other list traversals and could do another easily :-). >> Mihoko Tanaka posted a similar fix >> the other day. >yes and I think we need to do this.. There is more: the for loop in realitexpire() is very fragile. It wouldn't hurt for it to bail out if it is iterated more than a few times. It normally exits immediately. The exceptional cases are when the timeout is delayed (then the itimer may be several ticks behind the real time), when the clock is being slewed (this is equivalent to a delay of a few usec which looks like a delay of 1 tick after rounding), and when someone advances the clock. >> All versions forgot to remove or update the load comment about >> this problem. >eh? > >Oh you mean the comment in the code? >yeah ok, if I did this I'd change the comment.. :-) Oops, LOUD comment. >> I wonder why this problem has been around for so long, and why >> adjkerntz doesn't seem to cause it. > >sorry I don't follow.. >adjkerntz? >that doesn't change the internal time does it? It does, but there are probably no itimers with a nonzero it_interval set, or the delay is too small to see (max iterations of loop = 24 * 3600 * 1000000 / (tick = 10000) = 8640000 ... I guess that would be noticed. >just how we express it? >Oh I see, because we interpret the cmos clock differntly..? >well that will only be at most 24 hours, or 3600x24 loops. >Where we are talking about year sized changes.. That's with an it_interval of 1 second. Worst case is 1 us which gets rounded up to tick = 10000 us. >how about the following variation? (using splsoftclock) >(warning.. cut+paste used) > ... > static void timevalfix __P((struct timeval *)); >+ static void recalc_realtimer(struct timeval); Please put prototypes in alphabetical order and use a consistent style (__P(())). > timevaladd(&runtime, &delta); >+ recalc_realtimer(delta); > LEASE_UPDATETIME(delta.tv_sec); Why not do it inline? > } >+ >+ >+ static void Please use a consistent style (1 blank line between functions). >+ recalc_realtimer(struct timeval delta) Please use a consistent style (old style function headers). Passing a timeval instead of a pointer is just wasteful, since the timeval utility functions want a pointer. OTOH, using `delta' directly is better if the loop is inlined. >+ { >+ struct proc *p; >+ >+ for (p = allproc.lh_first; p != 0; p = p->p_list.le_next) { >+ if (timerisset(&p->p_realtimer.it_value)) { >+ timevaladd(&p->p_realtimer.it_value, &delta); >+ timevalfix(&p->p_realtimer.it_value); >+ } >+ } >+ } The timevalfix() call is bogus because timevaladd() has already done it. The style guide (lines 168 and 193-194) says not to use unnecessary braces. Bruce