Re: [RFC] Enabling CONFIG_NTP_PPS for NOHZ by adding ntp_error to system_time_snapshot

[David Woodhouse]

On Fri, 2026-06-19 at 15:34 +0200, Thomas Gleixner wrote:
> On Fri, Jun 19 2026 at 01:33, David Woodhouse wrote:
> > @@ -1285,6 +1286,45 @@ void ktime_get_snapshot_id(clockid_t clock_id, struct system_time_snapshot *syst
> >  
> >  		nsec_sys = timekeeping_cycles_to_ns(&tk->tkr_mono, now);
> >  		nsec_raw = timekeeping_cycles_to_ns(&tk->tkr_raw, now);
> > +
> > +		/*
> > +		 * For the NTP-disciplined mono-based clocks, report how far
> > +		 * @systime is from the ideal NTP time at @now, in signed ns,
> > +		 * so a caller can land on the ideal line by adding it. Four
> > +		 * terms, summed in ns << NTP_SCALE_SHIFT before converting:
> > +		 *
> > +		 *  - tk->ntp_error, the deviation as of the last update;
> > +		 *  - (cycle_delta * ntp_err_frac), the fractional-mult drift
> > +		 *    accrued since then (cycle_delta is at most a tick on a
> > +		 *    tickful kernel, but many ticks' worth under NO_HZ);
> > +		 *  - (cycle_delta * ntp_err_mult), subtracting the applied +1
> > +		 *    mult dither over the same span;
> > +		 *  - the sub-ns fraction @systime dropped when the read was
> > +		 *    truncated to whole ns (low @shift bits, exact despite the
> > +		 *    multiply overflowing).
> > +		 *
> > +		 * RAW is undisciplined and AUX has its own discipline, so they
> > +		 * carry no ntp_error.
> 
> AUX has ntp_error too. AUX clocks have a per clock NTP instance, which
> work exactly like the main timerkeeper's one. Only CLOCK_MONOTONIC_RAW
> needs to be excluded.

Ack.

> > +		 */
> > +		if (clock_id == CLOCK_REALTIME || clock_id == CLOCK_MONOTONIC ||
> > +		    clock_id == CLOCK_BOOTTIME) {
> > +			u32 nes = tk->ntp_error_shift;
> > +			u64 cycle_delta = (now - tk->tkr_mono.cycle_last) &
> > +					  tk->tkr_mono.mask;
> > +			s64 err = tk->ntp_error +
> > +				(((s64)mul_u64_u64_shr(cycle_delta,
> > +						       tk->ntp_err_frac, 32) -
> > +				  (s64)(cycle_delta * tk->ntp_err_mult)) << nes);
> > +
> > +			err += (s64)((cycle_delta * tk->tkr_mono.mult +
> > +				      tk->tkr_mono.xtime_nsec) &
> > +				     ((1ULL << tk->tkr_mono.shift) - 1)) << nes;
> > +			systime_snapshot->ntp_error =
> > +				(err + (1LL << (NTP_SCALE_SHIFT - 1))) >>
> > +				NTP_SCALE_SHIFT;
> 
> This formatting makes my brain hurt. Can you please split that out into
> a separate function?

Yep. There's also a potential error there — an *additional* discrepancy
comes from the enforced monotonicity that timekeeping_cycles_to_ns()
applies (the case where it just returns tkr->xtime_nsec >> tkr_shift).

I couldn't work out if I cared about the clocksource-is-non-monotonic
casse, and even if I did, what I should do about it. 

I also wasn't sure if this should be a new CLOCK_REALTIME_NONMONOTONIC
or something like that, such that e.g. PTP clients could *ask* for it.

It's all very well hard-coding it in pps_get_ts() and unconditionally
changing the behaviour... I *think* we could justify that. But the
example I actually used in the patch was PTP, and that's slightly
harder to justify the behavioural change.

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