Skip site navigation (1)Skip section navigation (2)
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>