From owner-svn-src-head@freebsd.org Sun Jul 19 08:47:25 2015 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 027489A5CB8; Sun, 19 Jul 2015 08:47:25 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id BCAA0151A; Sun, 19 Jul 2015 08:47:24 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id B13023C14AE; Sun, 19 Jul 2015 18:47:15 +1000 (AEST) Date: Sun, 19 Jul 2015 18:47:14 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Mark Johnston 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 In-Reply-To: <20150719003624.GB2808@raichu> Message-ID: <20150719173124.E941@besplex.bde.org> References: <201507180057.t6I0vVhS076519@repo.freebsd.org> <20150718185937.A1301@besplex.bde.org> <20150719003624.GB2808@raichu> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=eZjABOwH c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=kj9zAlcOel0A:10 a=KMhDvbfIn3Rwvs1JuGsA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 19 Jul 2015 08:47:25 -0000 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