Date: Sun, 19 Jul 2015 18:47:14 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Mark Johnston <markj@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r285664 - in head/sys: kern sys Message-ID: <20150719173124.E941@besplex.bde.org> In-Reply-To: <20150719003624.GB2808@raichu> References: <201507180057.t6I0vVhS076519@repo.freebsd.org> <20150718185937.A1301@besplex.bde.org> <20150719003624.GB2808@raichu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 18 Jul 2015, Mark Johnston wrote: > On Sat, Jul 18, 2015 at 08:55:07PM +1000, Bruce Evans wrote: >> On Sat, 18 Jul 2015, Mark Johnston wrote: >> >>> Log: >>> Pass the lock object to lockstat_nsecs() and return immediately if >>> LO_NOPROFILE is set. Some timecounter handlers acquire a spin mutex, and >>> we don't want to recurse if lockstat probes are enabled. >> >> It is an error to call timecounter code from a low-level place like the >> mutex implementation. This workaround depends on all locks in timecounter >> handlers being LO_NOPROFILE, and that breaks profiling of these locks. >> ... > > I noticed that lock_profile (which predates lockstat a bit) does the > exact same thing to avoid recursion. Specifically, > lock_profile_obtain_lock_{success,failed} return immediately if > LO_NOPROFILE is set on the target lock. As you pointed out, > this change breaks profiling of timecounter locks, but the only > timecounter implementation in the tree that currently acquires a lock > during a timer read is i8254. lock_profile also has another copy of lockstat_nsecs() (spelled nanoseconds()), with different style bugs. The style bugs start with lockstat_nsecs()'s existence and nanoseconds()'s name being too generic. > The other two locks that set MTX_NOPROFILE are the witness lock and the > i386 icu lock. Lock order checking can usually be done without taking > the witness lock, so contention would be unusual, and it would strike > me as strange to profile locking with witness enabled anyway. :) > I'm not sure why i386's icu_lock has profiling disabled; this was done > in r166001, but the commit log doesn't explain the reason. I didn't know that lock profiling was independent of witness. I can't see any reason why profiling must be disabled for icu_lock, but perhaps it should be disabled for efficiency reasons for all low-level mutexes. Low-level mutexes now use combinations of MTX_QUIET and MTX_NOWITNESS with no apparent pattern. It seems right to log everything and check everything by default (except witness's lock must not witness itself recursively), and never hard-code hiding from witness or anything just for efficiency. Then if a locking error is found in a console driver lock (there are many such errors that are not found now), the error must not be reported on the consoles with the locking error. MTX_QUIET's name suggests that it controls printing of error messages, but its documentation is ambiguous: it controls "logging" and it isn't clear if that is in-memory (for future use by witness or anything) or just printing. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150719173124.E941>