Date: Thu, 19 Nov 2015 09:58:45 -0800 From: John Baldwin <jhb@freebsd.org> To: "Jonathan T. Looney" <jtl@freebsd.org> Cc: 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: <8452745.P4SYfkWpxv@ralph.baldwin.cx> In-Reply-To: <201511191404.tAJE4reJ064779@repo.freebsd.org> References: <201511191404.tAJE4reJ064779@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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). 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8452745.P4SYfkWpxv>