Segregate `kernel/memory.hpp` into meaningful parts #2343

mazunki · 2025-10-28T20:10:43Z

The old kernel/memory.hpp had several responsibilities, and albeit not being super long it felt hard to read. It comprised at least two fairly different parts:

Setting up a mapping from virtual memory to physical pages (now mem/vmap.hpp)
Setting up the allocator for the heap (now mem/alloc.hpp)

Furthermore, I've also created mem/flags.hpp since it doesn't quite fit either of them entirely, and it facilitates plans going forward.

Currently, we're assuming architecture-specific/hardware protection flags match IncludeOS-allocation flags. While this is currently the case, I do intend to change this in the future for the sake of type safety. src/arch/x86_64/paging.cpp uses os::mem::Access referring to hardware flags, while src/kernel/multiboot.cpp or src/kernel/elf.cpp refers to IncludeOS semantics. I intend to create a per-architecture translation mapping for this (constexpr, so it won't affect any performance, of course).

The responsibility separation between mem/vmap and mem/alloc seems to be represented in the usages across the code too (most of the results are non-overlapping):

$ rg mem/vmap.hpp | wc -l
18
$ rg mem/alloc.hpp | wc -l
14

This PR essentially makes #2329 obsolete.

alfreb

This does a lot more than it says on the label. Adds a new allocator without any tests, introduces duplicate string representations of error codes, introduces new mmap functionality in a C patch - lots of things. We can't bundle all this together.

alfreb · 2025-11-08T09:58:50Z

api/mem/vmap.hpp

-  Mapping<Fl>::Mapping(uintptr_t linear, uintptr_t physical, Fl fl, size_t sz)
-      : lin{linear}, phys{physical}, flags{fl}, size{sz},
-        page_sizes{any_size} {}
+  inline Mapping<Fl>::Mapping(uintptr_t linear, uintptr_t physical, Fl fl, size_t sz)


Why inline this?

It doesn't seem to matter anymore, but I was running into ODR issues at one point. My understanding is that this is redundant but equivalent now.

alfreb · 2025-11-08T10:02:46Z

api/mem/vmap.hpp

-
-    const bool isseq = __builtin_popcount(page_sizes) == 1;
+
+    std::string s = std::format( "lin {} -> phys {}, size {}, flags {:#x}",


So using std::string means this will only work if the default allocator is ready at this point, because constructing a string allocates. Are we going to commit to that or is there a scenario where you'd want to do memory mapping before the default allocator is ready?

Thanks for bringing this up, this could be a reason why I was running into some trouble later down. That said, I didn't intend to push this here, so this isn't relevant at this point.

alfreb · 2025-11-08T10:07:09Z

deps/musl/patches/mmap.patch

+void *__includeos_mmap(void *start, size_t len, int prot, int flags, int fd, off_t off)
+{
+	long ret;
+	if (flags & MAP_FIXED) {


Why is this functionality in the patch and not in our mmap implementation? I don't remember what __vm_wait does, so I don't understand this, sorry. Please explain.

Again, not relevant to this PR. Didn't intend to push this.

That said, __vm_wait() is used by the original musl code, I am just mimicking that behaviour. I didn't dig too deep into why it's necessary. The patch fixes errno return codes, but I might have a better solution for it anyway.

alfreb · 2025-11-08T10:10:21Z

src/musl/mmap.cpp

-  auto* res = kalloc(length);
+  void *res = nullptr;
+
+  if (util::has_flag(flags, Flags::FixedOverride) or util::has_flag(flags, Flags::FixedFriendly)) {


Wait - this is not just reorganizing stuff it it? Is this copy-pasted in from somewhere else or is it new functionality?

Yep, these changes will come later.

alfreb · 2025-11-08T10:13:17Z

api/mem/alloc/arena.hpp

Is this a new allocator? The PR says that this is just splitting a header into multiple parts. If you want to commit a new allocator that has to come in its own PR with unit tests and also be plugged in to an integration test.

Yep, you're right. Sorry about that. Commit 9d8cc5e seems to have included a bunch of testing stuff I didn't intend to include. Let me fix that.

alfreb · 2025-11-08T10:25:19Z

api/util/errno.hpp

This is reinventing strerror - please separate out if you think that's the right way to go and explain why.

Yep. I did not intend to add this here. The difference between this and strerror is tracked in #2338.

mazunki · 2025-11-08T12:24:44Z

Welp, this should be much cleaner and easier to review now. I was continuing work on the same branch without branching out, and accidentally pushed some unfinished changes here without realizing.

mazunki added 4 commits November 1, 2025 16:58

give allocators their own location

43ab785

unite files into matching namespace

116b4f5

modularize protection flags into dedicated file

9efbce8

segregate virtual page mapping from conceptual allocations

7d0a670

mazunki force-pushed the ref-memory-hpp branch from 1763322 to f6e51f8 Compare November 1, 2025 15:58

alfreb requested changes Nov 8, 2025

View reviewed changes

mazunki force-pushed the ref-memory-hpp branch from f6e51f8 to 7d0a670 Compare November 8, 2025 12:01


		const bool isseq = __builtin_popcount(page_sizes) == 1;

		std::string s = std::format( "lin {} -> phys {}, size {}, flags {:#x}",

Segregate kernel/memory.hpp into meaningful parts #2343

Are you sure you want to change the base?

Segregate kernel/memory.hpp into meaningful parts #2343

Uh oh!

Conversation

mazunki commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alfreb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazunki Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazunki commented Nov 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Segregate `kernel/memory.hpp` into meaningful parts #2343

Segregate `kernel/memory.hpp` into meaningful parts #2343

mazunki commented Oct 28, 2025 •

edited

Loading

mazunki Nov 8, 2025 •

edited

Loading