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

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

On Tue, Jan 18, 2022 at 03:26:36PM +0100, Jan Beulich wrote:
> On 18.01.2022 15:05, Roger Pau Monné wrote:
> > On Tue, Jan 18, 2022 at 02:39:03PM +0100, Jan Beulich wrote:
> >> On 18.01.2022 13:45, Roger Pau Monné wrote:
> >>> On Thu, Jan 13, 2022 at 02:41:31PM +0100, Jan Beulich wrote:
> >>>> Calibration logic assumes that the platform timer (HPET or ACPI PM
> >>>> timer) and the TSC are read at about the same time. This assumption may
> >>>> not hold when a long latency event (e.g. SMI or NMI) occurs between the
> >>>> two reads. Reduce the risk of reading uncorrelated values by doing at
> >>>> least four pairs of reads, using the tuple where the delta between the
> >>>> enclosing TSC reads was smallest. From the fourth iteration onwards bail
> >>>> if the new TSC delta isn't better (smaller) than the best earlier one.
> >>>
> >>> Do you have some measurements as to whether this makes the frequency
> >>> detection any more accurate?
> >>
> >> In the normal case (no SMI or alike) I haven't observed any further
> >> improvement on top of the earlier patch.
> >>
> >>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>>> ---
> >>>> Obviously (I think) instead of having both read_{hpet,pmtmr}_and_tsc()
> >>>> the calibration logic could be folded between HPET and PMTMR, at the
> >>>
> >>> As an intermediate solution you could have a read_counter_and_tsc I
> >>> would think?
> >>
> >> Not sure in which way you view this as "intermediate".
> > 
> > As in not unifying both calibration logic into a single function, but
> > rather just use a generic function to read the counter and TSC.
> > 
> > With your original remark I assumed that you wanted to unify all the
> > calibration code in init_hpet and init_pmtimer into a generic
> > function, but maybe I've misunderstood.
> 
> Oh, I see. I have to admit that I see little value (at this point) to
> break out just the pair-read logic. While I did say we have similar
> issues elsewhere, my initial analysis has left me with the impression
> that those other cases are sufficiently different for such a helper to
> be of no use there.
> 
> >>>> expense of a couple more indirect calls (which may not be that much of a
> >>>> problem as this is all boot-time only). Really such folding would have
> >>>> been possible already before, just that the amount of duplicate code
> >>>> hasn't been this large so far. IOW if so desired, then perhaps the
> >>>> folding should be a separate prereq patch.
> >>>
> >>> You could even pass a platform_timesource as a parameter to that
> >>> generic read counter and TSC helper.
> >>
> >> This was the plan, yes, in case I was asked to follow that route (or
> >> if I decided myself that I'd prefer that significantly over the
> >> redundancy).
> > 
> > I think having a generic read_counter_and_tsc would be better than
> > having read_{hpet,pmtmr}_and_tsc.
> > 
> >>
> >>>> --- a/xen/arch/x86/time.c
> >>>> +++ b/xen/arch/x86/time.c
> >>>> @@ -392,9 +392,36 @@ static u64 read_hpet_count(void)
> >>>>      return hpet_read32(HPET_COUNTER);
> >>>>  }
> >>>>  
> >>>> +static uint32_t __init read_hpet_and_tsc(uint64_t *tsc)
> >>>> +{
> >>>> +    uint64_t tsc_prev = *tsc = rdtsc_ordered(), tsc_min = ~0;
> >>>> +    uint32_t best = best;
> >>>> +    unsigned int i;
> >>>> +
> >>>> +    for ( i = 0; ; ++i )
> >>>> +    {
> >>>> +        uint32_t hpet = hpet_read32(HPET_COUNTER);
> >>>> +        uint64_t tsc_cur = rdtsc_ordered();
> >>>> +        uint64_t tsc_delta = tsc_cur - tsc_prev;
> >>>> +
> >>>> +        if ( tsc_delta < tsc_min )
> >>>> +        {
> >>>> +            tsc_min = tsc_delta;
> >>>> +            *tsc = tsc_cur;
> >>>> +            best = hpet;
> >>>> +        }
> >>>> +        else if ( i > 2 )
> >>>> +            break;
> >>>> +
> >>>> +        tsc_prev = tsc_cur;
> >>>
> >>> Would it be better to set tsc_prev to the current TSC value here in
> >>> order to minimize the window you are measuring? Or else tsc_delta will
> >>> also account for the time in the loop code, and there's more
> >>> likeliness from being interrupted?
> >>
> >> I did consider this (or some variant thereof), but did for the moment
> >> conclude that it wouldn't make enough of a difference. If you think
> >> it would be meaningfully better that way, I'll switch.
> >>
> >> Both here and for the aspect above, please express you preference (i.e.
> >> not in the form of a question).
> > 
> > Let's leave it as-is (just one TSC read per loop iteration).
> > 
> >>> I guess being interrupted four times so that all loops are bad is very
> >>> unlikely.
> >>>
> >>> Since this loop could in theory run multiple times, do we need to
> >>> check that the counter doesn't overflow? Or else the elapsed counter
> >>> value would be miscalculated.
> >>
> >> I don't think I see any issue with overflow here. It's the callers
> >> who need to care about that. And I don't think this function will run
> >> for long enough such that a counter would not only wrap, but also
> >> reach its initial count again.
> > 
> > Right, I haven't expressed myself correctly. It's not overflowing the
> > issue, but rather wrapping to the start count value. Limiting the
> > iterations to some fixed value (10?) in order to remove that
> > possibility completely would be OK for me.
> 
> Hmm, I was in fact hoping (and trying) to get away without any arbitrarily
> chosen numbers. The loop cannot be infinite in any event ... Please let me
> know how strong you feel about putting in place such an arbitrary limit.

It was mostly for safety reasons, I wouldn't expect that loop to need
more than 4 iterations really (which is also an arbitrary chosen
number). Let's leave it without any limit then for the time being.

Roger.