Date: Thu, 19 Nov 2015 14:47:57 -0500 From: "Jonathan T. Looney" <jtl@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org> Subject: Re: svn commit: r291074 - in head: share/man/man9 sys/kern sys/vm Message-ID: <D2738CAB.4BD05%jlooney@juniper.net> In-Reply-To: <8452745.P4SYfkWpxv@ralph.baldwin.cx> References: <201511191404.tAJE4reJ064779@repo.freebsd.org> <8452745.P4SYfkWpxv@ralph.baldwin.cx>
index | next in thread | previous in thread | raw e-mail
On 11/19/15, 12:58 PM, "John Baldwin" <jhb@freebsd.org> wrote: >On Thursday, November 19, 2015 02:04:53 PM Jonathan T. Looney wrote: >> 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). The problem is that we don't always acquire a lock when calling malloc or free. In fact, on a lightly-loaded system and tested at low scale, it is possible for a raft of malloc and free calls to be handled in the cache without acquiring a lock. Therefore, even with WITNESS enabled, you won't see any indication that you've just written bad code. >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. Point taken. >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. As noted above, the point wasn't to enable checking when WITNESS was disabled; rather, the point was to make the existing checks more consistent. Basically, if you do something that engenders a panic at high scale, you should get consistent behavior at low scale, too. >Another question you might consider is why are you using spin mutexes in >the >first place (and then calling malloc())? Actually, it was accidental in this case. I hit this while testing some changes. I had accidentally added a malloc inside a critical section, but only realized it while testing at high scale where my free call couldn't be handled from the cache. Granted, that was a bug in my code. But, it would have been nice to have had WITNESS slap me in the face sooner than it did. Jonathanhome | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D2738CAB.4BD05%jlooney>