Segregate kernel/memory.hpp into meaningful parts#2343
Segregate kernel/memory.hpp into meaningful parts#2343mazunki wants to merge 4 commits intoincludeos:mainfrom
kernel/memory.hpp into meaningful parts#2343Conversation
mazunki
force-pushed
the
ref-memory-hpp
branch
from
1763322 to
f6e51f8
Compare
alfreb
left a comment
alfreb
left a comment
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
api/mem/vmap.hpp
Outdated
|
|
||
| const bool isseq = __builtin_popcount(page_sizes) == 1; | ||
|
|
||
| std::string s = std::format( "lin {} -> phys {}, size {}, flags {:#x}", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
deps/musl/patches/mmap.patch
Outdated
| +void *__includeos_mmap(void *start, size_t len, int prot, int flags, int fd, off_t off) | ||
| +{ | ||
| + long ret; | ||
| + if (flags & MAP_FIXED) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/musl/mmap.cpp
Outdated
| auto* res = kalloc(length); | ||
| void *res = nullptr; | ||
|
|
||
| if (util::has_flag(flags, Flags::FixedOverride) or util::has_flag(flags, Flags::FixedFriendly)) { |
There was a problem hiding this comment.
Wait - this is not just reorganizing stuff it it? Is this copy-pasted in from somewhere else or is it new functionality?
There was a problem hiding this comment.
Yep, these changes will come later.
api/mem/alloc/arena.hpp
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
api/util/errno.hpp
Outdated
There was a problem hiding this comment.
This is reinventing strerror - please separate out if you think that's the right way to go and explain why.
There was a problem hiding this comment.
Yep. I did not intend to add this here. The difference between this and strerror is tracked in #2338.
mazunki
force-pushed
the
ref-memory-hpp
branch
from
f6e51f8 to
7d0a670
Compare
|
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. |
2 participants
The old
kernel/memory.hpphad several responsibilities, and albeit not being super long it felt hard to read. It comprised at least two fairly different parts:mem/vmap.hpp)mem/alloc.hpp)Furthermore, I've also created
mem/flags.hppsince 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.cppusesos::mem::Accessreferring to hardware flags, whilesrc/kernel/multiboot.cpporsrc/kernel/elf.cpprefers 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/vmapandmem/allocseems to be represented in the usages across the code too (most of the results are non-overlapping):This PR essentially makes #2329 obsolete.