Date: Wed, 16 Mar 2011 16:34:10 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jung-uk Kim <jkim@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, John Baldwin <jhb@FreeBSD.org> Subject: Re: svn commit: r219676 - head/sys/x86/x86 Message-ID: <20110316155820.D4010@besplex.bde.org> In-Reply-To: <201103151714.15829.jkim@FreeBSD.org> References: <201103151947.p2FJlK3L057573@svn.freebsd.org> <201103151701.11708.jhb@freebsd.org> <201103151714.15829.jkim@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 15 Mar 2011, Jung-uk Kim wrote: > On Tuesday 15 March 2011 05:01 pm, John Baldwin wrote: >> On Tuesday, March 15, 2011 3:47:20 pm Jung-uk Kim wrote: >>> Author: jkim >>> Date: Tue Mar 15 19:47:20 2011 >>> New Revision: 219676 >>> URL: http://svn.freebsd.org/changeset/base/219676 >>> >>> Log: >>> Do not let machdep.tsc_freq modify tsc_freq itself. It is bad >>> for i386 as it does not operate atomically. Actually, it serves >>> no purpose. >>> >>> Noticed by: bde >> >> Ouch, please revert! We have depended on this for testing to >> workaround BIOS issues (e.g. the USB SMI# handler firing at a bad >> time during boot causing the TSC frequency to be calculated >> incorrectly). I agree it's a hack that this works, but it has >> actually proven useful in the past. Please revert. I use this for setting timecounters from userland. The sysctl is not a hack, but the primary user interface for managing the TSC frequency. The kern.timecounter.tc.TSC.frequency sysctl is secondary, and is readonly anyway, so it cannot be used to manage the TSC frequency or even the timecounter frequency. > You can still change timecounter frequency. Can you please explain > why you need to change tsc_freq directly? 1. All my scripts change tsc_freq directly. They are portable to systems that don't have kern.timecounter.tc.TSC.frequency or even timecounters. Even for systems that have kern.timecounter.tc.TSC.frequency, the scripts would need messes to use this sysctl to read the TSC frequency and the old sysctl to write the timecounter frequency, since the tsc_freq variable can no longer be changed using its own sysctl and the TSC's TSC's timecounter current frequency can no longer be seen using the TSC's sysctl. 2. tsc_freq is used by other subsystems and it doesn't hurt for it to be as accurate as possible: - cputicker - high resolution kernel profiling These are relatively unimportant since you errors of 1 part per million don't really matter and high resolution kernel profiling is broken. The change doesn't even fix races accessing frequency variables. They are still there when the local variable is copied to the timecounter frequency. Timecounter code divides by tc_frequency, so this race is more serious than in most places (other places tend not to use the TSC when tsc_freq is transiently 0 due to the races). The divisions are in: - tc_init - tc_windup. This is called from a fast interrupt handler with nothing but time-domain locking which doesn't help here. - pps_event. This can also be called from a fast interrupt handler. Locking of ntptime's variables is also broken by calling tc_windup and pps_event from fast interrupt handlers. kern_ntptime.c still says that all its routines must run at splclock or higher. But splclock has decayed to null. Some of ntptimes routines are locked by Giant. This is impossible starting in a fast interrupt handler, so tc_windup and pps_event just call up to ntptime (ntp_update_second() and hardpps()) with no locking whatsoever. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110316155820.D4010>