svn commit: r291074 - in head: share/man/man9 sys/kern sys/vm
John Baldwin
jhb at freebsd.org
Fri Nov 20 17:28:47 UTC 2015
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
More information about the svn-src-all
mailing list