vm: Replace the VMAreas OpenHashTable with an AVLTree.

Since we used a hash table with a fixed size (1024), collisions were
obviously inevitable, meaning that while insertions would always be
fast, lookups and deletions would take linear time to search the
linked-list for the area in question. For recently-created areas,
this would be fast; for less-recently-created areas, it would get
slower and slower and slower.

A particularly pathological case was the "mmap/24-1" test from the
Open POSIX Testsuite, which creates millions of areas until it hits
ENOMEM; it then simply exits, at which point it would run for minutes
and minutes in the kernel team deletion routines; how long I don't know,
as I rebooted before it finished.

This change fixes that problem, among others, at the cost of increased
area creation time, by using an AVL tree instead of a hash. For comparison,
mmap'ing 2 million areas with the "24-1" test before this change took
around 0m2.706s of real time, while afterwards it takes about 0m3.118s,
or around a 15% increase (1.152x).

On the other hand, the total test runtime for 2 million areas went from
around 2m11.050s to 0m4.035s, or around a 97% decrease (0.031x); in other
words, with this new code, it is *32 times faster.*

Area insertion will no longer be O(1), however, so the time increase
may go up with the number of areas present on the system; but if it's
only around 3 seconds to create 2 million areas, or about 1.56 us per area,
vs. 1.35 us before, I don't think that's worth worrying about.

My nonscientific "compile HaikuDepot with 2 cores in VM" benchmark
seems to be within the realm of "noise", anyway, with most results
both before and after this change coming in around 47s real time.

Change-Id: I230e17de4f80304d082152af83db8bd5abe7b831

headers/private/kernel/vm/VMArea.h

@@ -15,7 +15,7 @@
 #include <lock.h>
 #include <util/DoublyLinkedList.h>
 #include <util/SinglyLinkedList.h>
-#include <util/OpenHashTable.h>
+#include <util/AVLTree.h>
 #include <vm/vm_types.h>
 
 
@@ -88,7 +88,12 @@ struct VMPageWiringInfo {
 };
 
 
