Consider store `impl Iterator<Item = UnusedPhysFrame>` into `struct BootInfoFrameAllocator` in Paging Implementation

[myl7]

As mentioned in the post:

This implementation is not quite optimal since it recreates the usable_frame allocator on every allocation. It would be better to directly store the iterator as a struct field instead. Then we wouldn't need the nth method and could just call next on every allocation. The problem with this approach is that it's not possible to store an impl Trait type in a struct field currently. It might work someday when named existential types are fully implemented.

I suggest to use generic type for usable_frames and use an unassociated fn to return the inited BootInfoFrameAllocator. So that impl Iterator<Item = UnusedPhysFrame> will be in the fn return type and it works.

Example:

// In src/memory.rs

pub struct BootInfoFrameAllocator<I: Iterator<Item = UnusedPhysFrame>> {
    usable_frames: I,
}

pub unsafe fn boot_info_frame_allocator_init(
    memory_map: &'static MemoryMap,
) -> BootInfoFrameAllocator<impl Iterator<Item = UnusedPhysFrame>> {
    BootInfoFrameAllocator {
        usable_frames: usable_frames_inner(memory_map),
    }
}

// Use an inner safe fn to show unsafety.
fn usable_frames_inner(memory_map: &'static MemoryMap) -> impl Iterator<Item = UnusedPhysFrame> {
    let regions = memory_map.iter();
    let usable_regions = regions.filter(|r| r.region_type == MemoryRegionType::Usable);
    let addr_ranges = usable_regions.map(|r| r.range.start_addr()..r.range.end_addr());
    let frame_addrs = addr_ranges.flat_map(|r| r.step_by(4096));
    let frames = frame_addrs.map(|addr| PhysFrame::containing_address(PhysAddr::new(addr)));
    frames.map(|f| unsafe { UnusedPhysFrame::new(f) })
}

unsafe impl<I: Iterator<Item = UnusedPhysFrame>> FrameAllocator<Size4KiB>
    for BootInfoFrameAllocator<I>
{
    fn allocate_frame(&mut self) -> Option<UnusedPhysFrame<Size4KiB>> {
        self.usable_frames.next()
    }
}

// In src/main.rs

fn kernel_main(boot_info: &'static BootInfo) -> ! {
	// [...]

    let mut frame_allocator = unsafe { rsos::memory::boot_info_frame_allocator_init(&boot_info.memory_map) };

    // [...]
}

A full example can be found at my repo following the tutorial: https://github.com/myl7/rsos/commit/ec844a86173c48dbbf66a5f52d528280cdaea549

[ethindp]

This is pretty much what I wanted to do. Thanks for showing me the way -- I was going to ask about it and then caught the flue and was "out for the count." :)

[ethindp]

So, looking at your code, and I'm wondering how I'd use this in a static? I'd like to adapt this to my own kernel, but I need to hold it in a static:

static ref FRAME_ALLOCATOR: Mutex<Option<GlobalFrameAllocator<impl Iterator<Item = UnusedPhysFrame>>>> = Mutex::new(None); // Raises E0562

How would I go about making this work?

[myl7]

As Rust currently does not support to use impl Trait in places other than return value, in these places we should use generic type to avoid the problem.

But in lazy_static block I can not see an easy way to use generic type, so maybe we can only expect Rust to support broader impl Trait.

Or maybe we can declare the explicit type of FRAME_ALLOCATOR. But as we used closure to build the allocator, we can never give the type of anonymous struct used for closure, and the generation will be performed all before, other than lazy.

[ethindp]

Do I have to wait for this? Or is there some other workaround? I'm not on any kind of schedule, so I don't mind.

On 3/8/20, myl7 [email protected] wrote:

As Rust currently does not support to use impl Trait in places other than return value, in these places we should use generic type to avoid the problem. But in lazy_static block I can not see an easy way to use generic type, so maybe we can only expect Rust to support broader impl Trait.

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/phil-opp/blog_os/issues/736#issuecomment-596271781

-- Signed, Ethin D. Probst

[myl7]

So far I have not found a solution to this (static varible typed by impl Trait). If I find it, I will post it here as fast as possible.

Maybe we can just simulate the lazy generation? Store the context required by frame generation and generate the frame one by one in method calls, other than using map in usable_frames_inner method? I am not sure this will work. I will try it in next few days.