Re: [PATCH v6 3/7] timekeeping: Account for clocksource tick quantisation via NTP

[David Woodhouse]

On Thu, 2026-06-18 at 18:25 -0700, John Stultz wrote:
> 
> > +       /*
> > +        * cycle_interval is a whole number of counter cycles, so the real
> > +        * time it represents differs from the nominal NTP_INTERVAL_LENGTH by
> > +        * up to half a counter period (e.g. +127 PPM on the 3.579545 MHz ACPI
> > +        * PM timer at HZ=1000). Record this fixed per-tick offset, scaled up
> > +        * to a per-second value to match the ntp_update_frequency() addends,
> > +        * so it can be handed to NTP via ntp_clear() and reflected explicitly
> > +        * in tick_length rather than applied behind NTP's back.
> > +        */
> > +       tk->cs_tick_adj = (((s64)tk->xtime_interval - (s64)ntpinterval) <<
> > +                          tk->ntp_error_shift) * NTP_INTERVAL_FREQ;
> > +       tk->ntp_tick = (u64)tk->xtime_interval << tk->ntp_error_shift;
> > 
> 
> So I think it ends up being correct, but this bit setting ntp_tick
> (using xtime_interval instead of ntpinterval) /looks/ wrong at first
> glance.
> 
> Further ntpinterval is now basically only used in the cs_tick_adj but
> is set a fair bit earlier in this function. I wonder if getting rid of
> ntpinterval and open coding it to set cs_tick_adj might make it easier
> to understand (though obviously not trivial) the math? Maybe breaking
> it into two (or more) lines similar to how we set ntp_error in
> logarithmic_accumulation?
> 
> Otherwise looks good!

Thanks.

Yeah, I guess I could set ntp_tick = ntpinterval << tk->ntp_error_shift
precisely as before, and then *subtract* the calculated cs_tick_adj
from it, with a comment that it's going to be added back at the right
place.

The point is that ntp_tick is the 1/HZ part that NTP *disciplines*,
while cs_tick_adj is a *constant* addition that doesn't get scaled by
NTP's ±500PPM adjustment.

Arguably it *should* also get scaled, but it doesn't matter because
it's only up to ~125PPM of a tick anyway, and ±500PPM of ±125PPM is so
far in the noise of the existing variability of the clocksources that
we *really* don't care.

As with most of the other patches in the series, this is just about
*where* we do the accounting, so that we can get things precisely right
especially in the timekeeping_set_reference() case that I'm working to
enable.

Attachment: smime.p7s
Description: S/MIME cryptographic signature