Alan Cox <alc_at_cs.rice.edu> wrote: > On Wed, Apr 28, 2004 at 08:08:17PM -0400, Brian Fundakowski Feldman wrote: > > There are several severe wiring bugs in -CURRENT that I believe I have fixed. > > Please test/review as appropriate if you're affected by any of them. This > > is a superset of the previous patch which just mostly-fixed mlockall(2). > > > > * MAP_FUTUREWIRE was not unset in vmspace_dofree(), causing the next process > > to use that vmspace to wire all of its memory. > > Would setting flags to 0 in vmspace_alloc() accomplish the same? Yeah, same deal, except since we're optimizing by only clearing/ reinitializing some fields for the vmspace-zone objects, it makes a little more sense IMO to have it in the vmspace_dofree(). > > * kmem_*() calls either called vm_map_wire() or set MAP_ENTRY_NOFAULT, but > > kmem_free() did not undo the vm_map_wire() calls. > > I don't understand this comment. kmem_free() calls vm_map_remove(), which > calls vm_map_delete(), which calls vm_map_entry_unwire(), etc. > > However, I don't see any change corresponding to this statement in your > patch. I changed kmem_free() to do an unwire since it is the interface for unmapping wired memory, as opposed to kmem_free_wakeup(). > > * vm_fault_unlock() could not unwire pages that were not in the pmap already, > > leaking them permanently. > > By definition a wired mapping is in the pmap. A wired page can, however, > be mapped by a non-wired mapping. Thus, a non-wired mapping of a wired > page could be absent from the pmap. > > Problems such as PR/29915 are a result of vm_fault_unwire() not understanding > fake pages created by the device pager. Pmap_extract() followed by > PHYS_TO_VM_PAGE() does not result in a pointer to the fake page that was > wired. Thus, the panic on unexpected wiring count. > > Your patch to vm_fault_unwire() does look like a correct fix to this issue. I didn't realize the problem was so old! > > * vm_map_{un,}wire() did not keep track of wirings as they should. User > > wirings are separate from system wirings, and there can be exactly one. > > There can be unlimited system wirings, but wired_count will remain at > > zero for map entries that are allocated as MAP_ENTRY_NOFAULT. > > MAP_ENTRY_NOFAULT means that the vm_map_entry has no backing vm_object and > that you should never fault on it, which would create a backing object. > vm_map_{un,}wire(), however, deal with map entries that do have backing > objects. I'm not quite sure why you chose to introduce MAP_ENTRY_NOFAULT > into the implementation of vm_map_{un,}wire(). > > I'll have to look at this more carefully. I've done it the way I explained to preserve layering of vmspace -> map -> entry -> page as best I knew how. The kmem_free() implementation doesn't know if the entries are MAP_ENTRY_NOFAULT or not, so I pushed the intelligence into the wiring routines as to what that means. > > * vslock()/vsunlock() did not both use VM_MAP_WIRE_SYSTEM as they must; > > I believe this resulted in more wiring leaks. > > * vm_map_delete() did not wait for all wirings (except one user wiring) to > > drain, so vslock() guaranteed nothing. > > It is not vm_map_delete()'s responsibility to wait for all wirings to be > drained. It is only responsible for a single wired mapping. How can it be done, then? The vslock() function is called with the expectation that the memory it references is both "wired" and "held". I can't see any other to do it -- there is no refcount on entries. Since vslock() is the only interface for system-wiring arbitrary memory, there will always be a matching call to vsunlock() which causes the system unwire to happen and the waiting vm_map_delete() to wake up. > > * The condition in vm_fault() where all pages have been exhausted is easy to > > deadlock, but was impossible to recover from. The OOM killer works only > > when all memory has been used, not all wired memory. However, now it is > > possible to kill offending processes with SIGKILL instead of the > > vm_fault() in trap_pfault() looping forever. Would you rather see this or an "OOM-wired-killer"? > > * The init(8) program should really be using mlockall(2) so that it can kill > > off processes hogging all the wired memory. However, I have not fixed this > > because in such case, e.g. while I would like for Ctrl+Alt+Del to work, > > init(8) may actually need to allocate and wire new pages itself to keep > > running. I think a way to fix this is to conditionalize the > > vm_page_count_severe() condition on p->p_pid != 1 so that just like the > > REAL system processes, it can bring the page count lower than "severe". > > > > I haven't tested it out on SMP yet, but on UP the latest changes don't seem > > to have any negative effects. All wired memory leaks appear to be gone and > > although init(8) probably can't do it, I can enter DDB and "kill 9 <wirehog>" > > to take the machine out of an all-wired deadlock. See patch at URL: > > <http://green.homeunix.org/~green/vm-wiring.patch> > > > > In summary, consider this a positive review for the MAP_FUTUREWIRE and > the vm_fault_unwire() fixes. Thanks! -- Brian Fundakowski Feldman \'[ FreeBSD ]''''''''''\ <> green_at_FreeBSD.org \ The Power to Serve! \ Opinions expressed are my own. \,,,,,,,,,,,,,,,,,,,,,,\Received on Wed Apr 28 2004 - 18:05:13 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:37:52 UTC