Re: CVS commit: src

[Takashi YAMAMOTO <yamt9999%gmail.com@localhost>]

On Wed, Mar 18, 2026 at 9:18 PM Taylor R Campbell <riastradh%netbsd.org@localhost> wrote:
>
> > Module Name:    src
> > Committed By:   yamt
> > Date:           Tue Mar 17 08:12:54 UTC 2026
> >
> > Modified Files:
> >         src/sys/kern: subr_time_arith.c
> >         src/tests/kernel: t_time_arith.c
> >
> > Log Message:
> > itimer_transition: do not keep it_value unchanged after firing the event
>
> This appears to have broken the tests, cases 47 through 56:
>
> https://releng.netbsd.org/b5reports/i386/2026/2026.03.17.08.20.52/test.html#kernel_t_time_arith_itimer_transitions

hm, i'm sure i've run these tests locally.
maybe i was using atf wrong.
let me recheck.

>
> The change also makes several tests inconsistent with the explanation
> of the reasoning justifying them:
>
> >       /*
> >        * Fired more than one interval early -- treat clock as wound
> >        * backwards, not counting overruns.  Advance to the next
> >        * integral multiple of it_interval starting from it_value.
> >        */
> >       [0] = {{.it_value = {3,0}, .it_interval = {1,0}},
> > -            {0,1}, {1,0}, 0,
> > +            {0,1}, {4,0}, 0,
> >              NULL},
>
> OK, the comment was not clear here, but what it meant was to advance
> to the earliest value of it_value + k*it_interval, for some k, after
> the current time {0,1} = 0.0000000001sec.  And that is {1,0} = 1sec
> (with k = -2).

at this point, the signal or something for {3.0} has already been fired.
the next event should be the one for {4,0}.

>
> Consider what happens if we set a periodic timer that runs once per
> second starting from {0,0}.  The timer last fired at {59,1}, and is
> scheduled to fire at {60,0} next; at {59,999999999}, we wound the
> clock back by a minute to {0,0}, and then at {0,1} the timer fires
> again.

my logic is that, because the events for {0, 0} ... {59, 0} have been fired,
the next one should be for {60, 0}.

>
> Should there be a delay of another _minute_ before the next time this
> _once-per-second_ timer is scheduled to fire, at {61,0}, or should
> there be a delay of around a _second_ before the then, at {1,0}?
>
> I think {1,0} is better-justified (or maybe {2,0}), and {61,0} is
> surely wrong.
>
> (See PR kern/58925: `itimer(9) responds erratically to clock wound
> back' <https://gnats.NetBSD.org/58925> for what prompted the current
> logic.)

IMO, the correct answer for the question in the PR is (e).
it doesn't make sense to fire "an interval timer to fire every minute
starting at 03:00:00 on the real-time clock" at 02:01:00.

i suppose that it's somehow expected for realtime timers to behave a
bit strange on a large time adjustment.
although there might be a way to make it nicer, your trick doesn't
seem so nice as it doesn't work well for the cases i mentioned in the
commit message.

>
> >       /*
> >        * Fired exactly one interval early.  Treat this too as clock
> >        * wound backwards.
> >        */
> >       [7] = {{.it_value = {3,0}, .it_interval = {1,0}},
> > -            {2,0}, {3,0}, 0,
> > +            {2,0}, {4,0}, 0,
> >              NULL},
>
> Ditto.  This is an edge case between above and below, but the comment
> should match the test case, and I think it is better for the actual
> delays to aim for <2*it_interval rather than <=2*it_interval (but it's
> not that big a deal either way).
>
> >       /*
> >        * Fired less than one interval early -- callouts and real-time
> >        * clock might not be perfectly synced, counted as zero
> >        * overruns.  Advance by one interval from the scheduled time.
> >        */
> >       [8] = {{.it_value = {3,0}, .it_interval = {1,0}},
> > -            {2,1}, {3,0}, 0,
> > +            {2,1}, {4,0}, 0,
> >              NULL},
>
> This looks like a reasonable correction of an off-by-one error on my
> part.
>
> > -      *            now [backwards]         overruns    now [forwards]
> > -      *           |                      v    v    v  |
> > -      * |--+----+-*--x----+----+----|----+----+----+--*-x----+-->
> > -      *            \/               |               \/
> > -      *         remainder        last_val        remainder
> > -      *     (zero or negative)                (zero or positive)
> > -      *
> > -      * Set next_val to last_value + k*interval for some k.
> > [...]
> > +      *            overruns   now
> > +      *          v    v    v  |
> > +      *     |----+----+----+--*-x----+-->
> > +      *     |    |         |    |
> > +      * last_val |         |    next (to be computed)
> > +      *          |         |
> > +      *          |       last_tick
> > +      *          |
> > +      *          next (at this point)
>
> It is unfortunate that the diagram no longer represents the
> clock-wound-backwards case.

i removed it because backwards case doesn't actually need special treatment.