Bug 204633
| Summary: | If INVARIANTS is enabled, free() may attempt to acquire sleeping lock | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Jonathan T. Looney <jtl> |
| Component: | kern | Assignee: | Jonathan T. Looney <jtl> |
| Status: | Closed FIXED | ||
| Severity: | Affects Some People | CC: | emaste, markj |
| Priority: | --- | ||
| Version: | CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
|
Description
Jonathan T. Looney
2015-11-17 16:27:39 UTC
I think this indicates a bug in the caller rather than UMA. The nomenclature is a bit confusing: a sleep mutex is just a "default" mutex, i.e. a non-spin mutex. When a thread blocks on a sleep mutex, it enters bounded sleep; "sleep" in the free(9) man page refers to unbounded sleep. The assertion is failing because the thread holds a spin mutex or a critical section, in which case it is not valid to try and acquire a sleep mutex. It could probably be triggered in a non-INVARIANTS kernel too, since uma_zfree_arg() will attempt to acquire the corresponding zone lock, which is also a sleep mutex. (In reply to Mark Johnston from comment #1) > The assertion is failing because the thread holds a spin mutex or a critical section, > in which case it is not valid to try and acquire a sleep mutex. It could probably be > triggered in a non-INVARIANTS kernel too, since uma_zfree_arg() will attempt to > acquire the corresponding zone lock, which is also a sleep mutex. Good point. That makes this very much look like intended behavior. I only saw this when I tried testing my changes with an unusually high load. Presumably, that caused the allocator to need to acquire the zone lock when it would normally not need to do so. I wonder how many other things have slipped through without enough testing to actually trigger the assert? Perhaps, I should propose a man page change to make this more clear. And/or add a check in malloc/free to catch these problems more reliably. (In reply to Jonathan T. Looney from comment #2) I think it would be correct and reasonable to assert that curthread->td_critnest == 0 at the beginning of uma_zalloc_arg() and uma_zfree_arg(). In most cases, UMA can satisfy allocation and free requests without acquiring a lock, so the checking currently done by witness is not consistent. > Perhaps, I should propose a man page change to make this more clear.
> And/or add a check in malloc/free to catch these problems more reliably.
Indeed, it would be useful to have this clarified in the man page, and an explicit check under INVARIANTS in malloc/free would be sensible.
I have a proposed change out for review. See https://reviews.freebsd.org/D4197. A commit references this bug: Author: jtl Date: Thu Nov 19 14:04:54 UTC 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. 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. PR: 204633 Differential Revision: https://reviews.freebsd.org/D4197 Reviewed by: markj Approved by: gnn (mentor) Sponsored by: Juniper Networks Changes: head/share/man/man9/malloc.9 head/sys/kern/kern_malloc.c head/sys/vm/uma_core.c This was "working as intended". But, we used this opportunity to clarify that man page and add extra checks to catch these problems more reliably. |