Date: Thu, 19 Nov 2015 11:39:18 -0800 From: Mark Johnston <markj@FreeBSD.org> To: John Baldwin <jhb@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: <20151119193918.GA60481@wkstn-mjohnston.west.isilon.com> In-Reply-To: <8452745.P4SYfkWpxv@ralph.baldwin.cx> References: <201511191404.tAJE4reJ064779@repo.freebsd.org> <8452745.P4SYfkWpxv@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
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 >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20151119193918.GA60481>