-struct VMArea {
+struct VMAreasTreeNode {
+	AVLTreeNode tree_node;
+};
+
+
+struct VMArea : private VMAreasTreeNode {
 public:
 	enum {
 		// AddWaiterIfWired() flags
@@ -117,7 +122,6 @@ public:
 	struct VMAddressSpace*	address_space;
 	struct VMArea*			cache_next;
 	struct VMArea*			cache_prev;
-	struct VMArea*			hash_next;
 
 			addr_t				Base() const	{ return fBase; }
 			size_t				Size() const	{ return fSize; }
@@ -149,7 +153,7 @@ protected:
 			status_t			Init(const char* name, uint32 allocationFlags);
 
 protected:
-			friend struct VMAddressSpace;
+			friend struct VMAreasTreeDefinition;
 			friend struct VMKernelAddressSpace;
 			friend struct VMUserAddressSpace;
 
@@ -164,35 +168,42 @@ protected:
 };
 
 
-struct VMAreaHashDefinition {
+struct VMAreasTreeDefinition {
-	typedef area_id		KeyType;
+	typedef area_id		Key;
-	typedef VMArea		ValueType;
+	typedef VMArea		Value;
 
-	size_t HashKey(area_id key) const
+	AVLTreeNode* GetAVLTreeNode(VMArea* value) const
 	{
-		return key;
+		return &value->tree_node;
 	}
 
-	size_t Hash(const VMArea* value) const
+	VMArea* GetValue(AVLTreeNode* node) const
 	{
-		return HashKey(value->id);
+		const addr_t vmTreeNodeAddr = (addr_t)node
+			- offsetof(VMAreasTreeNode, tree_node);
+		VMAreasTreeNode* vmTreeNode =
+			reinterpret_cast<VMAreasTreeNode*>(vmTreeNodeAddr);
+		return static_cast<VMArea*>(vmTreeNode);
 	}
 
-	bool Compare(area_id key, const VMArea* value) const
+	int Compare(area_id key, const VMArea* value) const
 	{
-		return value->id == key;
+		const area_id valueId = value->id;
+		if (valueId == key)
+			return 0;
+		return key < valueId ? -1 : 1;
 	}
 
-	VMArea*& GetLink(VMArea* value) const
+	int Compare(const VMArea* a, const VMArea* b) const
 	{
-		return value->hash_next;
+		return Compare(a->id, b);
 	}
 };
 
-typedef BOpenHashTable<VMAreaHashDefinition> VMAreaHashTable;
+typedef AVLTree<VMAreasTreeDefinition> VMAreasTree;
 
 
-struct VMAreaHash {
+struct VMAreas {
 	static	status_t			Init();
 
 	static	status_t			ReadLock()
@@ -205,18 +216,18 @@ struct VMAreaHash {
 									{ rw_lock_write_unlock(&sLock); }
 
 	static	VMArea*				LookupLocked(area_id id)
-									{ return sTable.Lookup(id); }
+									{ return sTree.Find(id); }
 	static	VMArea*				Lookup(area_id id);
 	static	area_id				Find(const char* name);
 	static	void				Insert(VMArea* area);
 	static	void				Remove(VMArea* area);
 
-	static	VMAreaHashTable::Iterator GetIterator()
+	static	VMAreasTree::Iterator GetIterator()
-									{ return sTable.GetIterator(); }
+									{ return sTree.GetIterator(); }
 
 private:
 	static	rw_lock				sLock;
-	static	VMAreaHashTable		sTable;
+	static	VMAreasTree			sTree;
 };
 
 

src/system/kernel/arch/riscv64/arch_vm.cpp

@@ -161,16 +161,6 @@ DumpPageTableInt(Pte* pte, uint64_t virtAdr, uint32_t level, PageTableDumper& du
 }
 
 
-static VMArea* LookupArea(area_id id)
-{
-	VMAreaHash::ReadLock();
-	VMArea* area = VMAreaHash::LookupLocked(id);
-	VMAreaHash::ReadUnlock();
-
-	return area;
-}
-
-
 static int
 DumpPageTable(int argc, char** argv)
 {
@@ -198,7 +188,7 @@ DumpPageTable(int argc, char** argv)
 			uint64 areaId;
 			if (!evaluate_debug_expression(argv[curArg++], &areaId, false))
 				return 0;
-			VMArea* area = LookupArea((area_id)areaId);
+			VMArea* area = VMAreas::Lookup((area_id)areaId);
 			if (area == NULL) {
 				kprintf("could not find area %" B_PRId32 "\n", (area_id)areaId);
 				return 0;

src/system/kernel/device_manager/IOCache.cpp

@@ -107,7 +107,7 @@ IOCache::Init(const char* name)
 		VM_PRIORITY_SYSTEM);
 
 	// get the area's cache
-	VMArea* area = VMAreaHash::Lookup(fArea);
+	VMArea* area = VMAreas::Lookup(fArea);
 	if (area == NULL) {
 		panic("IOCache::Init(): Where's our area (id: %" B_PRId32 ")?!", fArea);
 		return B_ERROR;

src/system/kernel/slab/MemoryManager.cpp

@@ -1324,7 +1324,7 @@ MemoryManager::_AllocateArea(uint32 flags, Area*& _area)
 		pagesNeededToMap = translationMap->MaxPagesNeededToMap(
 			(addr_t)area, (addr_t)areaBase + SLAB_AREA_SIZE - 1);
 
-		vmArea = VMAreaHash::Lookup(areaID);
+		vmArea = VMAreas::Lookup(areaID);
 		status_t error = _MapChunk(vmArea, (addr_t)area, kAreaAdminSize,
 			pagesNeededToMap, flags);
 		if (error != B_OK) {
@@ -1607,7 +1607,7 @@ MemoryManager::_ConvertEarlyArea(Area* area)
 	if (areaID < 0)
 		panic("out of memory");
 
-	area->vmArea = VMAreaHash::Lookup(areaID);
+	area->vmArea = VMAreas::Lookup(areaID);
 }
 
 

src/system/kernel/vm/VMAddressSpaceLocking.cpp

@@ -23,15 +23,15 @@ AddressSpaceLockerBase::GetAddressSpaceByAreaID(area_id id)
 {
 	VMAddressSpace* addressSpace = NULL;
 
-	VMAreaHash::ReadLock();
+	VMAreas::ReadLock();
 
-	VMArea* area = VMAreaHash::LookupLocked(id);
+	VMArea* area = VMAreas::LookupLocked(id);
 	if (area != NULL) {
 		addressSpace = area->address_space;
 		addressSpace->Get();
 	}
 
-	VMAreaHash::ReadUnlock();
+	VMAreas::ReadUnlock();
 
 	return addressSpace;
 }
@@ -123,7 +123,7 @@ AddressSpaceReadLocker::SetFromArea(area_id areaID, VMArea*& area)
 
 	fSpace->ReadLock();
 
-	area = VMAreaHash::Lookup(areaID);
+	area = VMAreas::Lookup(areaID);
 
 	if (area == NULL || area->address_space != fSpace) {
 		fSpace->ReadUnlock();
@@ -243,7 +243,7 @@ AddressSpaceWriteLocker::SetFromArea(area_id areaID, VMArea*& area)
 
 	fSpace->WriteLock();
 
-	area = VMAreaHash::Lookup(areaID);
+	area = VMAreas::Lookup(areaID);
 
 	if (area == NULL || area->address_space != fSpace) {
 		fSpace->WriteUnlock();
@@ -259,9 +259,9 @@ status_t
 AddressSpaceWriteLocker::SetFromArea(team_id team, area_id areaID,
 	bool allowKernel, VMArea*& area)
 {
-	VMAreaHash::ReadLock();
+	VMAreas::ReadLock();
 
-	area = VMAreaHash::LookupLocked(areaID);
+	area = VMAreas::LookupLocked(areaID);
 	if (area != NULL
 		&& (area->address_space->ID() == team
 			|| (allowKernel && team == VMAddressSpace::KernelID()))) {
@@ -269,7 +269,7 @@ AddressSpaceWriteLocker::SetFromArea(team_id team, area_id areaID,
 		fSpace->Get();
 	}
 
-	VMAreaHash::ReadUnlock();
+	VMAreas::ReadUnlock();
 
 	if (fSpace == NULL)
 		return B_BAD_VALUE;
@@ -279,7 +279,7 @@ AddressSpaceWriteLocker::SetFromArea(team_id team, area_id areaID,
 
 	fSpace->WriteLock();
 
-	area = VMAreaHash::Lookup(areaID);
+	area = VMAreas::Lookup(areaID);
 
 	if (area == NULL) {
 		fSpace->WriteUnlock();
@@ -529,7 +529,7 @@ MultiAddressSpaceLocker::AddAreaCacheAndLock(area_id areaID,
 		// lock the cache again and check whether anything has changed
 
 		// check whether the area is gone in the meantime
-		area = VMAreaHash::Lookup(areaID);
+		area = VMAreas::Lookup(areaID);
 
 		if (area == NULL) {
 			Unlock();

src/system/kernel/vm/VMArea.cpp

@@ -16,11 +16,8 @@
 #include <vm/VMAddressSpace.h>
 
 
-#define AREA_HASH_TABLE_SIZE 1024
+rw_lock VMAreas::sLock = RW_LOCK_INITIALIZER("areas tree");
-
+VMAreasTree VMAreas::sTree;
-
-rw_lock VMAreaHash::sLock = RW_LOCK_INITIALIZER("area hash");
-VMAreaHashTable VMAreaHash::sTable;
 static area_id sNextAreaID = 1;
 
 
@@ -39,8 +36,7 @@ VMArea::VMArea(VMAddressSpace* addressSpace, uint32 wiring, uint32 protection)
 	page_protections(NULL),
 	address_space(addressSpace),
 	cache_next(NULL),
-	cache_prev(NULL),
+	cache_prev(NULL)
-	hash_next(NULL)
 {
 	new (&mappings) VMAreaMappings;
 }
@@ -203,18 +199,19 @@ VMArea::AddWaiterIfWired(VMAreaUnwiredWaiter* waiter, addr_t base, size_t size,
 }
 
 
-// #pragma mark - VMAreaHash
+// #pragma mark - VMAreas
 
 
 /*static*/ status_t
-VMAreaHash::Init()
+VMAreas::Init()
 {
-	return sTable.Init(AREA_HASH_TABLE_SIZE);
+	new(&sTree) VMAreasTree;
+	return B_OK;
 }
 
 
 /*static*/ VMArea*
-VMAreaHash::Lookup(area_id id)
+VMAreas::Lookup(area_id id)
 {
 	ReadLock();
 	VMArea* area = LookupLocked(id);
@@ -224,7 +221,7 @@ VMAreaHash::Lookup(area_id id)
 
 
 /*static*/ area_id
-VMAreaHash::Find(const char* name)
+VMAreas::Find(const char* name)
 {
 	ReadLock();
 
@@ -233,7 +230,7 @@ VMAreaHash::Find(const char* name)
 	// TODO: Iterating through the whole table can be very slow and the whole
 	// time we're holding the lock! Use a second hash table!
 
-	for (VMAreaHashTable::Iterator it = sTable.GetIterator();
+	for (VMAreasTree::Iterator it = sTree.GetIterator();
 			VMArea* area = it.Next();) {
 		if (strcmp(area->name, name) == 0) {
 			id = area->id;
@@ -248,18 +245,18 @@ VMAreaHash::Find(const char* name)
 
 
 /*static*/ void
-VMAreaHash::Insert(VMArea* area)
+VMAreas::Insert(VMArea* area)
 {
 	WriteLock();
-	sTable.InsertUnchecked(area);
+	sTree.Insert(area);
 	WriteUnlock();
 }
 
 
 /*static*/ void
-VMAreaHash::Remove(VMArea* area)
+VMAreas::Remove(VMArea* area)
 {
 	WriteLock();
-	sTable.RemoveUnchecked(area);
+	sTree.Remove(area);
 	WriteUnlock();
 }

src/system/kernel/vm/vm.cpp

@@ -446,13 +446,13 @@ virtual_page_address(VMArea* area, vm_page* page)
 static VMArea*
 lookup_area(VMAddressSpace* addressSpace, area_id id)
 {
-	VMAreaHash::ReadLock();
+	VMAreas::ReadLock();
 
-	VMArea* area = VMAreaHash::LookupLocked(id);
+	VMArea* area = VMAreas::LookupLocked(id);
 	if (area != NULL && area->address_space != addressSpace)
 		area = NULL;
 
-	VMAreaHash::ReadUnlock();
+	VMAreas::ReadUnlock();
 
 	return area;
 }
@@ -1030,8 +1030,8 @@ map_backing_store(VMAddressSpace* addressSpace, VMCache* cache, off_t offset,
 	if (mapping == REGION_PRIVATE_MAP)
 		cache->Unlock();
 
-	// insert the area in the global area hash table
+	// insert the area in the global areas map
-	VMAreaHash::Insert(area);
+	VMAreas::Insert(area);
 
 	// grab a ref to the address space (the area holds this)
 	addressSpace->Get();
@@ -2366,7 +2366,7 @@ delete_area(VMAddressSpace* addressSpace, VMArea* area,
 {
 	ASSERT(!area->IsWired());
 
-	VMAreaHash::Remove(area);
+	VMAreas::Remove(area);
 
 	// At this point the area is removed from the global hash table, but
 	// still exists in the area list.
@@ -3564,7 +3564,7 @@ dump_area(int argc, char** argv)
 	} else {
 		// walk through the area list, looking for the arguments as a name
 
-		VMAreaHashTable::Iterator it = VMAreaHash::GetIterator();
+		VMAreasTree::Iterator it = VMAreas::GetIterator();
 		while ((area = it.Next()) != NULL) {
 			if (((mode & 4) != 0
 					&& !strcmp(argv[index], area->name))
@@ -3601,7 +3601,7 @@ dump_area_list(int argc, char** argv)
 		B_PRINTF_POINTER_WIDTH, "addr", B_PRINTF_POINTER_WIDTH, "base",
 		B_PRINTF_POINTER_WIDTH, "size");
 
-	VMAreaHashTable::Iterator it = VMAreaHash::GetIterator();
+	VMAreasTree::Iterator it = VMAreas::GetIterator();
 	while ((area = it.Next()) != NULL) {
 		if ((id != 0 && area->address_space->ID() != id)
 			|| (name != NULL && strstr(area->name, name) == NULL))
@@ -4208,9 +4208,9 @@ vm_init(kernel_args* args)
 	vm_cache_init(args);
 
 	{
-		status_t error = VMAreaHash::Init();
+		status_t error = VMAreas::Init();
 		if (error != B_OK)
-			panic("vm_init: error initializing area hash table\n");
+			panic("vm_init: error initializing areas map\n");
 	}
 
 	VMAddressSpace::Init();
@@ -6125,7 +6125,7 @@ area_for(void* address)
 area_id
 find_area(const char* name)
 {
-	return VMAreaHash::Find(name);
+	return VMAreas::Find(name);
 }