> Date: Wed, 15 Jul 2020 20:36:04 -0500
> From: Scott Cheloha <scottchel...@gmail.com>
>
> On Sat, Jul 11, 2020 at 01:16:43PM +0200, Mark Kettenis wrote:
> > > From: Paul Irofti <p...@irofti.net>
> > > Date: Sat, 11 Jul 2020 13:50:37 +0300
> > >
> > > On 2020-07-11 13:46, Mark Kettenis wrote:
> > > >> From: Paul Irofti <p...@irofti.net>
> > > >> Date: Sat, 11 Jul 2020 13:32:22 +0300
> > > >>
> > > >> Hi,
> > > >>
> > > >> Getting lots of messages about people loving the new timekeep
> > > >> functionality, which I am very happy about, but also some that have the
> > > >> skew too large for it to be enabled.
> > > >>
> > > >> I plan on sending a diff next week to improve the situation via RDTSCP
> > > >> on the machines that have it. Which is basically all modern machines.
> > > >>
> > > >> The plan is to have an auxiliary value returned by RDTSCP which
> > > >> identifies the CPU we got the info from so that we can look-up its
> > > >> associated skew in a table saved at init inside the timekeep structure:
> > > >
> > > > I think that is the wrong approach. Instead we should synchronize the
> > > > TSC counters themselves. There are special MSRs you can write the
> > > > offset into IIRC. That seems to be what FreeBSD does.
> > >
> > > Yes, that is another option. I have not looked to see which are more
> > > popular in terms of hardware. Did the MSRs come with RDTSCP? Before? Or
> > > after? We should choose the most inclusive solution I guess. Or we could
> > > have both...
> >
> > One thing to keep in mind is that we only use the TSC as a timecounter
> > on CPUs that have the ITSC feature.
>
> In the meantime...
>
> Can we add the missing LFENCE instructions to userspace and the
> kernel? And can we excise the upper 32 bits? IIRC there was talk of
> doing these things anyway, regardless of how the skew problem is
> addressed.
>
> My understanding is that you need an "LFENCE sandwich" to prevent the
> RDTSC from being reordered in the pipeline.
Hmm, it is my understanding that only the lfence before is needed,
that is why rdtsc_lfence() looks the way it looks. If that isn't the
case, that function should probably be adapted.
> Index: lib/libc/arch/amd64/gen/usertc.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/arch/amd64/gen/usertc.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 usertc.c
> --- lib/libc/arch/amd64/gen/usertc.c 8 Jul 2020 09:17:48 -0000 1.2
> +++ lib/libc/arch/amd64/gen/usertc.c 16 Jul 2020 01:31:31 -0000
> @@ -21,9 +21,13 @@
> static inline u_int
> rdtsc(void)
> {
> - uint32_t hi, lo;
> - asm volatile("rdtsc" : "=a"(lo), "=d"(hi));
> - return ((uint64_t)lo)|(((uint64_t)hi)<<32);
> + uint32_t lo;
> +
> + asm volatile("lfence");
> + asm volatile("rdtsc" : "=a"(lo));
> + asm volatile("lfence");
> +
> + return lo;
> }
>
> static int
> Index: sys/arch/amd64/amd64/tsc.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/tsc.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 tsc.c
> --- sys/arch/amd64/amd64/tsc.c 6 Jul 2020 13:33:06 -0000 1.19
> +++ sys/arch/amd64/amd64/tsc.c 16 Jul 2020 01:31:31 -0000
> @@ -211,7 +211,13 @@ cpu_recalibrate_tsc(struct timecounter *
> u_int
> tsc_get_timecount(struct timecounter *tc)
> {
> - return rdtsc() + curcpu()->ci_tsc_skew;
> + uint32_t lo;
> +
> + asm volatile("lfence");
> + asm volatile("rdtsc" : "=a" (lo));
> + asm volatile("lfence");
> +
> + return lo + curcpu()->ci_tsc_skew;
> }
>
> void
>
>
