Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moved memory-management crates into kernel/memory #669

Open
wants to merge 2 commits into
base: theseus_main
Choose a base branch
from

Conversation

NathanRoyer
Copy link
Member

@NathanRoyer NathanRoyer commented Oct 25, 2022

This moves the following crates:

  • block_allocator
  • entryflags_x86_64
  • frame_allocator
  • memory_initialization
  • memory_structs
  • memory_x86_64
  • page_allocator
  • page_table_entry

into theseus/kernel/memory, similarly to what was done to ACPI-related crates.

With this PR, there are less crates in theseus/kernel/.

Additionally, most non-memory crates now use re-exports from the memory supercrate, which makes it easier to know what re-exports other crates actually depend on, from these crates. There is one exception: I left the direct import from nano_core to memory/memory_initialization because removing that dependency looks more difficult than what I did in this PR; removing that dependency can have its own PR, imo.

@NathanRoyer
Copy link
Member Author

Note: all file modifications with this icon:
image
are files that were simply moved without internal modification. I hope this simplifies your review.

@kevinaboos
Copy link
Member

Thanks, this looks pretty good. Also a step forwards in the right direction re: #51.

Currently there is a CI build error, but I'm going to wait until all the current memory subsystem changes are merged to address this anyway.

@kevinaboos kevinaboos added the blocked an issue or PR that is blocked on another issue or PR label Oct 26, 2022
@kevinaboos kevinaboos removed the blocked an issue or PR that is blocked on another issue or PR label Nov 4, 2022
@kevinaboos
Copy link
Member

alrighty, all the memory subsystem changes have been merged in now, so we can accept this PR whenever you're able to merge in the latest changes from theseus_main and address the failing CI checks. 👍

Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some top-level comments:

  • bootloader_modules shouldn't be part of memory; this is debatable, but I think we'll end up moving it into a bootloader-related directory after UEFI support is added.
  • more crates can be moved into memory/ (we could do this later too). Some of these are already part of this changeset but you didn't include them in the PR description (pls update the description accordingly)
    • heap
    • multiple_heaps
    • mapper_spillful
    • all the 3 slabmalloc* crates
    • stack

In general, we want to keep dependencies to a bare minimum for each given crate. It makes compilation way faster, and most importantly, we want to keep the scope and file size of the compiled nano_core to a minimum. Specific actions for this are:

  • avoid changing a crate to depend on the huge memory crate unless it already depends on memory
    • for example bootloader_modules only needs one tiny type from memory_structs, it doesn't need all of the memory crate
    • This is mostly important for crates that operate early on in the OS init procedure
  • avoid adding every sub-crate within the memory/ crate as a dependency/re-export in the top-level memory crate (this differs from how acpi is organized)
    • We can do this for some select crates (as we already to with the allocators, etc) but it's not necessary to apply it to all sub-crates

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants