Date: Wed, 2 Oct 2019 19:39:46 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: Sebastian Huber <sebastian.huber@embedded-brains.de>, FreeBSD <freebsd-hackers@freebsd.org> Subject: Re: Why is tc_get_timecount() called two times in tc_init()? Message-ID: <20191002163946.GE44691@kib.kiev.ua> In-Reply-To: <20191003013314.O2151@besplex.bde.org> References: <0e27fb3e-0f60-68e1-dbba-f17c3d91c332@embedded-brains.de> <20191002140040.GA44691@kib.kiev.ua> <20191003013314.O2151@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Oct 03, 2019 at 02:25:46AM +1000, Bruce Evans wrote: > On Wed, 2 Oct 2019, Konstantin Belousov wrote: > > > On Tue, Oct 01, 2019 at 01:11:18PM +0200, Sebastian Huber wrote: > >> Hello, > >> > >> since this commit > >> > >> https://github.com/freebsd/freebsd/commit/307f787e5a7f > >> > > It is not very useful to pass github hashes around. > > > > I think that the addition of the second tc_get_timecount() was done > > earlier, in r95530, and there it has semi-useful comment > > + /* Warm up new timecounter. */ > > + (void)newtc->tc_get_timecount(newtc); > > + (void)newtc->tc_get_timecount(newtc); > > > > The commit message is not helpful at all. > > The comment was correct when I added it in r48887. Then it was only > attached to the first tc_get_timecounter() call which is necessary to > start up or sync the hardware timecounter in some cases, e.g., for the > i8254. After the warmup, r48887 called the preexisting function > tc_switch_timecount() switch the software state. This function was > unused (ifdefed out) but required only minor modifications. > > r95530 removes tc_switch_timecount() and replaces it by a second call > to tc_get_timecounter() and an assignment of the new timecounter pointer > to the active timecounter pointer, and removes the blank line that > separates the warmup from the application. The second call and the > assignment are all that is left of the function after moving its > initialization. > > The second warmup looks like nonsense in both versions, and it is > unclear how activation by plain assignment can work in the second > version (the first version was in FreeBSD-3 or 4 and was locked by > splclock()). The assignment is sloppy about memory ordering. The > second warmup may have helped by accidentally forcing the assignment > to be after the first warmup. (Some timecounters that need at least > 1 warmup, e.g., the i854, use atomic ops that accidentally give > sufficient ordering.) Later work on ordering may have reduced the > sloppiness. > > > I do not see a timecounter which would need two get_timecount() calls > > to start working properly now, but I can imagine that at time it was. > > I think it never helped much. For the TSC, the 2 calls are ordered only > relatively each other on a single CPU. They are not ordered relative to > memory. For the i8254, 1 call is enough. The ACPI timer does hardware > accesses so it is in between. So the conclusion is that the second call can be removed, am I right ? > > > I added Bruce to Cc: to may be get more context and explanation. > > > >> tc_get_timecount() is called two times in tc_init(). > >> > >> /* > >> * Initialize a new timecounter and possibly use it. > >> */ > >> void > >> tc_init(struct timecounter *tc) > >> { > >> [...] > >> if (tc->tc_quality == timecounter->tc_quality && > >> tc->tc_frequency < timecounter->tc_frequency) > >> return; > >> (void)tc->tc_get_timecount(tc); > >> (void)tc->tc_get_timecount(tc); > >> timecounter = tc; > >> } > >> > >> What is the reason for this procedure? > > Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20191002163946.GE44691>