svn commit: r291074 - in head: share/man/man9 sys/kern sys/vm

[Jonathan T. Looney]

On 11/19/15, 12:58 PM, "John Baldwin" <jhb at 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