Date: Sat, 5 Oct 2019 18:22:20 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Evans <brde@optusnet.com.au> Cc: Poul-Henning Kamp <phk@phk.freebsd.dk>, Sebastian Huber <sebastian.huber@embedded-brains.de>, Warner Losh <imp@bsdimp.com>, Konstantin Belousov <kostikbel@gmail.com>, FreeBSD <freebsd-hackers@freebsd.org> Subject: Re: Why is tc_get_timecount() called two times in tc_init()? Message-ID: <20191005171343.X925@besplex.bde.org> In-Reply-To: <20191005024530.U1757@besplex.bde.org> References: <0e27fb3e-0f60-68e1-dbba-f17c3d91c332@embedded-brains.de> <20191002140040.GA44691@kib.kiev.ua> <20191003013314.O2151@besplex.bde.org> <20191002163946.GE44691@kib.kiev.ua> <20191003030837.C2787@besplex.bde.org> <20191003084021.GI44691@kib.kiev.ua> <CANCZdfpcOBJiYAKafhiWZS2g4vnLGVvzqhaOXetSSnU2Hj91nw@mail.gmail.com> <47834.1570116246@critter.freebsd.dk> <141ee0af-2ff4-50fc-b4e4-6d1fc47e04f3@embedded-brains.de> <60167.1570198248@critter.freebsd.dk> <20191005024530.U1757@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 5 Oct 2019, Bruce Evans wrote:
> On Fri, 4 Oct 2019, Poul-Henning Kamp wrote:
>>
>> As long as the counter can be read atomically and does not roll over
>> in a matter of milliseconds, two reads are not necessary.
>
> The i8254 timecounter rolls over in a matter of microseconds if suitably
> (mis)configured. E.g., for pcaudio i8254 periodic timer was run at
> 16 kHz so it rolled over every 74 or 75 cycles. I only used this for
> stress tests.
> ...
> No matter how fast the counter can roll over, 2 reads are only useful
> or needed accidentally. And they are needed for rollover detection
> for the i8254 timecounter:
>
> XX low = inb(TIMER_CNTR0);
> XX high = inb(TIMER_CNTR0);
> XX count = i8254_max_count - ((high << 8) | low);
> XX if (count < i8254_lastcount ||
>
> i8254_lastcount is garbage after only 1 read, but it is used here at the
> start of the rollover detection. Thus the first read returns garbage
> and also leaves some internal state as garbage, but it will update
> i8254_lastcount in its internal state and that is enough for the second
> read to work correctly.
Oops, actually no warmup is needed for some cases, but if warmup is needed
then 2 calls are little better than 1 for giving it.
The internal state is managed by both i8254_get_timecount() and clkintr().
When clkintr() is not active, no warmup is needed. Then the first call to
i8254_get_timecount() returns a random number that works as a reference
point for subsequent calls, so everything works if this is used immediately
to set the reference point th_offset. But when clkintr() is not active,
the internal state is not fully initialized until the next clkintr()...
> XX (!i8254_ticked && (clkintr_pending ||
> XX ((count < 20 || (!(flags & PSL_I) &&
> XX count < i8254_max_count / 2u)) &&
> XX i8254_pending != NULL && i8254_pending(i8254_intsrc))))) {
> XX i8254_ticked = 1;
> XX i8254_offset += i8254_max_count;
> XX }
... since this fails to advance i8254_offset when it should, the next
clkintr() does the advance, so the random reference point returned here
becomes invalid (the time appears to step forward).
> The need for 2 calls is only an optimization, but I don't a better way.
The better way is is to just add a warmup/initialization method and call
that instead of abusing tc_get_timecount(). Warmup is only enough if
the timer is already running. Then a null warmup may be enough. The
simplifications in r137037 depend on this. All attachable timecounters
are assumed to be running all the time, so that making one active doesn't
require starting its timer. For the i8254 timecounter, there were 2 cases
as above:
- i8254 not used for hardclock(), so not interrupting. No warmup needed
- i8254 used for hardclock(), so interrupting. Syncing with its interrupt
needed but not done.
Now event timers give more cases:
- i8254 used for one-shot timeouts, so interrupting. Syncing with its
interrupt needed but not done (programming one-shot timeouts doesn't
change internal state so leaves it as garbage). Syncing was done for
pcaudio by only reprogramming the timeouts in the interrupt handler
under suitable locks.
A related bug:
- the kern.timecounter.tc.<timecounter>.frequency sysctl gives a random
number corresponding to the above for timecounters other than the
active one unless they have not wrapped since they were last read.
Rollover is only detected for the active timecounter. The random
number is fairly predictable if the hardware timecounter is real
hardware (without complications for interrupt handling...). Reading
it twice is useless for fixing this.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20191005171343.X925>