From 3cd2094396dde9ca42263c535041a95d5f0d5fff Mon Sep 17 00:00:00 2001 From: Ingo Weinhold Date: Thu, 7 Jan 2010 02:37:05 +0000 Subject: [PATCH] * Added new debug feature (DEBUG_PAGE_ACCESS) to detect invalid concurrent access to a vm_page. It is basically an atomically accessed thread ID field in the vm_page structure, which is explicitly set by macros marking the critical sections. As a first positive effect I had to review quite a bit of code and found several issues. * Added several TODOs and comments. Some harmless ones, but also a few troublesome ones in vm.cpp regarding page unmapping. * file_cache: PrecacheIO::Prepare()/read_into_cache: Removed superfluous vm_page_allocate_page() return value checks. It cannot fail anymore. * Removed the heavily contended "pages" lock. We use different policies now: - sModifiedTemporaryPages is accessed atomically. - sPageDeficitLock and sFreePageCondition are protected by a new mutex. - The page queues have individual locks (mutexes). - Renamed set_page_state_nolock() to set_page_state(). Unless the caller says otherwise, it does now lock the affected pages queues itself. Also changed the return value to void -- we panic() anyway. * set_page_state(): Add free/clear pages to the beginning of their respective queues as this is more cache-friendly. * Pages with the states PAGE_STATE_WIRED or PAGE_STATE_UNUSED are no longer in any queue. They were in the "active" queue, but there's no good reason to have them there. In case we decide to let the page daemon work the queues (like FreeBSD) they would just be in the way. * Pulled the common part of vm_page_allocate_page_run[_no_base]() into a helper function. Also fixed a bug I introduced previously: The functions must not vm_page_unreserve_pages() on success, since they remove the pages from the free/clear queue without decrementing sUnreservedFreePages. * vm_page_set_state(): Changed return type to void. The function cannot really fail and no-one was checking it anyway. * vm_page_free(), vm_page_set_state(): Added assertion: The page must not be free/clear before. This is implied by the policy that no-one is allowed to access free/clear pages without holding the respective queue's lock, which is not the case at this point. This found the bug fixed in r34912. * vm_page_requeue(): Added general assertions. panic() when requeuing of free/clear pages is requested. Same reason as above. * vm_clone_area(), B_FULL_LOCK case: Don't map busy pages. The implementation is still not correct, though. My usual -j8 Haiku build test runs another 10% faster, now. The total kernel time drops about 18%. As hoped the new locks have only a fraction of the old "pages" lock contention. Other locks lead the "most wanted list" now. git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@34933 a95241bf-73f2-0310-859d-f6bbb57e9c96 --- build/config_headers/kernel_debug_config.h | 8 +- headers/private/kernel/vm/vm_page.h | 2 +- headers/private/kernel/vm/vm_types.h | 72 ++- .../kernel/bus_managers/agp_gart/agp_gart.cpp | 11 + .../m68k/arch_vm_translation_map_impl.cpp | 9 +- .../arch/x86/arch_vm_translation_map.cpp | 5 +- src/system/kernel/cache/file_cache.cpp | 39 +- src/system/kernel/vm/Jamfile | 1 + src/system/kernel/vm/VMAnonymousCache.cpp | 4 +- src/system/kernel/vm/VMCache.cpp | 3 + src/system/kernel/vm/VMPageQueue.cpp | 17 + src/system/kernel/vm/VMPageQueue.h | 147 +++++- src/system/kernel/vm/vm.cpp | 89 +++- src/system/kernel/vm/vm_daemons.cpp | 12 +- src/system/kernel/vm/vm_page.cpp | 464 +++++++++++------- 15 files changed, 664 insertions(+), 219 deletions(-) create mode 100644 src/system/kernel/vm/VMPageQueue.cpp diff --git a/build/config_headers/kernel_debug_config.h b/build/config_headers/kernel_debug_config.h index 186fefb7e2..5b767929fb 100644 --- a/build/config_headers/kernel_debug_config.h +++ b/build/config_headers/kernel_debug_config.h @@ -80,10 +80,14 @@ // VM -// Enables the vm_page::queue, i.e. it is tracked which queue the page should -// be in. +// Enables the vm_page::queue field, i.e. it is tracked which queue the page +// should be in. #define DEBUG_PAGE_QUEUE 0 +// Enables the vm_page::access_count field, which is used to detect invalid +// concurrent access to the page. +#define DEBUG_PAGE_ACCESS 1 + // Enables a global list of all vm_cache structures. #define DEBUG_CACHE_LIST KDEBUG_LEVEL_1 diff --git a/headers/private/kernel/vm/vm_page.h b/headers/private/kernel/vm/vm_page.h index 54e57fc111..8e19bf1c87 100644 --- a/headers/private/kernel/vm/vm_page.h +++ b/headers/private/kernel/vm/vm_page.h @@ -28,7 +28,7 @@ status_t vm_page_init_post_thread(struct kernel_args *args); status_t vm_mark_page_inuse(addr_t page); status_t vm_mark_page_range_inuse(addr_t startPage, addr_t length); void vm_page_free(struct VMCache *cache, struct vm_page *page); -status_t vm_page_set_state(struct vm_page *page, int state); +void vm_page_set_state(struct vm_page *page, int state); void vm_page_requeue(struct vm_page *page, bool tail); // get some data about the number of pages in the system diff --git a/headers/private/kernel/vm/vm_types.h b/headers/private/kernel/vm/vm_types.h index c435f7cb39..0e453c9d83 100644 --- a/headers/private/kernel/vm/vm_types.h +++ b/headers/private/kernel/vm/vm_types.h @@ -1,5 +1,5 @@ /* - * Copyright 2009, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2009-2010, Ingo Weinhold, ingo_weinhold@gmx.de. * Copyright 2002-2009, Axel Dörfler, axeld@pinc-software.de. * Distributed under the terms of the MIT License. * @@ -90,6 +90,10 @@ struct vm_page { void* queue; #endif +#if DEBUG_PAGE_ACCESS + vint32 accessing_thread; +#endif + uint8 type : 2; uint8 state : 3; @@ -121,4 +125,70 @@ enum { }; +#if DEBUG_PAGE_ACCESS +# include + +static inline void +vm_page_debug_access_start(vm_page* page) +{ + thread_id threadID = thread_get_current_thread_id(); + thread_id previousThread = atomic_test_and_set(&page->accessing_thread, + threadID, -1); + if (previousThread != -1) { + panic("Invalid concurrent access to page %p (start), currently " + "accessed by: %" B_PRId32, page, previousThread); + } +} + + +static inline void +vm_page_debug_access_end(vm_page* page) +{ + thread_id threadID = thread_get_current_thread_id(); + thread_id previousThread = atomic_test_and_set(&page->accessing_thread, -1, + threadID); + if (previousThread != threadID) { + panic("Invalid concurrent access to page %p (end) by current thread, " + "current accessor is: %" B_PRId32, page, previousThread); + } +} + + +static inline void +vm_page_debug_access_check(vm_page* page) +{ + thread_id thread = page->accessing_thread; + if (thread != thread_get_current_thread_id()) { + panic("Invalid concurrent access to page %p (check), currently " + "accessed by: %" B_PRId32, page, thread); + } +} + + +static inline void +vm_page_debug_access_transfer(vm_page* page, thread_id expectedPreviousThread) +{ + thread_id threadID = thread_get_current_thread_id(); + thread_id previousThread = atomic_test_and_set(&page->accessing_thread, + threadID, expectedPreviousThread); + if (previousThread != expectedPreviousThread) { + panic("Invalid access transfer for page %p, currently accessed by: " + "%" B_PRId32 ", expected: %" B_PRId32, page, previousThread, + expectedPreviousThread); + } +} + +# define DEBUG_PAGE_ACCESS_START(page) vm_page_debug_access_start(page) +# define DEBUG_PAGE_ACCESS_END(page) vm_page_debug_access_end(page) +# define DEBUG_PAGE_ACCESS_CHECK(page) vm_page_debug_access_check(page) +# define DEBUG_PAGE_ACCESS_TRANSFER(page, thread) \ + vm_page_debug_access_transfer(page, thread) +#else +# define DEBUG_PAGE_ACCESS_START(page) do {} while (false) +# define DEBUG_PAGE_ACCESS_END(page) do {} while (false) +# define DEBUG_PAGE_ACCESS_CHECK(page) do {} while (false) +# define DEBUG_PAGE_ACCESS_TRANSFER(page, thread) do {} while (false) +#endif + + #endif // _KERNEL_VM_VM_TYPES_H diff --git a/src/add-ons/kernel/bus_managers/agp_gart/agp_gart.cpp b/src/add-ons/kernel/bus_managers/agp_gart/agp_gart.cpp index 10e5eebe0c..eda0564504 100644 --- a/src/add-ons/kernel/bus_managers/agp_gart/agp_gart.cpp +++ b/src/add-ons/kernel/bus_managers/agp_gart/agp_gart.cpp @@ -83,6 +83,9 @@ struct aperture_memory { vm_page **pages; vm_page *page; }; +#ifdef DEBUG_PAGE_ACCESS + thread_id allocating_thread; +#endif #else area_id area; #endif @@ -550,6 +553,11 @@ Aperture::AllocateMemory(aperture_memory *memory, uint32 flags) memory->pages[i] = vm_page_allocate_page(PAGE_STATE_CLEAR); vm_page_unreserve_pages(count); } + +#ifdef DEBUG_PAGE_ACCESS + memory->allocating_thread = find_thread(NULL); +#endif + #else void *address; memory->area = create_area("GART memory", &address, B_ANY_KERNEL_ADDRESS, @@ -682,12 +690,15 @@ Aperture::_Free(aperture_memory *memory) if ((memory->flags & B_APERTURE_NEED_PHYSICAL) != 0) { vm_page *page = memory->page; for (uint32 i = 0; i < count; i++, page++) { + DEBUG_PAGE_ACCESS_TRANSFER(page, memory->allocating_thread); vm_page_set_state(page, PAGE_STATE_FREE); } memory->page = NULL; } else { for (uint32 i = 0; i < count; i++) { + DEBUG_PAGE_ACCESS_TRANSFER(memory->pages[i], + memory->allocating_thread); vm_page_set_state(memory->pages[i], PAGE_STATE_FREE); } diff --git a/src/system/kernel/arch/m68k/arch_vm_translation_map_impl.cpp b/src/system/kernel/arch/m68k/arch_vm_translation_map_impl.cpp index 0960da7369..a465713d31 100644 --- a/src/system/kernel/arch/m68k/arch_vm_translation_map_impl.cpp +++ b/src/system/kernel/arch/m68k/arch_vm_translation_map_impl.cpp @@ -376,10 +376,13 @@ destroy_tmap(vm_translation_map *map) panic("destroy_tmap: didn't find pgtable page\n"); return; } + DEBUG_PAGE_ACCESS_START(page); vm_page_set_state(page, PAGE_STATE_FREE); } - if (((i+1)%NUM_DIRTBL_PER_PAGE) == 0) + if (((i + 1) % NUM_DIRTBL_PER_PAGE) == 0) { + DEBUG_PAGE_ACCESS_END(dirpage); vm_page_set_state(dirpage, PAGE_STATE_FREE); + } } free(map->arch_data->rtdir_virt); } @@ -545,6 +548,8 @@ map_tmap(vm_translation_map *map, addr_t va, addr_t pa, uint32 attributes) // mark the page WIRED vm_page_set_state(page, PAGE_STATE_WIRED); + DEBUG_PAGE_ACCESS_END(page); + pgdir = page->physical_page_number * B_PAGE_SIZE; TRACE(("map_tmap: asked for free page for pgdir. 0x%lx\n", pgdir)); @@ -591,6 +596,8 @@ map_tmap(vm_translation_map *map, addr_t va, addr_t pa, uint32 attributes) // mark the page WIRED vm_page_set_state(page, PAGE_STATE_WIRED); + DEBUG_PAGE_ACCESS_END(page); + pgtable = page->physical_page_number * B_PAGE_SIZE; TRACE(("map_tmap: asked for free page for pgtable. 0x%lx\n", pgtable)); diff --git a/src/system/kernel/arch/x86/arch_vm_translation_map.cpp b/src/system/kernel/arch/x86/arch_vm_translation_map.cpp index de4d00d5a6..ac6ba6c692 100644 --- a/src/system/kernel/arch/x86/arch_vm_translation_map.cpp +++ b/src/system/kernel/arch/x86/arch_vm_translation_map.cpp @@ -1,5 +1,5 @@ /* - * Copyright 2008-2009, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2008-2010, Ingo Weinhold, ingo_weinhold@gmx.de. * Copyright 2002-2007, Axel Dörfler, axeld@pinc-software.de. All rights reserved. * Distributed under the terms of the MIT License. * @@ -274,6 +274,7 @@ destroy_tmap(vm_translation_map *map) page = vm_lookup_page(pgtable_addr); if (!page) panic("destroy_tmap: didn't find pgtable page\n"); + DEBUG_PAGE_ACCESS_START(page); vm_page_set_state(page, PAGE_STATE_FREE); } } @@ -369,6 +370,8 @@ map_tmap(vm_translation_map *map, addr_t va, addr_t pa, uint32 attributes) // mark the page WIRED vm_page_set_state(page, PAGE_STATE_WIRED); + DEBUG_PAGE_ACCESS_END(page); + pgtable = page->physical_page_number * B_PAGE_SIZE; TRACE(("map_tmap: asked for free page for pgtable. 0x%lx\n", pgtable)); diff --git a/src/system/kernel/cache/file_cache.cpp b/src/system/kernel/cache/file_cache.cpp index 9bcde8bbbc..d6b027b338 100644 --- a/src/system/kernel/cache/file_cache.cpp +++ b/src/system/kernel/cache/file_cache.cpp @@ -92,6 +92,9 @@ private: off_t fOffset; uint32 fVecCount; size_t fSize; +#if DEBUG_PAGE_ACCESS + thread_id fAllocatingThread; +#endif }; typedef status_t (*cache_func)(file_cache_ref* ref, void* cookie, off_t offset, @@ -150,8 +153,6 @@ PrecacheIO::Prepare() uint32 i = 0; for (size_t pos = 0; pos < fSize; pos += B_PAGE_SIZE) { vm_page* page = vm_page_allocate_page(PAGE_STATE_FREE); - if (page == NULL) - break; fCache->InsertPage(page, fOffset + pos); @@ -160,15 +161,9 @@ PrecacheIO::Prepare() fPages[i++] = page; } - if (i != fPageCount) { - // allocating pages failed - while (i-- > 0) { - fCache->NotifyPageEvents(fPages[i], PAGE_EVENT_NOT_BUSY); - fCache->RemovePage(fPages[i]); - vm_page_set_state(fPages[i], PAGE_STATE_FREE); - } - return B_NO_MEMORY; - } +#if DEBUG_PAGE_ACCESS + fAllocatingThread = find_thread(NULL); +#endif return B_OK; } @@ -207,12 +202,17 @@ PrecacheIO::IOFinished(status_t status, bool partialTransfer, + bytesTouched, 0, B_PAGE_SIZE - bytesTouched); } + DEBUG_PAGE_ACCESS_TRANSFER(fPages[i], fAllocatingThread); + fPages[i]->state = PAGE_STATE_ACTIVE; fCache->NotifyPageEvents(fPages[i], PAGE_EVENT_NOT_BUSY); + + DEBUG_PAGE_ACCESS_END(fPages[i]); } // Free pages after failed I/O for (uint32 i = pagesTransferred; i < fPageCount; i++) { + DEBUG_PAGE_ACCESS_TRANSFER(fPages[i], fAllocatingThread); fCache->NotifyPageEvents(fPages[i], PAGE_EVENT_NOT_BUSY); fCache->RemovePage(fPages[i]); vm_page_set_state(fPages[i], PAGE_STATE_FREE); @@ -305,6 +305,7 @@ reserve_pages(file_cache_ref* ref, size_t reservePages, bool isWrite) (page = it.Next()) != NULL && left > 0;) { if (page->state != PAGE_STATE_MODIFIED && page->state != PAGE_STATE_BUSY) { + DEBUG_PAGE_ACCESS_START(page); cache->RemovePage(page); vm_page_set_state(page, PAGE_STATE_FREE); left--; @@ -382,8 +383,6 @@ read_into_cache(file_cache_ref* ref, void* cookie, off_t offset, for (size_t pos = 0; pos < numBytes; pos += B_PAGE_SIZE) { vm_page* page = pages[pageIndex++] = vm_page_allocate_page( PAGE_STATE_FREE); - if (page == NULL) - panic("no more pages!"); cache->InsertPage(page, offset + pos); @@ -436,6 +435,8 @@ read_into_cache(file_cache_ref* ref, void* cookie, off_t offset, // make the pages accessible in the cache for (int32 i = pageIndex; i-- > 0;) { + DEBUG_PAGE_ACCESS_END(pages[i]); + pages[i]->state = PAGE_STATE_ACTIVE; cache->NotifyPageEvents(pages[i], PAGE_EVENT_NOT_BUSY); @@ -610,6 +611,8 @@ write_to_cache(file_cache_ref* ref, void* cookie, off_t offset, pages[i]->state = PAGE_STATE_ACTIVE; else vm_page_set_state(pages[i], PAGE_STATE_MODIFIED); + + DEBUG_PAGE_ACCESS_END(pages[i]); } return status; @@ -798,6 +801,9 @@ cache_io(void* _cacheRef, void* cookie, off_t offset, addr_t buffer, // Since we don't actually map pages as part of an area, we have // to manually maintain their usage_count page->usage_count = 2; + // TODO: Just because this request comes from the FS API, it + // doesn't mean the page is not mapped. We might actually + // decrease the usage count of a hot page here. if (doWrite || useBuffer) { // Since the following user_mem{cpy,set}() might cause a page @@ -827,8 +833,11 @@ cache_io(void* _cacheRef, void* cookie, off_t offset, addr_t buffer, locker.Lock(); page->state = oldPageState; - if (doWrite && page->state != PAGE_STATE_MODIFIED) + if (doWrite && page->state != PAGE_STATE_MODIFIED) { + DEBUG_PAGE_ACCESS_START(page); vm_page_set_state(page, PAGE_STATE_MODIFIED); + DEBUG_PAGE_ACCESS_END(page); + } cache->NotifyPageEvents(page, PAGE_EVENT_NOT_BUSY); } @@ -1001,6 +1010,8 @@ cache_prefetch_vnode(struct vnode* vnode, off_t offset, size_t size) cache->ReleaseRefAndUnlock(); vm_page_unreserve_pages(reservePages); + // TODO: We should periodically unreserve as we go, so we don't + // unnecessarily put pressure on the free page pool. } diff --git a/src/system/kernel/vm/Jamfile b/src/system/kernel/vm/Jamfile index 78f5cf8980..25b8ed16c0 100644 --- a/src/system/kernel/vm/Jamfile +++ b/src/system/kernel/vm/Jamfile @@ -18,6 +18,7 @@ KernelMergeObject kernel_vm.o : VMKernelAddressSpace.cpp VMKernelArea.cpp VMNullCache.cpp + VMPageQueue.cpp VMUserAddressSpace.cpp VMUserArea.cpp diff --git a/src/system/kernel/vm/VMAnonymousCache.cpp b/src/system/kernel/vm/VMAnonymousCache.cpp index d56514b9b9..0495cf7737 100644 --- a/src/system/kernel/vm/VMAnonymousCache.cpp +++ b/src/system/kernel/vm/VMAnonymousCache.cpp @@ -1,6 +1,6 @@ /* * Copyright 2008, Zhao Shuai, upczhsh@163.com. - * Copyright 2008-2009, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2008-2010, Ingo Weinhold, ingo_weinhold@gmx.de. * Copyright 2002-2009, Axel Dörfler, axeld@pinc-software.de. * Distributed under the terms of the MIT License. * @@ -952,6 +952,7 @@ VMAnonymousCache::_MergePagesSmallerConsumer(VMAnonymousCache* source) vm_page* sourcePage = source->LookupPage( (off_t)page->cache_offset << PAGE_SHIFT); if (sourcePage != NULL) { + DEBUG_PAGE_ACCESS_START(sourcePage); source->RemovePage(sourcePage); vm_page_free(source, sourcePage); } @@ -1000,6 +1001,7 @@ VMAnonymousCache::_MergeSwapPages(VMAnonymousCache* source) if (swapBlock->swap_slots[i] != SWAP_SLOT_NONE) { vm_page* page = source->LookupPage( (off_t)(swapBlockPageIndex + i) << PAGE_SHIFT); + DEBUG_PAGE_ACCESS_START(page); source->RemovePage(page); vm_page_free(source, page); } diff --git a/src/system/kernel/vm/VMCache.cpp b/src/system/kernel/vm/VMCache.cpp index 88cf072018..796e279484 100644 --- a/src/system/kernel/vm/VMCache.cpp +++ b/src/system/kernel/vm/VMCache.cpp @@ -612,6 +612,7 @@ VMCache::Delete() TRACE(("vm_cache_release_ref: freeing page 0x%lx\n", oldPage->physical_page_number)); + DEBUG_PAGE_ACCESS_START(page); vm_page_free(this, page); } @@ -1032,6 +1033,7 @@ VMCache::Resize(off_t newSize) } // remove the page and put it into the free queue + DEBUG_PAGE_ACCESS_START(page); vm_remove_all_page_mappings(page, NULL); ASSERT(page->wired_count == 0); // TODO: Find a real solution! Unmapping is probably fine, but @@ -1081,6 +1083,7 @@ VMCache::FlushAndRemoveAllPages() if (page->wired_count > 0 || !page->mappings.IsEmpty()) return B_BUSY; + DEBUG_PAGE_ACCESS_START(page); RemovePage(page); vm_page_free(this, page); // Note: When iterating through a IteratableSplayTree diff --git a/src/system/kernel/vm/VMPageQueue.cpp b/src/system/kernel/vm/VMPageQueue.cpp new file mode 100644 index 0000000000..290647524c --- /dev/null +++ b/src/system/kernel/vm/VMPageQueue.cpp @@ -0,0 +1,17 @@ +/* + * Copyright 2010, Ingo Weinhold, ingo_weinhold@gmx.de. + * Distributed under the terms of the MIT License. + */ + + +#include "VMPageQueue.h" + + +void +VMPageQueue::Init(const char* name, int lockingOrder) +{ + fName = name; + fLockingOrder = lockingOrder; + fCount = 0; + mutex_init(&fLock, fName); +} diff --git a/src/system/kernel/vm/VMPageQueue.h b/src/system/kernel/vm/VMPageQueue.h index c64d36db91..e5ae0b3050 100644 --- a/src/system/kernel/vm/VMPageQueue.h +++ b/src/system/kernel/vm/VMPageQueue.h @@ -1,5 +1,5 @@ /* - * Copyright 2009, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2010, Ingo Weinhold, ingo_weinhold@gmx.de. * Copyright 2002-2008, Axel Dörfler, axeld@pinc-software.de. * Distributed under the terms of the MIT License. * @@ -10,6 +10,13 @@ #define VM_PAGE_QUEUE_H +#include + +#include +#include + + + struct VMPageQueue { public: typedef DoublyLinkedListfLockingOrder) { + Lock(); + other->Lock(); + } else { + other->Lock(); + Lock(); + } } @@ -147,18 +183,6 @@ VMPageQueue::RemoveHead() } -/*! Moves a page to the tail of this queue, but only does so if - the page is currently in another queue. -*/ -void -VMPageQueue::MoveFrom(VMPageQueue* from, vm_page* page) -{ - if (from != this) { - from->Remove(page); - Append(page); - } -} - vm_page* VMPageQueue::Head() const { @@ -194,4 +218,87 @@ VMPageQueue::GetIterator() const } +// #pragma mark - VMPageQueuePairLocker + + +struct VMPageQueuePairLocker { + VMPageQueuePairLocker() + : + fQueue1(NULL), + fQueue2(NULL) + { + } + + VMPageQueuePairLocker(VMPageQueue& queue1, VMPageQueue& queue2) + : + fQueue1(&queue1), + fQueue2(&queue2) + { + _Lock(); + } + + ~VMPageQueuePairLocker() + { + _Unlock(); + } + + void SetTo(VMPageQueue* queue1, VMPageQueue* queue2) + { + _Unlock(); + fQueue1 = queue1; + fQueue2 = queue2; + _Lock(); + } + + void Unlock() + { + if (fQueue1 != NULL) { + fQueue1->Unlock(); + fQueue1 = NULL; + } + + if (fQueue2 != NULL) { + fQueue2->Unlock(); + fQueue2 = NULL; + } + } + +private: + void _Lock() + { + if (fQueue1 == fQueue2) { + if (fQueue1 == NULL) + return; + fQueue1->Lock(); + fQueue2 = NULL; + } else { + if (fQueue1 == NULL) { + fQueue2->Lock(); + } else if (fQueue2 == NULL) { + fQueue1->Lock(); + } else if (fQueue1->LockingOrder() < fQueue2->LockingOrder()) { + fQueue1->Lock(); + fQueue2->Lock(); + } else { + fQueue2->Lock(); + fQueue1->Lock(); + } + } + } + + void _Unlock() + { + if (fQueue1 != NULL) + fQueue1->Unlock(); + + if (fQueue2 != NULL) + fQueue2->Unlock(); + } + +private: + VMPageQueue* fQueue1; + VMPageQueue* fQueue2; +}; + + #endif // VM_PAGE_QUEUE_H diff --git a/src/system/kernel/vm/vm.cpp b/src/system/kernel/vm/vm.cpp index af422d08ac..bad8ab99b9 100644 --- a/src/system/kernel/vm/vm.cpp +++ b/src/system/kernel/vm/vm.cpp @@ -1,5 +1,5 @@ /* - * Copyright 2009, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2009-2010, Ingo Weinhold, ingo_weinhold@gmx.de. * Copyright 2002-2009, Axel Dörfler, axeld@pinc-software.de. * Distributed under the terms of the MIT License. * @@ -344,6 +344,9 @@ cut_area(VMAddressSpace* addressSpace, VMArea* area, addr_t address, // unmap pages vm_unmap_pages(area, address, oldSize - newSize, false); + // TODO: preserveModified = false is wrong, since this could be a + // cloned area or a write-mmap()ed file, in which case we'd lose + // information. // If no one else uses the area's cache, we can resize it, too. if (cache->areas == area && area->cache_next == NULL @@ -366,6 +369,7 @@ cut_area(VMAddressSpace* addressSpace, VMArea* area, addr_t address, // unmap pages vm_unmap_pages(area, oldBase, newBase - oldBase, false); + // TODO: See the vm_unmap_pages() above. // resize the area status_t error = addressSpace->ShrinkAreaHead(area, newSize); @@ -389,6 +393,7 @@ cut_area(VMAddressSpace* addressSpace, VMArea* area, addr_t address, // unmap pages vm_unmap_pages(area, address, area->Size() - firstNewSize, false); + // TODO: See the vm_unmap_pages() above. // resize the area addr_t oldSize = area->Size(); @@ -874,6 +879,10 @@ vm_create_anonymous_area(team_id team, const char* name, void** address, vm_page* page = vm_page_allocate_page(newPageState); cache->InsertPage(page, offset); vm_map_page(area, page, address, protection); + // TODO: This sets the page state to "active", but it would + // make more sense to set it to "wired". + + DEBUG_PAGE_ACCESS_END(page); // Periodically unreserve pages we've already allocated, so that // we don't unnecessarily increase the pressure on the VM. @@ -917,9 +926,13 @@ vm_create_anonymous_area(team_id team, const char* name, void** address, physicalAddress); } + DEBUG_PAGE_ACCESS_START(page); + increment_page_wired_count(page); - vm_page_set_state(page, PAGE_STATE_WIRED); cache->InsertPage(page, offset); + vm_page_set_state(page, PAGE_STATE_WIRED); + + DEBUG_PAGE_ACCESS_END(page); } map->ops->unlock(map); @@ -950,8 +963,10 @@ vm_create_anonymous_area(team_id team, const char* name, void** address, panic("couldn't map physical page in page run\n"); increment_page_wired_count(page); - vm_page_set_state(page, PAGE_STATE_WIRED); cache->InsertPage(page, offset); + vm_page_set_state(page, PAGE_STATE_WIRED); + + DEBUG_PAGE_ACCESS_END(page); } map->ops->unlock(map); @@ -1246,9 +1261,11 @@ pre_map_area_pages(VMArea* area, VMCache* cache) if (page->state == PAGE_STATE_BUSY || page->usage_count <= 0) continue; + DEBUG_PAGE_ACCESS_START(page); vm_map_page(area, page, baseAddress + (page->cache_offset * B_PAGE_SIZE - cacheOffset), B_READ_AREA | B_KERNEL_READ_AREA); + DEBUG_PAGE_ACCESS_END(page); } } @@ -1543,10 +1560,17 @@ vm_clone_area(team_id team, const char* name, void** address, // map in all pages from source for (VMCachePagesTree::Iterator it = cache->pages.GetIterator(); vm_page* page = it.Next();) { - vm_map_page(newArea, page, newArea->Base() - + ((page->cache_offset << PAGE_SHIFT) - - newArea->cache_offset), protection); + if (page->state != PAGE_STATE_BUSY) { + DEBUG_PAGE_ACCESS_START(page); + vm_map_page(newArea, page, + newArea->Base() + ((page->cache_offset << PAGE_SHIFT) + - newArea->cache_offset), + protection); + DEBUG_PAGE_ACCESS_END(page); + } } + // TODO: B_FULL_LOCK means that all pages are locked. We are not + // ensuring that! vm_page_unreserve_pages(reservePages); } @@ -1573,6 +1597,9 @@ delete_area(VMAddressSpace* addressSpace, VMArea* area) // Unmap the virtual address space the area occupied vm_unmap_pages(area, area->Base(), area->Size(), !area->cache->temporary); + // TODO: Even if the cache is temporary we might need to preserve the + // modified flag, since the area could be a clone and backed by swap. + // We would lose information in this case. if (!area->cache->temporary) area->cache->WriteModified(); @@ -1995,6 +2022,9 @@ vm_remove_all_page_mappings(vm_page* page, uint32* _flags) } +/*! If \a preserveModified is \c true, the caller must hold the lock of the + page's cache and the page must not be busy. +*/ bool vm_unmap_page(VMArea* area, addr_t virtualAddress, bool preserveModified) { @@ -2068,6 +2098,11 @@ vm_unmap_page(VMArea* area, addr_t virtualAddress, bool preserveModified) } +/*! If \a preserveModified is \c true, the caller must hold the lock of all + mapped pages' caches and none of the pages must be busy. + TODO: Particularly the latter is very inconvenient. See the TODOs below for + reasons for this requirement. +*/ status_t vm_unmap_pages(VMArea* area, addr_t base, size_t size, bool preserveModified) { @@ -2116,9 +2151,18 @@ vm_unmap_pages(VMArea* area, addr_t base, size_t size, bool preserveModified) physicalAddress); } + DEBUG_PAGE_ACCESS_START(page); + // TODO: No guarantee for that. See below. + if ((flags & PAGE_MODIFIED) != 0 - && page->state != PAGE_STATE_MODIFIED) + && page->state != PAGE_STATE_MODIFIED) { vm_page_set_state(page, PAGE_STATE_MODIFIED); + // TODO: We are only allowed to do this, if (a) we have also + // locked the cache and (b) the page is not busy! Not doing + // it is problematic, too, since we'd lose information. + } + + DEBUG_PAGE_ACCESS_END(page); } } map->ops->unlock(map); @@ -2170,6 +2214,8 @@ vm_map_page(VMArea* area, vm_page* page, addr_t address, uint32 protection) vm_translation_map* map = &area->address_space->TranslationMap(); vm_page_mapping* mapping = NULL; + DEBUG_PAGE_ACCESS_CHECK(page); + if (area->wiring == B_NO_LOCK) { mapping = (vm_page_mapping*)malloc_nogrow(sizeof(vm_page_mapping)); if (mapping == NULL) @@ -2883,8 +2929,10 @@ unmap_and_free_physical_pages(vm_translation_map* map, addr_t start, addr_t end) if (map->ops->query(map, current, &physicalAddress, &flags) == B_OK && (flags & PAGE_PRESENT) != 0) { vm_page* page = vm_lookup_page(physicalAddress / B_PAGE_SIZE); - if (page != NULL) + if (page != NULL) { + DEBUG_PAGE_ACCESS_START(page); vm_page_set_state(page, PAGE_STATE_FREE); + } } } @@ -3708,6 +3756,8 @@ fault_get_page(PageFaultContext& context) page->state = PAGE_STATE_ACTIVE; cache->NotifyPageEvents(page, PAGE_EVENT_NOT_BUSY); + DEBUG_PAGE_ACCESS_END(page); + // Since we needed to unlock everything temporarily, the area // situation might have changed. So we need to restart the whole // process. @@ -3749,7 +3799,8 @@ fault_get_page(PageFaultContext& context) // insert the new page into our cache context.topCache->InsertPage(page, context.cacheOffset); - } + } else + DEBUG_PAGE_ACCESS_START(page); context.page = page; return B_OK; @@ -3870,7 +3921,7 @@ vm_soft_fault(VMAddressSpace* addressSpace, addr_t originalAddress, addr_t physicalAddress; uint32 flags; - vm_page* mappedPage; + vm_page* mappedPage = NULL; if (context.map->ops->query(context.map, address, &physicalAddress, &flags) == B_OK && (flags & PAGE_PRESENT) != 0 @@ -3889,12 +3940,26 @@ vm_soft_fault(VMAddressSpace* addressSpace, addr_t originalAddress, context.map->ops->unlock(context.map); - if (unmapPage) + if (unmapPage) { + // Note: The mapped page is a page of a lower cache. We are + // guaranteed to have that cached locked, our new page is a copy of + // that page, and the page is not busy. The logic for that guarantee + // is as follows: Since the page is mapped, it must live in the top + // cache (ruled out above) or any of its lower caches, and there is + // (was before the new page was inserted) no other page in any + // cache between the top cache and the page's cache (otherwise that + // would be mapped instead). That in turn means that our algorithm + // must have found it and therefore it cannot be busy either. + DEBUG_PAGE_ACCESS_START(mappedPage); vm_unmap_page(area, address, true); + DEBUG_PAGE_ACCESS_END(mappedPage); + } if (mapPage) vm_map_page(area, context.page, address, newProtection); + DEBUG_PAGE_ACCESS_END(context.page); + break; } @@ -5241,6 +5306,8 @@ _user_set_memory_protection(void* _address, size_t size, int protection) if (unmapPage) vm_unmap_page(area, pageAddress, true); + // TODO: We need to lock the page's cache for that, since + // it potentially changes the page's state. } } diff --git a/src/system/kernel/vm/vm_daemons.cpp b/src/system/kernel/vm/vm_daemons.cpp index 0bb61bb6e1..0c8a01b9fd 100644 --- a/src/system/kernel/vm/vm_daemons.cpp +++ b/src/system/kernel/vm/vm_daemons.cpp @@ -271,6 +271,8 @@ check_page_activation(int32 index) if (!locker.IsLocked()) return false; + DEBUG_PAGE_ACCESS_START(page); + bool modified; int32 activation = vm_test_map_activation(page, &modified); if (modified && page->state != PAGE_STATE_MODIFIED) { @@ -293,6 +295,7 @@ check_page_activation(int32 index) track_page_usage(page); #endif + DEBUG_PAGE_ACCESS_END(page); return false; } @@ -310,8 +313,10 @@ check_page_activation(int32 index) // recheck eventual last minute changes if ((flags & PAGE_MODIFIED) != 0 && page->state != PAGE_STATE_MODIFIED) vm_page_set_state(page, PAGE_STATE_MODIFIED); - if ((flags & PAGE_ACCESSED) != 0 && ++page->usage_count >= 0) + if ((flags & PAGE_ACCESSED) != 0 && ++page->usage_count >= 0) { + DEBUG_PAGE_ACCESS_END(page); return false; + } if (page->state == PAGE_STATE_MODIFIED) vm_page_schedule_write_page(page); @@ -321,6 +326,7 @@ check_page_activation(int32 index) T(DeactivatePage(page)); } + DEBUG_PAGE_ACCESS_END(page); return true; } @@ -334,6 +340,8 @@ free_page_swap_space(int32 index) if (!locker.IsLocked()) return false; + DEBUG_PAGE_ACCESS_START(page); + if (page->cache->temporary && page->wired_count == 0 && page->cache->HasPage(page->cache_offset << PAGE_SHIFT) && page->usage_count > 0) { @@ -343,9 +351,11 @@ free_page_swap_space(int32 index) // stolen and we'd lose its data. vm_page_set_state(page, PAGE_STATE_MODIFIED); T(FreedPageSwap(page)); + DEBUG_PAGE_ACCESS_END(page); return true; } } + DEBUG_PAGE_ACCESS_END(page); return false; } #endif diff --git a/src/system/kernel/vm/vm_page.cpp b/src/system/kernel/vm/vm_page.cpp index 5abddea4f6..9d3f59de59 100644 --- a/src/system/kernel/vm/vm_page.cpp +++ b/src/system/kernel/vm/vm_page.cpp @@ -1,4 +1,5 @@ /* + * Copyright 2010, Ingo Weinhold, ingo_weinhold@gmx.de. * Copyright 2002-2009, Axel Dörfler, axeld@pinc-software.de. * Distributed under the terms of the MIT License. * @@ -71,10 +72,10 @@ static addr_t sPhysicalPageOffset; static size_t sNumPages; static vint32 sUnreservedFreePages; static vint32 sPageDeficit; -static size_t sModifiedTemporaryPages; +static vint32 sModifiedTemporaryPages; static ConditionVariable sFreePageCondition; -static mutex sPageLock = MUTEX_INITIALIZER("pages"); +static mutex sPageDeficitLock = MUTEX_INITIALIZER("page deficit"); static sem_id sWriterWaitSem; @@ -392,6 +393,9 @@ dump_page(int argc, char **argv) #if DEBUG_PAGE_QUEUE kprintf("queue: %p\n", page->queue); #endif + #if DEBUG_PAGE_ACCESS + kprintf("accessor: %" B_PRId32 "\n", page->accessing_thread); + #endif kprintf("area mappings:\n"); vm_page_mappings::Iterator iterator = page->mappings.GetIterator(); @@ -503,7 +507,7 @@ dump_page_stats(int argc, char **argv) kprintf("wired: %lu\nmodified: %lu\nfree: %lu\nclear: %lu\n", counter[PAGE_STATE_WIRED], counter[PAGE_STATE_MODIFIED], counter[PAGE_STATE_FREE], counter[PAGE_STATE_CLEAR]); - kprintf("unreserved free pages: %lu\n", sUnreservedFreePages); + kprintf("unreserved free pages: %" B_PRId32 "\n", sUnreservedFreePages); kprintf("page deficit: %lu\n", sPageDeficit); kprintf("mapped pages: %lu\n", gMappedPagesCount); @@ -522,22 +526,25 @@ dump_page_stats(int argc, char **argv) } -static status_t -set_page_state_nolock(vm_page *page, int pageState) +/*! The caller must make sure that no-one else tries to change the page's state + while the function is called. If the page has a cache, this can be done by + locking the cache. +*/ +static void +set_page_state(vm_page *page, int pageState, bool queuesLocked) { - if (pageState == page->state) - return B_OK; + DEBUG_PAGE_ACCESS_CHECK(page); - VMPageQueue *fromQueue = NULL; - VMPageQueue *toQueue = NULL; + if (pageState == page->state) + return; + + VMPageQueue* fromQueue; int32 freeCountDiff = 0; switch (page->state) { case PAGE_STATE_BUSY: case PAGE_STATE_ACTIVE: - case PAGE_STATE_WIRED: - case PAGE_STATE_UNUSED: fromQueue = &sActivePageQueue; break; case PAGE_STATE_INACTIVE: @@ -554,10 +561,14 @@ set_page_state_nolock(vm_page *page, int pageState) fromQueue = &sClearPageQueue; freeCountDiff = -1; break; - default: - panic("vm_page_set_state: vm_page %p in invalid state %d\n", - page, page->state); + case PAGE_STATE_WIRED: + case PAGE_STATE_UNUSED: + fromQueue = NULL; break; + default: + panic("set_page_state: vm_page %p in invalid state %d\n", + page, page->state); + return; } if (page->state == PAGE_STATE_CLEAR || page->state == PAGE_STATE_FREE) { @@ -565,11 +576,11 @@ set_page_state_nolock(vm_page *page, int pageState) panic("free page %p has cache", page); } + VMPageQueue* toQueue; + switch (pageState) { case PAGE_STATE_BUSY: case PAGE_STATE_ACTIVE: - case PAGE_STATE_WIRED: - case PAGE_STATE_UNUSED: toQueue = &sActivePageQueue; break; case PAGE_STATE_INACTIVE: @@ -586,14 +597,22 @@ set_page_state_nolock(vm_page *page, int pageState) toQueue = &sClearPageQueue; freeCountDiff++; break; + case PAGE_STATE_WIRED: + case PAGE_STATE_UNUSED: + toQueue = NULL; + break; default: - panic("vm_page_set_state: invalid target state %d\n", pageState); + panic("set_page_state: invalid target state %d\n", pageState); + return; } if (pageState == PAGE_STATE_CLEAR || pageState == PAGE_STATE_FREE || pageState == PAGE_STATE_INACTIVE) { - if (sPageDeficit > 0) - sFreePageCondition.NotifyOne(); + if (sPageDeficit > 0) { + MutexLocker pageDeficitLocker(sPageDeficitLock); + if (sPageDeficit > 0) + sFreePageCondition.NotifyOne(); + } if (pageState != PAGE_STATE_INACTIVE) { if (page->cache != NULL) @@ -604,9 +623,9 @@ set_page_state_nolock(vm_page *page, int pageState) } if (page->cache != NULL && page->cache->temporary) { if (pageState == PAGE_STATE_MODIFIED) - sModifiedTemporaryPages++; + atomic_add(&sModifiedTemporaryPages, 1); else if (page->state == PAGE_STATE_MODIFIED) - sModifiedTemporaryPages--; + atomic_add(&sModifiedTemporaryPages, -1); } #ifdef PAGE_ALLOCATION_TRACING @@ -616,22 +635,52 @@ set_page_state_nolock(vm_page *page, int pageState) } #endif // PAGE_ALLOCATION_TRACING - page->state = pageState; - toQueue->MoveFrom(fromQueue, page); + // move the page + if (toQueue == fromQueue) { + // Note: Theoretically we are required to lock when changing the page + // state, even if we don't change the queue. We actually don't have to + // do this, though, since only for the active queue there are different + // page states and active pages have a cache that must be locked at + // this point. So we rely on the fact that everyone must lock the cache + // before trying to change/interpret the page state. + ASSERT(page->cache != NULL); + page->cache->AssertLocked(); + page->state = pageState; + } else { + VMPageQueuePairLocker locker; + if (!queuesLocked) + locker.SetTo(fromQueue, toQueue); + + if (fromQueue != NULL) + fromQueue->Remove(page); + + page->state = pageState; + + if (toQueue != NULL) { + if (pageState == PAGE_STATE_CLEAR || pageState == PAGE_STATE_FREE) { + DEBUG_PAGE_ACCESS_END(page); + // prepend free/clear pages to be more cache friendly. + toQueue->Prepend(page); + } else + toQueue->Append(page); + + } + } if (freeCountDiff != 0) atomic_add(&sUnreservedFreePages, freeCountDiff); - - return B_OK; } /*! Moves a modified page into either the active or inactive page queue depending on its usage count and wiring. + The page queues must not be locked. */ static void move_page_to_active_or_inactive_queue(vm_page *page, bool dequeued) { + DEBUG_PAGE_ACCESS_CHECK(page); + // Note, this logic must be in sync with what the page daemon does int32 state; if (!page->mappings.IsEmpty() || page->usage_count >= 0 @@ -644,11 +693,13 @@ move_page_to_active_or_inactive_queue(vm_page *page, bool dequeued) page->state = state; VMPageQueue& queue = state == PAGE_STATE_ACTIVE ? sActivePageQueue : sInactivePageQueue; + queue.Lock(); queue.Append(page); + queue.Unlock(); if (page->cache->temporary) - sModifiedTemporaryPages--; + atomic_add(&sModifiedTemporaryPages, -1); } else - set_page_state_nolock(page, state); + set_page_state(page, state, false); } @@ -688,7 +739,7 @@ page_scrubber(void *unused) continue; // get some pages from the free queue - MutexLocker locker(sPageLock); + AutoLocker freeQueueLocker(sFreePageQueue); vm_page *page[SCRUB_SIZE]; int32 scrubCount = 0; @@ -697,11 +748,13 @@ page_scrubber(void *unused) if (page[i] == NULL) break; + DEBUG_PAGE_ACCESS_START(page[i]); + page[i]->state = PAGE_STATE_BUSY; scrubCount++; } - locker.Unlock(); + freeQueueLocker.Unlock(); if (scrubCount == 0) { vm_page_unreserve_pages(SCRUB_SIZE); @@ -714,15 +767,16 @@ page_scrubber(void *unused) for (int32 i = 0; i < scrubCount; i++) clear_page(page[i]); - locker.Lock(); + AutoLocker clearQueueLocker(sClearPageQueue); // and put them into the clear queue for (int32 i = 0; i < scrubCount; i++) { page[i]->state = PAGE_STATE_CLEAR; sClearPageQueue.Append(page[i]); + DEBUG_PAGE_ACCESS_END(page[i]); } - locker.Unlock(); + clearQueueLocker.Unlock(); vm_page_unreserve_pages(SCRUB_SIZE); @@ -746,6 +800,8 @@ remove_page_marker(struct vm_page &marker) if (marker.state == PAGE_STATE_UNUSED) return; + DEBUG_PAGE_ACCESS_CHECK(&marker); + VMPageQueue *queue; switch (marker.state) { @@ -763,8 +819,9 @@ remove_page_marker(struct vm_page &marker) return; } - MutexLocker locker(sPageLock); + queue->Lock(); queue->Remove(&marker); + queue->Unlock(); marker.state = PAGE_STATE_UNUSED; } @@ -773,9 +830,11 @@ remove_page_marker(struct vm_page &marker) static vm_page * next_modified_page(struct vm_page &marker) { - MutexLocker locker(sPageLock); + AutoLocker locker(sModifiedPageQueue); vm_page *page; + DEBUG_PAGE_ACCESS_CHECK(&marker); + if (marker.state == PAGE_STATE_MODIFIED) { page = sModifiedPageQueue.Next(&marker); sModifiedPageQueue.Remove(&marker); @@ -884,9 +943,14 @@ PageWriteWrapper::~PageWriteWrapper() } +/*! The page's cache must be locked. + The modified queue must be locked. +*/ void PageWriteWrapper::SetTo(vm_page* page, bool dequeuedPage) { + DEBUG_PAGE_ACCESS_CHECK(page); + if (page->state == PAGE_STATE_BUSY) panic("setting page write wrapper to busy page"); @@ -904,6 +968,8 @@ PageWriteWrapper::SetTo(vm_page* page, bool dequeuedPage) } +/*! The page's cache must be locked. +*/ void PageWriteWrapper::ClearModifiedFlag() { @@ -921,6 +987,8 @@ PageWriteWrapper::ClearModifiedFlag() } +/*! The page's cache must be locked. +*/ void PageWriteWrapper::CheckRemoveFromShrunkenCache() { @@ -932,26 +1000,34 @@ PageWriteWrapper::CheckRemoveFromShrunkenCache() } +/*! The page's cache must be locked. + The page queues must not be locked. +*/ void PageWriteWrapper::Done(status_t result) { if (!fIsActive) panic("completing page write wrapper that is not active"); + DEBUG_PAGE_ACCESS_CHECK(fPage); + if (result == B_OK) { // put it into the active/inactive queue move_page_to_active_or_inactive_queue(fPage, fDequeuedPage); fPage->busy_writing = false; + DEBUG_PAGE_ACCESS_END(fPage); } else { // Writing the page failed -- move to the modified queue. If we dequeued // it from there, just enqueue it again, otherwise set the page state // explicitly, which will take care of moving between the queues. if (fDequeuedPage) { fPage->state = PAGE_STATE_MODIFIED; + sModifiedPageQueue.Lock(); sModifiedPageQueue.Append(fPage); + sModifiedPageQueue.Unlock(); } else { fPage->state = fOldPageState; - set_page_state_nolock(fPage, PAGE_STATE_MODIFIED); + set_page_state(fPage, PAGE_STATE_MODIFIED, false); } if (!fPage->busy_writing) { @@ -961,12 +1037,14 @@ PageWriteWrapper::Done(status_t result) // Adjust temporary modified pages count, if necessary. if (fDequeuedPage && fCache->temporary) - sModifiedTemporaryPages--; + atomic_add(&sModifiedTemporaryPages, -1); // free the page - set_page_state_nolock(fPage, PAGE_STATE_FREE); - } else + set_page_state(fPage, PAGE_STATE_FREE, false); + } else { fPage->busy_writing = false; + DEBUG_PAGE_ACCESS_END(fPage); + } } @@ -975,6 +1053,8 @@ PageWriteWrapper::Done(status_t result) } +/*! The page's cache must be locked. +*/ void PageWriteTransfer::SetTo(PageWriterRun* run, vm_page* page, int32 maxPages) { @@ -991,6 +1071,8 @@ PageWriteTransfer::SetTo(PageWriterRun* run, vm_page* page, int32 maxPages) } +/*! The page's cache must be locked. +*/ bool PageWriteTransfer::AddPage(vm_page* page) { @@ -1114,6 +1196,9 @@ PageWriterRun::PrepareNextRun() } +/*! The page's cache must be locked. + The modified queue must be locked. +*/ void PageWriterRun::AddPage(vm_page* page) { @@ -1155,11 +1240,9 @@ PageWriterRun::Go() fWrappers[checkIndex++].CheckRemoveFromShrunkenCache(); } - MutexLocker locker(sPageLock); for (uint32 j = 0; j < transfer.PageCount(); j++) fWrappers[wrapperIndex++].Done(transfer.Status()); - locker.Unlock(); transfer.Cache()->Unlock(); } @@ -1214,9 +1297,16 @@ page_writer(void* /*unused*/) marker.type = PAGE_TYPE_DUMMY; marker.cache = NULL; marker.state = PAGE_STATE_UNUSED; +#if DEBUG_PAGE_QUEUE + marker.queue = NULL; +#endif +#if DEBUG_PAGE_ACCESS + marker.accessing_thread = thread_get_current_thread_id(); +#endif while (true) { - if (sModifiedPageQueue.Count() - sModifiedTemporaryPages < 1024) { + if ((int32)sModifiedPageQueue.Count() - sModifiedTemporaryPages + < 1024) { int32 count = 0; get_sem_count(sWriterWaitSem, &count); if (count == 0) @@ -1226,10 +1316,13 @@ page_writer(void* /*unused*/) // all 3 seconds when no one triggers us } + int32 modifiedPages = sModifiedPageQueue.Count() + - sModifiedTemporaryPages; + if (modifiedPages <= 0) + continue; + // depending on how urgent it becomes to get pages to disk, we adjust // our I/O priority - page_num_t modifiedPages = sModifiedPageQueue.Count() - - sModifiedTemporaryPages; uint32 lowPagesState = low_resource_state(B_KERNEL_RESOURCE_PAGES); int32 ioPriority = B_IDLE_PRIORITY; if (lowPagesState >= B_LOW_RESOURCE_CRITICAL @@ -1265,6 +1358,8 @@ page_writer(void* /*unused*/) if (!cacheLocker.IsLocked()) continue; + DEBUG_PAGE_ACCESS_START(page); + VMCache *cache = page->cache; // Don't write back wired (locked) pages and don't write RAM pages @@ -1278,32 +1373,33 @@ page_writer(void* /*unused*/) (off_t)page->cache_offset << PAGE_SHIFT)) #endif )) { + DEBUG_PAGE_ACCESS_END(page); continue; } // we need our own reference to the store, as it might // currently be destructed if (cache->AcquireUnreferencedStoreRef() != B_OK) { + DEBUG_PAGE_ACCESS_END(page); cacheLocker.Unlock(); thread_yield(true); continue; } - MutexLocker locker(sPageLock); - // state might have changed while we were locking the cache if (page->state != PAGE_STATE_MODIFIED) { // release the cache reference - locker.Unlock(); + DEBUG_PAGE_ACCESS_END(page); cache->ReleaseStoreRef(); continue; } - sModifiedPageQueue.Remove(page); + sModifiedPageQueue.Lock(); + sModifiedPageQueue.Remove(page); run.AddPage(page); - locker.Unlock(); + sModifiedPageQueue.Unlock(); //dprintf("write page %p, cache %p (%ld)\n", page, page->cache, page->cache->ref_count); TPW(WritePage(page)); @@ -1346,25 +1442,23 @@ page_writer(void* /*unused*/) static vm_page * find_page_candidate(struct vm_page &marker) { - MutexLocker locker(sPageLock); - VMPageQueue *queue; + DEBUG_PAGE_ACCESS_CHECK(&marker); + + AutoLocker locker(sInactivePageQueue); vm_page *page; if (marker.state == PAGE_STATE_UNUSED) { // Get the first free pages of the (in)active queue - queue = &sInactivePageQueue; page = sInactivePageQueue.Head(); } else { // Get the next page of the current queue - if (marker.state == PAGE_STATE_INACTIVE) { - queue = &sInactivePageQueue; - } else { + if (marker.state != PAGE_STATE_INACTIVE) { panic("invalid marker %p state", &marker); - queue = NULL; + return NULL; } - page = queue->Next(&marker); - queue->Remove(&marker); + page = sInactivePageQueue.Next(&marker); + sInactivePageQueue.Remove(&marker); marker.state = PAGE_STATE_UNUSED; } @@ -1372,11 +1466,11 @@ find_page_candidate(struct vm_page &marker) if (!is_marker_page(page) && page->state == PAGE_STATE_INACTIVE) { // we found a candidate, insert marker marker.state = PAGE_STATE_INACTIVE; - queue->InsertAfter(page, &marker); + sInactivePageQueue.InsertAfter(page, &marker); return page; } - page = queue->Next(page); + page = sInactivePageQueue.Next(page); } return NULL; @@ -1390,23 +1484,27 @@ steal_page(vm_page *page) if (vm_cache_acquire_locked_page_cache(page, false) == NULL) return false; - AutoLocker cacheLocker(page->cache, true, false); + AutoLocker cacheLocker(page->cache, true); MethodDeleter _2(page->cache, &VMCache::ReleaseRefLocked); // check again if that page is still a candidate if (page->state != PAGE_STATE_INACTIVE) return false; + DEBUG_PAGE_ACCESS_START(page); + // recheck eventual last minute changes uint32 flags; vm_remove_all_page_mappings(page, &flags); if ((flags & PAGE_MODIFIED) != 0) { // page was modified, don't steal it - vm_page_set_state(page, PAGE_STATE_MODIFIED); + set_page_state(page, PAGE_STATE_MODIFIED, false); + DEBUG_PAGE_ACCESS_END(page); return false; } else if ((flags & PAGE_ACCESSED) != 0) { // page is in active use, don't steal it - vm_page_set_state(page, PAGE_STATE_ACTIVE); + set_page_state(page, PAGE_STATE_ACTIVE, false); + DEBUG_PAGE_ACCESS_END(page); return false; } @@ -1417,7 +1515,7 @@ steal_page(vm_page *page) page->cache->RemovePage(page); - MutexLocker _(sPageLock); + AutoLocker _(sInactivePageQueue); sInactivePageQueue.Remove(page); return true; } @@ -1431,6 +1529,12 @@ steal_pages(vm_page **pages, size_t count) marker.type = PAGE_TYPE_DUMMY; marker.cache = NULL; marker.state = PAGE_STATE_UNUSED; +#if DEBUG_PAGE_QUEUE + marker.queue = NULL; +#endif +#if DEBUG_PAGE_ACCESS + marker.accessing_thread = thread_get_current_thread_id(); +#endif bool tried = false; size_t stolen = 0; @@ -1441,9 +1545,10 @@ steal_pages(vm_page **pages, size_t count) break; if (steal_page(page)) { - MutexLocker locker(sPageLock); + AutoLocker locker(sFreePageQueue); sFreePageQueue.Append(page); page->state = PAGE_STATE_FREE; + DEBUG_PAGE_ACCESS_END(page); locker.Unlock(); atomic_add(&sUnreservedFreePages, 1); @@ -1458,8 +1563,6 @@ steal_pages(vm_page **pages, size_t count) remove_page_marker(marker); - MutexLocker locker(sPageLock); - if (count == 0 || sUnreservedFreePages >= 0) return stolen; @@ -1470,6 +1573,8 @@ steal_pages(vm_page **pages, size_t count) // we need to wait for pages to become inactive + MutexLocker pageDeficitLocker(sPageDeficitLock); + sPageDeficit++; if (sUnreservedFreePages >= 0) { // There are enough pages available now. No need to wait after all. @@ -1479,7 +1584,7 @@ steal_pages(vm_page **pages, size_t count) ConditionVariableEntry freeConditionEntry; freeConditionEntry.Add(&sFreePageQueue); - locker.Unlock(); + pageDeficitLocker.Unlock(); if (tried) { // We tried all potential pages, but one or more couldn't be stolen @@ -1495,7 +1600,7 @@ steal_pages(vm_page **pages, size_t count) freeConditionEntry.Wait(); } - locker.Lock(); + pageDeficitLocker.Lock(); sPageDeficit--; if (sUnreservedFreePages >= 0) @@ -1554,12 +1659,15 @@ vm_page_write_modified_page_range(struct VMCache* cache, uint32 firstPage, bool dequeuedPage = false; if (page != NULL) { + DEBUG_PAGE_ACCESS_START(page); + if (page->state == PAGE_STATE_MODIFIED) { - MutexLocker locker(&sPageLock); + AutoLocker locker(sModifiedPageQueue); sModifiedPageQueue.Remove(page); dequeuedPage = true; } else if (page->state == PAGE_STATE_BUSY || !vm_test_map_modification(page)) { + DEBUG_PAGE_ACCESS_END(page); page = NULL; } } @@ -1599,13 +1707,9 @@ vm_page_write_modified_page_range(struct VMCache* cache, uint32 firstPage, wrappers[i]->CheckRemoveFromShrunkenCache(); } - MutexLocker locker(&sPageLock); - for (int32 i = 0; i < usedWrappers; i++) wrappers[i]->Done(status); - locker.Unlock(); - usedWrappers = 0; if (page != NULL) { @@ -1616,7 +1720,7 @@ vm_page_write_modified_page_range(struct VMCache* cache, uint32 firstPage, } if (wrapperPool != stackWrappers) - delete [] wrapperPool; + delete[] wrapperPool; return B_OK; } @@ -1661,10 +1765,14 @@ vm_page_schedule_write_page_range(struct VMCache *cache, uint32 firstPage, if (page->cache_offset >= endPage) break; + DEBUG_PAGE_ACCESS_START(page); + if (page->state == PAGE_STATE_MODIFIED) { vm_page_requeue(page, false); modified++; } + + DEBUG_PAGE_ACCESS_END(page); } if (modified > 0) @@ -1703,6 +1811,13 @@ vm_page_init(kernel_args *args) { TRACE(("vm_page_init: entry\n")); + // init page queues + sModifiedPageQueue.Init("modified pages queue", 5); + sInactivePageQueue.Init("inactive pages queue", 4); + sActivePageQueue.Init("active pages queue", 3); + sFreePageQueue.Init("free pages queue", 2); + sClearPageQueue.Init("clear pages queue", 1); + // map in the new free page table sPages = (vm_page *)vm_allocate_early(args, sNumPages * sizeof(vm_page), ~0L, B_KERNEL_READ_AREA | B_KERNEL_WRITE_AREA); @@ -1723,6 +1838,9 @@ vm_page_init(kernel_args *args) #if DEBUG_PAGE_QUEUE sPages[i].queue = NULL; #endif + #if DEBUG_PAGE_ACCESS + sPages[i].accessing_thread = -1; + #endif sFreePageQueue.Append(&sPages[i]); } @@ -1810,14 +1928,19 @@ vm_mark_page_range_inuse(addr_t startPage, addr_t length) return B_BAD_VALUE; } - MutexLocker _(sPageLock); + VMPageQueuePairLocker locker(sFreePageQueue, sClearPageQueue); for (addr_t i = 0; i < length; i++) { vm_page *page = &sPages[startPage + i]; switch (page->state) { case PAGE_STATE_FREE: case PAGE_STATE_CLEAR: - set_page_state_nolock(page, PAGE_STATE_UNUSED); +// TODO: This violates the page reservation policy, since we remove pages from +// the free/clear queues without having reserved them before. This should happen +// in the early boot process only, though. + DEBUG_PAGE_ACCESS_START(page); + set_page_state(page, PAGE_STATE_UNUSED, true); + DEBUG_PAGE_ACCESS_END(page); break; case PAGE_STATE_WIRED: break; @@ -1854,7 +1977,7 @@ vm_page_unreserve_pages(uint32 count) atomic_add(&sUnreservedFreePages, count); if (sPageDeficit > 0) { - MutexLocker locker(sPageLock); + MutexLocker pageDeficitLocker(sPageDeficitLock); if (sPageDeficit > 0) sFreePageCondition.NotifyAll(); } @@ -1879,13 +2002,9 @@ vm_page_reserve_pages(uint32 count) if (oldFreePages >= (int32)count) return; - MutexLocker locker(sPageLock); - if (oldFreePages > 0) count -= oldFreePages; - locker.Unlock(); - steal_pages(NULL, count + 1); // we get one more, just in case we can do something someone // else can't @@ -1936,15 +2055,15 @@ vm_page_allocate_page(int pageState) atomic_add(&sUnreservedFreePages, -1); - MutexLocker locker(sPageLock); - T(AllocatePage()); + VMPageQueuePairLocker freeClearQueuelocker(sFreePageQueue, sClearPageQueue); + vm_page *page = queue->RemoveHead(); if (page == NULL) { #if DEBUG_PAGE_QUEUE if (queue->Count() != 0) - panic("queue %p corrupted, count = %d\n", queue, queue->Count()); + panic("queue %p corrupted, count = %ld\n", queue, queue->Count()); #endif // if the primary queue was empty, grab the page from the @@ -1958,15 +2077,20 @@ vm_page_allocate_page(int pageState) if (page->cache != NULL) panic("supposed to be free page %p has cache\n", page); + DEBUG_PAGE_ACCESS_START(page); + int oldPageState = page->state; page->state = PAGE_STATE_BUSY; page->usage_count = 2; + freeClearQueuelocker.Unlock(); + + sActivePageQueue.Lock(); sActivePageQueue.Append(page); + sActivePageQueue.Unlock(); - locker.Unlock(); - - // if needed take the page from the free queue and zero it out + // clear the page, if we had to take it from the free queue and a clear + // page was requested if (pageState == PAGE_STATE_CLEAR && oldPageState != PAGE_STATE_CLEAR) clear_page(page); @@ -1974,10 +2098,58 @@ vm_page_allocate_page(int pageState) } +static vm_page* +allocate_page_run(page_num_t start, page_num_t length, int pageState, + VMPageQueuePairLocker& freeClearQueuelocker) +{ + T(AllocatePageRun(length)); + + // pull the pages out of the appropriate queues + for (page_num_t i = 0; i < length; i++) { + vm_page& page = sPages[start + i]; + DEBUG_PAGE_ACCESS_START(&page); + if (page.state == PAGE_STATE_CLEAR) { + page.is_cleared = true; + sClearPageQueue.Remove(&page); + } else { + page.is_cleared = false; + sFreePageQueue.Remove(&page); + } + + page.state = PAGE_STATE_BUSY; + } + + freeClearQueuelocker.Unlock(); + + // add the pages to the active queue + AutoLocker activeQueueLocker(sActivePageQueue); + + for (page_num_t i = 0; i < length; i++) { + vm_page& page = sPages[start + i]; + sActivePageQueue.Append(&page); + page.usage_count = 1; + } + + activeQueueLocker.Unlock(); + + // clear pages, if requested + if (pageState == PAGE_STATE_CLEAR) { + for (page_num_t i = 0; i < length; i++) { + if (!sPages[start + i].is_cleared) + clear_page(&sPages[start + i]); + } + } + + // Note: We don't unreserve the pages since we pulled them out of the + // free/clear queues without adjusting sUnreservedFreePages. + + return &sPages[start]; +} + + vm_page * vm_page_allocate_page_run(int pageState, addr_t base, addr_t length) { - vm_page *firstPage = NULL; uint32 start = base >> PAGE_SHIFT; if (!vm_page_try_reserve_pages(length)) @@ -1985,12 +2157,15 @@ vm_page_allocate_page_run(int pageState, addr_t base, addr_t length) // TODO: add more tries, ie. free some inactive, ... // no free space - MutexLocker locker(sPageLock); + VMPageQueuePairLocker freeClearQueuelocker(sFreePageQueue, sClearPageQueue); for (;;) { bool foundRun = true; - if (start + length > sNumPages) - break; + if (start + length > sNumPages) { + freeClearQueuelocker.Unlock(); + vm_page_unreserve_pages(length); + return NULL; + } uint32 i; for (i = 0; i < length; i++) { @@ -2002,35 +2177,12 @@ vm_page_allocate_page_run(int pageState, addr_t base, addr_t length) } } - if (foundRun) { - // pull the pages out of the appropriate queues - for (i = 0; i < length; i++) { - sPages[start + i].is_cleared - = sPages[start + i].state == PAGE_STATE_CLEAR; - set_page_state_nolock(&sPages[start + i], PAGE_STATE_BUSY); - sPages[start + i].usage_count = 2; - } + if (foundRun) + return allocate_page_run(start, length, pageState, + freeClearQueuelocker); - firstPage = &sPages[start]; - break; - } else - start += i; + start += i; } - - T(AllocatePageRun(length)); - - locker.Unlock(); - - vm_page_unreserve_pages(length); - - if (firstPage != NULL && pageState == PAGE_STATE_CLEAR) { - for (uint32 i = 0; i < length; i++) { - if (!sPages[start + i].is_cleared) - clear_page(&sPages[start + i]); - } - } - - return firstPage; } @@ -2057,10 +2209,9 @@ vm_page_allocate_page_run_no_base(int pageState, addr_t count) // TODO: add more tries, ie. free some inactive, ... // no free space - MutexLocker locker(sPageLock); + VMPageQueuePairLocker freeClearQueuelocker(sFreePageQueue, sClearPageQueue); - vm_page *firstPage = NULL; - for (uint32 twice = 0; twice < 2; twice++) { + for (;;) { VMPageQueue::Iterator it = queue->GetIterator(); while (vm_page *page = it.Next()) { vm_page *current = page; @@ -2077,42 +2228,18 @@ vm_page_allocate_page_run_no_base(int pageState, addr_t count) } if (foundRun) { - atomic_add(&sUnreservedFreePages, -count); - - // pull the pages out of the appropriate queues - current = page; - for (uint32 i = 0; i < count; i++, current++) { - current->is_cleared = current->state == PAGE_STATE_CLEAR; - set_page_state_nolock(current, PAGE_STATE_BUSY); - current->usage_count = 2; - } - - firstPage = page; - break; + return allocate_page_run(page - sPages, count, pageState, + freeClearQueuelocker); } } - if (firstPage != NULL) - break; + if (queue == otherQueue) { + freeClearQueuelocker.Unlock(); + vm_page_unreserve_pages(count); + } queue = otherQueue; } - - T(AllocatePageRun(count)); - - locker.Unlock(); - - vm_page_unreserve_pages(count); - - if (firstPage != NULL && pageState == PAGE_STATE_CLEAR) { - vm_page *current = firstPage; - for (uint32 i = 0; i < count; i++, current++) { - if (!current->is_cleared) - clear_page(current); - } - } - - return firstPage; } @@ -2144,40 +2271,40 @@ vm_lookup_page(addr_t pageNumber) void vm_page_free(VMCache *cache, vm_page *page) { - MutexLocker _(sPageLock); + ASSERT(page->state != PAGE_STATE_FREE && page->state != PAGE_STATE_CLEAR); - if (page->cache == NULL && page->state == PAGE_STATE_MODIFIED - && cache->temporary) { - sModifiedTemporaryPages--; - } + if (page->state == PAGE_STATE_MODIFIED && cache->temporary) + atomic_add(&sModifiedTemporaryPages, -1); - set_page_state_nolock(page, PAGE_STATE_FREE); + set_page_state(page, PAGE_STATE_FREE, false); } -status_t +void vm_page_set_state(vm_page *page, int pageState) { - MutexLocker _(sPageLock); + ASSERT(page->state != PAGE_STATE_FREE && page->state != PAGE_STATE_CLEAR); - return set_page_state_nolock(page, pageState); + set_page_state(page, pageState, false); } /*! Moves a page to either the tail of the head of its current queue, depending on \a tail. + The page must have a cache and the cache must be locked! */ void vm_page_requeue(struct vm_page *page, bool tail) { - MutexLocker _(sPageLock); + ASSERT(page->cache != NULL); + page->cache->AssertLocked(); + DEBUG_PAGE_ACCESS_CHECK(page); + VMPageQueue *queue = NULL; switch (page->state) { case PAGE_STATE_BUSY: case PAGE_STATE_ACTIVE: - case PAGE_STATE_WIRED: - case PAGE_STATE_UNUSED: queue = &sActivePageQueue; break; case PAGE_STATE_INACTIVE: @@ -2187,23 +2314,28 @@ vm_page_requeue(struct vm_page *page, bool tail) queue = &sModifiedPageQueue; break; case PAGE_STATE_FREE: - queue = &sFreePageQueue; - break; case PAGE_STATE_CLEAR: - queue = &sClearPageQueue; - break; + panic("vm_page_requeue() called for free/clear page %p", page); + return; + case PAGE_STATE_WIRED: + case PAGE_STATE_UNUSED: + return; default: panic("vm_page_touch: vm_page %p in invalid state %d\n", page, page->state); break; } + queue->Lock(); + queue->Remove(page); if (tail) queue->Append(page); else queue->Prepend(page); + + queue->Unlock(); }