> Date: Sat, 5 Sep 2020 09:49:25 -0500
> From: Scott Cheloha <[email protected]>
> Cc: Mark Kettenis <[email protected]>, Mike Larkin <[email protected]>
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
>
> On Tue, Aug 25, 2020 at 12:22:19PM -0700, Mike Larkin wrote:
> > On Tue, Aug 25, 2020 at 12:12:36PM -0700, Mike Larkin wrote:
> > > On Mon, Aug 24, 2020 at 01:55:45AM +0200, Mark Kettenis wrote:
> > > > > Date: Sun, 23 Aug 2020 18:11:12 -0500
> > > > > From: Scott Cheloha <[email protected]>
> > > > >
> > > > > Hi,
> > > > >
> > > > > Other BSDs use the TSC to implement delay(9) if the TSC is constant
> > > > > and invariant. Here's a patch to add something similar to our kernel.
> > > >
> > > > If the TSC is fine as a timecounter it should be absolutely fine for
> > > > use as delay(). And we could even use if the TSC isn't synchronized
> > > > between CPUs.
> > > >
> > > > > This patch (or something equivalent) is a prerequisite to running the
> > > > > lapic timer in oneshot or TSC deadline mode. Using the lapic timer to
> > > > > implement delay(9) when it isn't running in periodic mode is too
> > > > > complicated. However, using the i8254 for delay(9) is too slow. We
> > > > > need an alternative.
> > > >
> > > > Hmm, but what are we going to use on machines where the TSC isn't
> > > > constant/invariant?
> > > >
> > > > In what respect is the i8254 too slow? Does it take more than a
> > > > microsecond to read it?
> > >
> > > It's 3 outb/inb pairs to ensure you get the reading correct. So that could
> > > be quite a long time (as cheloha@ points out). Also, that's 6 VM exits if
> > > running virtually (I realize that's not the main use case here but just
> > > saying...)
> > >
> > > IIRC the 3 in/out pairs are the latch command followed by reading the
> > > LSB/MSB
> > > of the counter. It's not MMIO like the HPET or ACPI timer.
> > >
> > > And as cheloha@ also points out, it is highly likely that none of us have
> > > a
> > > real i8254 anymore, much of this is probably implemented in some EC
> > > somewhere
> > > and it's unlikely the developer of said EC put a lot of effort into
> > > optimizing
> > > the implementation of a legacy device like this.
> > >
> > > On the topic of virtualization:
> > >
> > > while (rdtsc() - start < want)
> > > rdtsc();
> >
> > I just realized the original diff didn't do two rdtscs. It did a pause
> > inside the
> > loop. So the effect is not *as* bad as I described but it's still
> > *somewhat* bad.
> >
> > PS - pause loop exiting can be enabled to improve performance in this
> > situation.
>
> What I'm getting from Mike's and kettenis@'s replies is that this is a
> generally good idea.
>
> We should add an HPET fallback for the nasty cases where your TSC has
> drift because the i8254 is slooooooow. But "hpet_delay()" can wait
> for a later patch because we haven't switched the lapic into oneshot
> mode yet, so lapic_delay() is still useable and fast.
>
> So, is this ok? Or do I need to tweak something? I think I'm setting
> delay_func to tsc_delay under the right circumstances: we know the TSC
> is invariant.
>
> kettenis@: as I mentioned, we need to do the delay_func pointer
> comparison in lapic_calibrate_timer() to keep from clobbering
> tsc_delay. We can't compare it with NULL because it is set to
> i8254_delay() by default in amd64/machdep.c.
Given that lapic_delay() will disappear in the future, maybe just
adding a local proptotype for tsc_delay() in lapic.c would make sense
instead of adding the header file?
Either way, ok kettenis@
> Index: amd64/lapic.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/lapic.c,v
> retrieving revision 1.56
> diff -u -p -r1.56 lapic.c
> --- amd64/lapic.c 3 Sep 2020 21:38:46 -0000 1.56
> +++ amd64/lapic.c 5 Sep 2020 14:44:08 -0000
> @@ -41,6 +41,7 @@
> #include <machine/codepatch.h>
> #include <machine/cpu.h>
> #include <machine/cpufunc.h>
> +#include <machine/cpuvar.h>
> #include <machine/pmap.h>
> #include <machine/mpbiosvar.h>
> #include <machine/specialreg.h>
> @@ -584,7 +585,8 @@ skip_calibration:
> * Now that the timer's calibrated, use the apic timer routines
> * for all our timing needs..
> */
> - delay_func = lapic_delay;
> + if (delay_func != tsc_delay)
> + delay_func = lapic_delay;
> initclock_func = lapic_initclocks;
> }
> }
> Index: amd64/tsc.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/tsc.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 tsc.c
> --- amd64/tsc.c 23 Aug 2020 21:38:47 -0000 1.20
> +++ amd64/tsc.c 5 Sep 2020 14:44:08 -0000
> @@ -26,6 +26,7 @@
>
> #include <machine/cpu.h>
> #include <machine/cpufunc.h>
> +#include <machine/cpuvar.h>
>
> #define RECALIBRATE_MAX_RETRIES 5
> #define RECALIBRATE_SMI_THRESHOLD 50000
> @@ -252,7 +253,8 @@ tsc_timecounter_init(struct cpu_info *ci
> tsc_timecounter.tc_quality = -1000;
> tsc_timecounter.tc_user = 0;
> tsc_is_invariant = 0;
> - }
> + } else
> + delay_func = tsc_delay;
>
> tc_init(&tsc_timecounter);
> }
> @@ -342,4 +344,15 @@ tsc_sync_ap(struct cpu_info *ci)
> {
> tsc_post_ap(ci);
> tsc_post_ap(ci);
> +}
> +
> +void
> +tsc_delay(int usecs)
> +{
> + uint64_t interval, start;
> +
> + interval = (uint64_t)usecs * tsc_frequency / 1000000;
> + start = rdtsc_lfence();
> + while (rdtsc_lfence() - start < interval)
> + CPU_BUSY_CYCLE();
> }
> Index: include/cpuvar.h
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/include/cpuvar.h,v
> retrieving revision 1.10
> diff -u -p -r1.10 cpuvar.h
> --- include/cpuvar.h 9 Aug 2019 15:20:05 -0000 1.10
> +++ include/cpuvar.h 5 Sep 2020 14:44:08 -0000
> @@ -102,4 +102,6 @@ void tsc_sync_drift(int64_t);
> void tsc_sync_bp(struct cpu_info *);
> void tsc_sync_ap(struct cpu_info *);
>
> +void tsc_delay(int);
> +
> #endif
> Index: include/i82489var.h
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/include/i82489var.h,v
> retrieving revision 1.18
> diff -u -p -r1.18 i82489var.h
> --- include/i82489var.h 4 Oct 2018 05:00:40 -0000 1.18
> +++ include/i82489var.h 5 Sep 2020 14:44:08 -0000
> @@ -128,4 +128,6 @@ extern void lapic_calibrate_timer(struct
> extern void lapic_startclock(void);
> extern void lapic_initclocks(void);
>
> +extern void lapic_delay(int);
> +
> #endif
>
