From 5bf7ca1c806443922526a3396d1c3804087fa176 Mon Sep 17 00:00:00 2001 From: Bananymous Date: Sun, 4 Jun 2023 01:25:57 +0300 Subject: [PATCH] Kernel: Allocators are now stored in UniqPtr This allows proper memory management, we had some memory leak --- .../kernel/Memory/FixedWidthAllocator.h | 6 ++- .../include/kernel/Memory/GeneralAllocator.h | 8 +++- kernel/include/kernel/Process.h | 6 +-- kernel/kernel/Memory/FixedWidthAllocator.cpp | 15 +++++--- kernel/kernel/Memory/GeneralAllocator.cpp | 15 ++++++-- kernel/kernel/Process.cpp | 38 +++++++------------ 6 files changed, 47 insertions(+), 41 deletions(-) diff --git a/kernel/include/kernel/Memory/FixedWidthAllocator.h b/kernel/include/kernel/Memory/FixedWidthAllocator.h index 5eb00699..bddab8eb 100644 --- a/kernel/include/kernel/Memory/FixedWidthAllocator.h +++ b/kernel/include/kernel/Memory/FixedWidthAllocator.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include @@ -12,10 +13,10 @@ namespace Kernel BAN_NON_MOVABLE(FixedWidthAllocator); public: - FixedWidthAllocator(PageTable&, uint32_t); + static BAN::ErrorOr> create(PageTable&, uint32_t); ~FixedWidthAllocator(); - BAN::ErrorOr clone(PageTable&); + BAN::ErrorOr> clone(PageTable&); vaddr_t allocate(); bool deallocate(vaddr_t); @@ -26,6 +27,7 @@ namespace Kernel uint32_t max_allocations() const; private: + FixedWidthAllocator(PageTable&, uint32_t); bool allocate_page_if_needed(vaddr_t, uint8_t flags); struct node diff --git a/kernel/include/kernel/Memory/GeneralAllocator.h b/kernel/include/kernel/Memory/GeneralAllocator.h index 4177c351..e14823f0 100644 --- a/kernel/include/kernel/Memory/GeneralAllocator.h +++ b/kernel/include/kernel/Memory/GeneralAllocator.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include @@ -13,13 +14,16 @@ namespace Kernel BAN_NON_MOVABLE(GeneralAllocator); public: - GeneralAllocator(PageTable&); + static BAN::ErrorOr> create(PageTable&); ~GeneralAllocator(); + BAN::ErrorOr> clone(PageTable&); + vaddr_t allocate(size_t); bool deallocate(vaddr_t); - BAN::ErrorOr clone(PageTable&); + private: + GeneralAllocator(PageTable&); private: struct Allocation diff --git a/kernel/include/kernel/Process.h b/kernel/include/kernel/Process.h index 6786cf06..1b1007c8 100644 --- a/kernel/include/kernel/Process.h +++ b/kernel/include/kernel/Process.h @@ -114,12 +114,12 @@ namespace Kernel BAN::String m_working_directory; BAN::Vector m_threads; - BAN::Vector m_fixed_width_allocators; - GeneralAllocator* m_general_allocator; + BAN::Vector> m_fixed_width_allocators; + BAN::UniqPtr m_general_allocator; userspace_entry_t m_userspace_entry; - PageTable* m_page_table { nullptr }; + BAN::UniqPtr m_page_table; TTY* m_tty { nullptr }; }; diff --git a/kernel/kernel/Memory/FixedWidthAllocator.cpp b/kernel/kernel/Memory/FixedWidthAllocator.cpp index 17c0cf52..22ccaa62 100644 --- a/kernel/kernel/Memory/FixedWidthAllocator.cpp +++ b/kernel/kernel/Memory/FixedWidthAllocator.cpp @@ -3,6 +3,14 @@ namespace Kernel { + BAN::ErrorOr> FixedWidthAllocator::create(PageTable& page_table, uint32_t allocation_size) + { + auto* allocator = new FixedWidthAllocator(page_table, allocation_size); + if (allocator == nullptr) + return BAN::Error::from_errno(ENOMEM); + return BAN::UniqPtr::adopt(allocator); + } + FixedWidthAllocator::FixedWidthAllocator(PageTable& page_table, uint32_t allocation_size) : m_page_table(page_table) , m_allocation_size(BAN::Math::max(allocation_size, m_min_allocation_size)) @@ -222,14 +230,11 @@ namespace Kernel ASSERT_NOT_REACHED(); } - BAN::ErrorOr FixedWidthAllocator::clone(PageTable& new_page_table) + BAN::ErrorOr> FixedWidthAllocator::clone(PageTable& new_page_table) { - FixedWidthAllocator* allocator = new FixedWidthAllocator(new_page_table, allocation_size()); - if (allocator == nullptr) - return BAN::Error::from_errno(ENOMEM); + auto allocator = TRY(FixedWidthAllocator::create(new_page_table, allocation_size())); m_page_table.lock(); - ASSERT(m_page_table.is_page_free(0)); for (node* node = m_used_list; node; node = node->next) diff --git a/kernel/kernel/Memory/GeneralAllocator.cpp b/kernel/kernel/Memory/GeneralAllocator.cpp index e481abdb..86dd5f4a 100644 --- a/kernel/kernel/Memory/GeneralAllocator.cpp +++ b/kernel/kernel/Memory/GeneralAllocator.cpp @@ -1,8 +1,17 @@ #include +#include namespace Kernel { + BAN::ErrorOr> GeneralAllocator::create(PageTable& page_table) + { + auto* allocator = new GeneralAllocator(page_table); + if (allocator == nullptr) + return BAN::Error::from_errno(ENOMEM); + return BAN::UniqPtr::adopt(allocator); + } + GeneralAllocator::GeneralAllocator(PageTable& page_table) : m_page_table(page_table) { } @@ -60,11 +69,9 @@ namespace Kernel return false; } - BAN::ErrorOr GeneralAllocator::clone(PageTable& new_page_table) + BAN::ErrorOr> GeneralAllocator::clone(PageTable& new_page_table) { - GeneralAllocator* allocator = new GeneralAllocator(new_page_table); - if (allocator == nullptr) - return BAN::Error::from_errno(ENOMEM); + auto allocator = TRY(GeneralAllocator::create(new_page_table)); m_page_table.lock(); diff --git a/kernel/kernel/Process.cpp b/kernel/kernel/Process.cpp index e331f943..81fb6670 100644 --- a/kernel/kernel/Process.cpp +++ b/kernel/kernel/Process.cpp @@ -56,7 +56,7 @@ namespace Kernel auto* process = create_process(); MUST(process->m_working_directory.push_back('/')); - process->m_page_table = MUST(PageTable::create_userspace()); + process->m_page_table = BAN::UniqPtr::adopt(MUST(PageTable::create_userspace())); process->load_elf(*elf); @@ -91,13 +91,9 @@ namespace Kernel { ASSERT(m_threads.empty()); ASSERT(m_fixed_width_allocators.empty()); - ASSERT(m_general_allocator == nullptr); + ASSERT(!m_general_allocator); ASSERT(m_mapped_ranges.empty()); - if (m_page_table) - { - PageTable::kernel().load(); - delete m_page_table; - } + ASSERT(&PageTable::current() != m_page_table.ptr()); dprintln("process {} exit", pid()); } @@ -133,11 +129,7 @@ namespace Kernel // NOTE: We must clear allocators while the page table is still alive m_fixed_width_allocators.clear(); - if (m_general_allocator) - { - delete m_general_allocator; - m_general_allocator = nullptr; - } + m_general_allocator.clear(); s_process_lock.lock(); for (size_t i = 0; i < s_processes.size(); i++) @@ -163,7 +155,7 @@ namespace Kernel { Process* forked = create_process(); - forked->m_page_table = MUST(PageTable::create_userspace()); + forked->m_page_table = BAN::UniqPtr::adopt(MUST(PageTable::create_userspace())); LockGuard _(m_lock); forked->m_tty = m_tty; @@ -180,7 +172,8 @@ namespace Kernel ASSERT(m_threads.front() == &Thread::current()); for (auto& allocator : m_fixed_width_allocators) - MUST(forked->m_fixed_width_allocators.push_back(MUST(allocator->clone(forked->page_table())))); + if (allocator->allocations() > 0) + MUST(forked->m_fixed_width_allocators.push_back(MUST(allocator->clone(forked->page_table())))); if (m_general_allocator) forked->m_general_allocator = MUST(m_general_allocator->clone(forked->page_table())); @@ -554,21 +547,20 @@ namespace Kernel bool needs_new_allocator { true }; - for (auto* allocator : m_fixed_width_allocators) + for (auto& allocator : m_fixed_width_allocators) { if (allocator->allocation_size() == allocation_size && allocator->allocations() < allocator->max_allocations()) { address = allocator->allocate(); needs_new_allocator = false; + break; } } if (needs_new_allocator) { - auto* allocator = new FixedWidthAllocator(page_table(), allocation_size); - if (allocator == nullptr) - return BAN::Error::from_errno(ENOMEM); - TRY(m_fixed_width_allocators.push_back(allocator)); + auto allocator = TRY(FixedWidthAllocator::create(page_table(), allocation_size)); + TRY(m_fixed_width_allocators.push_back(BAN::move(allocator))); address = m_fixed_width_allocators.back()->allocate(); } } @@ -577,11 +569,7 @@ namespace Kernel LockGuard _(m_lock); if (!m_general_allocator) - { - m_general_allocator = new GeneralAllocator(page_table()); - if (m_general_allocator == nullptr) - return BAN::Error::from_errno(ENOMEM); - } + m_general_allocator = TRY(GeneralAllocator::create(page_table())); address = m_general_allocator->allocate(bytes); } @@ -597,7 +585,7 @@ namespace Kernel for (size_t i = 0; i < m_fixed_width_allocators.size(); i++) { - auto* allocator = m_fixed_width_allocators[i]; + auto& allocator = m_fixed_width_allocators[i]; if (allocator->deallocate((vaddr_t)ptr)) { // TODO: This might be too much. Maybe we should only