Re: [PATCH 2/2] x86/time: improve TSC / CPU freq calibration accuracy

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jan 12, 2022 at 09:56:12AM +0100, Jan Beulich wrote:
> While the problem report was for extreme errors, even smaller ones would
> better be avoided: The calculated period to run calibration loops over
> can (and usually will) be shorter than the actual time elapsed between
> first and last platform timer and TSC reads. Adjust values returned from
> the init functions accordingly.
> 
> On a Skylake system I've tested this on accuracy (using HPET) went from
> detecting in some cases more than 220kHz too high a value to about
> ±2kHz. On other systems (or on this system, but with PMTMR) the original
> error range was much smaller, with less (in some cases only very little)
> improvement.
> 
> Reported-by: James Dingwall <james-xen@xxxxxxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> There's still a time window for the issue to occur between the final
> HPET/PMTMR read and the following TSC read. Improving this will be the
> subject of yet another patch.
> 
> TBD: Accuracy could be slightly further improved by using a (to be
>      introduced) rounding variant of muldiv64().

I'm unsure we care that much about such fine grained accuracy here.

> TBD: I'm not entirely sure how useful the conditional is - there
>      shouldn't be any inaccuracy from the division when actual equals
>      target (upon entry to the conditional), as then the divisor is
>      what the original value was just multiplied by. And as per the
>      logic in the callers actual can't be smaller than target.

Right, it's just overhead to do the muldiv64 if target == actual.

> TBD: I'm also no longer sure that the helper function is warranted
>      anymore. It started out with more contents, but is now
>      effectively only the [conditional] muldiv64() invocation.

Don't have a strong opinion, I'm fine with the helper, or else I would
likely request that the call to muldiv64 is not placed together with
the return in order to avoid overly long lines.

> 
> I'm afraid I don't see a way to deal with the same issue in init_pit().
> In particular the (multiple) specs I have to hand don't make clear
> whether the counter would continue counting after having reached zero.
> Obviously it wouldn't help to check this on a few systems, as their
> behavior could still be implementation specific.

We could likely set the counter to the maximum value it can hold
and then perform reads in a loop (like it's done for HPET or the PM
timers) and stop when start - target is reached. Not a great solution
either.

Thanks, Roger.