From 24ecf1d02189dc639a9779f328303a1fc76e6ccc Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Mon, 10 Jan 2022 16:00:46 +0100 Subject: [PATCH] Kernel: Remove redundant hash map of page tables in PageDirectory The purpose of the PageDirectory::m_page_tables map was really just to act as ref-counting storage for PhysicalPage objects that were being used for the directory's page tables. However, this was basically redundant, since we can find the physical address of each page table from the page directory, and we can find the PhysicalPage object from MemoryManager::get_physical_page_entry(). So if we just manually ref() and unref() the pages when they go in and out of the directory, we no longer need PageDirectory::m_page_tables! Not only does this remove a bunch of kmalloc() traffic, it also solves a race condition that would occur when lazily adding a new page table to a directory: Previously, when MemoryManager::ensure_pte() would call HashMap::set() to insert the new page table into m_page_tables, if the HashMap had to grow its internal storage, it would call kmalloc(). If that kmalloc() would need to perform heap expansion, it would end up calling ensure_pte() again, which would clobber the page directory mapping used by the outer invocation of ensure_pte(). The net result of the above bug would be that any invocation of MemoryManager::ensure_pte() could erroneously return a pointer into a kernel page table instead of the correct one! This whole problem goes away when we remove the HashMap, as ensure_pte() no longer does anything that allocates from the heap. --- Kernel/Memory/MemoryManager.cpp | 70 ++++++++++++++++----------------- Kernel/Memory/MemoryManager.h | 6 +-- Kernel/Memory/PageDirectory.h | 1 - 3 files changed, 34 insertions(+), 43 deletions(-) diff --git a/Kernel/Memory/MemoryManager.cpp b/Kernel/Memory/MemoryManager.cpp index cb361ffc84a..81ffa1db393 100644 --- a/Kernel/Memory/MemoryManager.cpp +++ b/Kernel/Memory/MemoryManager.cpp @@ -123,7 +123,7 @@ UNMAP_AFTER_INIT void MemoryManager::unmap_prekernel() auto end = end_of_prekernel_image.page_base().get(); for (auto i = start; i <= end; i += PAGE_SIZE) - release_pte(kernel_page_directory(), VirtualAddress(i), i == end ? IsLastPTERelease::Yes : IsLastPTERelease::No, UnsafeIgnoreMissingPageTable::Yes); + release_pte(kernel_page_directory(), VirtualAddress(i), i == end ? IsLastPTERelease::Yes : IsLastPTERelease::No); flush_tlb(&kernel_page_directory(), VirtualAddress(start), (end - start) / PAGE_SIZE); } @@ -505,9 +505,7 @@ UNMAP_AFTER_INIT void MemoryManager::initialize_physical_pages() // so finish setting up the kernel page directory m_kernel_page_directory->allocate_kernel_directory(); - // Now create legit PhysicalPage objects for the page tables we created, so that - // we can put them into kernel_page_directory().m_page_tables - auto& kernel_page_tables = kernel_page_directory().m_page_tables; + // Now create legit PhysicalPage objects for the page tables we created. virtual_page_array_current_page = virtual_page_array_base; for (size_t pt_index = 0; pt_index < needed_page_table_count; pt_index++) { VERIFY(virtual_page_array_current_page <= range.end().get()); @@ -515,8 +513,9 @@ UNMAP_AFTER_INIT void MemoryManager::initialize_physical_pages() auto physical_page_index = PhysicalAddress::physical_page_index(pt_paddr.get()); auto& physical_page_entry = m_physical_page_entries[physical_page_index]; auto physical_page = adopt_ref(*new (&physical_page_entry.allocated.physical_page) PhysicalPage(MayReturnToFreeList::No)); - auto result = kernel_page_tables.set(virtual_page_array_current_page & ~0x1fffff, move(physical_page)); - VERIFY(result == AK::HashSetResult::InsertedNewEntry); + + // NOTE: This leaked ref is matched by the unref in MemoryManager::release_pte() + (void)physical_page.leak_ref(); virtual_page_array_current_page += (PAGE_SIZE / sizeof(PageTableEntry)) * PAGE_SIZE; } @@ -568,39 +567,38 @@ PageTableEntry* MemoryManager::ensure_pte(PageDirectory& page_directory, Virtual u32 page_table_index = (vaddr.get() >> 12) & 0x1ff; auto* pd = quickmap_pd(page_directory, page_directory_table_index); - PageDirectoryEntry& pde = pd[page_directory_index]; - if (!pde.is_present()) { - bool did_purge = false; - auto page_table = allocate_user_physical_page(ShouldZeroFill::Yes, &did_purge); - if (!page_table) { - dbgln("MM: Unable to allocate page table to map {}", vaddr); - return nullptr; - } - if (did_purge) { - // If any memory had to be purged, ensure_pte may have been called as part - // of the purging process. So we need to re-map the pd in this case to ensure - // we're writing to the correct underlying physical page - pd = quickmap_pd(page_directory, page_directory_table_index); - VERIFY(&pde == &pd[page_directory_index]); // Sanity check + auto& pde = pd[page_directory_index]; + if (pde.is_present()) + return &quickmap_pt(PhysicalAddress(pde.page_table_base()))[page_table_index]; - VERIFY(!pde.is_present()); // Should have not changed - } - pde.set_page_table_base(page_table->paddr().get()); - pde.set_user_allowed(true); - pde.set_present(true); - pde.set_writable(true); - pde.set_global(&page_directory == m_kernel_page_directory.ptr()); - // Use page_directory_table_index and page_directory_index as key - // This allows us to release the page table entry when no longer needed - auto result = page_directory.m_page_tables.set(vaddr.get() & ~(FlatPtr)0x1fffff, page_table.release_nonnull()); - // If you're hitting this VERIFY on x86_64 chances are a 64-bit pointer was truncated somewhere - VERIFY(result == AK::HashSetResult::InsertedNewEntry); + bool did_purge = false; + auto page_table = allocate_user_physical_page(ShouldZeroFill::Yes, &did_purge); + if (!page_table) { + dbgln("MM: Unable to allocate page table to map {}", vaddr); + return nullptr; } + if (did_purge) { + // If any memory had to be purged, ensure_pte may have been called as part + // of the purging process. So we need to re-map the pd in this case to ensure + // we're writing to the correct underlying physical page + pd = quickmap_pd(page_directory, page_directory_table_index); + VERIFY(&pde == &pd[page_directory_index]); // Sanity check - return &quickmap_pt(PhysicalAddress((FlatPtr)pde.page_table_base()))[page_table_index]; + VERIFY(!pde.is_present()); // Should have not changed + } + pde.set_page_table_base(page_table->paddr().get()); + pde.set_user_allowed(true); + pde.set_present(true); + pde.set_writable(true); + pde.set_global(&page_directory == m_kernel_page_directory.ptr()); + + // NOTE: This leaked ref is matched by the unref in MemoryManager::release_pte() + (void)page_table.leak_ref(); + + return &quickmap_pt(PhysicalAddress(pde.page_table_base()))[page_table_index]; } -void MemoryManager::release_pte(PageDirectory& page_directory, VirtualAddress vaddr, IsLastPTERelease is_last_pte_release, UnsafeIgnoreMissingPageTable unsafe_ignore_missing_page_table) +void MemoryManager::release_pte(PageDirectory& page_directory, VirtualAddress vaddr, IsLastPTERelease is_last_pte_release) { VERIFY_INTERRUPTS_DISABLED(); VERIFY(s_mm_lock.is_locked_by_current_processor()); @@ -627,10 +625,8 @@ void MemoryManager::release_pte(PageDirectory& page_directory, VirtualAddress va } } if (all_clear) { + get_physical_page_entry(PhysicalAddress { pde.page_table_base() }).allocated.physical_page.unref(); pde.clear(); - - auto result = page_directory.m_page_tables.remove(vaddr.get() & ~0x1fffff); - VERIFY(unsafe_ignore_missing_page_table == UnsafeIgnoreMissingPageTable::Yes || result); } } } diff --git a/Kernel/Memory/MemoryManager.h b/Kernel/Memory/MemoryManager.h index 8c53855de9c..cdd23c3a47a 100644 --- a/Kernel/Memory/MemoryManager.h +++ b/Kernel/Memory/MemoryManager.h @@ -278,11 +278,7 @@ private: Yes, No }; - enum class UnsafeIgnoreMissingPageTable { - Yes, - No - }; - void release_pte(PageDirectory&, VirtualAddress, IsLastPTERelease, UnsafeIgnoreMissingPageTable = UnsafeIgnoreMissingPageTable::No); + void release_pte(PageDirectory&, VirtualAddress, IsLastPTERelease); RefPtr m_kernel_page_directory; diff --git a/Kernel/Memory/PageDirectory.h b/Kernel/Memory/PageDirectory.h index 125dd81f708..9a7e564ca79 100644 --- a/Kernel/Memory/PageDirectory.h +++ b/Kernel/Memory/PageDirectory.h @@ -64,7 +64,6 @@ private: #else RefPtr m_directory_pages[4]; #endif - HashMap> m_page_tables; RecursiveSpinlock m_lock; };