On Thu, 17 Mar 2011, Jung-uk Kim wrote:
Both get_cyclecount(9) and cpu_ticks() do almost exactly the same
thing now assuming set_cputicker() is called with a correct function
before get_cyclecount() is used, which is true for x86, at least.
The only difference is get_cyclecount() may be inlined but I don't
see much gain from the current uses.
Please review the patch. Note I didn't really remove get_cyclecount()
just because some random third-party module may use it as it is a
documented feature while cpu_ticks is an internal KPI.
What do you think?
I like this idea, but see some minor problems:
- cpu_ticks() API needs to become more public and stable, and guarantee to
use the ticker with the highest frequency
- it is hard to see when set_cputicker() is called, but on i386 it is
called very early so there seem to be no problems with the dummy
timecounter. It is called from init_TSC() which is called from
startrtclock() which is called from machdep.c:cpu_startup() just
after the garbage "Good morning" comment which was originally just
banal since it preceeded its code (the printf that prints the copyright
message). The ordering of this is now highly obfuscated, and in fact
there is now the following enormous amount of code between the printf
and its comment:
% enum sysinit_sub_id {
% SI_SUB_DUMMY = 0x0000000, /* not executed; for linker*/
% SI_SUB_DONE = 0x0000001, /* processed*/
% SI_SUB_TUNABLES = 0x0700000, /* establish tunable values */
% SI_SUB_COPYRIGHT = 0x0800001, /* first use of console*/
^^^^^^^^^ prints the copyright
% SI_SUB_SETTINGS = 0x0880000, /* check and recheck settings */
% SI_SUB_MTX_POOL_STATIC = 0x0900000, /* static mutex pool */
% SI_SUB_LOCKMGR = 0x0980000, /* lockmgr locks */
% SI_SUB_VM = 0x1000000, /* virtual memory system init*/
% SI_SUB_KMEM = 0x1800000, /* kernel memory*/
% SI_SUB_KVM_RSRC = 0x1A00000, /* kvm operational limits*/
% SI_SUB_WITNESS = 0x1A80000, /* witness initialization */
% SI_SUB_MTX_POOL_DYNAMIC = 0x1AC0000, /* dynamic mutex pool */
% SI_SUB_LOCK = 0x1B00000, /* various locks */
% SI_SUB_EVENTHANDLER = 0x1C00000, /* eventhandler init */
% SI_SUB_VNET_PRELINK = 0x1E00000, /* vnet init before modules */
% SI_SUB_KLD = 0x2000000, /* KLD and module setup */
% SI_SUB_CPU = 0x2100000, /* CPU resource(s)*/
^^^ calls cpu_startup()
but there seem to be no slow operations (like device initialization) in
there, so cpu_startup() seems to be called early enough.
The function names for clock initialization are almost equally rotted:
- the realtime clock was originally the i8254, and startrtclock()
started it. Now startrtclock() doesn't touch either the the realtime
clock (the timecounter) or the i8254. It only calls init_TSC(). This
might as well be called directly. The i8254 is now initialized (only?)
by i8254, which is called much earlier, from init386(), so that DELAY()
works when called early from console drivers. The same care should be
taken with initializing the TSC for early use, and would be essential
if the i8254 support in DELAY() were removed.
- the correct interface for attaching the realtime clock now seems to be
cpu_initclocks(). This is now used for the TSC (init_TSC_tc()). But
the i8254 is now attached as a timecounter in attimer_attach().
- set_cputicker() has various races and implementation bugs:
- it has no visible locking. This may be safe when it is called at boot
time. It is also called from tsc_levels_changed(). BTW, these calls
still don't know about P-state invariance. They always pass the
parameter saying that the ticker is variable. This despite the
invariance variable being checked to lines after the call that passes
a hard-coded for variance.
- the call to set_cputicker() after machdep.tsc_freq changes the ticker
freqency is missing. This is a feature if the ticker is variable,
since the dynamic ticker calibration code can be more accurate than
a fixed setting, and would undo any fixed setting. But this code is
buggy...
- set cputicker() has some design bugs. It assumes that the tick frequency
is the same across all CPUs, but the TSC is per-CPU. I have an old SMP
system with CPUs of different frequency that can demonstrate bugs from
this.
Bruce
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[email protected]"
