Date: Fri, 20 Nov 2015 08:35:33 -0800 From: John Baldwin <jhb@freebsd.org> To: Mark Johnston <markj@freebsd.org> Cc: "Jonathan T. Looney" <jtl@freebsd.org>, 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 Message-ID: <7664405.qsaSkmW6Va@ralph.baldwin.cx> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?7664405.qsaSkmW6Va>