On 11/19/15, 12:58 PM, "John Baldwin" <[email protected]> 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 _______________________________________________ [email protected] mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "[email protected]"
