Create a separate PhysMemAllocator trait #30

mark-i-m · 2018-06-07T03:06:40Z

I propose to more clearly separate out the notion of a Physical Frame Allocator. This PR creates a new PhysMemAllocator trait and uses it in the recursive page table. This also happens to solve a couple of compile errors around borrowing closures.

r? @phil-opp

phil-opp · 2018-06-07T10:05:53Z

Thanks for the PR, @mark-i-m!

What kind of compile errors do you mean?

phil-opp · 2018-06-07T10:06:52Z

src/structures/paging/phys_mem_alloc.rs

+/// An allocator of physical pages of memory.
+pub trait PhysMemAllocator {
+    /// Allocate a physical frame of size `S` if possible.
+    fn alloc<S: PageSize>(&mut self) -> Option<PhysFrame>;


The generic S parameter is not used. I think you meant to use PhysFrame<S> as return type.

phil-opp · 2018-06-07T10:09:25Z

src/structures/paging/recursive.rs

    {
        use structures::paging::PageTableFlags as Flags;

        if entry.is_unused() {
-            if let Some(frame) = allocator() {
+            if let Some(frame) = allocator.alloc::<S>() {


A page table is always 4KiB large, so it does not make sense to call this function with a generic S.

phil-opp · 2018-06-07T10:27:28Z

I'm a bit unsure about this PR, because it requires much more than needed from the frame_allocator arguments. For example, we only need to allocate 4KiB frames in map_to, but with this PR the passed allocator must be able to allocate and deallocate frames of all sizes.

So we can no longer do things like the following:

let mut frames = [None; 3];
for frame in &mut frames {
    *frame = Some(frame_allocator.allocate_frame());
}
// do something with `frame_allocator`

recursive_page_table.map_to(page, frame, flags, || {
    frames.iter_mut().find(|f| f.is_some()).take()
});

Here we preallocate three frames and use a closure around the frames array as allocator argument. This allows use to do something different with frame_allocator because we don't need it anymore for the map_to call. With this PR this would become much more complicated since we would also need to provide code for allocating 2MiB and 1GiB frames and code to deallocate frames, which isn't even needed here.

mark-i-m · 2018-06-08T02:40:48Z

What kind of compile errors do you mean?

On the current master, I get:

$ cargo check
    Checking x86_64 v0.2.0-alpha-019 (file:///nobackup/x86_64)
error[E0507]: cannot move out of borrowed content
   --> src/structures/paging/recursive.rs:201:9
    |
201 |         allocator(frame);
    |         ^^^^^^^^^ cannot move out of borrowed content

error[E0507]: cannot move out of borrowed content
   --> src/structures/paging/recursive.rs:315:9
    |
315 |         allocator(frame);
    |         ^^^^^^^^^ cannot move out of borrowed content

error[E0507]: cannot move out of borrowed content
   --> src/structures/paging/recursive.rs:449:9
    |
449 |         allocator(frame);
    |         ^^^^^^^^^ cannot move out of borrowed content

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0507`.
error: Could not compile `x86_64`.

To learn more, run the command again with --verbose.

I'm not really sure where my brain was yesterday... Thanks for pointing out those issues. Fixing them simplified a few things, too.

Re: your concerns in #30 (comment)

These are fair concerns. I had kind of imagined a different setup: One physical memory allocator for the whole kernel (a simple buddy allocator). This allocates backing pages for page tables, as well as normal pages, including huge pages. Also, I can't really imagine an allocator that doesn't know how to free memory; alloc and free just go together in my mind.

For example, I can imagine such an allocator being an impl PhysMemAllocator for BuddyAllocator<PhyAddr>, where BuddyAllocator could be something like this crate: https://github.com/mark-i-m/buddy

phil-opp · 2018-06-08T10:09:01Z

On the current master, I get: […]

That's strange. For me and travis it compiles fine. I made some recent changes, could you try again with the current master?

These are fair concerns. I had kind of imagined a different setup: One physical memory allocator for the whole kernel (a simple buddy allocator)

This should be possible too, of course. You should be able to do:

recursive_page_table.map_to(page, frame, flags, || {
    buddy_allocator.allocate_frame()
});

mark-i-m · 2018-06-09T01:26:42Z

That's strange. For me and travis it compiles fine. I made some recent changes, could you try again with the current master?

Works now :)

This should be possible too, of course.

That's true, but it seems like a really round-about way of doing it. The only purpose of the closure is as an abstraction. Traits seem like a more idiomatic way of accomplishing that.

Would the following interface address your concerns:

trait PhyMemAlloc<S: PageSize> {
    fn alloc(&mut self) -> Option<PhysFrame<S>>;
}

trait PhyMemFree<S: PageSize> {
   fn free(&mut self, frame: PhysFrame<S>);
}

fn map_to<A>(
    &mut self,
    page: Page<S>,
    frame: PhysFrame<S>,
    flags: PageTableFlags,
    allocator: &mut A,
) -> Result<MapperFlush<S>, MapToError>
where
    A: PhysMemAlloc<Size4KiB>;

EDIT: lol, I forgot the allocator

phil-opp · 2018-06-09T15:40:44Z

I would be fine with a PhyMemAlloc trait. I don't think that closures are unidiomatic, but I guess that you're right that a trait makes the common case more convenient.

I think I would choose different names, maybe AllocateFrame or FrameAllocator.

Another thing that I just noticed: The unmap method should not have an allocator parameter at all and instead return the freed frame. This allows the caller to reuse it for something else without going through an allocator. Also, it makes the method symmetrical to map_to, where the frame is supplied by the caller and not allocated from an allocator.

mark-i-m · 2018-06-09T22:13:25Z

I think that would conflate two different things: there is the page that was unmapped, but there are also possibly a few more pages freed that used to be used for page tables. I agree that the former should be returned to the caller, but I think it makes more sense for the latter to be freed to the allocator.

phil-opp · 2018-06-10T08:56:21Z

The thing is that we don't free any page table frames, because there is no simple and fast way to check whether a page table is empty. A function that returns a frame and takes no allocator would be less confusing in this regard.

mark-i-m · 2018-06-11T01:35:59Z

because there is no simple and fast way to check whether a page table is empty.

Oh, hmm... that's a bit troubling. There are a lot of cases, where one knows that all mappings in some range will be reclaimed, so attempting to reclaim page tables makes sense (e.g. process termination).

Perhaps we could add another method map_reclaim that does attempt to reclaim pages (by iterating over the present bits of the page tables) and return them to the allocator?

lachlansneff · 2018-06-11T01:38:13Z

@4e554c4c said that he had figured out a way of quickly checking if a page table is empty, I believe.

mark-i-m · 2018-06-11T01:44:52Z

Actually, checking present bits is not sufficient because there could be shared pages.

Linux solved this problem with struct page and reverse mappings.

It seems that a bit more thought is required to come up with an api that allows genericly plugging in a way to find out if you should reclaim a page...

phil-opp · 2018-06-11T10:01:10Z

The problem is that that all approaches are opinionated, so this library is not the right place to handle this in my opinion. I think the best way forward is: - Return a the unmapped frame from `unmap` instead of freeing it in an allocator. This allows the user to decide whether that frame should really be freed. So they could use a reverse mapping, additional page table bits or some other mechanism to identify shared frames. - Add some sort of `reclaim_unused_page_tables` method that frees unused page table frames in an allocator. The method could take an address range as argument to limit the scanning to a part of the table. This method could be called by the user if they know that there should be reclaimable tables, e.g. after they unmapped a process. Alternatively, it can be called regularly by a background thread. What do you think?

mark-i-m · 2018-06-11T13:58:03Z

That sounds reasonable to me.

I will try to come around to updating the PR later today...

phil-opp · 2018-06-11T14:00:40Z

@mark-i-m Perfect, thank you!

mark-i-m · 2018-06-12T01:48:50Z

@phil-opp Is it ok to use RangeBounds (which was stabilized 2 weeks ago)? That would limit the compiler version, but it would allow a nicer API...

mark-i-m · 2018-06-12T02:03:11Z

(Still WIP, don't review yet)

mark-i-m · 2018-06-12T03:29:21Z

@phil-opp Let me know what you think. I'm not sure how to test this, so if you have any ideas, let me know.

phil-opp

Thanks for the update, @mark-i-m!

The changes to unmap look good for me, apart from the two comments I left.

I'm not sure about the design of the reclaim_page_tables function. It seems a bit dangerous (it's possible to reclaim page tables that are still in use) and surprising (e.g. if a 1GiB range is passed the bounds are rounded so that not all page tables in the range might get freed).

Could you split off the reclaim_page_table changes to a separate PR? This would allow us to quickly merge the unmap change and release version 0.2.0. Adding reclaim_page_tables later is not a breaking change, so we don't need to block the release on this.

phil-opp · 2018-06-12T11:19:17Z

src/structures/paging/frame_alloc.rs

+/// A trait for types that can allocate a frame of memory.
+pub trait FrameAllocator {
+    /// Allocate a frame of the appropriate size and return it if possible.
+    fn alloc<S: PageSize>(&mut self) -> Option<PhysFrame<S>>;


We should move the generic paramenter S to the trait, otherwise every implementation must be able to allocate frames of all possible sizes (would this even be possible?).

phil-opp · 2018-06-12T11:19:37Z

src/structures/paging/frame_alloc.rs

+/// A trait for types that can deallocate a frame of memory.
+pub trait FrameDeallocator {
+    /// Deallocate the given frame of memory.
+    fn dealloc<S: PageSize>(&mut self, frame: PhysFrame<S>);


As above:

We should move the generic paramenter S to the trait, otherwise every implementation must be able to allocate frames of all possible sizes (would this even be possible?).

phil-opp · 2018-06-12T11:32:51Z

src/structures/paging/recursive.rs

+
+    /// Removes a mapping from the page table and returns the frame that used to be mapped.
+    ///
+    /// Note that no page tables or pages are deallocated.


What do you mean with "or pages"? How about "Note that no page table frames are deallocated."?

Meaning pages other than page tables.

phil-opp · 2018-06-12T11:37:15Z

Is it ok to use RangeBounds (which was stabilized 2 weeks ago)? That would limit the compiler version, but it would allow a nicer API...

We already rely on unstable features, so I ~~don't~~ think that requiring an up-to-date nightly is fine.

I'm not sure how to test this, so if you have any ideas, let me know.

I'm planning to test it with a minimal kernel and the upcoming bootimage test command. This way, the tests run in QEMU and manipulate real page tables.

lachlansneff · 2018-06-12T13:01:33Z

@phil-opp Do you mean that requiring an up-to-date nightly is okay, or not okay?

phil-opp · 2018-06-12T13:09:49Z

@lachlansneff Oh sorry, I meant to say that it's okay to use up-to-date nightly features.

Thanks for the hint, I updated my comment.

mark-i-m · 2018-06-14T01:51:37Z

Could you split off the reclaim_page_table changes to a separate PR? This would allow us to quickly merge the unmap change and release version 0.2.0. Adding reclaim_page_tables later is not a breaking change, so we don't need to block the release on this.

Done. I will open another PR soon.

mark-i-m · 2018-06-14T02:12:16Z

@phil-opp Opened #32

We can continue discussion about reclaim_page_tables there.

phil-opp · 2018-06-14T05:45:42Z

Thanks a lot!

phil-opp · 2018-07-11T08:51:28Z

I just updated the bootloader to the new trait based interface:

 pub(crate) fn map_page<'a, S>(
     page: Page<S>,
     phys_frame: PhysFrame<S>,
     flags: PageTableFlags,
     page_table: &mut RecursivePageTable<'a>,
     frame_allocator: &mut FrameAllocator,
 ) -> Result<MapperFlush<S>, MapToError>
 where
     S: PageSize,
     RecursivePageTable<'a>: Mapper<S>,
 {
-    page_table.map_to(page, phys_frame, flags, &mut || {
-        frame_allocator.allocate_frame(MemoryRegionType::PageTable)
-    })
+    struct PageTableAllocator<'a, 'b: 'a>(&'a mut FrameAllocator<'b>);
+
+    impl<'a, 'b> paging::FrameAllocator<Size4KiB> for PageTableAllocator<'a, 'b> {
+        fn alloc(&mut self) -> Option<PhysFrame<Size4KiB>> {
+            self.0.allocate_frame(MemoryRegionType::PageTable)
+        }
+    }
+
+    page_table.map_to(page, phys_frame, flags, &mut PageTableAllocator(frame_allocator))
 }

It's much more boilerplate now and we need two explicit lifetimes. So I think I liked the closure based interface more. I just wanted to bring it up here, maybe we can improve ergonomics somehow.

(Commit rust-osdev/bootloader@83ae580#diff-9c959995cefca11a6ba2160e735da057R161)

mark-i-m · 2018-07-11T23:46:53Z

@phil-opp I'm confused. Why not just impl paging::FrameAllocator for frame_allocator::FrameAllocator?

phil-opp · 2018-07-12T08:04:49Z

The problem is that paging::FrameAllocator is general purpose, i.e. the trait could be used for all kind of frame allocations, not only for page table allocations. The frame_allocator::FrameAllocator in contrast wants to keep track of the purpose of the allocated frames to create the bootinfo later. So if we implemented the trait for frame_allocator::FrameAllocator we would need to specify a fixed region type, which does not seem like a clean solution (because we might want to allocate different region types using the trait in the future).

I guess that's a relatively niche use case, but if similar cases appear we might think about implementing the trait for FnMut() -> PhysFrame, so that we can use the old closures based interface too.

mark-i-m · 2018-07-12T13:15:21Z

Interesting. What does MemoryRegionType do? Why can't you allocate an arbitrary frame of physical memory?

This problem suggests that perhaps FrameAllocator should take some sort of options argument?

phil-opp · 2018-07-15T08:00:58Z

We do allocate an arbitrary memory frame, we just remember for what it is used to create the bootinfo when control is passed to the kernel. This is really an uncommon use case, so adding an options argument is not worth it.

mark-i-m · 2018-07-15T13:10:58Z

Actually, when I thought about it, I don't think this is an unusual use case: https://github.com/torvalds/linux/blob/master/include/linux/gfp.h

phil-opp · 2018-07-15T18:59:53Z

Ah, good point. So I guess we have three options:

Add a generic option argument to the trait.
Do nothing and let people create wrapper types (like in the bootloader).
Implement the trait for FnMut() -> Option<PhysFrame> to support the previous closure based interface.

I don't like option 1 because we would need to also add the option parameter to map_to and all other mapping methods. Option 2 is the current state and requires no modification to the mapper methods, but requires some boilerplate. Option 3 would eliminate that boilerplate by allowing any closure that returns frames as allocator.

The difference between option 2 and 3 is whether the caller must explicitly define a frame allocator or can implicitly use any closure that returns frames. The question is if there's any danger from this implicitness, i.e. is it possible pass a non-allocator closure by accident.

mark-i-m · 2018-09-05T15:14:54Z

I'll comment more later, but I wanted to leave some notes in addition to your comments:

If you want to use a third-party allocator, you still need to create a wrapper type that impls the trait. This is a bit annoying. We might consider adding some stock implementations. The one I wrote is here:

https://github.com/mark-i-m/os2/blob/288f1e3edd28234ad6a6e096be9d9f76fda41a7f/kernel/memory/paging/mod.rs#L47-L55

We could also consider a NoAllocator implementation of FrameAllocator, where any allocation just cause a panic. This could be used when we want to ensure there is no allocation (e.g. under memory pressure or in the memory reclamation algorithm itself (to prevent recursion (to prevent recursion (to prevent recursion (...))))).

This reverts commit 0c8756f.

phil-opp reviewed Jun 7, 2018

View reviewed changes

phil-opp mentioned this pull request Jun 9, 2018

Beta testing 0.2.0 #31

Closed

mark-i-m force-pushed the phys_mem_alloc branch from 38fe82e to 4d838de Compare June 12, 2018 03:28

phil-opp reviewed Jun 12, 2018

View reviewed changes

mark-i-m added 4 commits June 13, 2018 20:50

start on new api

eb417f1

rustfmt

25b66a7

implement reclaim pages

7707895

Remove reclaim_page_tables and move S

407bbff

mark-i-m force-pushed the phys_mem_alloc branch from b0c031f to 407bbff Compare June 14, 2018 01:50

mark-i-m mentioned this pull request Jun 14, 2018

Reclaim page tables #32

Closed

phil-opp merged commit 21c9715 into rust-osdev:master Jun 14, 2018

phil-opp referenced this pull request Apr 11, 2020

Revert "Add new UnsafePhysFrame type and use it in Mapper::map_to"

8a257cc

This reverts commit 0c8756f.

Create a separate PhysMemAllocator trait #30

Create a separate PhysMemAllocator trait #30

Conversation

mark-i-m commented Jun 7, 2018

phil-opp commented Jun 7, 2018

phil-opp Jun 7, 2018

Choose a reason for hiding this comment

phil-opp Jun 7, 2018

Choose a reason for hiding this comment

phil-opp commented Jun 7, 2018

mark-i-m commented Jun 8, 2018

phil-opp commented Jun 8, 2018

mark-i-m commented Jun 9, 2018 • edited Loading

phil-opp commented Jun 9, 2018

mark-i-m commented Jun 9, 2018

phil-opp commented Jun 10, 2018

mark-i-m commented Jun 11, 2018

lachlansneff commented Jun 11, 2018

mark-i-m commented Jun 11, 2018

phil-opp commented Jun 11, 2018 via email • edited Loading

mark-i-m commented Jun 11, 2018

phil-opp commented Jun 11, 2018

mark-i-m commented Jun 12, 2018

mark-i-m commented Jun 12, 2018

mark-i-m commented Jun 12, 2018

phil-opp left a comment

Choose a reason for hiding this comment

phil-opp Jun 12, 2018

Choose a reason for hiding this comment

phil-opp Jun 12, 2018

Choose a reason for hiding this comment

phil-opp Jun 12, 2018

Choose a reason for hiding this comment

mark-i-m Jun 14, 2018

Choose a reason for hiding this comment

phil-opp commented Jun 12, 2018 • edited Loading

lachlansneff commented Jun 12, 2018

phil-opp commented Jun 12, 2018

mark-i-m commented Jun 14, 2018

mark-i-m commented Jun 14, 2018

phil-opp commented Jun 14, 2018

phil-opp commented Jul 11, 2018

mark-i-m commented Jul 11, 2018

phil-opp commented Jul 12, 2018

mark-i-m commented Jul 12, 2018

phil-opp commented Jul 15, 2018

mark-i-m commented Jul 15, 2018

phil-opp commented Jul 15, 2018

mark-i-m commented Sep 5, 2018

mark-i-m commented Jun 9, 2018 •

edited

Loading

phil-opp commented Jun 11, 2018 via email •

edited

Loading

phil-opp commented Jun 12, 2018 •

edited

Loading