mm: fix page frame allocator bugs

This also includes a minor refactor of everything,
as well as some optimizations.  The bitmap
operations have been moved into a separate file
because they are probably gonna come in handy in
other parts of the system as well.

cmake/config.cmake

@@ -10,13 +10,13 @@ include("${CMAKE_CURRENT_LIST_DIR}/config-${ARCH}.cmake")
 
 set(CFG_KERN_ORIGIN "0x00100000" CACHE STRING "Physical address where the kernel is loaded (don't touch this)")
 
-set(CFG_POISON_PAGES "Poison pages after allocate and free" ON)
+option(CFG_POISON_PAGES "Poison pages after allocate and free" ON)
 
-set(CFG_POISON_HEAP "Poison heap memory after kmalloc() and kfree()" ON)
+option(CFG_POISON_HEAP "Poison heap memory after kmalloc() and kfree()" ON)
 
-set(CFG_DEBUG_IRQ "Debug IRQs" ON)
+option(CFG_DEBUG_IRQ "Debug IRQs" ON)
 
-set(CFG_DEBUG_PAGE_ALLOCS "Debug page frame allocations" ON)
+option(CFG_DEBUG_PAGE_ALLOCS "Debug page frame allocations" OFF)
 
 # This file is part of GayBSD.
 # Copyright (c) 2021 fef <owo@fef.moe>.

include/gay/bits.h

@@ -0,0 +1,65 @@
+/* See the end of this file for copyright and license terms. */
+
+#pragma once
+
+#include <gay/cdefs.h>
+#include <gay/types.h>
+
+#include <limits.h>
+
+/**
+ * @brief Test whether a bit in a bitfield is set.
+ *
+ * @param bitfield Bitfield to test a bit of
+ * @param pos Index of the bit within the bitfield, counting from 0
+ * @return `true` if the bit is set, `false` if not
+ */
+bool bit_tst(const unsigned long *bitfield, usize pos);
+
+/**
+ * @brief Set a single bit in a bitfield.
+ *
+ * @param bitfield Bitfield to set the bit in
+ * @param pos Index of the bit within the bitfield, counting from 0
+ */
+void bit_set(unsigned long *bitfield, usize pos);
+
+/**
+ * @brief Clear a single bit in a bitfield.
+ *
+ * @param bitfield Bitfield to clear the bit in
+ * @param pos Index of the bit within the bitfield, counting from 0
+ */
+void bit_clr(unsigned long *bitfield, usize pos);
+
+/**
+ * @brief Set a range of bits in a bitfield.
+ *
+ * @param bitfield Bitfield to set bits in
+ * @param first Index of the first bit to set in the bitfield, counting from 0
+ * @param count Number of bits to set, starting from index `first`
+ */
+void bit_set_range(unsigned long *bitfield, usize first, usize count);
+
+/**
+ * @brief Clear a range of bits in a bitfield.
+ *
+ * @param bitfield Bitfield to clear bits in
+ * @param first Index of the first bit to clear in the bitfield, counting from 0
+ * @param count Number of bits to clear, starting from index `first`
+ */
+void bit_clr_range(unsigned long *bitfield, usize first, usize count);
+
+/*
+ * This file is part of GayBSD.
+ * Copyright (c) 2021 fef <owo@fef.moe>.
+ *
+ * GayBSD is nonviolent software: you may only use, redistribute, and/or
+ * modify it under the terms of the Cooperative Nonviolent Public License
+ * (CNPL) as found in the LICENSE file in the source code root directory
+ * or at <https://git.pixie.town/thufie/npl-builder>; either version 7
+ * of the license, or (at your option) any later version.
+ *
+ * GayBSD comes with ABSOLUTELY NO WARRANTY, to the extent
+ * permitted by applicable law.  See the CNPL for details.
+ */

kernel/CMakeLists.txt

