On 05.04.2021 06:13, Scott Cheloha wrote:
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 -0500On 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(); } voidWe 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.
The diff is obviously fine. But it is still a heuristic with no real motivation except for this particular ESXi VM case. So my question about why we choose the minimum instead of the median or the mean has not been answered.
Another issue that I see is that people have not reported, at least publicly, that this runs fine on their normal OpenBSD machines.
If you think you have enough tests and you want to go ahead with the current heuristic, then the diff is obviously fine. OK pirofti@
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
