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>
next in thread | previous in thread | raw e-mail | index | archive | help
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. Jonathan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D2738CAB.4BD05%jlooney>