@@ -7,6 +7,7 @@ target_compile_definitions(gay_kernel INTERFACE ${GAY_KERNEL_DEFINITIONS})
 target_link_libraries(gay_kernel PRIVATE c gay_arch)
 
 target_sources(gay_kernel PRIVATE
+    bits.c
     clist.c
     irq.c
     kprintf.c

kernel/bits.c

@@ -0,0 +1,104 @@
+/* See the end of this file for copyright and license terms. */
+
+#include <gay/bits.h>
+#include <gay/types.h>
+
+#include <limits.h>
+
+void bit_set_range(unsigned long *bitfield, usize first, usize count)
+{
+	/* skip ahead to the longword containing the first bit we need to set */
+	bitfield += first / LONG_BIT;
+	unsigned int bit = first % LONG_BIT;
+
+	/* test if the entire bit range is contained within this longword */
+	if (bit + count < LONG_BIT) {
+		unsigned long low_mask = (1lu << bit) - 1;			/* 0b000..011 */
+		unsigned long high_mask = ~0lu ^ ((1lu << (bit + count)) - 1);	/* 0b110..000 */
+		*bitfield |= ~(low_mask | high_mask);
+	} else {
+		/* if the first bit isn't longword aligned, manually set the upper
+		 * bits in that longword, starting from the first bit's position */
+		if (bit != 0) {
+			unsigned long mask = (1lu << bit) - 1;
+			*bitfield++ |= ~mask;
+			count -= LONG_BIT - bit;
+		}
+
+		/* write out full longwords while we can */
+		while (count >= LONG_BIT) {
+			*bitfield++ = ~0lu;
+			count -= LONG_BIT;
+		}
+
+		/* set the remaining lower bits in the last longword, if any */
+		if (count != 0) {
+			unsigned long mask = (1lu << count) - 1;
+			*bitfield |= mask;
+		}
+	}
+}
+
+/* this works the same way as bit_set_range, it's just the inverse */
+void bit_clr_range(unsigned long *bitfield, usize first, usize count)
+{
+	bitfield += first / LONG_BIT;
+	unsigned int bit = first % LONG_BIT;
+
+	if (bit + count < LONG_BIT) {
+		unsigned long low_mask = (1lu << bit) - 1;
+		unsigned long high_mask = ~0lu ^ ((1lu << (bit + count)) - 1);
+		*bitfield &= low_mask | high_mask;
+	} else {
+		if (bit != 0) {
+			unsigned long mask = (1lu << bit) - 1;
+			*bitfield++ &= mask;
+			count -= LONG_BIT - bit;
+		}
+
+		while (count >= LONG_BIT) {
+			*bitfield++ = 0;
+			count -= LONG_BIT;
+		}
+
+		if (count != 0) {
+			unsigned long mask = (1lu << count) - 1;
+			*bitfield &= ~mask;
+		}
+	}
+}
+
+bool bit_tst(const unsigned long *bitfield, usize pos)
+{
+	usize index = pos / LONG_BIT;
+	unsigned long mask = 1 << (pos % LONG_BIT);
+	return (bitfield[index] & mask) != 0;
+}
+
+void bit_set(unsigned long *bitfield, usize pos)
+{
+	usize index = pos / LONG_BIT;
+	unsigned long mask = 1 << (pos % LONG_BIT);
+	bitfield[index] |= mask;
+}
+
+void bit_clr(unsigned long *bitfield, usize pos)
+{
+	usize index = pos / LONG_BIT;
+	unsigned long mask = 1 << (pos % LONG_BIT);
+	bitfield[index] &= ~mask;
+}
+
+/*
+ * This file is part of GayBSD.
+ * Copyright (c) 2021 fef <owo@fef.moe>.
+ *
+ * GayBSD is nonviolent software: you may only use, redistribute, and/or
+ * modify it under the terms of the Cooperative Nonviolent Public License
+ * (CNPL) as found in the LICENSE file in the source code root directory
+ * or at <https://git.pixie.town/thufie/npl-builder>; either version 7
+ * of the license, or (at your option) any later version.
+ *
+ * GayBSD comes with ABSOLUTELY NO WARRANTY, to the extent
+ * permitted by applicable law.  See the CNPL for details.
+ */

kernel/mm/kmalloc.c

@@ -3,6 +3,7 @@
 #include <gay/kprintf.h>
 #include <gay/mm.h>
 #include <gay/types.h>
+#include <gay/util.h>
 
 extern void _image_start_phys;
 extern void _image_end_phys;

kernel/mm/page.c

@@ -2,6 +2,7 @@
 
 #include <arch/page.h>
 
+#include <gay/bits.h>
 #include <gay/clist.h>
 #include <gay/config.h>
 #include <gay/kprintf.h>
@@ -44,46 +45,29 @@
  */
 #define CACHE_LEVELS (HUGEPAGE_SHIFT - PAGE_SHIFT + 1)
 
+/** @brief There is one of this for every cache level. */
 struct cache_pool {
+	/**
+	 * @brief List of free blocks on this level of granularity.
+	 * The individual entries sit right at the beginning of each free block,
+	 * and are always aligned to `entry_size` bytes.
+	 */
 	struct clist freelist;
+	/**
+	 * @brief Bitmap that stores the allocated status of each entry.
+	 * 1 means allocated, 0 means not.
+	 */
 	unsigned long *bitmap;
+	/** @brief Number of items in `freelist`. */
 	usize free_entries;
-	usize bitmap_len;
 };
 static struct cache_pool caches[CACHE_LEVELS];
 
-#define LONG_MASK ( ~(usize)(sizeof(long) - 1) )
+#define LONG_BIT_MASK (~(LONG_BIT - 1))
 
 uintptr_t phys_start;
 uintptr_t phys_end;
 
-/**
- * @brief Split a block and return the lower half.
- * The block is assumed to already have been removed from its freelist.
- * The high half (i.e. the block that is *not* returned) is inserted into the
- * freelist one level below `level`.
- */
-static void *split_buddy(void *ptr, int level);
-/**
- * @brief Attempt to coalesce a block with its buddy.
- * If coalition is possible, the buddy is removed from its freelist at
- * `level` and the union block is inserted at `level + 1`.
- *
- * @param ptr Pointer to the block
- * @param level Cache level, must be less than `CACHE_LEVELS - 1` (because you
- *	can't join blocks at the highest cache level)
- * @return The joined block, or `nil` if coalition was not possible
- */
-static void *try_join_buddy(void *ptr, int level);
-
-static usize get_bit_number(void *ptr, int level);
-
-static void set_bits(unsigned long *bitfield, usize first, usize count);
-static void clr_bits(unsigned long *bitfield, usize first, usize count);
-
-static bool get_bit(const unsigned long *bitfield, usize bit_number);
-static void set_bit(unsigned long *bitfield, usize bit_number);
-
 static int sanity_check(void)
 {
 	if (phys_end != HUGEPAGE_ALIGN(phys_end) || phys_start != HUGEPAGE_ALIGN(phys_start)) {
@@ -107,22 +91,12 @@ static int sanity_check(void)
 	return 0;
 }
 
-static void init_freelist(void)
-{
-	for (int i = 0; i < CACHE_LEVELS; i++) {
-		clist_init(&caches[i].freelist);
-		caches[i].free_entries = 0;
-	}
-
-	struct cache_pool *pool = &caches[CACHE_LEVELS - 1];
-	const usize step = 1 << (CACHE_LEVELS + PAGE_SHIFT);
-	for (void *pos = kheap_start; pos < kheap_end; pos += step) {
-		struct clist *entry = pos;
-		clist_add(&pool->freelist, entry);
-		pool->free_entries += 1;
-	}
-}
-
+/*
+ * This function maps the entire physical memory into the direct region
+ * (DMAP_START - DMAP_END) and sets up the caches.
+ * The bitmaps are stored one after another at the end of physical memory, and
+ *
+ */
 int pages_init(void)
 {
 	usize phys_size = phys_end - phys_start;
@@ -142,54 +116,92 @@ int pages_init(void)
 	/*
 	 * calculate the size of each bitmap, as well as their combined size
 	 */
-	usize cache_bytes = 0;
+	usize bitmap_bytes = 0;
 	for (int i = 0; i < CACHE_LEVELS; i++) {
 		usize bits = phys_size >> (PAGE_SHIFT + i);
 		/* round up to the next full long */
-		if (bits % LONG_BIT) {
-			bits &= LONG_MASK;
+		if (bits & ~LONG_BIT_MASK) {
+			bits &= LONG_BIT_MASK;
 			bits += LONG_BIT;
 		}
-		cache_bytes += bits / 8;
-		caches[i].bitmap_len = bits / LONG_BIT;
+		bitmap_bytes += bits / 8;
 	}
-	/* smol buffer in case we overshoot for whatever reason */
-	cache_bytes += sizeof(long);
 
-	page_debug("Page frame overhead = %zu bytes\n", cache_bytes);
+	page_debug("Page frame overhead = %zu bytes\n", bitmap_bytes);
 
 	/*
 	 * zero out all bitmaps
 	 */
-	uintptr_t cache_start_phys = phys_end - cache_bytes;
-	unsigned long *cache_start = __v(cache_start_phys);
-	memset(cache_start, 0, cache_bytes);
+	uintptr_t bitmap_start_phys = phys_end - bitmap_bytes;
+	unsigned long *bitmap_start = __v(bitmap_start_phys);
+	memset(bitmap_start, 0, bitmap_bytes);
 
 	/*
-	 * populate the caches array and preallocate pages that can't be handed
-	 * out (i.e. the cache bitmaps)
+	 * populate the remaining members of the cache_pool structures and
+	 * preallocate entries that can't be handed out (i.e. the cache bitmaps)
 	 */
-	unsigned long *cache_pos = cache_start;
+	unsigned long *bitmap_pos = bitmap_start;
 	for (int i = 0; i < CACHE_LEVELS; i++) {
-		usize total_bits = caches[i].bitmap_len * LONG_BIT;
-		usize wasted_bits = total_bits - (cache_bytes >> (PAGE_SHIFT + i));
+		/* total amount of entries on this level */
+		usize total_bits = phys_size >> (PAGE_SHIFT + i);
+		/* number of entries on this level that the bitmap itself takes up */
+		usize wasted_bits = bitmap_bytes >> (PAGE_SHIFT + i);
 		if (wasted_bits == 0)
 			wasted_bits = 1;
-		set_bits(cache_pos, total_bits - wasted_bits, wasted_bits);
+		bit_set_range(bitmap_pos, total_bits - wasted_bits, wasted_bits);
+
+		caches[i].bitmap = bitmap_pos;
+		bitmap_pos += total_bits / LONG_BIT;
 
-		caches[i].bitmap = cache_pos;
-		cache_pos += caches[i].bitmap_len;
+		clist_init(&caches[i].freelist);
+		caches[i].free_entries = 0;
 	}
 
 	/* kheap_start and kheap_end are globals */
 	kheap_start = __v(phys_start);
-	kheap_end = cache_start;
+	kheap_end = ptr_align(bitmap_start, -HUGEPAGE_SHIFT);
 
-	init_freelist();
+	/*
+	 * populate the freelist on the highest level, all levels beneath it
+	 * stay empty until one of the large blocks gets split up
+	 */
+	struct cache_pool *high_pool = &caches[CACHE_LEVELS - 1];
+	usize step = 1 << (PAGE_SHIFT + CACHE_LEVELS - 1);
+	for (void *pos = kheap_start; pos < kheap_end; pos += step) {
+		struct clist *entry = pos;
+		clist_add(&high_pool->freelist, entry);
+		high_pool->free_entries++;
+	}
 
 	return 0;
 }
 
+/**
+ * @brief Split a block and return the lower half.
+ * The block is assumed to already have been removed from its freelist.
+ * The high half (i.e. the block that is *not* returned) is inserted into the
+ * freelist one level below `level`.
+ *
+ * @param ptr Pointer to the block
+ * @param level Current level of the block
+ *	(`ptr` must be aligned to `1 << level` pages)
+ */
+static void *split_buddy(void *ptr, int level);
+
+/**
+ * @brief Attempt to coalesce a block with its buddy.
+ * If coalition is possible, the buddy is removed from its freelist at
+ * `level` and the union block is inserted at `level + 1`.
+ *
+ * @param ptr Pointer to the block
+ * @param level Cache level, must be less than `CACHE_LEVELS - 1` (because you
+ *	can't join blocks at the highest cache level)
+ * @return The joined block, or `nil` if coalition was not possible
+ */
+static void *try_join_buddy(void *ptr, int level);
+
+static usize get_bit_number(void *ptr, int level);
+
 static int get_level(usize count)
 {
 	int level;
@@ -203,8 +215,10 @@ static int get_level(usize count)
 void *get_pages(usize count, enum mm_flags flags)
 {
 	int level = get_level(count);
-	if (level == CACHE_LEVELS)
+	if (level == CACHE_LEVELS) {
+		page_debug("get_pages(%zu, %08x): count too large!\n", count, flags);
 		return nil;
+	}
 
 	struct clist *entry;
 	int entry_level;
@@ -220,18 +234,14 @@ void *get_pages(usize count, enum mm_flags flags)
 	clist_del(entry);
 	caches[entry_level].free_entries--;
 
-	usize bit_number = get_bit_number(entry, level);
+	usize bit_number = get_bit_number(entry, entry_level);
 	while (entry_level > level) {
 		entry = split_buddy(entry, entry_level);
-		set_bit(caches[entry_level].bitmap, bit_number);
+		bit_set(caches[entry_level].bitmap, bit_number);
 		entry_level--;
 		bit_number <<= 1;
 	}
-
-	do {
-		usize bit_count = 1 << (level - entry_level);
-		set_bits(caches[entry_level].bitmap, bit_number, bit_count);
-	} while (entry_level-- != 0);
+	bit_set(caches[level].bitmap, bit_number);
 
 	return (void *)entry;
 }
@@ -239,20 +249,29 @@ void *get_pages(usize count, enum mm_flags flags)
 void free_pages(void *ptr, usize count)
 {
 	int level = get_level(count);
-	if (level == CACHE_LEVELS)
+	if (level == CACHE_LEVELS) {
+		page_debug("free_pages(%p, %zu): count too large!\n", ptr, count);
 		return;
+	}
 
 	usize bit_number = get_bit_number(ptr, level);
-	usize bit_count = 1;
-	for (int i = level; i >= 0; i--) {
-		clr_bits(caches[i].bitmap, bit_number, bit_count);
-		bit_number <<= 1;
-		bit_count <<= 1;
-	}
 
-	while (ptr != nil && level < CACHE_LEVELS - 1) {
+#	if CFG_DEBUG_PAGE_ALLOCS
+		if (!bit_tst(caches[level].bitmap, bit_number)) {
+			kprintf("free_pages(%p, %zu): double free!\n", ptr, count);
+			return;
+		}
+#	endif
+
+	bit_clr(caches[level].bitmap, bit_number);
+
+	while (level < CACHE_LEVELS - 1) {
+		bit_clr(ptr, bit_number);
 		ptr = try_join_buddy(ptr, level);
+		if (ptr == nil)
+			break;
 		level++;
+		bit_number >>= 1;
 	}
 }
 
@@ -261,88 +280,15 @@ static inline usize get_bit_number(void *ptr, int level)
 	return ((uintptr_t)ptr - (uintptr_t)kheap_start) >> (PAGE_SHIFT + level);
 }
 
-/**
- * @brief Set a range of bits in a bitfield.
- *
- * @param bitfield Pointer to the beginning of the bitfield
- * @param first Number of the first bit to set, counting from 0
- * @param count Amount of bits to set
- */
-static void set_bits(unsigned long *bitfield, usize first, usize count)
-{
-	bitfield += first / LONG_BIT;
-	unsigned int bit = first % LONG_BIT;
-
-	if (bit != 0) {
-		unsigned long mask = (1lu << bit) - 1;
-		*bitfield++ |= ~mask;
-		count -= bit;
-	}
-
-	while (count >= LONG_BIT) {
-		*bitfield++ = ULONG_MAX;
-		count -= LONG_BIT;
-	}
-
-	if (count != 0) {
-		unsigned long mask = (1lu << count) - 1;
-		*bitfield |= mask;
-	}
-}
-
-/**
- * @brief Clear a range of bits in a bitfield.
- *
- * The algorithm is similar to `set_bits()`, it just does the inverse.
- *
- * @param bitfield Pointer to the beginning of the bitfield
- * @param first Number of the first bit to clear, counting from 0
- * @param count Amount of bits to clear
- */
-static void clr_bits(unsigned long *bitfield, usize first, usize count)
-{
-	bitfield += first / LONG_BIT;
-	unsigned int bit = first % LONG_BIT;
-
-	if (bit != 0) {
-		unsigned long mask = (1lu << bit) - 1;
-		*bitfield++ &= mask;
-		count -= bit;
-	}
-
-	while (count >= LONG_BIT) {
-		*bitfield++ = 0;
-		count -= LONG_BIT;
-	}
-
-	if (count != 0) {
-		unsigned long mask = (1lu << bit) - 1;
-		*bitfield &= ~mask;
-	}
-}
-
-static inline bool get_bit(const unsigned long *bitfield, usize bit_number)
-{
-	unsigned long longword = bitfield[bit_number / LONG_BIT];
-	unsigned long mask = 1lu << (bit_number % LONG_BIT);
-	return (longword & mask) != 0;
-}
-
-static inline void set_bit(unsigned long *bitfield, usize bit_number)
-{
-	unsigned long mask = 1lu << (bit_number % LONG_BIT);
-	bitfield[bit_number / LONG_BIT] |= mask;
-}
-
 static inline void *split_buddy(void *ptr, int level)
 {
 #	if CFG_DEBUG_PAGE_ALLOCS
 		if ((uintptr_t)ptr % (1 << (PAGE_SHIFT + level))) {
-			kprintf("split_buddy(%p, %d): unaligned ptr!\n", ptr, level);
+			kprintf("split_buddy(ptr = %p, level = %d): unaligned ptr!\n", ptr, level);
 			return nil;
 		}
 		if (level < 1 || level >= CACHE_LEVELS) {
-			kprintf("split_buddy(%p, %d): invalid level!\n", ptr, level);
+			kprintf("split_buddy(ptr = %p, level = %d): invalid level!\n", ptr, level);
 			return nil;
 		}
 #	endif
@@ -351,6 +297,8 @@ static inline void *split_buddy(void *ptr, int level)
 	clist_add(&caches[level - 1].freelist, high_buddy);
 	caches[level - 1].free_entries++;
 
+	page_debug("split (%p:%p), lvl=%d\n", ptr, (void *)high_buddy, level);
+
 	return ptr;
 }
 
@@ -371,21 +319,32 @@ static void *try_join_buddy(void *ptr, int level)
 		}
 #	endif
 
-	/* test if the buddy block is allocated and return nil if it is */
+	/*
+	 * Test whether the buddy block is allocated and return nil if it is.
+	 * entry_size is a power of 2, so we can quickly get to the buddy block
+	 * with a cheap XOR of the address and the entry size without the need
+	 * for any if branches.
+	 */
 	uintptr_t buddy = (uintptr_t)ptr ^ entry_size;
 	usize buddy_bitnum = get_bit_number((void *)buddy, level);
-	if (get_bit(caches[level].bitmap, buddy_bitnum))
+	if (bit_tst(caches[level].bitmap, buddy_bitnum))
 		return nil;
 
-	/* if it is not, remove it from the freelist */
+	page_debug("join (%p:%p), lvl=%d\n", ptr, (void *)buddy, level);
+
+	/* If the buddy is free, we remove it from the freelist ... */
 	clist_del((struct clist *)buddy);
 	caches[level].free_entries--;
 
-	/* add the coalesced block to the freelist one level above */
-	struct clist *even = (struct clist *)((uintptr_t)ptr & ~entry_size);
-	clist_add(&caches[level + 1].freelist, even);
+	/*
+	 * ... and add the coalesced block to the freelist one level above.
+	 * We use the same trick as above to get to the even (lower) block, just
+	 * that this time we're zeroing the bit out rather than flipping it.
+	 */
+	uintptr_t even = (uintptr_t)ptr & ~entry_size;
+	clist_add(&caches[level + 1].freelist, (struct clist *)even);
 	caches[level + 1].free_entries++;
-	return even;
+	return (void *)even;
 }
 
 /*