Skip to content

Commit

Permalink
Kernel: Fix heap expansion loop
Browse files Browse the repository at this point in the history
By being a bit too greedy and only allocating how much we need for
the failing allocation, we can end up in an infinite loop trying
to expand the heap further. That's because there are other allocations
(e.g. logging, vmobjects, regions, ...) that happen before we finally
retry the failed allocation request.

Also fix allocating in page size increments, which lead to an assertion
when the heap had to grow more than the 1 MiB backup.
  • Loading branch information
tomuta authored and awesomekling committed Sep 9, 2020
1 parent efe2b75 commit 678bbd2
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 1 deletion.
5 changes: 5 additions & 0 deletions Kernel/Heap/Heap.h
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ class ExpandableHeap {

void* allocate(size_t size)
{
int attempt = 0;
do {
for (auto* subheap = &m_heaps; subheap; subheap = subheap->next) {
if (void* ptr = subheap->heap.allocate(size))
Expand All @@ -269,6 +270,10 @@ class ExpandableHeap {
// This is especially true for the kmalloc heap, where adding memory
// requires several other objects to be allocated just to be able to
// expand the heap.

// To avoid an infinite expansion loop, limit to two attempts
if (attempt++ >= 2)
break;
} while (expand_memory(size));
return nullptr;
}
Expand Down
14 changes: 13 additions & 1 deletion Kernel/Heap/kmalloc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,22 +59,30 @@ struct KmallocGlobalHeap {
{
}

bool m_adding { false };
bool add_memory(size_t allocation_request)
{
if (!MemoryManager::is_initialized()) {
klog() << "kmalloc(): Cannot expand heap before MM is initialized!";
return false;
}
ASSERT(!m_adding);
TemporaryChange change(m_adding, true);
// At this point we have very little memory left. Any attempt to
// kmalloc() could fail, so use our backup memory first, so we
// can't really reliably allocate even a new region of memory.
// This is why we keep a backup region, which we can
auto region = move(m_global_heap.m_backup_memory);
if (!region) {
// Be careful to not log too much here. We don't want to trigger
// any further calls to kmalloc(). We're already out of memory
// and don't have any backup memory, either!
klog() << "kmalloc(): Cannot expand heap: no backup memory";
return false;
}

// At this point we should have at least enough memory from the
// backup region to be able to log properly
klog() << "kmalloc(): Adding memory to heap at " << region->vaddr() << ", bytes: " << region->size();

auto& subheap = m_global_heap.m_heap.add_subheap(region->vaddr().as_ptr(), region->size());
Expand All @@ -91,7 +99,11 @@ struct KmallocGlobalHeap {
// was big enough to likely satisfy the request
if (subheap.free_bytes() < allocation_request) {
// Looks like we probably need more
size_t memory_size = max(decltype(m_global_heap.m_heap)::calculate_memory_for_bytes(allocation_request), (size_t)(1 * MiB));
size_t memory_size = PAGE_ROUND_UP(decltype(m_global_heap.m_heap)::calculate_memory_for_bytes(allocation_request));
// Add some more to the new heap. We're already using it for other
// allocations not including the original allocation_request
// that triggered heap expansion. If we don't allocate
memory_size += 1 * MiB;
region = MM.allocate_kernel_region(memory_size, "kmalloc subheap", Region::Access::Read | Region::Access::Write);
if (region) {
klog() << "kmalloc(): Adding even more memory to heap at " << region->vaddr() << ", bytes: " << region->size();
Expand Down

0 comments on commit 678bbd2

Please sign in to comment.