From owner-freebsd-hackers@freebsd.org Sat Oct 5 08:22:34 2019 Return-Path: Delivered-To: freebsd-hackers@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id A6E4412BA52 for ; Sat, 5 Oct 2019 08:22:34 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 46lfqr3521z4SZv for ; Sat, 5 Oct 2019 08:22:32 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id B0C38363A37; Sat, 5 Oct 2019 18:22:24 +1000 (AEST) Date: Sat, 5 Oct 2019 18:22:20 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Bruce Evans cc: Poul-Henning Kamp , Sebastian Huber , Warner Losh , Konstantin Belousov , FreeBSD Subject: Re: Why is tc_get_timecount() called two times in tc_init()? In-Reply-To: <20191005024530.U1757@besplex.bde.org> Message-ID: <20191005171343.X925@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> <47834.1570116246@critter.freebsd.dk> <141ee0af-2ff4-50fc-b4e4-6d1fc47e04f3@embedded-brains.de> <60167.1570198248@critter.freebsd.dk> <20191005024530.U1757@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=D+Q3ErZj c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=kj9zAlcOel0A:10 a=hPcphjJ6FNuFfpMK5-0A:9 a=oKGDZX8tWB3N5HoI:21 a=y4fVjnv8CydkU4kY:21 a=CjuIK1q_8ugA:10 X-Rspamd-Queue-Id: 46lfqr3521z4SZv X-Spamd-Bar: - Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=pass (mx1.freebsd.org: domain of brde@optusnet.com.au designates 211.29.132.249 as permitted sender) smtp.mailfrom=brde@optusnet.com.au X-Spamd-Result: default: False [-1.30 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; RCVD_IN_DNSWL_LOW(-0.10)[249.132.29.211.list.dnswl.org : 127.0.5.1]; FROM_HAS_DN(0.00)[]; FREEMAIL_FROM(0.00)[optusnet.com.au]; R_SPF_ALLOW(-0.20)[+ip4:211.29.132.0/23:c]; MIME_GOOD(-0.10)[text/plain]; SUBJECT_ENDS_QUESTION(1.00)[]; DMARC_NA(0.00)[optusnet.com.au]; IP_SCORE_FREEMAIL(0.00)[]; RCPT_COUNT_FIVE(0.00)[6]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; RWL_MAILSPIKE_POSSIBLE(0.00)[249.132.29.211.rep.mailspike.net : 127.0.0.17]; TO_MATCH_ENVRCPT_SOME(0.00)[]; TO_DN_ALL(0.00)[]; IP_SCORE(0.00)[ip: (-5.09), ipnet: 211.28.0.0/14(-3.22), asn: 4804(-2.37), country: AU(0.01)]; FREEMAIL_TO(0.00)[optusnet.com.au]; RCVD_NO_TLS_LAST(0.10)[]; FROM_EQ_ENVFROM(0.00)[]; R_DKIM_NA(0.00)[]; FREEMAIL_ENVFROM(0.00)[optusnet.com.au]; ASN(0.00)[asn:4804, ipnet:211.28.0.0/14, country:AU]; MIME_TRACE(0.00)[0:+]; RCVD_COUNT_TWO(0.00)[2] X-Mailman-Approved-At: Sat, 12 Oct 2019 23:39:36 +0000 X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 05 Oct 2019 08:22:34 -0000 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..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