Re: [RFC PATCH v2 2/8] timekeeping: Account for clawback adjustment in ntp_error

[John Stultz]

On Tue, May 19, 2026 at 3:04 AM David Woodhouse <dwmw2@xxxxxxxxxxxxx> wrote:
> On Mon, 2026-05-18 at 18:59 -0700, John Stultz wrote:
> > On Sun, May 17, 2026 at 3:03 PM David Woodhouse <dwmw2@xxxxxxxxxxxxx> wrote:
> > >
> > > From: David Woodhouse <dwmw@xxxxxxxxxxxx>
> > >
> > > timekeeping_apply_adjustment() modifies xtime_nsec to maintain vDSO
> > > monotonicity when mult changes:
> > >
> > >     xtime_nsec -= offset
> > >
> > > This ensures that the time reported to userspace doesn't jump when the
> > > multiplier is adjusted. However, ntp_error — which tracks the difference
> > > between intended and actual clock position — was not updated to reflect
> > > this change.
> > >
> > > After a mult change, xtime_nsec has moved but ntp_error still reflects
> > > the old position. For the normal ±1 dithering this is negligible (the
> > > adjustments cancel over time), but for larger mult changes — such as
> > > when an external reference clock sets a new frequency — the one-time
> > > uncompensated offset is significant (~38ns for a 700-count mult change).
> > >
> > > Fix by adjusting ntp_error by the same amount:
> > >
> > >     ntp_error += offset << ntp_error_shift
> > >
> > > This keeps ntp_error consistent with the actual xtime_nsec position
> > > after the clawback.
> > >
> > > Fixes: 1b1b3e2a3671 ("timekeeping: Rework frequency adjustments to work better w/ nohz")
> >
> > That doesn't seem to be the right commit. Do you mean dc491596f639 ?
>
> Er yes, that 1b1b commit doesn't even exist. I've been keeping the AI
> on a *very* tight rein as I navigate all this, but that one escaped.

You probably should be including the Assisted-by tags then.
https://docs.kernel.org/process/coding-assistants.html

> > Also, since you're re-adding it, could you add a detailed rationale to
> > the comment in timekeeping_apply_adjustment()? (It had long been on my
> > todo, but by the time I started adding the commits the details had
> > faded and I never got the time to re-derive the math.)
>
> I was hoping not to have to think about that part. The fact here is
> that it *does* apply an offset to 'xtime' and thus of course the delta
> from xtime(A) to where we ought to be right now (B) has changed by the
> same amount.
>
> Calculating *what* that offset should be, is... above my pay grade :)
>
> And I do think it's mostly working, so is there a particular reason you
> want me to take a closer look?

Well, mostly just to document the context you have and the justification for
adding the ntp_error adjustment.  I think you've done a reasonable job
articulating it in commit message and discussion here, but having it
in the comment helps others understand how its drerived in the future.
Part of why the ntp_err adjustment was previously dropped is because I
never got around to documenting the math for why it was necessary, so
I didn't have a case to keep it.


> Because the moment I start looking at the comment, I see the part which
> says
>          * So given the same offset value, we need the time to be the same
>          * both before and after the freq adjustment.
>
> ... and I come to believe that 'before' the freq adjustment is actually
> some point in the *future*; the last counter reading at which a vDSO
> *currently* running on another CPU might possibly apply the faster
> formula from the previous tick? And then my brain falls out and I have
> to sit under the desk rocking back and forth for a while... ?

So the timekeeping seq locking should avoid vDSOs from using stale
values across mult updates.
However, the "fast" (a name I hate, really its just lockfree)
ktime_get_real_fast_ns() and friends do not have that protection.

But yea, it does get subtle and complicated, which is all the more
reason to make sure we have things well documented.

> > Miroslav's review and input here would also be good.
>
> Ack. Thomas had nudged me to add Miroslav to Cc, which
> get_maintainers.pl had not.

Yeah. Maybe it would be good to get Miroslav added as a reviewer in
the MAINTAINERS file.

Miroslav: Any objection to that?

thanks
-john