Date: Mon, 23 Sep 1996 13:41:58 +1000 From: Bruce Evans <bde@zeta.org.au> To: bde@zeta.org.au, julian@whistle.com Cc: freebsd-bugs@freefall.freebsd.org Subject: Re: kern/1652: Version of itimer patch for -current Message-ID: <199609230341.NAA02998@godzilla.zeta.org.au>
next in thread | raw e-mail | index | archive | help
>> >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 <mihoko@pa.yokogawa.co.jp> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199609230341.NAA02998>