From owner-svn-src-all@freebsd.org Fri Nov 20 17:28:47 2015 Return-Path: Delivered-To: svn-src-all@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 0C746A337B8; Fri, 20 Nov 2015 17:28:47 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id DA14A137F; Fri, 20 Nov 2015 17:28:46 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 2C4E8B95E; Fri, 20 Nov 2015 12:28:45 -0500 (EST) From: John Baldwin To: Mark Johnston Cc: "Jonathan T. Looney" , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r291074 - in head: share/man/man9 sys/kern sys/vm Date: Fri, 20 Nov 2015 08:35:33 -0800 Message-ID: <7664405.qsaSkmW6Va@ralph.baldwin.cx> User-Agent: KMail/4.14.3 (FreeBSD/10.2-STABLE; KDE/4.14.3; amd64; ; ) In-Reply-To: <20151119193918.GA60481@wkstn-mjohnston.west.isilon.com> References: <201511191404.tAJE4reJ064779@repo.freebsd.org> <8452745.P4SYfkWpxv@ralph.baldwin.cx> <20151119193918.GA60481@wkstn-mjohnston.west.isilon.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Fri, 20 Nov 2015 12:28:45 -0500 (EST) X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 Nov 2015 17:28:47 -0000 On Thursday, November 19, 2015 11:39:18 AM Mark Johnston wrote: > On Thu, Nov 19, 2015 at 09:58:45AM -0800, John Baldwin wrote: > > On Thursday, November 19, 2015 02:04:53 PM Jonathan T. Looney wrote: > > All of these KASSERTs are redundant. WITNESS will already warn for all of > > these in mtx_lock() itself. If your argument is that you want a panic when > > WITNESS is not present, then the better place to add assertions is in the > > locking primitives themselves (e.g. mtx_lock/rw_*lock). > > I think the argument is that mtx_lock() is not called at all in the > allocation/free path most of the time, so WITNESS will only catch this > sort of bug if you happen to get lucky. But it's always incorrect to call > uma_zalloc or umz_free with a critical section held. > > This is not needed in most APIs, but given that malloc/free and their UMA > underpinnings are rather central, it seemed reasonable to me to add this > extra checking. My bad, I had forgotten that a cached allocation only uses critical_enter() and nothing more. We have an explicit WITNESS_WARN() for all M_WAITOK allocations for precisely the same reason (they don't always block), so the assertions are useful (and not entirely redundant). > > Note that if you are going to document in each section 9 manpage which APIs > > are not safe to call in a critical section, you will need to update just > > about every section 9 manpage. A more prudent approach would probably be to > > instead go the sigaction(2) route and instead document the subset of APIs > > which are permissible to call in critical_enter(9). The list is probably not > > very long. Off the top of my head I can think of sched_*, swi_sched, > > taskqueue_enqueue_fast, and little else. > > > > In summary, I would prefer you to revert this. If you want the assertions to > > fire even when WITNESS is disabled then I think we should move them into the > > the non-sleepable lock primitives themselves so that we catch 90+% of the > > problem APIs instead of just 1. Documenting the "safe" APIs in critical(9) > > is also more scalable than one-off notes in section 9 manpages for similar > > reasons. > > > > Longer term I think it would be nice to have a separate section for section > > 9 pages that indicates which contexts it can be called in, though I'd like > > that to have a consistent name and consistent language. Note though that we > > do not have this section currently for all of section 2/3 to indicate which > > are safe to call in signal context or not, in part because of the enormity of > > the task. Perhaps we can start this with malloc(9) and pull the newly added text into that. I'll try to write up something. I'll try to add something to critical_enter(9) as well. -- John Baldwin