svn commit: r291074 - in head: share/man/man9 sys/kern sys/vm

[Mark Johnston]

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:
> > Author: jtl
> > Date: Thu Nov 19 14:04:53 2015
> > New Revision: 291074
> > URL: https://svnweb.freebsd.org/changeset/base/291074
> > 
> > Log:
> >   Consistently enforce the restriction against calling malloc/free when in a
> >   critical section.
> >   
> >   uma_zalloc_arg()/uma_zalloc_free() may acquire a sleepable lock on the
> >   zone. The malloc() family of functions may call uma_zalloc_arg() or
> >   uma_zalloc_free().
> >   
> >   The malloc(9) man page currently claims that free() will never sleep.
> >   It also implies that the malloc() family of functions will not sleep
> >   when called with M_NOWAIT. However, it is more correct to say that
> >   these functions will not sleep indefinitely. Indeed, they may acquire
> >   a sleepable lock. However, a developer may overlook this restriction
> >   because the WITNESS check that catches attempts to call the malloc()
> >   family of functions within a critical section is inconsistenly
> >   applied.
> 
> Mutexes are not sleepable locks.  sx(9) locks are sleepable locks.  "sleep"
> in synchronization language in FreeBSD means "indefinite sleep", or at least
> any resource contention that cannot be alleviated purely by CPU execution
> (when you "block" on a mutex, you will get to run once the lock owner has
> sufficient CPU time to make progress and release the mutex, whereas waiting
> for a disk I/O to complete (and thus possibly for M_WAITOK malloc()) or a
> network packet to arrive is not purely dependent on CPU cycles).
> 
> The locking(9) page expounds on this more and explicitly lists which locks
> are sleepable and which are not.
> 
> >   This change clarifies the language of the malloc(9) man page to clarify
> >   the restriction against calling the malloc() family of functions
> >   while in a critical section or holding a spin lock. It also adds
> >   KASSERTs at appropriate points to make the enforcement of this
> >   restriction more consistent.
> 
> 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.

> 
> 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.
> 
> Another question you might consider is why are you using spin mutexes in the
> first place (and then calling malloc())?  Other OS's that I am familiar with
> do not permit you to malloc() while holding a spin lock (Linux, Solaris, OS X,
> etc.).  This is fairly common and even folks who aren't familiar with FreeBSD
>  and use MTX_SPIN in drivers because they are used to using spin locks in
> drivers on Linux (when they should use MTX_DEF instead) don't make this
> mistake.
> 
> -- 
> John Baldwin
>