From 4f774c503c464ae50bfd3c710d78c372f9ca03e2 Mon Sep 17 00:00:00 2001 From: Ingo Weinhold Date: Mon, 5 Apr 2010 12:12:36 +0000 Subject: [PATCH] * VMArea::Unwire(addr_t, size_t, bool): Don't delete the removed range, but return it. * lock_memory_etc(): On error the VMAreaWiredRange object could be leaked. * [un]lock_memory_etc(): Call VMArea::Unwire() with the cache locked and explicitly delete the range object after unlocking the cache to avoid potential deadlocks. git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@36035 a95241bf-73f2-0310-859d-f6bbb57e9c96 --- headers/private/kernel/vm/VMArea.h | 2 +- src/system/kernel/vm/VMArea.cpp | 10 ++++--- src/system/kernel/vm/vm.cpp | 43 ++++++++++++++++++++++++------ 3 files changed, 42 insertions(+), 13 deletions(-) diff --git a/headers/private/kernel/vm/VMArea.h b/headers/private/kernel/vm/VMArea.h index fc5b235692..8edf6ca9d9 100644 --- a/headers/private/kernel/vm/VMArea.h +++ b/headers/private/kernel/vm/VMArea.h @@ -95,7 +95,7 @@ struct VMArea { void Wire(VMAreaWiredRange* range); void Unwire(VMAreaWiredRange* range); - void Unwire(addr_t base, size_t size, bool writable); + VMAreaWiredRange* Unwire(addr_t base, size_t size, bool writable); bool AddWaiterIfWired(VMAreaUnwiredWaiter* waiter); bool AddWaiterIfWired(VMAreaUnwiredWaiter* waiter, diff --git a/src/system/kernel/vm/VMArea.cpp b/src/system/kernel/vm/VMArea.cpp index dd704ebfaa..5bf224ee97 100644 --- a/src/system/kernel/vm/VMArea.cpp +++ b/src/system/kernel/vm/VMArea.cpp @@ -131,10 +131,12 @@ VMArea::Unwire(VMAreaWiredRange* range) /*! Removes a wired range from this area. - Must balance a previous Wire() call. + Must balance a previous Wire() call. The first implicit range with matching + \a base, \a size, and \a writable attributes is removed and returned. It's + waiters are woken up as well. The area's top cache must be locked. */ -void +VMAreaWiredRange* VMArea::Unwire(addr_t base, size_t size, bool writable) { for (VMAreaWiredRangeList::Iterator it = fWiredRanges.GetIterator(); @@ -142,13 +144,13 @@ VMArea::Unwire(addr_t base, size_t size, bool writable) if (range->implicit && range->base == base && range->size == size && range->writable == writable) { Unwire(range); - delete range; - return; + return range; } } panic("VMArea::Unwire(%#" B_PRIxADDR ", %#" B_PRIxADDR ", %d): no such " "range", base, size, writable); + return NULL; } diff --git a/src/system/kernel/vm/vm.cpp b/src/system/kernel/vm/vm.cpp index 1bc8dea432..d40140bf65 100644 --- a/src/system/kernel/vm/vm.cpp +++ b/src/system/kernel/vm/vm.cpp @@ -4762,6 +4762,9 @@ lock_memory_etc(team_id team, void* address, size_t numBytes, uint32 flags) if (writable) requiredProtection |= B_KERNEL_WRITE_AREA | (isUser ? B_WRITE_AREA : 0); + uint32 mallocFlags = isUser + ? 0 : HEAP_DONT_WAIT_FOR_MEMORY | HEAP_DONT_LOCK_KERNEL_SPACE; + // get and read lock the address space VMAddressSpace* addressSpace = NULL; if (isUser) { @@ -4794,8 +4797,6 @@ lock_memory_etc(team_id team, void* address, size_t numBytes, uint32 flags) // allocate the wired range (do that before locking the cache to avoid // deadlocks) - uint32 mallocFlags = isUser - ? 0 : HEAP_DONT_WAIT_FOR_MEMORY | HEAP_DONT_LOCK_KERNEL_SPACE; VMAreaWiredRange* range = new(malloc_flags(mallocFlags)) VMAreaWiredRange(areaStart, areaEnd - areaStart, writable, true); if (range == NULL) { @@ -4860,14 +4861,21 @@ lock_memory_etc(team_id team, void* address, size_t numBytes, uint32 flags) } map->Unlock(); - cacheChainLocker.Unlock(); - if (error != B_OK) { + if (error == B_OK) { + cacheChainLocker.Unlock(); + } else { // An error occurred, so abort right here. If the current address // is the first in this area, unwire the area, since we won't get // to it when reverting what we've done so far. - if (nextAddress == areaStart) + if (nextAddress == areaStart) { area->Unwire(range); + cacheChainLocker.Unlock(); + range->~VMAreaWiredRange(); + free_etc(range, mallocFlags); + } else + cacheChainLocker.Unlock(); + break; } } @@ -4911,6 +4919,9 @@ unlock_memory_etc(team_id team, void* address, size_t numBytes, uint32 flags) if (writable) requiredProtection |= B_KERNEL_WRITE_AREA | (isUser ? B_WRITE_AREA : 0); + uint32 mallocFlags = isUser + ? 0 : HEAP_DONT_WAIT_FOR_MEMORY | HEAP_DONT_LOCK_KERNEL_SPACE; + // get and read lock the address space VMAddressSpace* addressSpace = NULL; if (isUser) { @@ -4951,8 +4962,16 @@ unlock_memory_etc(team_id team, void* address, size_t numBytes, uint32 flags) || area->cache_type == CACHE_TYPE_DEVICE || area->wiring == B_FULL_LOCK || area->wiring == B_CONTIGUOUS) { + // unwire the range (to avoid deadlocks we delete the range after + // unlocking the cache) nextAddress = areaEnd; - area->Unwire(areaStart, areaEnd - areaStart, writable); + VMAreaWiredRange* range = area->Unwire(areaStart, + areaEnd - areaStart, writable); + cacheChainLocker.Unlock(); + if (range != NULL) { + range->~VMAreaWiredRange(); + free_etc(range, mallocFlags); + } continue; } @@ -4984,10 +5003,18 @@ unlock_memory_etc(team_id team, void* address, size_t numBytes, uint32 flags) } map->Unlock(); + + // All pages are unwired. Remove the area's wired range as well (to + // avoid deadlocks we delete the range after unlocking the cache). + VMAreaWiredRange* range = area->Unwire(areaStart, + areaEnd - areaStart, writable); + cacheChainLocker.Unlock(); - // all pages are unwired -- remove the area's wired range as well - area->Unwire(areaStart, areaEnd - areaStart, writable); + if (range != NULL) { + range->~VMAreaWiredRange(); + free_etc(range, mallocFlags); + } if (error != B_OK) break;