On Mon, Mar 29, 2021 at 02:00:01PM +0900, YASUOKA Masahiko wrote:
> On Thu, 25 Mar 2021 19:41:35 +0100 (CET)
> Mark Kettenis <mark.kette...@xs4all.nl> wrote:
> >> From: Scott Cheloha <scottchel...@gmail.com>
> >> Date: Thu, 25 Mar 2021 13:18:04 -0500
> >> > On Wed, Mar 24, 2021 at 05:40:21PM +0900, YASUOKA Masahiko wrote:
> >> Which diff did you apply? Yasuoka provided two diffs.
> >>
> >> In any case, ignore this diff:
> >>
> >> > diff --git a/sys/arch/amd64/amd64/tsc.c b/sys/arch/amd64/amd64/tsc.c
> >> > index 238a5a068e1..3b951a8b5a3 100644
> >> > --- a/sys/arch/amd64/amd64/tsc.c
> >> > +++ b/sys/arch/amd64/amd64/tsc.c
> >> > @@ -212,7 +212,8 @@ cpu_recalibrate_tsc(struct timecounter *tc)
> >> > u_int
> >> > tsc_get_timecount(struct timecounter *tc)
> >> > {
> >> > - return rdtsc_lfence() + curcpu()->ci_tsc_skew;
> >> > + //return rdtsc_lfence() + curcpu()->ci_tsc_skew;
> >> > + return rdtsc_lfence();
> >> > }
> >> >
> >> > void
> >>
> >>
> >> We don't want to discard the skews, that's wrong.
>
> I'm sorry for the confusion.
No problem.
> >> The reason it "fixes" Yasuoka's problem is because the real skews
> >> on the ESXi VMs in question are probably close to zero but our
> >> synchronization algorithm is picking huge (wrong) skews due to
> >> some other variable interfering with our measurement.
> >
> > Right. If a VM exit happens while we're doing our measurement, you'll
> > see a significant delay. And a guest OS can't prevent those from
> > happening. But even on real hardware SMM mode may interfere with our
> > measurement.
>
> For machines like the ESXi VMs, the measurement seems to have to
> exclude such delayed values as outliers. I think taking a lot of
> samples and choice the minimum is a good enough way for the purpose.
>
> I updated the diff.
>
> - delete lines for debug
> - make tsc quality lower if skew is not good enough
> - reduce difference from NetBSD
>
> comment? ok?
If more iterations fixes your problem, great. It isn't going to make
things worse for machines with sync'd TSCs, makes the TSC usable on
another class of machine, and is relatively cheap.
This is ok cheloha@.
You need another ok, though.
> Index: sys/arch/amd64//amd64/tsc.c
> ===================================================================
> RCS file: /disk/cvs/openbsd/src/sys/arch/amd64/amd64/tsc.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 tsc.c
> --- sys/arch/amd64//amd64/tsc.c 23 Feb 2021 04:44:30 -0000 1.23
> +++ sys/arch/amd64//amd64/tsc.c 29 Mar 2021 04:18:31 -0000
> @@ -38,6 +38,7 @@ int tsc_is_invariant;
>
> #define TSC_DRIFT_MAX 250
> #define TSC_SKEW_MAX 100
> +#define TSC_SYNC_ROUNDS 1000
> int64_t tsc_drift_observed;
>
> volatile int64_t tsc_sync_val;
> @@ -235,6 +236,7 @@ tsc_timecounter_init(struct cpu_info *ci
> printf("%s: disabling user TSC (skew=%lld)\n",
> ci->ci_dev->dv_xname, (long long)ci->ci_tsc_skew);
> tsc_timecounter.tc_user = 0;
> + tsc_timecounter.tc_quality = -1000;
> }
>
> if (!(ci->ci_flags & CPUF_PRIMARY) ||
> @@ -314,13 +316,19 @@ tsc_read_bp(struct cpu_info *ci, uint64_
> void
> tsc_sync_bp(struct cpu_info *ci)
> {
> + int i, val, diff;
> uint64_t bptsc, aptsc;
>
> - tsc_read_bp(ci, &bptsc, &aptsc); /* discarded - cache effects */
> - tsc_read_bp(ci, &bptsc, &aptsc);
> + val = INT_MAX;
> + for (i = 0; i < TSC_SYNC_ROUNDS; i++) {
> + tsc_read_bp(ci, &bptsc, &aptsc);
> + diff = bptsc - aptsc;
> + if (abs(diff) < abs(val))
> + val = diff;
> + }
>
> /* Compute final value to adjust for skew. */
> - ci->ci_tsc_skew = bptsc - aptsc;
> + ci->ci_tsc_skew = val;
> }
>
> /*
> @@ -351,8 +359,10 @@ tsc_post_ap(struct cpu_info *ci)
> void
> tsc_sync_ap(struct cpu_info *ci)
> {
> - tsc_post_ap(ci);
> - tsc_post_ap(ci);
> + int i;
> +
> + for (i = 0; i < TSC_SYNC_ROUNDS; i++)
> + tsc_post_ap(ci);
> }
>
> void
