From 3d4d28edc93c7af7f6731809ffb8c5cb709c32bb Mon Sep 17 00:00:00 2001 From: Ramla Ijaz Date: Thu, 6 Jul 2023 12:40:59 -0400 Subject: [PATCH 01/40] added frames typestate, just finalizing chunk finding functions --- kernel/frame_allocator/src/frames.rs | 382 +++++++++ kernel/frame_allocator/src/lib.rs | 777 ++++++++++-------- .../src/static_array_rb_tree.rs | 5 + 3 files changed, 818 insertions(+), 346 deletions(-) create mode 100644 kernel/frame_allocator/src/frames.rs diff --git a/kernel/frame_allocator/src/frames.rs b/kernel/frame_allocator/src/frames.rs new file mode 100644 index 0000000000..b0658859bb --- /dev/null +++ b/kernel/frame_allocator/src/frames.rs @@ -0,0 +1,382 @@ +//! A range of unmapped frames that stores a verified `TrustedChunk`. +//! A `Frames` object is uncloneable and is the only way to access the range of frames it references. + +use kernel_config::memory::PAGE_SIZE; +use memory_structs::{FrameRange, Frame, PhysicalAddress}; +use range_inclusive::RangeInclusive; +use crate::{MemoryRegionType, contains_any, RESERVED_REGIONS, FREE_GENERAL_FRAMES_LIST, FREE_RESERVED_FRAMES_LIST}; +use core::{borrow::Borrow, cmp::Ordering, ops::{Deref, DerefMut}, fmt, marker::ConstParamTy}; +use spin::Mutex; +use static_assertions::assert_not_impl_any; +use log::error; + +pub type AllocatedFrames = Frames<{FrameState::Unmapped}>; + +#[derive(PartialEq, Eq, ConstParamTy)] +pub enum FrameState { + Unmapped, + Mapped +} + +/// A range of contiguous frames. +/// Owning a `Frames` object gives ownership of the range of frames it references. +/// The `verified_chunk` field is a verified `TrustedChunk` that stores the actual frames, +/// and has the invariant that it does not overlap with any other `TrustedChunk` created by the +/// `CHUNK_ALLOCATOR`. +/// +/// The frames can be in an unmapped or mapped state. In the unmapped state, the frames are not +/// immediately accessible because they're not yet mapped by any virtual memory pages. +/// They are converted into a mapped state once they are used to create a `MappedPages` object. +/// +/// When a `Frames` object in an unmapped state is dropped, it is deallocated and returned to the free frames list. +/// We expect that `Frames` in a mapped state will never be dropped, but instead will be forgotten. +/// +/// # Ordering and Equality +/// +/// `Frames` implements the `Ord` trait, and its total ordering is ONLY based on +/// its **starting** `Frame`. This is useful so we can store `Frames` in a sorted collection. +/// +/// Similarly, `Frames` implements equality traits, `Eq` and `PartialEq`, +/// both of which are also based ONLY on the **starting** `Frame` of the `Frames`. +/// Thus, comparing two `Frames` with the `==` or `!=` operators may not work as expected. +/// since it ignores their actual range of frames. +#[derive(Eq)] +pub struct Frames { + /// The type of this memory chunk, e.g., whether it's in a free or reserved region. + typ: MemoryRegionType, + /// The Frames covered by this chunk, an inclusive range. Equal to the frames in the verified chunk. + /// Needed because verification fails on a trusted chunk that stores a FrameRange or RangeInclusive, + /// but succeeds with RangeInclusive. + frames: FrameRange +} + +// Frames must not be Cloneable, and it must not expose its inner frames as mutable. +assert_not_impl_any!(Frames<{FrameState::Unmapped}>: DerefMut, Clone); +assert_not_impl_any!(Frames<{FrameState::Mapped}>: DerefMut, Clone); + + +impl Frames<{FrameState::Unmapped}> { + /// Creates a new `Frames` object in an unmapped state. + /// If `frames` is empty, there is no space to store the new `Frames` information pre-heap intialization, + /// or a `TrustedChunk` already exists which overlaps with the given `frames`, then an error is returned. + pub(crate) fn new(typ: MemoryRegionType, frames: FrameRange) -> Self { + // assert!(frames.start().number() == verified_chunk.start()); + // assert!(frames.end().number() == verified_chunk.end()); + + Frames { + typ, + frames, + } + // warn!("NEW FRAMES: {:?}", f); + // Ok(f) + } + + + /// Consumes the `Frames` in an unmapped state and converts them to `Frames` in a mapped state. + /// This should only be called once a `MappedPages` has been created from the `Frames`. + pub fn into_mapped_frames(mut self) -> Frames<{FrameState::Mapped}> { + Frames { + typ: self.typ, + frames: self.frames, + } + } + + /// Returns an `UnmappedFrame` if this `Frames<{FrameState::Unmapped}>` object contains only one frame. + /// + /// ## Panic + /// Panics if this `AllocatedFrame` contains multiple frames or zero frames. + pub fn as_unmapped_frame(&self) -> UnmappedFrame { + assert!(self.size_in_frames() == 1); + UnmappedFrame { + frame: *self.start(), + _phantom: core::marker::PhantomData, + } + } +} + + +/// This function is a callback used to convert `UnmappedFrames` into `Frames<{FrameState::Unmapped}>`. +/// `UnmappedFrames` represents frames that have been unmapped from a page that had +/// exclusively mapped them, indicating that no others pages have been mapped +/// to those same frames, and thus, they can be safely deallocated. +/// +/// This exists to break the cyclic dependency cycle between this crate and +/// the `page_table_entry` crate, since `page_table_entry` must depend on types +/// from this crate in order to enforce safety when modifying page table entries. +pub(crate) fn into_allocated_frames(frames: FrameRange) -> Frames<{FrameState::Unmapped}> { + let typ = if contains_any(&RESERVED_REGIONS.lock(), &frames) { + MemoryRegionType::Reserved + } else { + MemoryRegionType::Free + }; + Frames::new(typ, frames) +} + +impl Drop for Frames { + fn drop(&mut self) { + match S { + FrameState::Unmapped => { + if self.size_in_frames() == 0 { return; } + // trace!("FRAMES DROP {:?}", self); + + let unmapped_frames: Frames<{FrameState::Unmapped}> = Frames { + typ: self.typ, + frames: self.frames, + }; + + // Should we remove these lines since we store the typ in Frames? + let (list, _typ) = if contains_any(&RESERVED_REGIONS.lock(), &unmapped_frames) { + (&FREE_RESERVED_FRAMES_LIST, MemoryRegionType::Reserved) + } else { + (&FREE_GENERAL_FRAMES_LIST, MemoryRegionType::Free) + }; + + // Simply add the newly-deallocated chunk to the free frames list. + let mut locked_list = list.lock(); + let res = locked_list.insert(unmapped_frames); + match res { + Ok(_inserted_free_chunk) => (), + Err(c) => error!("BUG: couldn't insert deallocated chunk {:?} into free frame list", c), + } + + // Here, we could optionally use above `_inserted_free_chunk` to merge the adjacent (contiguous) chunks + // before or after the newly-inserted free chunk. + // However, there's no *need* to do so until we actually run out of address space or until + // a requested address is in a chunk that needs to be merged. + // Thus, for performance, we save that for those future situations. + } + FrameState::Mapped => panic!("We should never drop a mapped frame! It should be forgotten instead."), + } + } +} + +impl<'f> IntoIterator for &'f Frames<{FrameState::Unmapped}> { + type IntoIter = UnmappedFramesIter<'f>; + type Item = UnmappedFrame<'f>; + fn into_iter(self) -> Self::IntoIter { + UnmappedFramesIter { + _owner: self, + range: self.frames.clone().into_iter(), + } + } +} + +/// An iterator over each [`UnmappedFrame`] in a range of [`Frames<{FrameState::Unmapped}>`]. +/// +/// To Do: Description is no longer valid, since we have an iterator for RangeInclusive now. +/// but I still think it's useful to have a `Frames<{FrameState::Unmapped}>` iterator that ties the lifetime +/// of the `UnmappedFrame` to the original object. +/// +/// We must implement our own iterator type here in order to tie the lifetime `'f` +/// of a returned `UnmappedFrame<'f>` type to the lifetime of its containing `Frames<{FrameState::Unmapped}>`. +/// This is because the underlying type of `Frames<{FrameState::Unmapped}>` is a [`FrameRange`], +/// which itself is a [`core::ops::RangeInclusive`] of [`Frame`]s, and unfortunately the +/// `RangeInclusive` type doesn't implement an immutable iterator. +/// +/// Iterating through a `RangeInclusive` actually modifies its own internal range, +/// so we must avoid doing that because it would break the semantics of a `FrameRange`. +/// In fact, this is why [`FrameRange`] only implements `IntoIterator` but +/// does not implement [`Iterator`] itself. +pub struct UnmappedFramesIter<'f> { + _owner: &'f Frames<{FrameState::Unmapped}>, + range: range_inclusive::RangeInclusiveIterator, +} +impl<'f> Iterator for UnmappedFramesIter<'f> { + type Item = UnmappedFrame<'f>; + fn next(&mut self) -> Option { + self.range.next().map(|frame| + UnmappedFrame { + frame, _phantom: core::marker::PhantomData, + } + ) + } +} + +/// A reference to a single frame within a range of `Frames<{FrameState::Unmapped}>`. +/// +/// The lifetime of this type is tied to the lifetime of its owning `Frames<{FrameState::Unmapped}>`. +#[derive(Debug)] +pub struct UnmappedFrame<'f> { + frame: Frame, + _phantom: core::marker::PhantomData<&'f Frame>, +} +impl<'f> Deref for UnmappedFrame<'f> { + type Target = Frame; + fn deref(&self) -> &Self::Target { + &self.frame + } +} +assert_not_impl_any!(UnmappedFrame: DerefMut, Clone); + + +impl Frames { + #[allow(dead_code)] + pub(crate) fn frames(&self) -> FrameRange { + self.frames.clone() + } + + pub(crate) fn typ(&self) -> MemoryRegionType { + self.typ + } + + /// Returns a new `Frames` with an empty range of frames. + /// Can be used as a placeholder, but will not permit any real usage. + pub const fn empty() -> Frames { + Frames { + typ: MemoryRegionType::Unknown, + frames: FrameRange::empty(), + } + } + + /// Merges the given `Frames` object `other` into this `Frames` object (`self`). + /// This is just for convenience and usability purposes, it performs no allocation or remapping. + /// + /// The given `other` must be physically contiguous with `self`, i.e., come immediately before or after `self`. + /// That is, either `self.start == other.end + 1` or `self.end + 1 == other.start` must be true. + /// + /// If either of those conditions are met, `self` is modified and `Ok(())` is returned, + /// otherwise `Err(other)` is returned. + pub fn merge(&mut self, mut other: Self) -> Result<(), Self> { + if self.is_empty() || other.is_empty() { + return Err(other); + } + + if *self.start() == *other.end() + 1 { + // `other` comes contiguously before `self` + self.frames = FrameRange::new(*other.start(), *self.end()); + } + else if *self.end() + 1 == *other.start() { + // `self` comes contiguously before `other` + self.frames = FrameRange::new(*self.start(), *other.end()); + } + else { + // non-contiguous + return Err(other); + } + + // ensure the now-merged TrustedChunk doesn't run its drop handler (currently not implemented, but to prevent any future issues.) + core::mem::forget(other); + Ok(()) + } + + /// Splits up the given `Frames` into multiple smaller `Frames`. + /// + /// Returns a tuple of three `Frames`: + /// 1. The `Frames` containing the requested range of frames starting at `start_frame`. + /// 2. The range of frames in the `self` that came before the beginning of the requested frame range. + /// 3. The range of frames in the `self` that came after the end of the requested frame range. + /// + /// If `start_frame` is not contained within `self` or `num_frames` results in an end frame greater than the end of `self`, + /// then `self` is not changed and we return (self, None, None). + pub fn split( + mut self, + start_frame: Frame, + num_frames: usize, + ) -> (Self, Option, Option) { + if (start_frame < *self.start()) || (start_frame + num_frames - 1 > *self.end()) || (num_frames <= 0) { + return (self, None, None); + } + + let before = if start_frame == *self.start() { + None + } else { + Some(Frames{ frames: FrameRange::new(*self.start(), start_frame - 1), ..self }) + }; + let required = Frames{ frames: FrameRange::new(start_frame, start_frame + num_frames - 1), ..self }; + + let after = if start_frame + num_frames - 1 == *self.end() { + None + } else { + Some(Frames{ frames: FrameRange::new(start_frame + num_frames, *self.end()), ..self }) + }; + + core::mem::forget(self); + (required, before, after) + } + + /// Splits this `Frames` into two separate `Frames` objects: + /// * `[beginning : at_frame - 1]` + /// * `[at_frame : end]` + /// + /// This function follows the behavior of [`core::slice::split_at()`], + /// thus, either one of the returned `Frames` objects may be empty. + /// * If `at_frame == self.start`, the first returned `Frames` object will be empty. + /// * If `at_frame == self.end + 1`, the second returned `Frames` object will be empty. + /// + /// Returns an `Err` containing this `Frames` if `at_frame` is otherwise out of bounds. + /// + /// [`core::slice::split_at()`]: https://doc.rust-lang.org/core/primitive.slice.html#method.split_at + pub fn split_at(mut self, at_frame: Frame) -> Result<(Self, Self), Self> { + if self.is_empty() { return Err(self); } + + let end_of_first = at_frame - 1; + + let (first, second) = if at_frame == *self.start() && at_frame <= *self.end() { + let first = FrameRange::empty(); + let second = FrameRange::new(at_frame, *self.end()); + (first, second) + } + else if at_frame == (*self.end() + 1) && end_of_first >= *self.start() { + let first = FrameRange::new(*self.start(), *self.end()); + let second = FrameRange::empty(); + (first, second) + } + else if at_frame > *self.start() && end_of_first <= *self.end() { + let first = FrameRange::new(*self.start(), end_of_first); + let second = FrameRange::new(at_frame, *self.end()); + (first, second) + } + else { + return Err(self); + }; + + let typ = self.typ; + // ensure the original AllocatedFrames doesn't run its drop handler and free its frames. + core::mem::forget(self); + Ok(( + Frames { typ, frames: first }, + Frames { typ, frames: second }, + )) + } +} + +impl Deref for Frames { + type Target = FrameRange; + fn deref(&self) -> &FrameRange { + &self.frames + } +} +impl Ord for Frames { + fn cmp(&self, other: &Self) -> Ordering { + self.frames.start().cmp(other.frames.start()) + } +} +impl PartialOrd for Frames { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} +// To Do: will this be an issue as now this applies to Chunk as well as AllocatedFrames? +#[cfg(not(test))] +impl PartialEq for Frames { + fn eq(&self, other: &Self) -> bool { + self.frames.start() == other.frames.start() + } +} +#[cfg(test)] +impl PartialEq for Frames { + fn eq(&self, other: &Self) -> bool { + self.frames == other.frames + } +} +impl Borrow for &'_ Frames { + fn borrow(&self) -> &Frame { + self.frames.start() + } +} + +impl fmt::Debug for Frames { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "Frames({:?}, {:?})", self.typ, self.frames) + } +} diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index 6dedeee303..8e248a84d2 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -20,6 +20,8 @@ #![no_std] #![allow(clippy::blocks_in_if_conditions)] +#![allow(incomplete_features)] +#![feature(adt_const_params)] extern crate alloc; #[cfg(test)] @@ -27,7 +29,7 @@ mod test; mod static_array_rb_tree; // mod static_array_linked_list; - +mod frames; use core::{borrow::Borrow, cmp::{Ordering, min, max}, fmt, ops::{Deref, DerefMut}, marker::PhantomData}; use intrusive_collections::Bound; @@ -38,6 +40,8 @@ use range_inclusive::RangeInclusiveIterator; use spin::Mutex; use static_array_rb_tree::*; use static_assertions::assert_not_impl_any; +use frames::{Frames, FrameState}; +pub use frames::{AllocatedFrames, UnmappedFrame}; const FRAME_SIZE: usize = PAGE_SIZE; const MIN_FRAME: Frame = Frame::containing_address(PhysicalAddress::zero()); @@ -46,18 +50,18 @@ const MAX_FRAME: Frame = Frame::containing_address(PhysicalAddress::new_canonica // Note: we keep separate lists for "free, general-purpose" areas and "reserved" areas, as it's much faster. /// The single, system-wide list of free physical memory frames available for general usage. -static FREE_GENERAL_FRAMES_LIST: Mutex> = Mutex::new(StaticArrayRBTree::empty()); +static FREE_GENERAL_FRAMES_LIST: Mutex>> = Mutex::new(StaticArrayRBTree::empty()); /// The single, system-wide list of free physical memory frames reserved for specific usage. -static FREE_RESERVED_FRAMES_LIST: Mutex> = Mutex::new(StaticArrayRBTree::empty()); +static FREE_RESERVED_FRAMES_LIST: Mutex>> = Mutex::new(StaticArrayRBTree::empty()); /// The fixed list of all known regions that are available for general use. /// This does not indicate whether these regions are currently allocated, /// rather just where they exist and which regions are known to this allocator. -static GENERAL_REGIONS: Mutex> = Mutex::new(StaticArrayRBTree::empty()); +static GENERAL_REGIONS: Mutex> = Mutex::new(StaticArrayRBTree::empty()); /// The fixed list of all known regions that are reserved for specific purposes. /// This does not indicate whether these regions are currently allocated, /// rather just where they exist and which regions are known to this allocator. -static RESERVED_REGIONS: Mutex> = Mutex::new(StaticArrayRBTree::empty()); +static RESERVED_REGIONS: Mutex> = Mutex::new(StaticArrayRBTree::empty()); /// Initialize the frame allocator with the given list of available and reserved physical memory regions. @@ -89,7 +93,7 @@ pub fn init( return Err("BUG: Frame allocator was already initialized, cannot be initialized twice."); } - let mut free_list: [Option; 32] = Default::default(); + let mut free_list: [Option; 32] = Default::default(); let mut free_list_idx = 0; // Populate the list of free regions for general-purpose usage. @@ -105,9 +109,9 @@ pub fn init( } - let mut reserved_list: [Option; 32] = Default::default(); + let mut reserved_list: [Option; 32] = Default::default(); for (i, area) in reserved_physical_memory_areas.into_iter().enumerate() { - reserved_list[i] = Some(Chunk { + reserved_list[i] = Some(Region { typ: MemoryRegionType::Reserved, frames: area.borrow().frames.clone(), }); @@ -115,7 +119,7 @@ pub fn init( let mut changed = true; while changed { - let mut temp_reserved_list: [Option; 32] = Default::default(); + let mut temp_reserved_list: [Option; 32] = Default::default(); changed = false; let mut temp_reserved_list_idx = 0; @@ -157,12 +161,28 @@ pub fn init( } } - *FREE_GENERAL_FRAMES_LIST.lock() = StaticArrayRBTree::new(free_list.clone()); - *FREE_RESERVED_FRAMES_LIST.lock() = StaticArrayRBTree::new(reserved_list.clone()); + // Here, since we're sure we now have a list of regions that don't overlap, we can create lists of Frames objects. + let mut free_list_w_frames: [Option>; 32] = Default::default(); + let mut reserved_list_w_frames: [Option>; 32] = Default::default(); + for (i, elem) in reserved_list.iter().flatten().enumerate() { + reserved_list_w_frames[i] = Some(Frames::new( + MemoryRegionType::Reserved, + elem.frames.clone() + )); + } + + for (i, elem) in free_list.iter().flatten().enumerate() { + free_list_w_frames[i] = Some(Frames::new( + MemoryRegionType::Free, + elem.frames.clone() + )); + } + *FREE_GENERAL_FRAMES_LIST.lock() = StaticArrayRBTree::new(free_list_w_frames); + *FREE_RESERVED_FRAMES_LIST.lock() = StaticArrayRBTree::new(reserved_list_w_frames); *GENERAL_REGIONS.lock() = StaticArrayRBTree::new(free_list); *RESERVED_REGIONS.lock() = StaticArrayRBTree::new(reserved_list); - Ok(into_allocated_frames) + Ok(frames::into_allocated_frames) } @@ -174,7 +194,7 @@ pub fn init( /// the given list of `reserved_physical_memory_areas`. fn check_and_add_free_region( area: &FrameRange, - free_list: &mut [Option; 32], + free_list: &mut [Option; 32], free_list_idx: &mut usize, reserved_physical_memory_areas: R, ) @@ -220,7 +240,7 @@ fn check_and_add_free_region( let new_area = FrameRange::new(current_start, current_end); if new_area.size_in_frames() > 0 { - free_list[*free_list_idx] = Some(Chunk { + free_list[*free_list_idx] = Some(Region { typ: MemoryRegionType::Free, frames: new_area, }); @@ -274,267 +294,261 @@ pub enum MemoryRegionType { /// Thus, comparing two `Chunk`s with the `==` or `!=` operators may not work as expected. /// since it ignores their actual range of frames. #[derive(Debug, Clone, Eq)] -struct Chunk { +struct Region { /// The type of this memory chunk, e.g., whether it's in a free or reserved region. typ: MemoryRegionType, /// The Frames covered by this chunk, an inclusive range. frames: FrameRange, } -impl Chunk { - fn as_allocated_frames(&self) -> AllocatedFrames { - AllocatedFrames { - frames: self.frames.clone(), - } - } - +impl Region { /// Returns a new `Chunk` with an empty range of frames. - const fn empty() -> Chunk { - Chunk { + const fn empty() -> Region { + Region { typ: MemoryRegionType::Unknown, frames: FrameRange::empty(), } } } -impl Deref for Chunk { +impl Deref for Region { type Target = FrameRange; fn deref(&self) -> &FrameRange { &self.frames } } -impl Ord for Chunk { +impl Ord for Region { fn cmp(&self, other: &Self) -> Ordering { self.frames.start().cmp(other.frames.start()) } } -impl PartialOrd for Chunk { +impl PartialOrd for Region { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } } -impl PartialEq for Chunk { +impl PartialEq for Region { fn eq(&self, other: &Self) -> bool { self.frames.start() == other.frames.start() } } -impl Borrow for &'_ Chunk { +impl Borrow for &'_ Region { fn borrow(&self) -> &Frame { self.frames.start() } } -/// Represents a range of allocated physical memory [`Frame`]s; derefs to [`FrameRange`]. -/// -/// These frames are not immediately accessible because they're not yet mapped -/// by any virtual memory pages. -/// You must do that separately in order to create a `MappedPages` type, -/// which can then be used to access the contents of these frames. -/// -/// This object represents ownership of the range of allocated physical frames; -/// if this object falls out of scope, its allocated frames will be auto-deallocated upon drop. -pub struct AllocatedFrames { - frames: FrameRange, -} - -// AllocatedFrames must not be Cloneable, and it must not expose its inner frames as mutable. -assert_not_impl_any!(AllocatedFrames: DerefMut, Clone); - -impl Deref for AllocatedFrames { - type Target = FrameRange; - fn deref(&self) -> &FrameRange { - &self.frames - } -} -impl fmt::Debug for AllocatedFrames { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "AllocatedFrames({:?})", self.frames) - } -} - -impl AllocatedFrames { - /// Returns an empty AllocatedFrames object that performs no frame allocation. - /// Can be used as a placeholder, but will not permit any real usage. - pub const fn empty() -> AllocatedFrames { - AllocatedFrames { - frames: FrameRange::empty() - } - } - - /// Merges the given `AllocatedFrames` object `other` into this `AllocatedFrames` object (`self`). - /// This is just for convenience and usability purposes, it performs no allocation or remapping. - /// - /// The given `other` must be physically contiguous with `self`, i.e., come immediately before or after `self`. - /// That is, either `self.start == other.end + 1` or `self.end + 1 == other.start` must be true. - /// - /// If either of those conditions are met, `self` is modified and `Ok(())` is returned, - /// otherwise `Err(other)` is returned. - pub fn merge(&mut self, other: AllocatedFrames) -> Result<(), AllocatedFrames> { - if *self.start() == *other.end() + 1 { - // `other` comes contiguously before `self` - self.frames = FrameRange::new(*other.start(), *self.end()); - } - else if *self.end() + 1 == *other.start() { - // `self` comes contiguously before `other` - self.frames = FrameRange::new(*self.start(), *other.end()); - } - else { - // non-contiguous - return Err(other); - } - - // ensure the now-merged AllocatedFrames doesn't run its drop handler and free its frames. - core::mem::forget(other); - Ok(()) - } - - /// Splits this `AllocatedFrames` into two separate `AllocatedFrames` objects: - /// * `[beginning : at_frame - 1]` - /// * `[at_frame : end]` - /// - /// This function follows the behavior of [`core::slice::split_at()`], - /// thus, either one of the returned `AllocatedFrames` objects may be empty. - /// * If `at_frame == self.start`, the first returned `AllocatedFrames` object will be empty. - /// * If `at_frame == self.end + 1`, the second returned `AllocatedFrames` object will be empty. - /// - /// Returns an `Err` containing this `AllocatedFrames` if `at_frame` is otherwise out of bounds. - /// - /// [`core::slice::split_at()`]: https://doc.rust-lang.org/core/primitive.slice.html#method.split_at - pub fn split(self, at_frame: Frame) -> Result<(AllocatedFrames, AllocatedFrames), AllocatedFrames> { - let end_of_first = at_frame - 1; - - let (first, second) = if at_frame == *self.start() && at_frame <= *self.end() { - let first = FrameRange::empty(); - let second = FrameRange::new(at_frame, *self.end()); - (first, second) - } - else if at_frame == (*self.end() + 1) && end_of_first >= *self.start() { - let first = FrameRange::new(*self.start(), *self.end()); - let second = FrameRange::empty(); - (first, second) - } - else if at_frame > *self.start() && end_of_first <= *self.end() { - let first = FrameRange::new(*self.start(), end_of_first); - let second = FrameRange::new(at_frame, *self.end()); - (first, second) - } - else { - return Err(self); - }; - - // ensure the original AllocatedFrames doesn't run its drop handler and free its frames. - core::mem::forget(self); - Ok(( - AllocatedFrames { frames: first }, - AllocatedFrames { frames: second }, - )) - } - - /// Returns an `AllocatedFrame` if this `AllocatedFrames` object contains only one frame. - /// - /// ## Panic - /// Panics if this `AllocatedFrame` contains multiple frames or zero frames. - pub fn as_allocated_frame(&self) -> AllocatedFrame { - assert!(self.size_in_frames() == 1); - AllocatedFrame { - frame: *self.start(), - _phantom: PhantomData, - } - } -} - -/// This function is a callback used to convert `UnmappedFrames` into `AllocatedFrames`. -/// `UnmappedFrames` represents frames that have been unmapped from a page that had -/// exclusively mapped them, indicating that no others pages have been mapped -/// to those same frames, and thus, they can be safely deallocated. -/// -/// This exists to break the cyclic dependency cycle between this crate and -/// the `page_table_entry` crate, since `page_table_entry` must depend on types -/// from this crate in order to enforce safety when modifying page table entries. -fn into_allocated_frames(frames: FrameRange) -> AllocatedFrames { - AllocatedFrames { frames } -} - -impl Drop for AllocatedFrames { - fn drop(&mut self) { - if self.size_in_frames() == 0 { return; } - - let (list, typ) = if contains_any(&RESERVED_REGIONS.lock(), &self.frames) { - (&FREE_RESERVED_FRAMES_LIST, MemoryRegionType::Reserved) - } else { - (&FREE_GENERAL_FRAMES_LIST, MemoryRegionType::Free) - }; - // trace!("frame_allocator: deallocating {:?}, typ {:?}", self, typ); - - // Simply add the newly-deallocated chunk to the free frames list. - let mut locked_list = list.lock(); - let res = locked_list.insert(Chunk { - typ, - frames: self.frames.clone(), - }); - match res { - Ok(_inserted_free_chunk) => (), - Err(c) => error!("BUG: couldn't insert deallocated chunk {:?} into free frame list", c), - } +// /// Represents a range of allocated physical memory [`Frame`]s; derefs to [`FrameRange`]. +// /// +// /// These frames are not immediately accessible because they're not yet mapped +// /// by any virtual memory pages. +// /// You must do that separately in order to create a `MappedPages` type, +// /// which can then be used to access the contents of these frames. +// /// +// /// This object represents ownership of the range of allocated physical frames; +// /// if this object falls out of scope, its allocated frames will be auto-deallocated upon drop. +// pub struct AllocatedFrames { +// frames: FrameRange, +// } + +// // AllocatedFrames must not be Cloneable, and it must not expose its inner frames as mutable. +// assert_not_impl_any!(AllocatedFrames: DerefMut, Clone); + +// impl Deref for AllocatedFrames { +// type Target = FrameRange; +// fn deref(&self) -> &FrameRange { +// &self.frames +// } +// } +// impl fmt::Debug for AllocatedFrames { +// fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { +// write!(f, "AllocatedFrames({:?})", self.frames) +// } +// } + +// impl AllocatedFrames { +// /// Returns an empty AllocatedFrames object that performs no frame allocation. +// /// Can be used as a placeholder, but will not permit any real usage. +// pub const fn empty() -> AllocatedFrames { +// AllocatedFrames { +// frames: FrameRange::empty() +// } +// } + +// /// Merges the given `AllocatedFrames` object `other` into this `AllocatedFrames` object (`self`). +// /// This is just for convenience and usability purposes, it performs no allocation or remapping. +// /// +// /// The given `other` must be physically contiguous with `self`, i.e., come immediately before or after `self`. +// /// That is, either `self.start == other.end + 1` or `self.end + 1 == other.start` must be true. +// /// +// /// If either of those conditions are met, `self` is modified and `Ok(())` is returned, +// /// otherwise `Err(other)` is returned. +// pub fn merge(&mut self, other: AllocatedFrames) -> Result<(), AllocatedFrames> { +// if *self.start() == *other.end() + 1 { +// // `other` comes contiguously before `self` +// self.frames = FrameRange::new(*other.start(), *self.end()); +// } +// else if *self.end() + 1 == *other.start() { +// // `self` comes contiguously before `other` +// self.frames = FrameRange::new(*self.start(), *other.end()); +// } +// else { +// // non-contiguous +// return Err(other); +// } + +// // ensure the now-merged AllocatedFrames doesn't run its drop handler and free its frames. +// core::mem::forget(other); +// Ok(()) +// } + +// /// Splits this `AllocatedFrames` into two separate `AllocatedFrames` objects: +// /// * `[beginning : at_frame - 1]` +// /// * `[at_frame : end]` +// /// +// /// This function follows the behavior of [`core::slice::split_at()`], +// /// thus, either one of the returned `AllocatedFrames` objects may be empty. +// /// * If `at_frame == self.start`, the first returned `AllocatedFrames` object will be empty. +// /// * If `at_frame == self.end + 1`, the second returned `AllocatedFrames` object will be empty. +// /// +// /// Returns an `Err` containing this `AllocatedFrames` if `at_frame` is otherwise out of bounds. +// /// +// /// [`core::slice::split_at()`]: https://doc.rust-lang.org/core/primitive.slice.html#method.split_at +// pub fn split(self, at_frame: Frame) -> Result<(AllocatedFrames, AllocatedFrames), AllocatedFrames> { +// let end_of_first = at_frame - 1; + +// let (first, second) = if at_frame == *self.start() && at_frame <= *self.end() { +// let first = FrameRange::empty(); +// let second = FrameRange::new(at_frame, *self.end()); +// (first, second) +// } +// else if at_frame == (*self.end() + 1) && end_of_first >= *self.start() { +// let first = FrameRange::new(*self.start(), *self.end()); +// let second = FrameRange::empty(); +// (first, second) +// } +// else if at_frame > *self.start() && end_of_first <= *self.end() { +// let first = FrameRange::new(*self.start(), end_of_first); +// let second = FrameRange::new(at_frame, *self.end()); +// (first, second) +// } +// else { +// return Err(self); +// }; + +// // ensure the original AllocatedFrames doesn't run its drop handler and free its frames. +// core::mem::forget(self); +// Ok(( +// AllocatedFrames { frames: first }, +// AllocatedFrames { frames: second }, +// )) +// } + +// /// Returns an `AllocatedFrame` if this `AllocatedFrames` object contains only one frame. +// /// +// /// ## Panic +// /// Panics if this `AllocatedFrame` contains multiple frames or zero frames. +// pub fn as_allocated_frame(&self) -> AllocatedFrame { +// assert!(self.size_in_frames() == 1); +// AllocatedFrame { +// frame: *self.start(), +// _phantom: PhantomData, +// } +// } +// } + +// /// This function is a callback used to convert `UnmappedFrames` into `AllocatedFrames`. +// /// `UnmappedFrames` represents frames that have been unmapped from a page that had +// /// exclusively mapped them, indicating that no others pages have been mapped +// /// to those same frames, and thus, they can be safely deallocated. +// /// +// /// This exists to break the cyclic dependency cycle between this crate and +// /// the `page_table_entry` crate, since `page_table_entry` must depend on types +// /// from this crate in order to enforce safety when modifying page table entries. +// fn into_allocated_frames(frames: FrameRange) -> AllocatedFrames { +// AllocatedFrames { frames } +// } + +// impl Drop for AllocatedFrames { +// fn drop(&mut self) { +// if self.size_in_frames() == 0 { return; } + +// let (list, typ) = if contains_any(&RESERVED_REGIONS.lock(), &self.frames) { +// (&FREE_RESERVED_FRAMES_LIST, MemoryRegionType::Reserved) +// } else { +// (&FREE_GENERAL_FRAMES_LIST, MemoryRegionType::Free) +// }; +// // trace!("frame_allocator: deallocating {:?}, typ {:?}", self, typ); + +// // Simply add the newly-deallocated chunk to the free frames list. +// let mut locked_list = list.lock(); +// let res = locked_list.insert(Chunk { +// typ, +// frames: self.frames.clone(), +// }); +// match res { +// Ok(_inserted_free_chunk) => (), +// Err(c) => error!("BUG: couldn't insert deallocated chunk {:?} into free frame list", c), +// } - // Here, we could optionally use above `_inserted_free_chunk` to merge the adjacent (contiguous) chunks - // before or after the newly-inserted free chunk. - // However, there's no *need* to do so until we actually run out of address space or until - // a requested address is in a chunk that needs to be merged. - // Thus, for performance, we save that for those future situations. - } -} - -impl<'f> IntoIterator for &'f AllocatedFrames { - type IntoIter = AllocatedFramesIter<'f>; - type Item = AllocatedFrame<'f>; - fn into_iter(self) -> Self::IntoIter { - AllocatedFramesIter { - _owner: self, - range_iter: self.frames.iter(), - } - } -} - -/// An iterator over each [`AllocatedFrame`] in a range of [`AllocatedFrames`]. -/// -/// We must implement our own iterator type here in order to tie the lifetime `'f` -/// of a returned `AllocatedFrame<'f>` type to the lifetime of its containing `AllocatedFrames`. -/// This is because the underlying type of `AllocatedFrames` is a [`FrameRange`], -/// which itself is a [`RangeInclusive`] of [`Frame`]s. -/// Currently, the [`RangeInclusiveIterator`] type creates a clone of the original -/// [`RangeInclusive`] instances rather than borrowing a reference to it. -/// -/// [`RangeInclusive`]: range_inclusive::RangeInclusive -pub struct AllocatedFramesIter<'f> { - _owner: &'f AllocatedFrames, - range_iter: RangeInclusiveIterator, -} -impl<'f> Iterator for AllocatedFramesIter<'f> { - type Item = AllocatedFrame<'f>; - fn next(&mut self) -> Option { - self.range_iter.next().map(|frame| - AllocatedFrame { - frame, _phantom: PhantomData, - } - ) - } -} - -/// A reference to a single frame within a range of `AllocatedFrames`. -/// -/// The lifetime of this type is tied to the lifetime of its owning `AllocatedFrames`. -#[derive(Debug)] -pub struct AllocatedFrame<'f> { - frame: Frame, - _phantom: PhantomData<&'f Frame>, -} -impl<'f> Deref for AllocatedFrame<'f> { - type Target = Frame; - fn deref(&self) -> &Self::Target { - &self.frame - } -} -assert_not_impl_any!(AllocatedFrame: DerefMut, Clone); +// // Here, we could optionally use above `_inserted_free_chunk` to merge the adjacent (contiguous) chunks +// // before or after the newly-inserted free chunk. +// // However, there's no *need* to do so until we actually run out of address space or until +// // a requested address is in a chunk that needs to be merged. +// // Thus, for performance, we save that for those future situations. +// } +// } + +// impl<'f> IntoIterator for &'f AllocatedFrames { +// type IntoIter = AllocatedFramesIter<'f>; +// type Item = AllocatedFrame<'f>; +// fn into_iter(self) -> Self::IntoIter { +// AllocatedFramesIter { +// _owner: self, +// range_iter: self.frames.iter(), +// } +// } +// } + +// /// An iterator over each [`AllocatedFrame`] in a range of [`AllocatedFrames`]. +// /// +// /// We must implement our own iterator type here in order to tie the lifetime `'f` +// /// of a returned `AllocatedFrame<'f>` type to the lifetime of its containing `AllocatedFrames`. +// /// This is because the underlying type of `AllocatedFrames` is a [`FrameRange`], +// /// which itself is a [`RangeInclusive`] of [`Frame`]s. +// /// Currently, the [`RangeInclusiveIterator`] type creates a clone of the original +// /// [`RangeInclusive`] instances rather than borrowing a reference to it. +// /// +// /// [`RangeInclusive`]: range_inclusive::RangeInclusive +// pub struct AllocatedFramesIter<'f> { +// _owner: &'f AllocatedFrames, +// range_iter: RangeInclusiveIterator, +// } +// impl<'f> Iterator for AllocatedFramesIter<'f> { +// type Item = AllocatedFrame<'f>; +// fn next(&mut self) -> Option { +// self.range_iter.next().map(|frame| +// AllocatedFrame { +// frame, _phantom: PhantomData, +// } +// ) +// } +// } + +// /// A reference to a single frame within a range of `AllocatedFrames`. +// /// +// /// The lifetime of this type is tied to the lifetime of its owning `AllocatedFrames`. +// #[derive(Debug)] +// pub struct AllocatedFrame<'f> { +// frame: Frame, +// _phantom: PhantomData<&'f Frame>, +// } +// impl<'f> Deref for AllocatedFrame<'f> { +// type Target = Frame; +// fn deref(&self) -> &Self::Target { +// &self.frame +// } +// } +// assert_not_impl_any!(AllocatedFrame: DerefMut, Clone); /// A series of pending actions related to frame allocator bookkeeping, @@ -551,21 +565,21 @@ assert_not_impl_any!(AllocatedFrame: DerefMut, Clone); /// with a `let _ = ...` binding to instantly drop it. pub struct DeferredAllocAction<'list> { /// A reference to the list into which we will insert the free general-purpose `Chunk`s. - free_list: &'list Mutex>, + free_list: &'list Mutex>>, /// A reference to the list into which we will insert the free "reserved" `Chunk`s. - reserved_list: &'list Mutex>, + reserved_list: &'list Mutex>>, /// A free chunk that needs to be added back to the free list. - free1: Chunk, + free1: Frames<{FrameState::Unmapped}>, /// Another free chunk that needs to be added back to the free list. - free2: Chunk, + free2: Frames<{FrameState::Unmapped}>, } impl<'list> DeferredAllocAction<'list> { fn new(free1: F1, free2: F2) -> DeferredAllocAction<'list> - where F1: Into>, - F2: Into>, + where F1: Into>>, + F2: Into>>, { - let free1 = free1.into().unwrap_or_else(Chunk::empty); - let free2 = free2.into().unwrap_or_else(Chunk::empty); + let free1 = free1.into().unwrap_or_else(Frames::empty); + let free2 = free2.into().unwrap_or_else(Frames::empty); DeferredAllocAction { free_list: &FREE_GENERAL_FRAMES_LIST, reserved_list: &FREE_RESERVED_FRAMES_LIST, @@ -576,19 +590,22 @@ impl<'list> DeferredAllocAction<'list> { } impl<'list> Drop for DeferredAllocAction<'list> { fn drop(&mut self) { + let frames1 = core::mem::replace(&mut self.free1, Frames::empty()); + let frames2 = core::mem::replace(&mut self.free2, Frames::empty()); + // Insert all of the chunks, both allocated and free ones, into the list. - if self.free1.size_in_frames() > 0 { - match self.free1.typ { - MemoryRegionType::Free => { self.free_list.lock().insert(self.free1.clone()).unwrap(); } - MemoryRegionType::Reserved => { self.reserved_list.lock().insert(self.free1.clone()).unwrap(); } - _ => error!("BUG likely: DeferredAllocAction encountered free1 chunk {:?} of a type Unknown", self.free1), + if frames1.size_in_frames() > 0 { + match frames1.typ() { + MemoryRegionType::Free => { self.free_list.lock().insert(frames1).unwrap(); } + MemoryRegionType::Reserved => { self.reserved_list.lock().insert(frames1).unwrap(); } + _ => error!("BUG likely: DeferredAllocAction encountered free1 chunk {:?} of a type Unknown", frames1), } } - if self.free2.size_in_frames() > 0 { - match self.free2.typ { - MemoryRegionType::Free => { self.free_list.lock().insert(self.free2.clone()).unwrap(); } - MemoryRegionType::Reserved => { self.reserved_list.lock().insert(self.free2.clone()).unwrap(); } - _ => error!("BUG likely: DeferredAllocAction encountered free2 chunk {:?} of a type Unknown", self.free2), + if frames2.size_in_frames() > 0 { + match frames2.typ() { + MemoryRegionType::Free => { self.free_list.lock().insert(frames2).unwrap(); } + MemoryRegionType::Reserved => { self.reserved_list.lock().insert(frames2).unwrap(); } + _ => error!("BUG likely: DeferredAllocAction encountered free2 chunk {:?} of a type Unknown", frames2), }; } } @@ -606,7 +623,9 @@ enum AllocationError { /// or enough remaining chunks that could satisfy the requested allocation size. OutOfAddressSpace(usize), /// The starting address was found, but not all successive contiguous frames were available. - ContiguousChunkNotFound(Frame, usize) + ContiguousChunkNotFound(Frame, usize), + /// Failed to remove a chunk from the free list given a reference to it. + ChunkRemovalFailed, } impl From for &'static str { fn from(alloc_err: AllocationError) -> &'static str { @@ -615,6 +634,7 @@ impl From for &'static str { AllocationError::AddressNotFound(..) => "requested address was outside of this frame allocator's range", AllocationError::OutOfAddressSpace(..) => "out of physical address space", AllocationError::ContiguousChunkNotFound(..) => "only some of the requested frames were available", + AllocationError::ChunkRemovalFailed => "Failed to remove a Chunk from the free list, this is most likely due to some logical error", } } } @@ -623,7 +643,7 @@ impl From for &'static str { /// Searches the given `list` for the chunk that contains the range of frames from /// `requested_frame` to `requested_frame + num_frames`. fn find_specific_chunk( - list: &mut StaticArrayRBTree, + list: &mut StaticArrayRBTree>, requested_frame: Frame, num_frames: usize ) -> Result<(AllocatedFrames, DeferredAllocAction<'static>), AllocationError> { @@ -637,17 +657,17 @@ fn find_specific_chunk( if let Some(chunk) = elem { if requested_frame >= *chunk.start() && requested_end_frame <= *chunk.end() { // Here: `chunk` was big enough and did contain the requested address. - return Ok(allocate_from_chosen_chunk(requested_frame, num_frames, &chunk.clone(), ValueRefMut::Array(elem))); + return allocate_from_chosen_chunk(requested_frame, num_frames, ValueRefMut::Array(elem)); } } } } Inner::RBTree(ref mut tree) => { let mut cursor_mut = tree.upper_bound_mut(Bound::Included(&requested_frame)); - if let Some(chunk) = cursor_mut.get().map(|w| w.deref().clone()) { + if let Some(chunk) = cursor_mut.get().map(|w| w.deref().deref().clone()) { if chunk.contains(&requested_frame) { if requested_end_frame <= *chunk.end() { - return Ok(allocate_from_chosen_chunk(requested_frame, num_frames, &chunk, ValueRefMut::RBTree(cursor_mut))); + return allocate_from_chosen_chunk(requested_frame, num_frames, ValueRefMut::RBTree(cursor_mut)); } else { // We found the chunk containing the requested address, but it was too small to cover all of the requested frames. // Let's try to merge the next-highest contiguous chunk to see if those two chunks together @@ -658,14 +678,17 @@ fn find_specific_chunk( // Requested address: {:?}, num_frames: {}, chunk: {:?}", // requested_frame, num_frames, chunk, // ); - let next_contiguous_chunk: Option = { - let next_cursor = cursor_mut.peek_next(); - if let Some(next_chunk) = next_cursor.get().map(|w| w.deref()) { + let next_contiguous_chunk: Option> = { + cursor_mut.move_next();// cursor now points to the next chunk + if let Some(next_chunk) = cursor_mut.get().map(|w| w.deref()) { if *chunk.end() + 1 == *next_chunk.start() { // Here: next chunk was contiguous with the original chunk. if requested_end_frame <= *next_chunk.end() { // trace!("Frame allocator: found suitably-large contiguous next {:?} after initial too-small {:?}", next_chunk, chunk); - Some(next_chunk.clone()) + let next = cursor_mut.remove().map(|f| f.into_inner()); + // after removal, the cursor has been moved to the next chunk, so move it back to the original chunk + cursor_mut.move_prev(); + next } else { todo!("Frame allocator: found chunk containing requested address, but it was too small. \ Theseus does not yet support merging more than two chunks during an allocation request. \ @@ -684,15 +707,13 @@ fn find_specific_chunk( return Err(AllocationError::ContiguousChunkNotFound(*chunk.end() + 1, requested_end_frame.number() - chunk.end().number())); } }; - if let Some(mut next_chunk) = next_contiguous_chunk { + if let Some(next_chunk) = next_contiguous_chunk { // We found a suitable chunk that came contiguously after the initial too-small chunk. - // Remove the initial chunk (since we have a cursor pointing to it already) - // and "merge" it into this `next_chunk`. - let _removed_initial_chunk = cursor_mut.remove(); - // trace!("Frame allocator: removed suitably-large contiguous next {:?} after initial too-small {:?}", _removed_initial_chunk, chunk); - // Here, `cursor_mut` has been moved forward to point to the `next_chunk` now. - next_chunk.frames = FrameRange::new(*chunk.start(), *next_chunk.end()); - return Ok(allocate_from_chosen_chunk(requested_frame, num_frames, &next_chunk, ValueRefMut::RBTree(cursor_mut))); + // Now merge it into the initial chunk (since we have a cursor pointing to it already) + let initial_chunk = cursor_mut.get().map(|w| w.deref_mut()).unwrap(); + // We're using the merge function with all the chunks, but we could just set the frame range since we've already conducted all the checks + let _ = initial_chunk.merge(next_chunk); // this shoiuld always succeed + return allocate_from_chosen_chunk(requested_frame, num_frames, ValueRefMut::RBTree(cursor_mut)); } } } @@ -706,7 +727,7 @@ fn find_specific_chunk( /// Searches the given `list` for any chunk large enough to hold at least `num_frames`. fn find_any_chunk( - list: &mut StaticArrayRBTree, + list: &mut StaticArrayRBTree>, num_frames: usize ) -> Result<(AllocatedFrames, DeferredAllocAction<'static>), AllocationError> { // During the first pass, we ignore designated regions. @@ -715,11 +736,11 @@ fn find_any_chunk( for elem in arr.iter_mut() { if let Some(chunk) = elem { // Skip chunks that are too-small or in the designated regions. - if chunk.size_in_frames() < num_frames || chunk.typ != MemoryRegionType::Free { + if chunk.size_in_frames() < num_frames || chunk.typ() != MemoryRegionType::Free { continue; } else { - return Ok(allocate_from_chosen_chunk(*chunk.start(), num_frames, &chunk.clone(), ValueRefMut::Array(elem))); + return allocate_from_chosen_chunk(*chunk.start(), num_frames, ValueRefMut::Array(elem)); } } } @@ -728,10 +749,10 @@ fn find_any_chunk( // Because we allocate new frames by peeling them off from the beginning part of a chunk, // it's MUCH faster to start the search for free frames from higher addresses moving down. // This results in an O(1) allocation time in the general case, until all address ranges are already in use. - let mut cursor = tree.upper_bound_mut(Bound::<&Chunk>::Unbounded); + let mut cursor = tree.upper_bound_mut(Bound::<&Frames<{FrameState::Unmapped}>>::Unbounded); while let Some(chunk) = cursor.get().map(|w| w.deref()) { - if num_frames <= chunk.size_in_frames() && chunk.typ == MemoryRegionType::Free { - return Ok(allocate_from_chosen_chunk(*chunk.start(), num_frames, &chunk.clone(), ValueRefMut::RBTree(cursor))); + if num_frames <= chunk.size_in_frames() && chunk.typ() == MemoryRegionType::Free { + return allocate_from_chosen_chunk(*chunk.start(), num_frames, ValueRefMut::RBTree(cursor)); } warn!("Frame allocator: inefficient scenario: had to search multiple chunks \ (skipping {:?}) while trying to allocate {} frames at any address.", @@ -750,6 +771,20 @@ fn find_any_chunk( } +/// Removes a `Frames` object from the RBTree. +/// `frames_ref` is basically a wrapper over the cursor which stores the position of the frames. +fn retrieve_frames_from_ref(mut frames_ref: ValueRefMut>) -> Option> { + // Remove the chosen chunk from the free frame list. + let removed_val = frames_ref.remove(); + + match removed_val { + RemovedValue::Array(c) => c, + RemovedValue::RBTree(option_frames) => { + option_frames.map(|c| c.into_inner()) + } + } +} + /// The final part of the main allocation routine that splits the given chosen chunk /// into multiple smaller chunks, thereby "allocating" frames from it. @@ -759,79 +794,77 @@ fn find_any_chunk( fn allocate_from_chosen_chunk( start_frame: Frame, num_frames: usize, - chosen_chunk: &Chunk, - mut chosen_chunk_ref: ValueRefMut, -) -> (AllocatedFrames, DeferredAllocAction<'static>) { - let (new_allocation, before, after) = split_chosen_chunk(start_frame, num_frames, chosen_chunk); - + mut chosen_chunk_ref: ValueRefMut>, +) -> Result<(AllocatedFrames, DeferredAllocAction<'static>), AllocationError> { // Remove the chosen chunk from the free frame list. - let _removed_chunk = chosen_chunk_ref.remove(); + let chosen_chunk = retrieve_frames_from_ref(chosen_chunk_ref).ok_or(AllocationError::ChunkRemovalFailed)?; + let (new_allocation, before, after) = chosen_chunk.split(start_frame, num_frames); // TODO: Re-use the allocated wrapper if possible, rather than allocate a new one entirely. // if let RemovedValue::RBTree(Some(wrapper_adapter)) = _removed_chunk { ... } - ( - new_allocation.as_allocated_frames(), + Ok(( + new_allocation, DeferredAllocAction::new(before, after), - ) + )) } -/// An inner function that breaks up the given chunk into multiple smaller chunks. -/// -/// Returns a tuple of three chunks: -/// 1. The `Chunk` containing the requested range of frames starting at `start_frame`. -/// 2. The range of frames in the `chosen_chunk` that came before the beginning of the requested frame range. -/// 3. The range of frames in the `chosen_chunk` that came after the end of the requested frame range. -fn split_chosen_chunk( - start_frame: Frame, - num_frames: usize, - chosen_chunk: &Chunk, -) -> (Chunk, Option, Option) { - // The new allocated chunk might start in the middle of an existing chunk, - // so we need to break up that existing chunk into 3 possible chunks: before, newly-allocated, and after. - // - // Because Frames and PhysicalAddresses use saturating add/subtract, we need to double-check that - // we don't create overlapping duplicate Chunks at either the very minimum or the very maximum of the address space. - let new_allocation = Chunk { - typ: chosen_chunk.typ, - // The end frame is an inclusive bound, hence the -1. Parentheses are needed to avoid overflow. - frames: FrameRange::new(start_frame, start_frame + (num_frames - 1)), - }; - let before = if start_frame == MIN_FRAME { - None - } else { - Some(Chunk { - typ: chosen_chunk.typ, - frames: FrameRange::new(*chosen_chunk.start(), *new_allocation.start() - 1), - }) - }; - let after = if new_allocation.end() == &MAX_FRAME { - None - } else { - Some(Chunk { - typ: chosen_chunk.typ, - frames: FrameRange::new(*new_allocation.end() + 1, *chosen_chunk.end()), - }) - }; - - // some sanity checks -- these can be removed or disabled for better performance - if let Some(ref b) = before { - assert!(!new_allocation.contains(b.end())); - assert!(!b.contains(new_allocation.start())); - } - if let Some(ref a) = after { - assert!(!new_allocation.contains(a.start())); - assert!(!a.contains(new_allocation.end())); - } - - (new_allocation, before, after) -} +// /// An inner function that breaks up the given chunk into multiple smaller chunks. +// /// +// /// Returns a tuple of three chunks: +// /// 1. The `Chunk` containing the requested range of frames starting at `start_frame`. +// /// 2. The range of frames in the `chosen_chunk` that came before the beginning of the requested frame range. +// /// 3. The range of frames in the `chosen_chunk` that came after the end of the requested frame range. +// fn split_chosen_chunk( +// start_frame: Frame, +// num_frames: usize, +// chosen_chunk: &Chunk, +// ) -> (Chunk, Option, Option) { +// // The new allocated chunk might start in the middle of an existing chunk, +// // so we need to break up that existing chunk into 3 possible chunks: before, newly-allocated, and after. +// // +// // Because Frames and PhysicalAddresses use saturating add/subtract, we need to double-check that +// // we don't create overlapping duplicate Chunks at either the very minimum or the very maximum of the address space. +// let new_allocation = Chunk { +// typ: chosen_chunk.typ, +// // The end frame is an inclusive bound, hence the -1. Parentheses are needed to avoid overflow. +// frames: FrameRange::new(start_frame, start_frame + (num_frames - 1)), +// }; +// let before = if start_frame == MIN_FRAME { +// None +// } else { +// Some(Chunk { +// typ: chosen_chunk.typ, +// frames: FrameRange::new(*chosen_chunk.start(), *new_allocation.start() - 1), +// }) +// }; +// let after = if new_allocation.end() == &MAX_FRAME { +// None +// } else { +// Some(Chunk { +// typ: chosen_chunk.typ, +// frames: FrameRange::new(*new_allocation.end() + 1, *chosen_chunk.end()), +// }) +// }; + +// // some sanity checks -- these can be removed or disabled for better performance +// if let Some(ref b) = before { +// assert!(!new_allocation.contains(b.end())); +// assert!(!b.contains(new_allocation.start())); +// } +// if let Some(ref a) = after { +// assert!(!new_allocation.contains(a.start())); +// assert!(!a.contains(new_allocation.end())); +// } + +// (new_allocation, before, after) +// } /// Returns `true` if the given list contains *any* of the given `frames`. fn contains_any( - list: &StaticArrayRBTree, + list: &StaticArrayRBTree, frames: &FrameRange, ) -> bool { match &list.0 { @@ -870,8 +903,60 @@ fn contains_any( /// Currently, this function adds no new frames at all if any frames within the given `frames` list /// overlap any existing regions at all. /// TODO: handle partially-overlapping regions by extending existing regions on either end. -fn add_reserved_region( - list: &mut StaticArrayRBTree, +fn add_reserved_region_to_frames_list( + list: &mut StaticArrayRBTree>, + frames: FrameRange, +) -> Result { + + // Check whether the reserved region overlaps any existing regions. + match &mut list.0 { + Inner::Array(ref mut arr) => { + for chunk in arr.iter().flatten() { + if let Some(_overlap) = chunk.overlap(&frames) { + // trace!("Failed to add reserved region {:?} due to overlap {:?} with existing chunk {:?}", + // frames, _overlap, chunk + // ); + return Err("Failed to add reserved region that overlapped with existing reserved regions (array)."); + } + } + } + Inner::RBTree(ref mut tree) => { + let mut cursor_mut = tree.upper_bound_mut(Bound::Included(frames.start())); + while let Some(chunk) = cursor_mut.get().map(|w| w.deref()) { + if chunk.start() > frames.end() { + // We're iterating in ascending order over a sorted tree, + // so we can stop looking for overlapping regions once we pass the end of the new frames to add. + break; + } + if let Some(_overlap) = chunk.overlap(&frames) { + // trace!("Failed to add reserved region {:?} due to overlap {:?} with existing chunk {:?}", + // frames, _overlap, chunk + // ); + return Err("Failed to add reserved region that overlapped with existing reserved regions (RBTree)."); + } + cursor_mut.move_next(); + } + } + } + + list.insert(Frames::new( + MemoryRegionType::Reserved, + frames.clone(), + )).map_err(|_c| "BUG: Failed to insert non-overlapping frames into list.")?; + + Ok(frames) +} + +/// Adds the given `frames` to the given `list` as a Chunk of reserved frames. +/// +/// Returns the range of **new** frames that were added to the list, +/// which will be a subset of the given input `frames`. +/// +/// Currently, this function adds no new frames at all if any frames within the given `frames` list +/// overlap any existing regions at all. +/// TODO: handle partially-overlapping regions by extending existing regions on either end. +fn add_reserved_region_to_regions_list( + list: &mut StaticArrayRBTree, frames: FrameRange, ) -> Result { @@ -906,7 +991,7 @@ fn add_reserved_region( } } - list.insert(Chunk { + list.insert(Region { typ: MemoryRegionType::Reserved, frames: frames.clone(), }).map_err(|_c| "BUG: Failed to insert non-overlapping frames into list.")?; @@ -978,10 +1063,10 @@ pub fn allocate_frames_deferred( // but ONLY if those frames are *NOT* in the general-purpose region. let requested_frames = FrameRange::new(requested_start_frame, requested_start_frame + (requested_num_frames - 1)); if !contains_any(&GENERAL_REGIONS.lock(), &requested_frames) { - let new_reserved_frames = add_reserved_region(&mut RESERVED_REGIONS.lock(), requested_frames)?; + let new_reserved_frames = add_reserved_region_to_regions_list(&mut RESERVED_REGIONS.lock(), requested_frames)?; // If we successfully added a new reserved region, // then add those frames to the actual list of *available* reserved regions. - let _new_free_reserved_frames = add_reserved_region(&mut free_reserved_frames_list, new_reserved_frames.clone())?; + let _new_free_reserved_frames = add_reserved_region_to_frames_list(&mut free_reserved_frames_list, new_reserved_frames.clone())?; assert_eq!(new_reserved_frames, _new_free_reserved_frames); find_specific_chunk(&mut free_reserved_frames_list, start_frame, num_frames) } diff --git a/kernel/frame_allocator/src/static_array_rb_tree.rs b/kernel/frame_allocator/src/static_array_rb_tree.rs index e41953746e..2d9d0299dd 100644 --- a/kernel/frame_allocator/src/static_array_rb_tree.rs +++ b/kernel/frame_allocator/src/static_array_rb_tree.rs @@ -42,6 +42,11 @@ impl Wrapper { inner: value, }) } + + /// Returns the inner value, consuming this wrapper. + pub(crate) fn into_inner(self) -> T { + self.inner + } } From 34c2d8ee2b58871613e871884443027e056ec2f4 Mon Sep 17 00:00:00 2001 From: ramla-i Date: Thu, 6 Jul 2023 13:51:58 -0400 Subject: [PATCH 02/40] runs --- kernel/frame_allocator/src/frames.rs | 30 ++++----- kernel/frame_allocator/src/lib.rs | 99 +++++++++++----------------- kernel/frame_allocator/src/test.rs | 31 ++++----- 3 files changed, 68 insertions(+), 92 deletions(-) diff --git a/kernel/frame_allocator/src/frames.rs b/kernel/frame_allocator/src/frames.rs index b0658859bb..95c6500a68 100644 --- a/kernel/frame_allocator/src/frames.rs +++ b/kernel/frame_allocator/src/frames.rs @@ -1,16 +1,15 @@ //! A range of unmapped frames that stores a verified `TrustedChunk`. //! A `Frames` object is uncloneable and is the only way to access the range of frames it references. -use kernel_config::memory::PAGE_SIZE; -use memory_structs::{FrameRange, Frame, PhysicalAddress}; -use range_inclusive::RangeInclusive; +use memory_structs::{FrameRange, Frame}; use crate::{MemoryRegionType, contains_any, RESERVED_REGIONS, FREE_GENERAL_FRAMES_LIST, FREE_RESERVED_FRAMES_LIST}; use core::{borrow::Borrow, cmp::Ordering, ops::{Deref, DerefMut}, fmt, marker::ConstParamTy}; -use spin::Mutex; use static_assertions::assert_not_impl_any; use log::error; pub type AllocatedFrames = Frames<{FrameState::Unmapped}>; +pub type AllocatedFrame<'f> = UnmappedFrame<'f>; + #[derive(PartialEq, Eq, ConstParamTy)] pub enum FrameState { @@ -74,18 +73,18 @@ impl Frames<{FrameState::Unmapped}> { /// Consumes the `Frames` in an unmapped state and converts them to `Frames` in a mapped state. /// This should only be called once a `MappedPages` has been created from the `Frames`. - pub fn into_mapped_frames(mut self) -> Frames<{FrameState::Mapped}> { + pub fn into_mapped_frames(self) -> Frames<{FrameState::Mapped}> { Frames { typ: self.typ, - frames: self.frames, + frames: self.frames.clone(), } } /// Returns an `UnmappedFrame` if this `Frames<{FrameState::Unmapped}>` object contains only one frame. - /// + /// I've kept the terminology of allocated frame here to avoid changing code outside of this crate. /// ## Panic - /// Panics if this `AllocatedFrame` contains multiple frames or zero frames. - pub fn as_unmapped_frame(&self) -> UnmappedFrame { + /// Panics if this `UnmappedFrame` contains multiple frames or zero frames. + pub fn as_allocated_frame(&self) -> UnmappedFrame { assert!(self.size_in_frames() == 1); UnmappedFrame { frame: *self.start(), @@ -121,14 +120,13 @@ impl Drop for Frames { let unmapped_frames: Frames<{FrameState::Unmapped}> = Frames { typ: self.typ, - frames: self.frames, + frames: self.frames.clone(), }; - // Should we remove these lines since we store the typ in Frames? - let (list, _typ) = if contains_any(&RESERVED_REGIONS.lock(), &unmapped_frames) { - (&FREE_RESERVED_FRAMES_LIST, MemoryRegionType::Reserved) + let list = if unmapped_frames.typ == MemoryRegionType::Reserved { + &FREE_RESERVED_FRAMES_LIST } else { - (&FREE_GENERAL_FRAMES_LIST, MemoryRegionType::Free) + &FREE_GENERAL_FRAMES_LIST }; // Simply add the newly-deallocated chunk to the free frames list. @@ -156,7 +154,7 @@ impl<'f> IntoIterator for &'f Frames<{FrameState::Unmapped}> { fn into_iter(self) -> Self::IntoIter { UnmappedFramesIter { _owner: self, - range: self.frames.clone().into_iter(), + range: self.frames.iter(), } } } @@ -236,7 +234,7 @@ impl Frames { /// /// If either of those conditions are met, `self` is modified and `Ok(())` is returned, /// otherwise `Err(other)` is returned. - pub fn merge(&mut self, mut other: Self) -> Result<(), Self> { + pub fn merge(&mut self, other: Self) -> Result<(), Self> { if self.is_empty() || other.is_empty() { return Err(other); } diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index 8e248a84d2..a613d2b08b 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -31,17 +31,15 @@ mod static_array_rb_tree; // mod static_array_linked_list; mod frames; -use core::{borrow::Borrow, cmp::{Ordering, min, max}, fmt, ops::{Deref, DerefMut}, marker::PhantomData}; +use core::{borrow::Borrow, cmp::{Ordering, min, max}, ops::Deref}; use intrusive_collections::Bound; use kernel_config::memory::*; use log::{error, warn, debug, trace}; use memory_structs::{PhysicalAddress, Frame, FrameRange}; -use range_inclusive::RangeInclusiveIterator; use spin::Mutex; use static_array_rb_tree::*; -use static_assertions::assert_not_impl_any; use frames::{Frames, FrameState}; -pub use frames::{AllocatedFrames, UnmappedFrame}; +pub use frames::{AllocatedFrames, AllocatedFrame}; const FRAME_SIZE: usize = PAGE_SIZE; const MIN_FRAME: Frame = Frame::containing_address(PhysicalAddress::zero()); @@ -296,12 +294,14 @@ pub enum MemoryRegionType { #[derive(Debug, Clone, Eq)] struct Region { /// The type of this memory chunk, e.g., whether it's in a free or reserved region. + #[allow(unused)] typ: MemoryRegionType, /// The Frames covered by this chunk, an inclusive range. frames: FrameRange, } impl Region { /// Returns a new `Chunk` with an empty range of frames. + #[allow(unused)] const fn empty() -> Region { Region { typ: MemoryRegionType::Unknown, @@ -626,6 +626,8 @@ enum AllocationError { ContiguousChunkNotFound(Frame, usize), /// Failed to remove a chunk from the free list given a reference to it. ChunkRemovalFailed, + /// Failed to merge or split a Chunk. + ChunkOperationFailed, } impl From for &'static str { fn from(alloc_err: AllocationError) -> &'static str { @@ -635,6 +637,7 @@ impl From for &'static str { AllocationError::OutOfAddressSpace(..) => "out of physical address space", AllocationError::ContiguousChunkNotFound(..) => "only some of the requested frames were available", AllocationError::ChunkRemovalFailed => "Failed to remove a Chunk from the free list, this is most likely due to some logical error", + AllocationError::ChunkOperationFailed => "Could not merge or split a Chunk", } } } @@ -709,11 +712,9 @@ fn find_specific_chunk( }; if let Some(next_chunk) = next_contiguous_chunk { // We found a suitable chunk that came contiguously after the initial too-small chunk. - // Now merge it into the initial chunk (since we have a cursor pointing to it already) - let initial_chunk = cursor_mut.get().map(|w| w.deref_mut()).unwrap(); - // We're using the merge function with all the chunks, but we could just set the frame range since we've already conducted all the checks - let _ = initial_chunk.merge(next_chunk); // this shoiuld always succeed - return allocate_from_chosen_chunk(requested_frame, num_frames, ValueRefMut::RBTree(cursor_mut)); + // We would like to merge it into the initial chunk (since we have a cursor pointing to it already), + // but we can't get a mutable reference to the element the cursor is pointing to. + return merge_contiguous_chunks_and_allocate(requested_frame, num_frames, ValueRefMut::RBTree(cursor_mut), next_chunk); } } } @@ -785,6 +786,33 @@ fn retrieve_frames_from_ref(mut frames_ref: ValueRefMut>, + next_chunk: Frames<{FrameState::Unmapped}>, +) -> Result<(AllocatedFrames, DeferredAllocAction<'static>), AllocationError> { + // Remove the chosen chunk from the free frame list. + let mut chosen_chunk = retrieve_frames_from_ref(initial_chunk_ref).ok_or(AllocationError::ChunkRemovalFailed)?; + // This should always succeed, since we've already checked the conditions for a merge + // We should return the next_chunk back to the list, but a failure at this point implies a bug in the frame allocator. + chosen_chunk.merge(next_chunk).map_err(|_| AllocationError::ChunkOperationFailed)?; + let (new_allocation, before, after) = chosen_chunk.split(start_frame, num_frames); + + // TODO: Re-use the allocated wrapper if possible, rather than allocate a new one entirely. + // if let RemovedValue::RBTree(Some(wrapper_adapter)) = _removed_chunk { ... } + + Ok(( + new_allocation, + DeferredAllocAction::new(before, after), + )) + +} /// The final part of the main allocation routine that splits the given chosen chunk /// into multiple smaller chunks, thereby "allocating" frames from it. @@ -794,7 +822,7 @@ fn retrieve_frames_from_ref(mut frames_ref: ValueRefMut>, + chosen_chunk_ref: ValueRefMut>, ) -> Result<(AllocatedFrames, DeferredAllocAction<'static>), AllocationError> { // Remove the chosen chunk from the free frame list. let chosen_chunk = retrieve_frames_from_ref(chosen_chunk_ref).ok_or(AllocationError::ChunkRemovalFailed)?; @@ -810,57 +838,6 @@ fn allocate_from_chosen_chunk( } -// /// An inner function that breaks up the given chunk into multiple smaller chunks. -// /// -// /// Returns a tuple of three chunks: -// /// 1. The `Chunk` containing the requested range of frames starting at `start_frame`. -// /// 2. The range of frames in the `chosen_chunk` that came before the beginning of the requested frame range. -// /// 3. The range of frames in the `chosen_chunk` that came after the end of the requested frame range. -// fn split_chosen_chunk( -// start_frame: Frame, -// num_frames: usize, -// chosen_chunk: &Chunk, -// ) -> (Chunk, Option, Option) { -// // The new allocated chunk might start in the middle of an existing chunk, -// // so we need to break up that existing chunk into 3 possible chunks: before, newly-allocated, and after. -// // -// // Because Frames and PhysicalAddresses use saturating add/subtract, we need to double-check that -// // we don't create overlapping duplicate Chunks at either the very minimum or the very maximum of the address space. -// let new_allocation = Chunk { -// typ: chosen_chunk.typ, -// // The end frame is an inclusive bound, hence the -1. Parentheses are needed to avoid overflow. -// frames: FrameRange::new(start_frame, start_frame + (num_frames - 1)), -// }; -// let before = if start_frame == MIN_FRAME { -// None -// } else { -// Some(Chunk { -// typ: chosen_chunk.typ, -// frames: FrameRange::new(*chosen_chunk.start(), *new_allocation.start() - 1), -// }) -// }; -// let after = if new_allocation.end() == &MAX_FRAME { -// None -// } else { -// Some(Chunk { -// typ: chosen_chunk.typ, -// frames: FrameRange::new(*new_allocation.end() + 1, *chosen_chunk.end()), -// }) -// }; - -// // some sanity checks -- these can be removed or disabled for better performance -// if let Some(ref b) = before { -// assert!(!new_allocation.contains(b.end())); -// assert!(!b.contains(new_allocation.start())); -// } -// if let Some(ref a) = after { -// assert!(!new_allocation.contains(a.start())); -// assert!(!a.contains(new_allocation.end())); -// } - -// (new_allocation, before, after) -// } - /// Returns `true` if the given list contains *any* of the given `frames`. fn contains_any( diff --git a/kernel/frame_allocator/src/test.rs b/kernel/frame_allocator/src/test.rs index d68ced8356..b26f155ae3 100644 --- a/kernel/frame_allocator/src/test.rs +++ b/kernel/frame_allocator/src/test.rs @@ -8,12 +8,13 @@ use super::*; impl PartialEq for AllocatedFrames { fn eq(&self, other: &Self) -> bool { - self.frames == other.frames + self.frames() == other.frames() } } fn from_addr(start_addr: usize, end_addr: usize) -> AllocatedFrames { AllocatedFrames { + typ: MemoryRegionType::Free, frames: FrameRange::new( Frame::containing_address(PhysicalAddress::new_canonical(start_addr)), Frame::containing_address(PhysicalAddress::new_canonical(end_addr)), @@ -30,7 +31,7 @@ fn split_before_beginning() { let original = from_addr( 0x4275000, 0x4285000); let split_at = frame_addr(0x4274000); - let result = original.split(split_at); + let result = original.split_at(split_at); dbg!(&result); assert!(result.is_err()); } @@ -42,7 +43,7 @@ fn split_at_beginning() { let first = AllocatedFrames::empty(); let second = from_addr( 0x4275000, 0x4285000); - let result = original.split(split_at); + let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); assert_eq!(result1, first); @@ -57,7 +58,7 @@ fn split_at_middle() { let first = from_addr( 0x4275000, 0x427C000); let second = from_addr( 0x427D000, 0x4285000); - let result = original.split(split_at); + let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); assert_eq!(result1, first); @@ -71,7 +72,7 @@ fn split_at_end() { let first = from_addr( 0x4275000, 0x4284000); let second = from_addr( 0x4285000, 0x4285000); - let result = original.split(split_at); + let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); assert_eq!(result1, first); @@ -86,7 +87,7 @@ fn split_after_end() { let first = from_addr( 0x4275000, 0x4285000); let second = AllocatedFrames::empty(); - let result = original.split(split_at); + let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); assert_eq!(result1, first); @@ -99,7 +100,7 @@ fn split_empty_at_zero() { let original = AllocatedFrames::empty(); let split_at = frame_addr(0x0000); - let result = original.split(split_at); + let result = original.split_at(split_at); dbg!(&result); assert!(result.is_err()); } @@ -109,7 +110,7 @@ fn split_empty_at_one() { let original = AllocatedFrames::empty(); let split_at = frame_addr(0x1000); - let result = original.split(split_at); + let result = original.split_at(split_at); dbg!(&result); assert!(result.is_err()); } @@ -119,7 +120,7 @@ fn split_empty_at_two() { let original = AllocatedFrames::empty(); let split_at = frame_addr(0x2000); - let result = original.split(split_at); + let result = original.split_at(split_at); dbg!(&result); assert!(result.is_err()); } @@ -133,7 +134,7 @@ fn split_at_beginning_zero() { let first = AllocatedFrames::empty(); let second = from_addr(0x0, 0x5000); - let result = original.split(split_at); + let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); assert_eq!(result1, first); @@ -147,7 +148,7 @@ fn split_at_beginning_one() { let first = from_addr( 0x0000, 0x0000); let second = from_addr( 0x1000, 0x5000); - let result = original.split(split_at); + let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); assert_eq!(result1, first); @@ -161,7 +162,7 @@ fn split_at_beginning_max_length_one() { let first = AllocatedFrames::empty(); let second = from_addr(0xFFFF_FFFF_FFFF_F000, 0xFFFF_FFFF_FFFF_F000); - let result = original.split(split_at); + let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); assert_eq!(result1, first); @@ -175,7 +176,7 @@ fn split_at_end_max_length_two() { let first = from_addr( 0xFFFF_FFFF_FFFF_E000, 0xFFFF_FFFF_FFFF_E000); let second = from_addr( 0xFFFF_FFFF_FFFF_F000, 0xFFFF_FFFF_FFFF_F000); - let result = original.split(split_at); + let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); assert_eq!(result1, first); @@ -190,7 +191,7 @@ fn split_after_end_max() { let first = from_addr( 0xFFFF_FFFF_FFFF_E000, 0xFFFF_FFFF_FFFF_E000); let second = AllocatedFrames::empty(); - let result = original.split(split_at); + let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); assert_eq!(result1, first); @@ -204,7 +205,7 @@ fn split_at_beginning_max() { let first = AllocatedFrames::empty(); let second = from_addr(0xFFFF_FFFF_FFFF_E000, 0xFFFF_FFFF_FFFF_E000); - let result = original.split(split_at); + let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); assert_eq!(result1, first); From b78dc4efcff0b8d89be69ebe1d8d813005899c01 Mon Sep 17 00:00:00 2001 From: ramla-i Date: Thu, 6 Jul 2023 14:16:16 -0400 Subject: [PATCH 03/40] runs and tests pass --- kernel/frame_allocator/src/frames.rs | 26 +++++++++++++------------- kernel/frame_allocator/src/test.rs | 18 +++++++++--------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/kernel/frame_allocator/src/frames.rs b/kernel/frame_allocator/src/frames.rs index 95c6500a68..be99415dc7 100644 --- a/kernel/frame_allocator/src/frames.rs +++ b/kernel/frame_allocator/src/frames.rs @@ -2,7 +2,7 @@ //! A `Frames` object is uncloneable and is the only way to access the range of frames it references. use memory_structs::{FrameRange, Frame}; -use crate::{MemoryRegionType, contains_any, RESERVED_REGIONS, FREE_GENERAL_FRAMES_LIST, FREE_RESERVED_FRAMES_LIST}; +use crate::{MemoryRegionType, contains_any, RESERVED_REGIONS, FREE_GENERAL_FRAMES_LIST, FREE_RESERVED_FRAMES_LIST, MIN_FRAME, MAX_FRAME}; use core::{borrow::Borrow, cmp::Ordering, ops::{Deref, DerefMut}, fmt, marker::ConstParamTy}; use static_assertions::assert_not_impl_any; use log::error; @@ -252,7 +252,7 @@ impl Frames { return Err(other); } - // ensure the now-merged TrustedChunk doesn't run its drop handler (currently not implemented, but to prevent any future issues.) + // ensure the now-merged Frames doesn't run its drop handler core::mem::forget(other); Ok(()) } @@ -267,29 +267,29 @@ impl Frames { /// If `start_frame` is not contained within `self` or `num_frames` results in an end frame greater than the end of `self`, /// then `self` is not changed and we return (self, None, None). pub fn split( - mut self, + self, start_frame: Frame, num_frames: usize, ) -> (Self, Option, Option) { - if (start_frame < *self.start()) || (start_frame + num_frames - 1 > *self.end()) || (num_frames <= 0) { + if (start_frame < *self.start()) || (start_frame + (num_frames - 1) > *self.end()) || (num_frames <= 0) { return (self, None, None); } - let before = if start_frame == *self.start() { + let new_allocation = Frames{ frames: FrameRange::new(start_frame, start_frame + (num_frames - 1)), ..self }; + let before = if start_frame == MIN_FRAME || start_frame == *self.start() { None } else { - Some(Frames{ frames: FrameRange::new(*self.start(), start_frame - 1), ..self }) + Some(Frames{ frames: FrameRange::new(*self.start(), *new_allocation.start() - 1), ..self }) }; - let required = Frames{ frames: FrameRange::new(start_frame, start_frame + num_frames - 1), ..self }; - let after = if start_frame + num_frames - 1 == *self.end() { + let after = if *new_allocation.end() == MAX_FRAME || *new_allocation.end() == *self.end(){ None } else { - Some(Frames{ frames: FrameRange::new(start_frame + num_frames, *self.end()), ..self }) + Some(Frames{ frames: FrameRange::new(*new_allocation.end() + 1, *self.end()), ..self }) }; core::mem::forget(self); - (required, before, after) + (new_allocation, before, after) } /// Splits this `Frames` into two separate `Frames` objects: @@ -301,10 +301,10 @@ impl Frames { /// * If `at_frame == self.start`, the first returned `Frames` object will be empty. /// * If `at_frame == self.end + 1`, the second returned `Frames` object will be empty. /// - /// Returns an `Err` containing this `Frames` if `at_frame` is otherwise out of bounds. + /// Returns an `Err` containing this `Frames` if `at_frame` is otherwise out of bounds, or if `self` was empty. /// /// [`core::slice::split_at()`]: https://doc.rust-lang.org/core/primitive.slice.html#method.split_at - pub fn split_at(mut self, at_frame: Frame) -> Result<(Self, Self), Self> { + pub fn split_at(self, at_frame: Frame) -> Result<(Self, Self), Self> { if self.is_empty() { return Err(self); } let end_of_first = at_frame - 1; @@ -329,7 +329,7 @@ impl Frames { }; let typ = self.typ; - // ensure the original AllocatedFrames doesn't run its drop handler and free its frames. + // ensure the original Frames doesn't run its drop handler and free its frames. core::mem::forget(self); Ok(( Frames { typ, frames: first }, diff --git a/kernel/frame_allocator/src/test.rs b/kernel/frame_allocator/src/test.rs index b26f155ae3..14a1629352 100644 --- a/kernel/frame_allocator/src/test.rs +++ b/kernel/frame_allocator/src/test.rs @@ -6,20 +6,20 @@ use self::std::dbg; use super::*; -impl PartialEq for AllocatedFrames { - fn eq(&self, other: &Self) -> bool { - self.frames() == other.frames() - } -} +// impl PartialEq for AllocatedFrames { +// fn eq(&self, other: &Self) -> bool { +// self.frames() == other.frames() +// } +// } fn from_addr(start_addr: usize, end_addr: usize) -> AllocatedFrames { - AllocatedFrames { - typ: MemoryRegionType::Free, - frames: FrameRange::new( + AllocatedFrames::new( + MemoryRegionType::Free, + FrameRange::new( Frame::containing_address(PhysicalAddress::new_canonical(start_addr)), Frame::containing_address(PhysicalAddress::new_canonical(end_addr)), ) - } + ) } fn frame_addr(addr: usize) -> Frame { From 14fea3e9938221e0ceba025864f0406488b6dea1 Mon Sep 17 00:00:00 2001 From: ramla-i Date: Thu, 6 Jul 2023 14:47:42 -0400 Subject: [PATCH 04/40] moved frames to lib.rs --- kernel/frame_allocator/src/frames.rs | 27 +- kernel/frame_allocator/src/lib.rs | 600 +++++++++++++++++---------- 2 files changed, 384 insertions(+), 243 deletions(-) diff --git a/kernel/frame_allocator/src/frames.rs b/kernel/frame_allocator/src/frames.rs index be99415dc7..a33611f83c 100644 --- a/kernel/frame_allocator/src/frames.rs +++ b/kernel/frame_allocator/src/frames.rs @@ -1,4 +1,4 @@ -//! A range of unmapped frames that stores a verified `TrustedChunk`. +//! A range of frames that are either mapped or unmapped. //! A `Frames` object is uncloneable and is the only way to access the range of frames it references. use memory_structs::{FrameRange, Frame}; @@ -19,9 +19,6 @@ pub enum FrameState { /// A range of contiguous frames. /// Owning a `Frames` object gives ownership of the range of frames it references. -/// The `verified_chunk` field is a verified `TrustedChunk` that stores the actual frames, -/// and has the invariant that it does not overlap with any other `TrustedChunk` created by the -/// `CHUNK_ALLOCATOR`. /// /// The frames can be in an unmapped or mapped state. In the unmapped state, the frames are not /// immediately accessible because they're not yet mapped by any virtual memory pages. @@ -43,9 +40,7 @@ pub enum FrameState { pub struct Frames { /// The type of this memory chunk, e.g., whether it's in a free or reserved region. typ: MemoryRegionType, - /// The Frames covered by this chunk, an inclusive range. Equal to the frames in the verified chunk. - /// Needed because verification fails on a trusted chunk that stores a FrameRange or RangeInclusive, - /// but succeeds with RangeInclusive. + /// The Frames covered by this chunk, an inclusive range. frames: FrameRange } @@ -56,32 +51,29 @@ assert_not_impl_any!(Frames<{FrameState::Mapped}>: DerefMut, Clone); impl Frames<{FrameState::Unmapped}> { /// Creates a new `Frames` object in an unmapped state. - /// If `frames` is empty, there is no space to store the new `Frames` information pre-heap intialization, - /// or a `TrustedChunk` already exists which overlaps with the given `frames`, then an error is returned. + /// The frame allocator is reponsible for making sure no two `Frames` objects overlap. pub(crate) fn new(typ: MemoryRegionType, frames: FrameRange) -> Self { - // assert!(frames.start().number() == verified_chunk.start()); - // assert!(frames.end().number() == verified_chunk.end()); - Frames { typ, frames, } - // warn!("NEW FRAMES: {:?}", f); - // Ok(f) } /// Consumes the `Frames` in an unmapped state and converts them to `Frames` in a mapped state. /// This should only be called once a `MappedPages` has been created from the `Frames`. pub fn into_mapped_frames(self) -> Frames<{FrameState::Mapped}> { - Frames { + let f = Frames { typ: self.typ, frames: self.frames.clone(), - } + }; + core::mem::forget(self); + f } /// Returns an `UnmappedFrame` if this `Frames<{FrameState::Unmapped}>` object contains only one frame. /// I've kept the terminology of allocated frame here to avoid changing code outside of this crate. + /// /// ## Panic /// Panics if this `UnmappedFrame` contains multiple frames or zero frames. pub fn as_allocated_frame(&self) -> UnmappedFrame { @@ -116,7 +108,6 @@ impl Drop for Frames { match S { FrameState::Unmapped => { if self.size_in_frames() == 0 { return; } - // trace!("FRAMES DROP {:?}", self); let unmapped_frames: Frames<{FrameState::Unmapped}> = Frames { typ: self.typ, @@ -161,7 +152,7 @@ impl<'f> IntoIterator for &'f Frames<{FrameState::Unmapped}> { /// An iterator over each [`UnmappedFrame`] in a range of [`Frames<{FrameState::Unmapped}>`]. /// -/// To Do: Description is no longer valid, since we have an iterator for RangeInclusive now. +/// To Do (get Kevin's input): Description is no longer valid, since we have an iterator for RangeInclusive now. /// but I still think it's useful to have a `Frames<{FrameState::Unmapped}>` iterator that ties the lifetime /// of the `UnmappedFrame` to the original object. /// diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index a613d2b08b..755c6fa5df 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -29,17 +29,16 @@ mod test; mod static_array_rb_tree; // mod static_array_linked_list; -mod frames; +// mod frames; -use core::{borrow::Borrow, cmp::{Ordering, min, max}, ops::Deref}; +use core::{borrow::Borrow, cmp::{Ordering, min, max}, ops::{Deref, DerefMut}, fmt, marker::ConstParamTy}; use intrusive_collections::Bound; use kernel_config::memory::*; use log::{error, warn, debug, trace}; use memory_structs::{PhysicalAddress, Frame, FrameRange}; use spin::Mutex; use static_array_rb_tree::*; -use frames::{Frames, FrameState}; -pub use frames::{AllocatedFrames, AllocatedFrame}; +use static_assertions::assert_not_impl_any; const FRAME_SIZE: usize = PAGE_SIZE; const MIN_FRAME: Frame = Frame::containing_address(PhysicalAddress::zero()); @@ -180,7 +179,7 @@ pub fn init( *GENERAL_REGIONS.lock() = StaticArrayRBTree::new(free_list); *RESERVED_REGIONS.lock() = StaticArrayRBTree::new(reserved_list); - Ok(frames::into_allocated_frames) + Ok(into_allocated_frames) } @@ -280,27 +279,28 @@ pub enum MemoryRegionType { Unknown, } -/// A range of contiguous frames. +/// `Region` represents a range of contiguous frames for bookkeeping purposes. +/// It does not give access to the underlying frames. /// /// # Ordering and Equality /// -/// `Chunk` implements the `Ord` trait, and its total ordering is ONLY based on -/// its **starting** `Frame`. This is useful so we can store `Chunk`s in a sorted collection. +/// `Region` implements the `Ord` trait, and its total ordering is ONLY based on +/// its **starting** `Frame`. This is useful so we can store `Region`s in a sorted collection. /// -/// Similarly, `Chunk` implements equality traits, `Eq` and `PartialEq`, -/// both of which are also based ONLY on the **starting** `Frame` of the `Chunk`. -/// Thus, comparing two `Chunk`s with the `==` or `!=` operators may not work as expected. +/// Similarly, `Region` implements equality traits, `Eq` and `PartialEq`, +/// both of which are also based ONLY on the **starting** `Frame` of the `Region`. +/// Thus, comparing two `Region`s with the `==` or `!=` operators may not work as expected. /// since it ignores their actual range of frames. #[derive(Debug, Clone, Eq)] struct Region { - /// The type of this memory chunk, e.g., whether it's in a free or reserved region. + /// The type of this memory region, e.g., whether it's in a free or reserved region. #[allow(unused)] typ: MemoryRegionType, - /// The Frames covered by this chunk, an inclusive range. + /// The Frames covered by this region, an inclusive range. frames: FrameRange, } impl Region { - /// Returns a new `Chunk` with an empty range of frames. + /// Returns a new `Region` with an empty range of frames. #[allow(unused)] const fn empty() -> Region { Region { @@ -337,218 +337,368 @@ impl Borrow for &'_ Region { } -// /// Represents a range of allocated physical memory [`Frame`]s; derefs to [`FrameRange`]. -// /// -// /// These frames are not immediately accessible because they're not yet mapped -// /// by any virtual memory pages. -// /// You must do that separately in order to create a `MappedPages` type, -// /// which can then be used to access the contents of these frames. -// /// -// /// This object represents ownership of the range of allocated physical frames; -// /// if this object falls out of scope, its allocated frames will be auto-deallocated upon drop. -// pub struct AllocatedFrames { -// frames: FrameRange, -// } - -// // AllocatedFrames must not be Cloneable, and it must not expose its inner frames as mutable. -// assert_not_impl_any!(AllocatedFrames: DerefMut, Clone); - -// impl Deref for AllocatedFrames { -// type Target = FrameRange; -// fn deref(&self) -> &FrameRange { -// &self.frames -// } -// } -// impl fmt::Debug for AllocatedFrames { -// fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { -// write!(f, "AllocatedFrames({:?})", self.frames) -// } -// } - -// impl AllocatedFrames { -// /// Returns an empty AllocatedFrames object that performs no frame allocation. -// /// Can be used as a placeholder, but will not permit any real usage. -// pub const fn empty() -> AllocatedFrames { -// AllocatedFrames { -// frames: FrameRange::empty() -// } -// } - -// /// Merges the given `AllocatedFrames` object `other` into this `AllocatedFrames` object (`self`). -// /// This is just for convenience and usability purposes, it performs no allocation or remapping. -// /// -// /// The given `other` must be physically contiguous with `self`, i.e., come immediately before or after `self`. -// /// That is, either `self.start == other.end + 1` or `self.end + 1 == other.start` must be true. -// /// -// /// If either of those conditions are met, `self` is modified and `Ok(())` is returned, -// /// otherwise `Err(other)` is returned. -// pub fn merge(&mut self, other: AllocatedFrames) -> Result<(), AllocatedFrames> { -// if *self.start() == *other.end() + 1 { -// // `other` comes contiguously before `self` -// self.frames = FrameRange::new(*other.start(), *self.end()); -// } -// else if *self.end() + 1 == *other.start() { -// // `self` comes contiguously before `other` -// self.frames = FrameRange::new(*self.start(), *other.end()); -// } -// else { -// // non-contiguous -// return Err(other); -// } - -// // ensure the now-merged AllocatedFrames doesn't run its drop handler and free its frames. -// core::mem::forget(other); -// Ok(()) -// } - -// /// Splits this `AllocatedFrames` into two separate `AllocatedFrames` objects: -// /// * `[beginning : at_frame - 1]` -// /// * `[at_frame : end]` -// /// -// /// This function follows the behavior of [`core::slice::split_at()`], -// /// thus, either one of the returned `AllocatedFrames` objects may be empty. -// /// * If `at_frame == self.start`, the first returned `AllocatedFrames` object will be empty. -// /// * If `at_frame == self.end + 1`, the second returned `AllocatedFrames` object will be empty. -// /// -// /// Returns an `Err` containing this `AllocatedFrames` if `at_frame` is otherwise out of bounds. -// /// -// /// [`core::slice::split_at()`]: https://doc.rust-lang.org/core/primitive.slice.html#method.split_at -// pub fn split(self, at_frame: Frame) -> Result<(AllocatedFrames, AllocatedFrames), AllocatedFrames> { -// let end_of_first = at_frame - 1; - -// let (first, second) = if at_frame == *self.start() && at_frame <= *self.end() { -// let first = FrameRange::empty(); -// let second = FrameRange::new(at_frame, *self.end()); -// (first, second) -// } -// else if at_frame == (*self.end() + 1) && end_of_first >= *self.start() { -// let first = FrameRange::new(*self.start(), *self.end()); -// let second = FrameRange::empty(); -// (first, second) -// } -// else if at_frame > *self.start() && end_of_first <= *self.end() { -// let first = FrameRange::new(*self.start(), end_of_first); -// let second = FrameRange::new(at_frame, *self.end()); -// (first, second) -// } -// else { -// return Err(self); -// }; - -// // ensure the original AllocatedFrames doesn't run its drop handler and free its frames. -// core::mem::forget(self); -// Ok(( -// AllocatedFrames { frames: first }, -// AllocatedFrames { frames: second }, -// )) -// } - -// /// Returns an `AllocatedFrame` if this `AllocatedFrames` object contains only one frame. -// /// -// /// ## Panic -// /// Panics if this `AllocatedFrame` contains multiple frames or zero frames. -// pub fn as_allocated_frame(&self) -> AllocatedFrame { -// assert!(self.size_in_frames() == 1); -// AllocatedFrame { -// frame: *self.start(), -// _phantom: PhantomData, -// } -// } -// } - -// /// This function is a callback used to convert `UnmappedFrames` into `AllocatedFrames`. -// /// `UnmappedFrames` represents frames that have been unmapped from a page that had -// /// exclusively mapped them, indicating that no others pages have been mapped -// /// to those same frames, and thus, they can be safely deallocated. -// /// -// /// This exists to break the cyclic dependency cycle between this crate and -// /// the `page_table_entry` crate, since `page_table_entry` must depend on types -// /// from this crate in order to enforce safety when modifying page table entries. -// fn into_allocated_frames(frames: FrameRange) -> AllocatedFrames { -// AllocatedFrames { frames } -// } - -// impl Drop for AllocatedFrames { -// fn drop(&mut self) { -// if self.size_in_frames() == 0 { return; } - -// let (list, typ) = if contains_any(&RESERVED_REGIONS.lock(), &self.frames) { -// (&FREE_RESERVED_FRAMES_LIST, MemoryRegionType::Reserved) -// } else { -// (&FREE_GENERAL_FRAMES_LIST, MemoryRegionType::Free) -// }; -// // trace!("frame_allocator: deallocating {:?}, typ {:?}", self, typ); - -// // Simply add the newly-deallocated chunk to the free frames list. -// let mut locked_list = list.lock(); -// let res = locked_list.insert(Chunk { -// typ, -// frames: self.frames.clone(), -// }); -// match res { -// Ok(_inserted_free_chunk) => (), -// Err(c) => error!("BUG: couldn't insert deallocated chunk {:?} into free frame list", c), -// } +pub type AllocatedFrames = Frames<{FrameState::Unmapped}>; +pub type AllocatedFrame<'f> = UnmappedFrame<'f>; + + +#[derive(PartialEq, Eq, ConstParamTy)] +pub enum FrameState { + Unmapped, + Mapped +} + +/// A range of contiguous frames. +/// Owning a `Frames` object gives ownership of the range of frames it references. +/// +/// The frames can be in an unmapped or mapped state. In the unmapped state, the frames are not +/// immediately accessible because they're not yet mapped by any virtual memory pages. +/// They are converted into a mapped state once they are used to create a `MappedPages` object. +/// +/// When a `Frames` object in an unmapped state is dropped, it is deallocated and returned to the free frames list. +/// We expect that `Frames` in a mapped state will never be dropped, but instead will be forgotten. +/// +/// # Ordering and Equality +/// +/// `Frames` implements the `Ord` trait, and its total ordering is ONLY based on +/// its **starting** `Frame`. This is useful so we can store `Frames` in a sorted collection. +/// +/// Similarly, `Frames` implements equality traits, `Eq` and `PartialEq`, +/// both of which are also based ONLY on the **starting** `Frame` of the `Frames`. +/// Thus, comparing two `Frames` with the `==` or `!=` operators may not work as expected. +/// since it ignores their actual range of frames. +#[derive(Eq)] +pub struct Frames { + /// The type of this memory chunk, e.g., whether it's in a free or reserved region. + typ: MemoryRegionType, + /// The Frames covered by this chunk, an inclusive range. + frames: FrameRange +} + +// Frames must not be Cloneable, and it must not expose its inner frames as mutable. +assert_not_impl_any!(Frames<{FrameState::Unmapped}>: DerefMut, Clone); +assert_not_impl_any!(Frames<{FrameState::Mapped}>: DerefMut, Clone); + + +impl Frames<{FrameState::Unmapped}> { + /// Creates a new `Frames` object in an unmapped state. + /// The frame allocator is reponsible for making sure no two `Frames` objects overlap. + pub(crate) fn new(typ: MemoryRegionType, frames: FrameRange) -> Self { + Frames { + typ, + frames, + } + } + + + /// Consumes the `Frames` in an unmapped state and converts them to `Frames` in a mapped state. + /// This should only be called once a `MappedPages` has been created from the `Frames`. + pub fn into_mapped_frames(self) -> Frames<{FrameState::Mapped}> { + let f = Frames { + typ: self.typ, + frames: self.frames.clone(), + }; + core::mem::forget(self); + f + } + + /// Returns an `UnmappedFrame` if this `Frames<{FrameState::Unmapped}>` object contains only one frame. + /// I've kept the terminology of allocated frame here to avoid changing code outside of this crate. + /// + /// ## Panic + /// Panics if this `UnmappedFrame` contains multiple frames or zero frames. + pub fn as_allocated_frame(&self) -> UnmappedFrame { + assert!(self.size_in_frames() == 1); + UnmappedFrame { + frame: *self.start(), + _phantom: core::marker::PhantomData, + } + } +} + + +/// This function is a callback used to convert `UnmappedFrames` into `Frames<{FrameState::Unmapped}>`. +/// `UnmappedFrames` represents frames that have been unmapped from a page that had +/// exclusively mapped them, indicating that no others pages have been mapped +/// to those same frames, and thus, they can be safely deallocated. +/// +/// This exists to break the cyclic dependency cycle between this crate and +/// the `page_table_entry` crate, since `page_table_entry` must depend on types +/// from this crate in order to enforce safety when modifying page table entries. +pub(crate) fn into_allocated_frames(frames: FrameRange) -> Frames<{FrameState::Unmapped}> { + let typ = if contains_any(&RESERVED_REGIONS.lock(), &frames) { + MemoryRegionType::Reserved + } else { + MemoryRegionType::Free + }; + Frames::new(typ, frames) +} + +impl Drop for Frames { + fn drop(&mut self) { + match S { + FrameState::Unmapped => { + if self.size_in_frames() == 0 { return; } + + let unmapped_frames: Frames<{FrameState::Unmapped}> = Frames { + typ: self.typ, + frames: self.frames.clone(), + }; + + let list = if unmapped_frames.typ == MemoryRegionType::Reserved { + &FREE_RESERVED_FRAMES_LIST + } else { + &FREE_GENERAL_FRAMES_LIST + }; -// // Here, we could optionally use above `_inserted_free_chunk` to merge the adjacent (contiguous) chunks -// // before or after the newly-inserted free chunk. -// // However, there's no *need* to do so until we actually run out of address space or until -// // a requested address is in a chunk that needs to be merged. -// // Thus, for performance, we save that for those future situations. -// } -// } - -// impl<'f> IntoIterator for &'f AllocatedFrames { -// type IntoIter = AllocatedFramesIter<'f>; -// type Item = AllocatedFrame<'f>; -// fn into_iter(self) -> Self::IntoIter { -// AllocatedFramesIter { -// _owner: self, -// range_iter: self.frames.iter(), -// } -// } -// } - -// /// An iterator over each [`AllocatedFrame`] in a range of [`AllocatedFrames`]. -// /// -// /// We must implement our own iterator type here in order to tie the lifetime `'f` -// /// of a returned `AllocatedFrame<'f>` type to the lifetime of its containing `AllocatedFrames`. -// /// This is because the underlying type of `AllocatedFrames` is a [`FrameRange`], -// /// which itself is a [`RangeInclusive`] of [`Frame`]s. -// /// Currently, the [`RangeInclusiveIterator`] type creates a clone of the original -// /// [`RangeInclusive`] instances rather than borrowing a reference to it. -// /// -// /// [`RangeInclusive`]: range_inclusive::RangeInclusive -// pub struct AllocatedFramesIter<'f> { -// _owner: &'f AllocatedFrames, -// range_iter: RangeInclusiveIterator, -// } -// impl<'f> Iterator for AllocatedFramesIter<'f> { -// type Item = AllocatedFrame<'f>; -// fn next(&mut self) -> Option { -// self.range_iter.next().map(|frame| -// AllocatedFrame { -// frame, _phantom: PhantomData, -// } -// ) -// } -// } - -// /// A reference to a single frame within a range of `AllocatedFrames`. -// /// -// /// The lifetime of this type is tied to the lifetime of its owning `AllocatedFrames`. -// #[derive(Debug)] -// pub struct AllocatedFrame<'f> { -// frame: Frame, -// _phantom: PhantomData<&'f Frame>, -// } -// impl<'f> Deref for AllocatedFrame<'f> { -// type Target = Frame; -// fn deref(&self) -> &Self::Target { -// &self.frame -// } -// } -// assert_not_impl_any!(AllocatedFrame: DerefMut, Clone); + // Simply add the newly-deallocated chunk to the free frames list. + let mut locked_list = list.lock(); + let res = locked_list.insert(unmapped_frames); + match res { + Ok(_inserted_free_chunk) => (), + Err(c) => error!("BUG: couldn't insert deallocated chunk {:?} into free frame list", c), + } + + // Here, we could optionally use above `_inserted_free_chunk` to merge the adjacent (contiguous) chunks + // before or after the newly-inserted free chunk. + // However, there's no *need* to do so until we actually run out of address space or until + // a requested address is in a chunk that needs to be merged. + // Thus, for performance, we save that for those future situations. + } + FrameState::Mapped => panic!("We should never drop a mapped frame! It should be forgotten instead."), + } + } +} + +impl<'f> IntoIterator for &'f Frames<{FrameState::Unmapped}> { + type IntoIter = UnmappedFramesIter<'f>; + type Item = UnmappedFrame<'f>; + fn into_iter(self) -> Self::IntoIter { + UnmappedFramesIter { + _owner: self, + range: self.frames.iter(), + } + } +} + +/// An iterator over each [`UnmappedFrame`] in a range of [`Frames<{FrameState::Unmapped}>`]. +/// +/// To Do (get Kevin's input): Description is no longer valid, since we have an iterator for RangeInclusive now. +/// but I still think it's useful to have a `Frames<{FrameState::Unmapped}>` iterator that ties the lifetime +/// of the `UnmappedFrame` to the original object. +/// +/// We must implement our own iterator type here in order to tie the lifetime `'f` +/// of a returned `UnmappedFrame<'f>` type to the lifetime of its containing `Frames<{FrameState::Unmapped}>`. +/// This is because the underlying type of `Frames<{FrameState::Unmapped}>` is a [`FrameRange`], +/// which itself is a [`core::ops::RangeInclusive`] of [`Frame`]s, and unfortunately the +/// `RangeInclusive` type doesn't implement an immutable iterator. +/// +/// Iterating through a `RangeInclusive` actually modifies its own internal range, +/// so we must avoid doing that because it would break the semantics of a `FrameRange`. +/// In fact, this is why [`FrameRange`] only implements `IntoIterator` but +/// does not implement [`Iterator`] itself. +pub struct UnmappedFramesIter<'f> { + _owner: &'f Frames<{FrameState::Unmapped}>, + range: range_inclusive::RangeInclusiveIterator, +} +impl<'f> Iterator for UnmappedFramesIter<'f> { + type Item = UnmappedFrame<'f>; + fn next(&mut self) -> Option { + self.range.next().map(|frame| + UnmappedFrame { + frame, _phantom: core::marker::PhantomData, + } + ) + } +} + +/// A reference to a single frame within a range of `Frames<{FrameState::Unmapped}>`. +/// +/// The lifetime of this type is tied to the lifetime of its owning `Frames<{FrameState::Unmapped}>`. +#[derive(Debug)] +pub struct UnmappedFrame<'f> { + frame: Frame, + _phantom: core::marker::PhantomData<&'f Frame>, +} +impl<'f> Deref for UnmappedFrame<'f> { + type Target = Frame; + fn deref(&self) -> &Self::Target { + &self.frame + } +} +assert_not_impl_any!(UnmappedFrame: DerefMut, Clone); + + +impl Frames { + #[allow(dead_code)] + pub(crate) fn frames(&self) -> FrameRange { + self.frames.clone() + } + + pub(crate) fn typ(&self) -> MemoryRegionType { + self.typ + } + + /// Returns a new `Frames` with an empty range of frames. + /// Can be used as a placeholder, but will not permit any real usage. + pub const fn empty() -> Frames { + Frames { + typ: MemoryRegionType::Unknown, + frames: FrameRange::empty(), + } + } + + /// Merges the given `Frames` object `other` into this `Frames` object (`self`). + /// This is just for convenience and usability purposes, it performs no allocation or remapping. + /// + /// The given `other` must be physically contiguous with `self`, i.e., come immediately before or after `self`. + /// That is, either `self.start == other.end + 1` or `self.end + 1 == other.start` must be true. + /// + /// If either of those conditions are met, `self` is modified and `Ok(())` is returned, + /// otherwise `Err(other)` is returned. + pub fn merge(&mut self, other: Self) -> Result<(), Self> { + if self.is_empty() || other.is_empty() { + return Err(other); + } + + if *self.start() == *other.end() + 1 { + // `other` comes contiguously before `self` + self.frames = FrameRange::new(*other.start(), *self.end()); + } + else if *self.end() + 1 == *other.start() { + // `self` comes contiguously before `other` + self.frames = FrameRange::new(*self.start(), *other.end()); + } + else { + // non-contiguous + return Err(other); + } + + // ensure the now-merged Frames doesn't run its drop handler + core::mem::forget(other); + Ok(()) + } + + /// Splits up the given `Frames` into multiple smaller `Frames`. + /// + /// Returns a tuple of three `Frames`: + /// 1. The `Frames` containing the requested range of frames starting at `start_frame`. + /// 2. The range of frames in the `self` that came before the beginning of the requested frame range. + /// 3. The range of frames in the `self` that came after the end of the requested frame range. + /// + /// If `start_frame` is not contained within `self` or `num_frames` results in an end frame greater than the end of `self`, + /// then `self` is not changed and we return (self, None, None). + pub fn split( + self, + start_frame: Frame, + num_frames: usize, + ) -> (Self, Option, Option) { + if (start_frame < *self.start()) || (start_frame + (num_frames - 1) > *self.end()) || (num_frames <= 0) { + return (self, None, None); + } + + let new_allocation = Frames{ frames: FrameRange::new(start_frame, start_frame + (num_frames - 1)), ..self }; + let before = if start_frame == MIN_FRAME || start_frame == *self.start() { + None + } else { + Some(Frames{ frames: FrameRange::new(*self.start(), *new_allocation.start() - 1), ..self }) + }; + + let after = if *new_allocation.end() == MAX_FRAME || *new_allocation.end() == *self.end(){ + None + } else { + Some(Frames{ frames: FrameRange::new(*new_allocation.end() + 1, *self.end()), ..self }) + }; + + core::mem::forget(self); + (new_allocation, before, after) + } + + /// Splits this `Frames` into two separate `Frames` objects: + /// * `[beginning : at_frame - 1]` + /// * `[at_frame : end]` + /// + /// This function follows the behavior of [`core::slice::split_at()`], + /// thus, either one of the returned `Frames` objects may be empty. + /// * If `at_frame == self.start`, the first returned `Frames` object will be empty. + /// * If `at_frame == self.end + 1`, the second returned `Frames` object will be empty. + /// + /// Returns an `Err` containing this `Frames` if `at_frame` is otherwise out of bounds, or if `self` was empty. + /// + /// [`core::slice::split_at()`]: https://doc.rust-lang.org/core/primitive.slice.html#method.split_at + pub fn split_at(self, at_frame: Frame) -> Result<(Self, Self), Self> { + if self.is_empty() { return Err(self); } + + let end_of_first = at_frame - 1; + + let (first, second) = if at_frame == *self.start() && at_frame <= *self.end() { + let first = FrameRange::empty(); + let second = FrameRange::new(at_frame, *self.end()); + (first, second) + } + else if at_frame == (*self.end() + 1) && end_of_first >= *self.start() { + let first = FrameRange::new(*self.start(), *self.end()); + let second = FrameRange::empty(); + (first, second) + } + else if at_frame > *self.start() && end_of_first <= *self.end() { + let first = FrameRange::new(*self.start(), end_of_first); + let second = FrameRange::new(at_frame, *self.end()); + (first, second) + } + else { + return Err(self); + }; + + let typ = self.typ; + // ensure the original Frames doesn't run its drop handler and free its frames. + core::mem::forget(self); + Ok(( + Frames { typ, frames: first }, + Frames { typ, frames: second }, + )) + } +} + +impl Deref for Frames { + type Target = FrameRange; + fn deref(&self) -> &FrameRange { + &self.frames + } +} +impl Ord for Frames { + fn cmp(&self, other: &Self) -> Ordering { + self.frames.start().cmp(other.frames.start()) + } +} +impl PartialOrd for Frames { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} +// To Do: will this be an issue as now this applies to Chunk as well as AllocatedFrames? +#[cfg(not(test))] +impl PartialEq for Frames { + fn eq(&self, other: &Self) -> bool { + self.frames.start() == other.frames.start() + } +} +#[cfg(test)] +impl PartialEq for Frames { + fn eq(&self, other: &Self) -> bool { + self.frames == other.frames + } +} +impl Borrow for &'_ Frames { + fn borrow(&self) -> &Frame { + self.frames.start() + } +} + +impl fmt::Debug for Frames { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "Frames({:?}, {:?})", self.typ, self.frames) + } +} /// A series of pending actions related to frame allocator bookkeeping, From bd0a7a5ba3ca9650ef8ac64607c62cbd51b2a42e Mon Sep 17 00:00:00 2001 From: ramla-i Date: Thu, 6 Jul 2023 14:48:13 -0400 Subject: [PATCH 05/40] removed frames file --- kernel/frame_allocator/src/frames.rs | 371 --------------------------- kernel/frame_allocator/src/lib.rs | 1 - 2 files changed, 372 deletions(-) delete mode 100644 kernel/frame_allocator/src/frames.rs diff --git a/kernel/frame_allocator/src/frames.rs b/kernel/frame_allocator/src/frames.rs deleted file mode 100644 index a33611f83c..0000000000 --- a/kernel/frame_allocator/src/frames.rs +++ /dev/null @@ -1,371 +0,0 @@ -//! A range of frames that are either mapped or unmapped. -//! A `Frames` object is uncloneable and is the only way to access the range of frames it references. - -use memory_structs::{FrameRange, Frame}; -use crate::{MemoryRegionType, contains_any, RESERVED_REGIONS, FREE_GENERAL_FRAMES_LIST, FREE_RESERVED_FRAMES_LIST, MIN_FRAME, MAX_FRAME}; -use core::{borrow::Borrow, cmp::Ordering, ops::{Deref, DerefMut}, fmt, marker::ConstParamTy}; -use static_assertions::assert_not_impl_any; -use log::error; - -pub type AllocatedFrames = Frames<{FrameState::Unmapped}>; -pub type AllocatedFrame<'f> = UnmappedFrame<'f>; - - -#[derive(PartialEq, Eq, ConstParamTy)] -pub enum FrameState { - Unmapped, - Mapped -} - -/// A range of contiguous frames. -/// Owning a `Frames` object gives ownership of the range of frames it references. -/// -/// The frames can be in an unmapped or mapped state. In the unmapped state, the frames are not -/// immediately accessible because they're not yet mapped by any virtual memory pages. -/// They are converted into a mapped state once they are used to create a `MappedPages` object. -/// -/// When a `Frames` object in an unmapped state is dropped, it is deallocated and returned to the free frames list. -/// We expect that `Frames` in a mapped state will never be dropped, but instead will be forgotten. -/// -/// # Ordering and Equality -/// -/// `Frames` implements the `Ord` trait, and its total ordering is ONLY based on -/// its **starting** `Frame`. This is useful so we can store `Frames` in a sorted collection. -/// -/// Similarly, `Frames` implements equality traits, `Eq` and `PartialEq`, -/// both of which are also based ONLY on the **starting** `Frame` of the `Frames`. -/// Thus, comparing two `Frames` with the `==` or `!=` operators may not work as expected. -/// since it ignores their actual range of frames. -#[derive(Eq)] -pub struct Frames { - /// The type of this memory chunk, e.g., whether it's in a free or reserved region. - typ: MemoryRegionType, - /// The Frames covered by this chunk, an inclusive range. - frames: FrameRange -} - -// Frames must not be Cloneable, and it must not expose its inner frames as mutable. -assert_not_impl_any!(Frames<{FrameState::Unmapped}>: DerefMut, Clone); -assert_not_impl_any!(Frames<{FrameState::Mapped}>: DerefMut, Clone); - - -impl Frames<{FrameState::Unmapped}> { - /// Creates a new `Frames` object in an unmapped state. - /// The frame allocator is reponsible for making sure no two `Frames` objects overlap. - pub(crate) fn new(typ: MemoryRegionType, frames: FrameRange) -> Self { - Frames { - typ, - frames, - } - } - - - /// Consumes the `Frames` in an unmapped state and converts them to `Frames` in a mapped state. - /// This should only be called once a `MappedPages` has been created from the `Frames`. - pub fn into_mapped_frames(self) -> Frames<{FrameState::Mapped}> { - let f = Frames { - typ: self.typ, - frames: self.frames.clone(), - }; - core::mem::forget(self); - f - } - - /// Returns an `UnmappedFrame` if this `Frames<{FrameState::Unmapped}>` object contains only one frame. - /// I've kept the terminology of allocated frame here to avoid changing code outside of this crate. - /// - /// ## Panic - /// Panics if this `UnmappedFrame` contains multiple frames or zero frames. - pub fn as_allocated_frame(&self) -> UnmappedFrame { - assert!(self.size_in_frames() == 1); - UnmappedFrame { - frame: *self.start(), - _phantom: core::marker::PhantomData, - } - } -} - - -/// This function is a callback used to convert `UnmappedFrames` into `Frames<{FrameState::Unmapped}>`. -/// `UnmappedFrames` represents frames that have been unmapped from a page that had -/// exclusively mapped them, indicating that no others pages have been mapped -/// to those same frames, and thus, they can be safely deallocated. -/// -/// This exists to break the cyclic dependency cycle between this crate and -/// the `page_table_entry` crate, since `page_table_entry` must depend on types -/// from this crate in order to enforce safety when modifying page table entries. -pub(crate) fn into_allocated_frames(frames: FrameRange) -> Frames<{FrameState::Unmapped}> { - let typ = if contains_any(&RESERVED_REGIONS.lock(), &frames) { - MemoryRegionType::Reserved - } else { - MemoryRegionType::Free - }; - Frames::new(typ, frames) -} - -impl Drop for Frames { - fn drop(&mut self) { - match S { - FrameState::Unmapped => { - if self.size_in_frames() == 0 { return; } - - let unmapped_frames: Frames<{FrameState::Unmapped}> = Frames { - typ: self.typ, - frames: self.frames.clone(), - }; - - let list = if unmapped_frames.typ == MemoryRegionType::Reserved { - &FREE_RESERVED_FRAMES_LIST - } else { - &FREE_GENERAL_FRAMES_LIST - }; - - // Simply add the newly-deallocated chunk to the free frames list. - let mut locked_list = list.lock(); - let res = locked_list.insert(unmapped_frames); - match res { - Ok(_inserted_free_chunk) => (), - Err(c) => error!("BUG: couldn't insert deallocated chunk {:?} into free frame list", c), - } - - // Here, we could optionally use above `_inserted_free_chunk` to merge the adjacent (contiguous) chunks - // before or after the newly-inserted free chunk. - // However, there's no *need* to do so until we actually run out of address space or until - // a requested address is in a chunk that needs to be merged. - // Thus, for performance, we save that for those future situations. - } - FrameState::Mapped => panic!("We should never drop a mapped frame! It should be forgotten instead."), - } - } -} - -impl<'f> IntoIterator for &'f Frames<{FrameState::Unmapped}> { - type IntoIter = UnmappedFramesIter<'f>; - type Item = UnmappedFrame<'f>; - fn into_iter(self) -> Self::IntoIter { - UnmappedFramesIter { - _owner: self, - range: self.frames.iter(), - } - } -} - -/// An iterator over each [`UnmappedFrame`] in a range of [`Frames<{FrameState::Unmapped}>`]. -/// -/// To Do (get Kevin's input): Description is no longer valid, since we have an iterator for RangeInclusive now. -/// but I still think it's useful to have a `Frames<{FrameState::Unmapped}>` iterator that ties the lifetime -/// of the `UnmappedFrame` to the original object. -/// -/// We must implement our own iterator type here in order to tie the lifetime `'f` -/// of a returned `UnmappedFrame<'f>` type to the lifetime of its containing `Frames<{FrameState::Unmapped}>`. -/// This is because the underlying type of `Frames<{FrameState::Unmapped}>` is a [`FrameRange`], -/// which itself is a [`core::ops::RangeInclusive`] of [`Frame`]s, and unfortunately the -/// `RangeInclusive` type doesn't implement an immutable iterator. -/// -/// Iterating through a `RangeInclusive` actually modifies its own internal range, -/// so we must avoid doing that because it would break the semantics of a `FrameRange`. -/// In fact, this is why [`FrameRange`] only implements `IntoIterator` but -/// does not implement [`Iterator`] itself. -pub struct UnmappedFramesIter<'f> { - _owner: &'f Frames<{FrameState::Unmapped}>, - range: range_inclusive::RangeInclusiveIterator, -} -impl<'f> Iterator for UnmappedFramesIter<'f> { - type Item = UnmappedFrame<'f>; - fn next(&mut self) -> Option { - self.range.next().map(|frame| - UnmappedFrame { - frame, _phantom: core::marker::PhantomData, - } - ) - } -} - -/// A reference to a single frame within a range of `Frames<{FrameState::Unmapped}>`. -/// -/// The lifetime of this type is tied to the lifetime of its owning `Frames<{FrameState::Unmapped}>`. -#[derive(Debug)] -pub struct UnmappedFrame<'f> { - frame: Frame, - _phantom: core::marker::PhantomData<&'f Frame>, -} -impl<'f> Deref for UnmappedFrame<'f> { - type Target = Frame; - fn deref(&self) -> &Self::Target { - &self.frame - } -} -assert_not_impl_any!(UnmappedFrame: DerefMut, Clone); - - -impl Frames { - #[allow(dead_code)] - pub(crate) fn frames(&self) -> FrameRange { - self.frames.clone() - } - - pub(crate) fn typ(&self) -> MemoryRegionType { - self.typ - } - - /// Returns a new `Frames` with an empty range of frames. - /// Can be used as a placeholder, but will not permit any real usage. - pub const fn empty() -> Frames { - Frames { - typ: MemoryRegionType::Unknown, - frames: FrameRange::empty(), - } - } - - /// Merges the given `Frames` object `other` into this `Frames` object (`self`). - /// This is just for convenience and usability purposes, it performs no allocation or remapping. - /// - /// The given `other` must be physically contiguous with `self`, i.e., come immediately before or after `self`. - /// That is, either `self.start == other.end + 1` or `self.end + 1 == other.start` must be true. - /// - /// If either of those conditions are met, `self` is modified and `Ok(())` is returned, - /// otherwise `Err(other)` is returned. - pub fn merge(&mut self, other: Self) -> Result<(), Self> { - if self.is_empty() || other.is_empty() { - return Err(other); - } - - if *self.start() == *other.end() + 1 { - // `other` comes contiguously before `self` - self.frames = FrameRange::new(*other.start(), *self.end()); - } - else if *self.end() + 1 == *other.start() { - // `self` comes contiguously before `other` - self.frames = FrameRange::new(*self.start(), *other.end()); - } - else { - // non-contiguous - return Err(other); - } - - // ensure the now-merged Frames doesn't run its drop handler - core::mem::forget(other); - Ok(()) - } - - /// Splits up the given `Frames` into multiple smaller `Frames`. - /// - /// Returns a tuple of three `Frames`: - /// 1. The `Frames` containing the requested range of frames starting at `start_frame`. - /// 2. The range of frames in the `self` that came before the beginning of the requested frame range. - /// 3. The range of frames in the `self` that came after the end of the requested frame range. - /// - /// If `start_frame` is not contained within `self` or `num_frames` results in an end frame greater than the end of `self`, - /// then `self` is not changed and we return (self, None, None). - pub fn split( - self, - start_frame: Frame, - num_frames: usize, - ) -> (Self, Option, Option) { - if (start_frame < *self.start()) || (start_frame + (num_frames - 1) > *self.end()) || (num_frames <= 0) { - return (self, None, None); - } - - let new_allocation = Frames{ frames: FrameRange::new(start_frame, start_frame + (num_frames - 1)), ..self }; - let before = if start_frame == MIN_FRAME || start_frame == *self.start() { - None - } else { - Some(Frames{ frames: FrameRange::new(*self.start(), *new_allocation.start() - 1), ..self }) - }; - - let after = if *new_allocation.end() == MAX_FRAME || *new_allocation.end() == *self.end(){ - None - } else { - Some(Frames{ frames: FrameRange::new(*new_allocation.end() + 1, *self.end()), ..self }) - }; - - core::mem::forget(self); - (new_allocation, before, after) - } - - /// Splits this `Frames` into two separate `Frames` objects: - /// * `[beginning : at_frame - 1]` - /// * `[at_frame : end]` - /// - /// This function follows the behavior of [`core::slice::split_at()`], - /// thus, either one of the returned `Frames` objects may be empty. - /// * If `at_frame == self.start`, the first returned `Frames` object will be empty. - /// * If `at_frame == self.end + 1`, the second returned `Frames` object will be empty. - /// - /// Returns an `Err` containing this `Frames` if `at_frame` is otherwise out of bounds, or if `self` was empty. - /// - /// [`core::slice::split_at()`]: https://doc.rust-lang.org/core/primitive.slice.html#method.split_at - pub fn split_at(self, at_frame: Frame) -> Result<(Self, Self), Self> { - if self.is_empty() { return Err(self); } - - let end_of_first = at_frame - 1; - - let (first, second) = if at_frame == *self.start() && at_frame <= *self.end() { - let first = FrameRange::empty(); - let second = FrameRange::new(at_frame, *self.end()); - (first, second) - } - else if at_frame == (*self.end() + 1) && end_of_first >= *self.start() { - let first = FrameRange::new(*self.start(), *self.end()); - let second = FrameRange::empty(); - (first, second) - } - else if at_frame > *self.start() && end_of_first <= *self.end() { - let first = FrameRange::new(*self.start(), end_of_first); - let second = FrameRange::new(at_frame, *self.end()); - (first, second) - } - else { - return Err(self); - }; - - let typ = self.typ; - // ensure the original Frames doesn't run its drop handler and free its frames. - core::mem::forget(self); - Ok(( - Frames { typ, frames: first }, - Frames { typ, frames: second }, - )) - } -} - -impl Deref for Frames { - type Target = FrameRange; - fn deref(&self) -> &FrameRange { - &self.frames - } -} -impl Ord for Frames { - fn cmp(&self, other: &Self) -> Ordering { - self.frames.start().cmp(other.frames.start()) - } -} -impl PartialOrd for Frames { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} -// To Do: will this be an issue as now this applies to Chunk as well as AllocatedFrames? -#[cfg(not(test))] -impl PartialEq for Frames { - fn eq(&self, other: &Self) -> bool { - self.frames.start() == other.frames.start() - } -} -#[cfg(test)] -impl PartialEq for Frames { - fn eq(&self, other: &Self) -> bool { - self.frames == other.frames - } -} -impl Borrow for &'_ Frames { - fn borrow(&self) -> &Frame { - self.frames.start() - } -} - -impl fmt::Debug for Frames { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "Frames({:?}, {:?})", self.typ, self.frames) - } -} diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index 755c6fa5df..a4a8656fdf 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -29,7 +29,6 @@ mod test; mod static_array_rb_tree; // mod static_array_linked_list; -// mod frames; use core::{borrow::Borrow, cmp::{Ordering, min, max}, ops::{Deref, DerefMut}, fmt, marker::ConstParamTy}; use intrusive_collections::Bound; From dbcf46234eb1cafa56c43ef618778681065be1f1 Mon Sep 17 00:00:00 2001 From: ramla-i Date: Thu, 6 Jul 2023 14:58:20 -0400 Subject: [PATCH 06/40] comments --- kernel/frame_allocator/src/lib.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index a4a8656fdf..78827aa1d4 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -786,7 +786,7 @@ impl From for &'static str { AllocationError::OutOfAddressSpace(..) => "out of physical address space", AllocationError::ContiguousChunkNotFound(..) => "only some of the requested frames were available", AllocationError::ChunkRemovalFailed => "Failed to remove a Chunk from the free list, this is most likely due to some logical error", - AllocationError::ChunkOperationFailed => "Could not merge or split a Chunk", + AllocationError::ChunkOperationFailed => "Could not merge or split a Chunk. This indicated a bug in the frame allocator.", } } } @@ -861,8 +861,9 @@ fn find_specific_chunk( }; if let Some(next_chunk) = next_contiguous_chunk { // We found a suitable chunk that came contiguously after the initial too-small chunk. - // We would like to merge it into the initial chunk (since we have a cursor pointing to it already), - // but we can't get a mutable reference to the element the cursor is pointing to. + // We would like to merge it into the initial chunk with just the reference (since we have a cursor pointing to it already), + // but we can't get a mutable reference to the element the cursor is pointing to. + // So both chunks will be removed and then merged. return merge_contiguous_chunks_and_allocate(requested_frame, num_frames, ValueRefMut::RBTree(cursor_mut), next_chunk); } } @@ -935,10 +936,10 @@ fn retrieve_frames_from_ref(mut frames_ref: ValueRefMut>, next_chunk: Frames<{FrameState::Unmapped}>, ) -> Result<(AllocatedFrames, DeferredAllocAction<'static>), AllocationError> { - // Remove the chosen chunk from the free frame list. + // Remove the initial chunk from the free frame list. let mut chosen_chunk = retrieve_frames_from_ref(initial_chunk_ref).ok_or(AllocationError::ChunkRemovalFailed)?; // This should always succeed, since we've already checked the conditions for a merge // We should return the next_chunk back to the list, but a failure at this point implies a bug in the frame allocator. @@ -1073,7 +1074,8 @@ fn add_reserved_region_to_frames_list( Ok(frames) } -/// Adds the given `frames` to the given `list` as a Chunk of reserved frames. +/// ToDo: combine both functions for adding reserved regions using a trait. +/// Adds the given `frames` to the given `list` as a Region of reserved frames. /// /// Returns the range of **new** frames that were added to the list, /// which will be a subset of the given input `frames`. From 52ce8c30e202e9e10423ddcb5eb5a52868e87c3c Mon Sep 17 00:00:00 2001 From: ramla-i Date: Thu, 6 Jul 2023 15:09:06 -0400 Subject: [PATCH 07/40] comments --- kernel/frame_allocator/src/test.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/kernel/frame_allocator/src/test.rs b/kernel/frame_allocator/src/test.rs index 14a1629352..f1ff589e95 100644 --- a/kernel/frame_allocator/src/test.rs +++ b/kernel/frame_allocator/src/test.rs @@ -6,12 +6,6 @@ use self::std::dbg; use super::*; -// impl PartialEq for AllocatedFrames { -// fn eq(&self, other: &Self) -> bool { -// self.frames() == other.frames() -// } -// } - fn from_addr(start_addr: usize, end_addr: usize) -> AllocatedFrames { AllocatedFrames::new( MemoryRegionType::Free, From e5ec8f5991708198d3d95ffd117c2ed2949b03f3 Mon Sep 17 00:00:00 2001 From: ramla-i Date: Thu, 6 Jul 2023 15:15:01 -0400 Subject: [PATCH 08/40] comments and clippy errors --- kernel/frame_allocator/src/lib.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index 78827aa1d4..851b4cfee1 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -591,7 +591,7 @@ impl Frames { start_frame: Frame, num_frames: usize, ) -> (Self, Option, Option) { - if (start_frame < *self.start()) || (start_frame + (num_frames - 1) > *self.end()) || (num_frames <= 0) { + if (start_frame < *self.start()) || (start_frame + (num_frames - 1) > *self.end()) || (num_frames == 0) { return (self, None, None); } @@ -1074,7 +1074,6 @@ fn add_reserved_region_to_frames_list( Ok(frames) } -/// ToDo: combine both functions for adding reserved regions using a trait. /// Adds the given `frames` to the given `list` as a Region of reserved frames. /// /// Returns the range of **new** frames that were added to the list, @@ -1083,6 +1082,8 @@ fn add_reserved_region_to_frames_list( /// Currently, this function adds no new frames at all if any frames within the given `frames` list /// overlap any existing regions at all. /// TODO: handle partially-overlapping regions by extending existing regions on either end. +/// +/// TODO: combine both functions for adding reserved regions/ frames using a trait OR make one function which adds to both lists, since they are always called together. fn add_reserved_region_to_regions_list( list: &mut StaticArrayRBTree, frames: FrameRange, From 2c0e482eecba99e55b610679c2b54fd9a0a206db Mon Sep 17 00:00:00 2001 From: ramla-i Date: Mon, 17 Jul 2023 16:09:50 -0400 Subject: [PATCH 09/40] pr changes --- kernel/frame_allocator/src/lib.rs | 36 +++++++----------- kernel/frame_allocator/src/test.rs | 60 ++++++++++++++++++++---------- 2 files changed, 54 insertions(+), 42 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index 851b4cfee1..1e00b3463f 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -588,9 +588,11 @@ impl Frames { /// then `self` is not changed and we return (self, None, None). pub fn split( self, - start_frame: Frame, - num_frames: usize, + frames: FrameRange ) -> (Self, Option, Option) { + let start_frame = *frames.start(); + let num_frames = frames.size_in_frames(); + if (start_frame < *self.start()) || (start_frame + (num_frames - 1) > *self.end()) || (num_frames == 0) { return (self, None, None); } @@ -674,19 +676,11 @@ impl PartialOrd for Frames { Some(self.cmp(other)) } } -// To Do: will this be an issue as now this applies to Chunk as well as AllocatedFrames? -#[cfg(not(test))] impl PartialEq for Frames { fn eq(&self, other: &Self) -> bool { self.frames.start() == other.frames.start() } } -#[cfg(test)] -impl PartialEq for Frames { - fn eq(&self, other: &Self) -> bool { - self.frames == other.frames - } -} impl Borrow for &'_ Frames { fn borrow(&self) -> &Frame { self.frames.start() @@ -809,7 +803,7 @@ fn find_specific_chunk( if let Some(chunk) = elem { if requested_frame >= *chunk.start() && requested_end_frame <= *chunk.end() { // Here: `chunk` was big enough and did contain the requested address. - return allocate_from_chosen_chunk(requested_frame, num_frames, ValueRefMut::Array(elem)); + return allocate_from_chosen_chunk(FrameRange::new(requested_frame, requested_frame + num_frames - 1), ValueRefMut::Array(elem)); } } } @@ -819,7 +813,7 @@ fn find_specific_chunk( if let Some(chunk) = cursor_mut.get().map(|w| w.deref().deref().clone()) { if chunk.contains(&requested_frame) { if requested_end_frame <= *chunk.end() { - return allocate_from_chosen_chunk(requested_frame, num_frames, ValueRefMut::RBTree(cursor_mut)); + return allocate_from_chosen_chunk(FrameRange::new(requested_frame, requested_frame + num_frames - 1), ValueRefMut::RBTree(cursor_mut)); } else { // We found the chunk containing the requested address, but it was too small to cover all of the requested frames. // Let's try to merge the next-highest contiguous chunk to see if those two chunks together @@ -864,7 +858,7 @@ fn find_specific_chunk( // We would like to merge it into the initial chunk with just the reference (since we have a cursor pointing to it already), // but we can't get a mutable reference to the element the cursor is pointing to. // So both chunks will be removed and then merged. - return merge_contiguous_chunks_and_allocate(requested_frame, num_frames, ValueRefMut::RBTree(cursor_mut), next_chunk); + return merge_contiguous_chunks_and_allocate(FrameRange::new(requested_frame, requested_frame + num_frames -1), ValueRefMut::RBTree(cursor_mut), next_chunk); } } } @@ -891,7 +885,7 @@ fn find_any_chunk( continue; } else { - return allocate_from_chosen_chunk(*chunk.start(), num_frames, ValueRefMut::Array(elem)); + return allocate_from_chosen_chunk(FrameRange::new(*chunk.start(), *chunk.start() + num_frames - 1), ValueRefMut::Array(elem)); } } } @@ -903,7 +897,7 @@ fn find_any_chunk( let mut cursor = tree.upper_bound_mut(Bound::<&Frames<{FrameState::Unmapped}>>::Unbounded); while let Some(chunk) = cursor.get().map(|w| w.deref()) { if num_frames <= chunk.size_in_frames() && chunk.typ() == MemoryRegionType::Free { - return allocate_from_chosen_chunk(*chunk.start(), num_frames, ValueRefMut::RBTree(cursor)); + return allocate_from_chosen_chunk(FrameRange::new(*chunk.start(), *chunk.start() + num_frames - 1), ValueRefMut::RBTree(cursor)); } warn!("Frame allocator: inefficient scenario: had to search multiple chunks \ (skipping {:?}) while trying to allocate {} frames at any address.", @@ -942,8 +936,7 @@ fn retrieve_frames_from_ref(mut frames_ref: ValueRefMut>, next_chunk: Frames<{FrameState::Unmapped}>, ) -> Result<(AllocatedFrames, DeferredAllocAction<'static>), AllocationError> { @@ -952,7 +945,7 @@ fn merge_contiguous_chunks_and_allocate( // This should always succeed, since we've already checked the conditions for a merge // We should return the next_chunk back to the list, but a failure at this point implies a bug in the frame allocator. chosen_chunk.merge(next_chunk).map_err(|_| AllocationError::ChunkOperationFailed)?; - let (new_allocation, before, after) = chosen_chunk.split(start_frame, num_frames); + let (new_allocation, before, after) = chosen_chunk.split(frames_to_allocate); // TODO: Re-use the allocated wrapper if possible, rather than allocate a new one entirely. // if let RemovedValue::RBTree(Some(wrapper_adapter)) = _removed_chunk { ... } @@ -968,15 +961,14 @@ fn merge_contiguous_chunks_and_allocate( /// into multiple smaller chunks, thereby "allocating" frames from it. /// /// This function breaks up that chunk into multiple ones and returns an `AllocatedFrames` -/// from (part of) that chunk, ranging from `start_frame` to `start_frame + num_frames`. +/// from (part of) that chunk, ranging in the frames represented by `frames_to_allocate`. fn allocate_from_chosen_chunk( - start_frame: Frame, - num_frames: usize, + frames_to_allocate: FrameRange, chosen_chunk_ref: ValueRefMut>, ) -> Result<(AllocatedFrames, DeferredAllocAction<'static>), AllocationError> { // Remove the chosen chunk from the free frame list. let chosen_chunk = retrieve_frames_from_ref(chosen_chunk_ref).ok_or(AllocationError::ChunkRemovalFailed)?; - let (new_allocation, before, after) = chosen_chunk.split(start_frame, num_frames); + let (new_allocation, before, after) = chosen_chunk.split(frames_to_allocate); // TODO: Re-use the allocated wrapper if possible, rather than allocate a new one entirely. // if let RemovedValue::RBTree(Some(wrapper_adapter)) = _removed_chunk { ... } diff --git a/kernel/frame_allocator/src/test.rs b/kernel/frame_allocator/src/test.rs index f1ff589e95..9cadaa7b79 100644 --- a/kernel/frame_allocator/src/test.rs +++ b/kernel/frame_allocator/src/test.rs @@ -40,8 +40,10 @@ fn split_at_beginning() { let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); - assert_eq!(result1, first); - assert_eq!(result2, second); + assert_eq!(result1.start(), first.start()); + assert_eq!(result1.end(), first.end()); + assert_eq!(result2.start(), second.start()); + assert_eq!(result2.end(), second.end()); } @@ -55,8 +57,10 @@ fn split_at_middle() { let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); - assert_eq!(result1, first); - assert_eq!(result2, second); + assert_eq!(result1.start(), first.start()); + assert_eq!(result1.end(), first.end()); + assert_eq!(result2.start(), second.start()); + assert_eq!(result2.end(), second.end()); } #[test] @@ -69,8 +73,10 @@ fn split_at_end() { let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); - assert_eq!(result1, first); - assert_eq!(result2, second); + assert_eq!(result1.start(), first.start()); + assert_eq!(result1.end(), first.end()); + assert_eq!(result2.start(), second.start()); + assert_eq!(result2.end(), second.end()); } @@ -84,8 +90,10 @@ fn split_after_end() { let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); - assert_eq!(result1, first); - assert_eq!(result2, second); + assert_eq!(result1.start(), first.start()); + assert_eq!(result1.end(), first.end()); + assert_eq!(result2.start(), second.start()); + assert_eq!(result2.end(), second.end()); } @@ -131,8 +139,10 @@ fn split_at_beginning_zero() { let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); - assert_eq!(result1, first); - assert_eq!(result2, second); + assert_eq!(result1.start(), first.start()); + assert_eq!(result1.end(), first.end()); + assert_eq!(result2.start(), second.start()); + assert_eq!(result2.end(), second.end()); } #[test] @@ -145,8 +155,10 @@ fn split_at_beginning_one() { let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); - assert_eq!(result1, first); - assert_eq!(result2, second); + assert_eq!(result1.start(), first.start()); + assert_eq!(result1.end(), first.end()); + assert_eq!(result2.start(), second.start()); + assert_eq!(result2.end(), second.end()); } #[test] @@ -159,8 +171,10 @@ fn split_at_beginning_max_length_one() { let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); - assert_eq!(result1, first); - assert_eq!(result2, second); + assert_eq!(result1.start(), first.start()); + assert_eq!(result1.end(), first.end()); + assert_eq!(result2.start(), second.start()); + assert_eq!(result2.end(), second.end()); } #[test] @@ -173,8 +187,10 @@ fn split_at_end_max_length_two() { let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); - assert_eq!(result1, first); - assert_eq!(result2, second); + assert_eq!(result1.start(), first.start()); + assert_eq!(result1.end(), first.end()); + assert_eq!(result2.start(), second.start()); + assert_eq!(result2.end(), second.end()); } @@ -188,8 +204,10 @@ fn split_after_end_max() { let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); - assert_eq!(result1, first); - assert_eq!(result2, second); + assert_eq!(result1.start(), first.start()); + assert_eq!(result1.end(), first.end()); + assert_eq!(result2.start(), second.start()); + assert_eq!(result2.end(), second.end()); } #[test] @@ -202,6 +220,8 @@ fn split_at_beginning_max() { let result = original.split_at(split_at); dbg!(&result); let (result1, result2) = result.unwrap(); - assert_eq!(result1, first); - assert_eq!(result2, second); + assert_eq!(result1.start(), first.start()); + assert_eq!(result1.end(), first.end()); + assert_eq!(result2.start(), second.start()); + assert_eq!(result2.end(), second.end()); } From c57aa169a28f92262bc721053f36d3ff971d4c71 Mon Sep 17 00:00:00 2001 From: ramla-i Date: Tue, 18 Jul 2023 18:24:13 -0400 Subject: [PATCH 10/40] PR issues --- kernel/frame_allocator/src/lib.rs | 348 ++++++++++++++---------------- 1 file changed, 164 insertions(+), 184 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index 1e00b3463f..5d01e6edca 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -30,7 +30,7 @@ mod test; mod static_array_rb_tree; // mod static_array_linked_list; -use core::{borrow::Borrow, cmp::{Ordering, min, max}, ops::{Deref, DerefMut}, fmt, marker::ConstParamTy}; +use core::{borrow::Borrow, cmp::{Ordering, min, max}, ops::{Deref, DerefMut}, fmt, marker::ConstParamTy, slice::Split}; use intrusive_collections::Bound; use kernel_config::memory::*; use log::{error, warn, debug, trace}; @@ -46,18 +46,18 @@ const MAX_FRAME: Frame = Frame::containing_address(PhysicalAddress::new_canonica // Note: we keep separate lists for "free, general-purpose" areas and "reserved" areas, as it's much faster. /// The single, system-wide list of free physical memory frames available for general usage. -static FREE_GENERAL_FRAMES_LIST: Mutex>> = Mutex::new(StaticArrayRBTree::empty()); +static FREE_GENERAL_FRAMES_LIST: Mutex> = Mutex::new(StaticArrayRBTree::empty()); /// The single, system-wide list of free physical memory frames reserved for specific usage. -static FREE_RESERVED_FRAMES_LIST: Mutex>> = Mutex::new(StaticArrayRBTree::empty()); +static FREE_RESERVED_FRAMES_LIST: Mutex> = Mutex::new(StaticArrayRBTree::empty()); /// The fixed list of all known regions that are available for general use. /// This does not indicate whether these regions are currently allocated, /// rather just where they exist and which regions are known to this allocator. -static GENERAL_REGIONS: Mutex> = Mutex::new(StaticArrayRBTree::empty()); +static GENERAL_REGIONS: Mutex> = Mutex::new(StaticArrayRBTree::empty()); /// The fixed list of all known regions that are reserved for specific purposes. /// This does not indicate whether these regions are currently allocated, /// rather just where they exist and which regions are known to this allocator. -static RESERVED_REGIONS: Mutex> = Mutex::new(StaticArrayRBTree::empty()); +static RESERVED_REGIONS: Mutex> = Mutex::new(StaticArrayRBTree::empty()); /// Initialize the frame allocator with the given list of available and reserved physical memory regions. @@ -89,7 +89,7 @@ pub fn init( return Err("BUG: Frame allocator was already initialized, cannot be initialized twice."); } - let mut free_list: [Option; 32] = Default::default(); + let mut free_list: [Option; 32] = Default::default(); let mut free_list_idx = 0; // Populate the list of free regions for general-purpose usage. @@ -105,9 +105,9 @@ pub fn init( } - let mut reserved_list: [Option; 32] = Default::default(); + let mut reserved_list: [Option; 32] = Default::default(); for (i, area) in reserved_physical_memory_areas.into_iter().enumerate() { - reserved_list[i] = Some(Region { + reserved_list[i] = Some(PhysicalMemoryRegion { typ: MemoryRegionType::Reserved, frames: area.borrow().frames.clone(), }); @@ -115,7 +115,7 @@ pub fn init( let mut changed = true; while changed { - let mut temp_reserved_list: [Option; 32] = Default::default(); + let mut temp_reserved_list: [Option; 32] = Default::default(); changed = false; let mut temp_reserved_list_idx = 0; @@ -158,8 +158,8 @@ pub fn init( } // Here, since we're sure we now have a list of regions that don't overlap, we can create lists of Frames objects. - let mut free_list_w_frames: [Option>; 32] = Default::default(); - let mut reserved_list_w_frames: [Option>; 32] = Default::default(); + let mut free_list_w_frames: [Option; 32] = Default::default(); + let mut reserved_list_w_frames: [Option; 32] = Default::default(); for (i, elem) in reserved_list.iter().flatten().enumerate() { reserved_list_w_frames[i] = Some(Frames::new( MemoryRegionType::Reserved, @@ -190,7 +190,7 @@ pub fn init( /// the given list of `reserved_physical_memory_areas`. fn check_and_add_free_region( area: &FrameRange, - free_list: &mut [Option; 32], + free_list: &mut [Option; 32], free_list_idx: &mut usize, reserved_physical_memory_areas: R, ) @@ -236,7 +236,7 @@ fn check_and_add_free_region( let new_area = FrameRange::new(current_start, current_end); if new_area.size_in_frames() > 0 { - free_list[*free_list_idx] = Some(Region { + free_list[*free_list_idx] = Some(PhysicalMemoryRegion { typ: MemoryRegionType::Free, frames: new_area, }); @@ -245,104 +245,89 @@ fn check_and_add_free_region( } -/// A region of physical memory. -#[derive(Clone, Debug)] +/// `PhysicalMemoryRegion` represents a range of contiguous frames in phsical memory for bookkeeping purposes. +/// It does not give access to the underlying frames. +/// +/// # Ordering and Equality +/// +/// `PhysicalMemoryRegion` implements the `Ord` trait, and its total ordering is ONLY based on +/// its **starting** `Frame`. This is useful so we can store `PhysicalMemoryRegion`s in a sorted collection. +/// +/// Similarly, `PhysicalMemoryRegion` implements equality traits, `Eq` and `PartialEq`, +/// both of which are also based ONLY on the **starting** `Frame` of the `PhysicalMemoryRegion`. +/// Thus, comparing two `PhysicalMemoryRegion`s with the `==` or `!=` operators may not work as expected. +/// since it ignores their actual range of frames. +#[derive(Clone, Debug, Eq)] pub struct PhysicalMemoryRegion { + /// The Frames covered by this region, an inclusive range. pub frames: FrameRange, + /// The type of this memory region, e.g., whether it's in a free or reserved region. pub typ: MemoryRegionType, } impl PhysicalMemoryRegion { pub fn new(frames: FrameRange, typ: MemoryRegionType) -> PhysicalMemoryRegion { PhysicalMemoryRegion { frames, typ } } -} -impl Deref for PhysicalMemoryRegion { - type Target = FrameRange; - fn deref(&self) -> &FrameRange { - &self.frames - } -} - -/// Types of physical memory. See each variant's documentation. -#[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub enum MemoryRegionType { - /// Memory that is available for any general purpose. - Free, - /// Memory that is reserved for special use and is only ever allocated from if specifically requested. - /// This includes custom memory regions added by third parties, e.g., - /// device memory discovered and added by device drivers later during runtime. - Reserved, - /// Memory of an unknown type. - /// This is a default value that acts as a sanity check, because it is invalid - /// to do any real work (e.g., allocation, access) with an unknown memory region. - Unknown, -} -/// `Region` represents a range of contiguous frames for bookkeeping purposes. -/// It does not give access to the underlying frames. -/// -/// # Ordering and Equality -/// -/// `Region` implements the `Ord` trait, and its total ordering is ONLY based on -/// its **starting** `Frame`. This is useful so we can store `Region`s in a sorted collection. -/// -/// Similarly, `Region` implements equality traits, `Eq` and `PartialEq`, -/// both of which are also based ONLY on the **starting** `Frame` of the `Region`. -/// Thus, comparing two `Region`s with the `==` or `!=` operators may not work as expected. -/// since it ignores their actual range of frames. -#[derive(Debug, Clone, Eq)] -struct Region { - /// The type of this memory region, e.g., whether it's in a free or reserved region. - #[allow(unused)] - typ: MemoryRegionType, - /// The Frames covered by this region, an inclusive range. - frames: FrameRange, -} -impl Region { - /// Returns a new `Region` with an empty range of frames. + /// Returns a new `PhysicalMemoryRegion` with an empty range of frames. #[allow(unused)] - const fn empty() -> Region { - Region { + const fn empty() -> PhysicalMemoryRegion { + PhysicalMemoryRegion { typ: MemoryRegionType::Unknown, frames: FrameRange::empty(), } } } -impl Deref for Region { +impl Deref for PhysicalMemoryRegion { type Target = FrameRange; fn deref(&self) -> &FrameRange { &self.frames } } -impl Ord for Region { +impl Ord for PhysicalMemoryRegion { fn cmp(&self, other: &Self) -> Ordering { self.frames.start().cmp(other.frames.start()) } } -impl PartialOrd for Region { +impl PartialOrd for PhysicalMemoryRegion { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } } -impl PartialEq for Region { +impl PartialEq for PhysicalMemoryRegion { fn eq(&self, other: &Self) -> bool { self.frames.start() == other.frames.start() } } -impl Borrow for &'_ Region { +impl Borrow for &'_ PhysicalMemoryRegion { fn borrow(&self) -> &Frame { self.frames.start() } } +/// Types of physical memory. See each variant's documentation. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum MemoryRegionType { + /// Memory that is available for any general purpose. + Free, + /// Memory that is reserved for special use and is only ever allocated from if specifically requested. + /// This includes custom memory regions added by third parties, e.g., + /// device memory discovered and added by device drivers later during runtime. + Reserved, + /// Memory of an unknown type. + /// This is a default value that acts as a sanity check, because it is invalid + /// to do any real work (e.g., allocation, access) with an unknown memory region. + Unknown, +} -pub type AllocatedFrames = Frames<{FrameState::Unmapped}>; -pub type AllocatedFrame<'f> = UnmappedFrame<'f>; - +pub type FreeFrames = Frames<{FrameState::Free}>; +pub type AllocatedFrames = Frames<{FrameState::Allocated}>; +pub type MappedFrames = Frames<{FrameState::Mapped}>; #[derive(PartialEq, Eq, ConstParamTy)] pub enum FrameState { - Unmapped, + Free, + Allocated, Mapped } @@ -374,12 +359,13 @@ pub struct Frames { } // Frames must not be Cloneable, and it must not expose its inner frames as mutable. -assert_not_impl_any!(Frames<{FrameState::Unmapped}>: DerefMut, Clone); +assert_not_impl_any!(Frames<{FrameState::Free}>: DerefMut, Clone); +assert_not_impl_any!(Frames<{FrameState::Allocated}>: DerefMut, Clone); assert_not_impl_any!(Frames<{FrameState::Mapped}>: DerefMut, Clone); -impl Frames<{FrameState::Unmapped}> { - /// Creates a new `Frames` object in an unmapped state. +impl FreeFrames { + /// Creates a new `Frames` object in an free state. /// The frame allocator is reponsible for making sure no two `Frames` objects overlap. pub(crate) fn new(typ: MemoryRegionType, frames: FrameRange) -> Self { Frames { @@ -388,10 +374,21 @@ impl Frames<{FrameState::Unmapped}> { } } + /// Consumes the `Frames` in an free state and converts them to `Frames` in a allocated state. + pub fn into_allocated_frames(self) -> AllocatedFrames { + let f = Frames { + typ: self.typ, + frames: self.frames.clone(), + }; + core::mem::forget(self); + f + } +} - /// Consumes the `Frames` in an unmapped state and converts them to `Frames` in a mapped state. +impl AllocatedFrames { + /// Consumes the `Frames` in an allocated state and converts them to `Frames` in a mapped state. /// This should only be called once a `MappedPages` has been created from the `Frames`. - pub fn into_mapped_frames(self) -> Frames<{FrameState::Mapped}> { + pub fn into_mapped_frames(self) -> MappedFrames { let f = Frames { typ: self.typ, frames: self.frames.clone(), @@ -400,14 +397,13 @@ impl Frames<{FrameState::Unmapped}> { f } - /// Returns an `UnmappedFrame` if this `Frames<{FrameState::Unmapped}>` object contains only one frame. - /// I've kept the terminology of allocated frame here to avoid changing code outside of this crate. + /// Returns an `AllocatedFrame` if this `AllocatedFrames` object contains only one frame. /// /// ## Panic - /// Panics if this `UnmappedFrame` contains multiple frames or zero frames. - pub fn as_allocated_frame(&self) -> UnmappedFrame { + /// Panics if this `AllocatedFrame` contains multiple frames or zero frames. + pub fn as_allocated_frame(&self) -> AllocatedFrame { assert!(self.size_in_frames() == 1); - UnmappedFrame { + AllocatedFrame { frame: *self.start(), _phantom: core::marker::PhantomData, } @@ -415,30 +411,30 @@ impl Frames<{FrameState::Unmapped}> { } -/// This function is a callback used to convert `UnmappedFrames` into `Frames<{FrameState::Unmapped}>`. -/// `UnmappedFrames` represents frames that have been unmapped from a page that had +/// This function is a callback used to convert `UnmappedFrames` into `AllocatedFrames`. +/// `AllocatedFrames` represents frames that have been unmapped from a page that had /// exclusively mapped them, indicating that no others pages have been mapped /// to those same frames, and thus, they can be safely deallocated. /// /// This exists to break the cyclic dependency cycle between this crate and /// the `page_table_entry` crate, since `page_table_entry` must depend on types /// from this crate in order to enforce safety when modifying page table entries. -pub(crate) fn into_allocated_frames(frames: FrameRange) -> Frames<{FrameState::Unmapped}> { +pub(crate) fn into_allocated_frames(frames: FrameRange) -> AllocatedFrames { let typ = if contains_any(&RESERVED_REGIONS.lock(), &frames) { MemoryRegionType::Reserved } else { MemoryRegionType::Free }; - Frames::new(typ, frames) + Frames{ typ, frames} } impl Drop for Frames { fn drop(&mut self) { match S { - FrameState::Unmapped => { + FrameState::Free => { if self.size_in_frames() == 0 { return; } - let unmapped_frames: Frames<{FrameState::Unmapped}> = Frames { + let unmapped_frames: FreeFrames = Frames { typ: self.typ, frames: self.frames.clone(), }; @@ -462,70 +458,76 @@ impl Drop for Frames { // However, there's no *need* to do so until we actually run out of address space or until // a requested address is in a chunk that needs to be merged. // Thus, for performance, we save that for those future situations. - } + }, + FrameState::Allocated => { FreeFrames::new(self.typ, self.frames.clone()); }, //ToDo: Check drop handler correctness FrameState::Mapped => panic!("We should never drop a mapped frame! It should be forgotten instead."), } } } -impl<'f> IntoIterator for &'f Frames<{FrameState::Unmapped}> { - type IntoIter = UnmappedFramesIter<'f>; - type Item = UnmappedFrame<'f>; +impl<'f> IntoIterator for &'f AllocatedFrames { + type IntoIter = AllocatedFramesIter<'f>; + type Item = AllocatedFrame<'f>; fn into_iter(self) -> Self::IntoIter { - UnmappedFramesIter { + AllocatedFramesIter { _owner: self, range: self.frames.iter(), } } } -/// An iterator over each [`UnmappedFrame`] in a range of [`Frames<{FrameState::Unmapped}>`]. -/// -/// To Do (get Kevin's input): Description is no longer valid, since we have an iterator for RangeInclusive now. -/// but I still think it's useful to have a `Frames<{FrameState::Unmapped}>` iterator that ties the lifetime -/// of the `UnmappedFrame` to the original object. -/// +/// An iterator over each [`AllocatedFrame`] in a range of [`AllocatedFrames`]. +/// /// We must implement our own iterator type here in order to tie the lifetime `'f` -/// of a returned `UnmappedFrame<'f>` type to the lifetime of its containing `Frames<{FrameState::Unmapped}>`. -/// This is because the underlying type of `Frames<{FrameState::Unmapped}>` is a [`FrameRange`], -/// which itself is a [`core::ops::RangeInclusive`] of [`Frame`]s, and unfortunately the -/// `RangeInclusive` type doesn't implement an immutable iterator. -/// -/// Iterating through a `RangeInclusive` actually modifies its own internal range, -/// so we must avoid doing that because it would break the semantics of a `FrameRange`. -/// In fact, this is why [`FrameRange`] only implements `IntoIterator` but -/// does not implement [`Iterator`] itself. -pub struct UnmappedFramesIter<'f> { - _owner: &'f Frames<{FrameState::Unmapped}>, +/// of a returned `AllocatedFrame<'f>` type to the lifetime of its containing `AllocatedFrames`. +/// This is because the underlying type of `AllocatedFrames` is a [`FrameRange`], +/// which itself is a [`RangeInclusive`] of [`Frame`]s. +/// Currently, the [`RangeInclusiveIterator`] type creates a clone of the original +/// [`RangeInclusive`] instances rather than borrowing a reference to it. +/// +/// [`RangeInclusive`]: range_inclusive::RangeInclusive +pub struct AllocatedFramesIter<'f> { + _owner: &'f AllocatedFrames, range: range_inclusive::RangeInclusiveIterator, } -impl<'f> Iterator for UnmappedFramesIter<'f> { - type Item = UnmappedFrame<'f>; +impl<'f> Iterator for AllocatedFramesIter<'f> { + type Item = AllocatedFrame<'f>; fn next(&mut self) -> Option { self.range.next().map(|frame| - UnmappedFrame { + AllocatedFrame { frame, _phantom: core::marker::PhantomData, } ) } } -/// A reference to a single frame within a range of `Frames<{FrameState::Unmapped}>`. +/// A reference to a single frame within a range of `AllocatedFrames`. /// -/// The lifetime of this type is tied to the lifetime of its owning `Frames<{FrameState::Unmapped}>`. +/// The lifetime of this type is tied to the lifetime of its owning `AllocatedFrames`. #[derive(Debug)] -pub struct UnmappedFrame<'f> { +pub struct AllocatedFrame<'f> { frame: Frame, _phantom: core::marker::PhantomData<&'f Frame>, } -impl<'f> Deref for UnmappedFrame<'f> { +impl<'f> Deref for AllocatedFrame<'f> { type Target = Frame; fn deref(&self) -> &Self::Target { &self.frame } } -assert_not_impl_any!(UnmappedFrame: DerefMut, Clone); +assert_not_impl_any!(AllocatedFrame: DerefMut, Clone); + +pub struct SplitFrames { + before_start: Option>, + start_to_end: Frames, + after_end: Option>, +} +impl SplitFrames { + fn destucture(self) -> (Option>, Frames, Option>) { + (self.before_start, self.start_to_end, self.after_end) + } +} impl Frames { #[allow(dead_code)] @@ -580,38 +582,37 @@ impl Frames { /// Splits up the given `Frames` into multiple smaller `Frames`. /// /// Returns a tuple of three `Frames`: - /// 1. The `Frames` containing the requested range of frames starting at `start_frame`. /// 2. The range of frames in the `self` that came before the beginning of the requested frame range. + /// 1. The `Frames` containing the requested range of frames starting at `frames.start`. /// 3. The range of frames in the `self` that came after the end of the requested frame range. /// - /// If `start_frame` is not contained within `self` or `num_frames` results in an end frame greater than the end of `self`, - /// then `self` is not changed and we return (self, None, None). - pub fn split( + /// If `frames` is not contained within `self`, then `self` is not changed and we return it within an Err. + pub fn extract_frames_with_range( self, frames: FrameRange - ) -> (Self, Option, Option) { + ) -> Result, Self> { let start_frame = *frames.start(); let num_frames = frames.size_in_frames(); if (start_frame < *self.start()) || (start_frame + (num_frames - 1) > *self.end()) || (num_frames == 0) { - return (self, None, None); + return Err(self); } - let new_allocation = Frames{ frames: FrameRange::new(start_frame, start_frame + (num_frames - 1)), ..self }; - let before = if start_frame == MIN_FRAME || start_frame == *self.start() { + let new_allocation = Frames{ frames, ..self }; + let before_start = if start_frame == MIN_FRAME || start_frame == *self.start() { None } else { Some(Frames{ frames: FrameRange::new(*self.start(), *new_allocation.start() - 1), ..self }) }; - let after = if *new_allocation.end() == MAX_FRAME || *new_allocation.end() == *self.end(){ + let after_end = if *new_allocation.end() == MAX_FRAME || *new_allocation.end() == *self.end(){ None } else { Some(Frames{ frames: FrameRange::new(*new_allocation.end() + 1, *self.end()), ..self }) }; core::mem::forget(self); - (new_allocation, before, after) + Ok(SplitFrames{ before_start, start_to_end: new_allocation, after_end}) } /// Splits this `Frames` into two separate `Frames` objects: @@ -708,18 +709,18 @@ impl fmt::Debug for Frames { /// with a `let _ = ...` binding to instantly drop it. pub struct DeferredAllocAction<'list> { /// A reference to the list into which we will insert the free general-purpose `Chunk`s. - free_list: &'list Mutex>>, + free_list: &'list Mutex>, /// A reference to the list into which we will insert the free "reserved" `Chunk`s. - reserved_list: &'list Mutex>>, + reserved_list: &'list Mutex>, /// A free chunk that needs to be added back to the free list. - free1: Frames<{FrameState::Unmapped}>, + free1: FreeFrames, /// Another free chunk that needs to be added back to the free list. - free2: Frames<{FrameState::Unmapped}>, + free2: FreeFrames, } impl<'list> DeferredAllocAction<'list> { fn new(free1: F1, free2: F2) -> DeferredAllocAction<'list> - where F1: Into>>, - F2: Into>>, + where F1: Into>, + F2: Into>, { let free1 = free1.into().unwrap_or_else(Frames::empty); let free2 = free2.into().unwrap_or_else(Frames::empty); @@ -767,10 +768,6 @@ enum AllocationError { OutOfAddressSpace(usize), /// The starting address was found, but not all successive contiguous frames were available. ContiguousChunkNotFound(Frame, usize), - /// Failed to remove a chunk from the free list given a reference to it. - ChunkRemovalFailed, - /// Failed to merge or split a Chunk. - ChunkOperationFailed, } impl From for &'static str { fn from(alloc_err: AllocationError) -> &'static str { @@ -779,8 +776,6 @@ impl From for &'static str { AllocationError::AddressNotFound(..) => "requested address was outside of this frame allocator's range", AllocationError::OutOfAddressSpace(..) => "out of physical address space", AllocationError::ContiguousChunkNotFound(..) => "only some of the requested frames were available", - AllocationError::ChunkRemovalFailed => "Failed to remove a Chunk from the free list, this is most likely due to some logical error", - AllocationError::ChunkOperationFailed => "Could not merge or split a Chunk. This indicated a bug in the frame allocator.", } } } @@ -789,7 +784,7 @@ impl From for &'static str { /// Searches the given `list` for the chunk that contains the range of frames from /// `requested_frame` to `requested_frame + num_frames`. fn find_specific_chunk( - list: &mut StaticArrayRBTree>, + list: &mut StaticArrayRBTree, requested_frame: Frame, num_frames: usize ) -> Result<(AllocatedFrames, DeferredAllocAction<'static>), AllocationError> { @@ -803,7 +798,7 @@ fn find_specific_chunk( if let Some(chunk) = elem { if requested_frame >= *chunk.start() && requested_end_frame <= *chunk.end() { // Here: `chunk` was big enough and did contain the requested address. - return allocate_from_chosen_chunk(FrameRange::new(requested_frame, requested_frame + num_frames - 1), ValueRefMut::Array(elem)); + return allocate_from_chosen_chunk(FrameRange::new(requested_frame, requested_frame + num_frames - 1), ValueRefMut::Array(elem), None); } } } @@ -813,7 +808,7 @@ fn find_specific_chunk( if let Some(chunk) = cursor_mut.get().map(|w| w.deref().deref().clone()) { if chunk.contains(&requested_frame) { if requested_end_frame <= *chunk.end() { - return allocate_from_chosen_chunk(FrameRange::new(requested_frame, requested_frame + num_frames - 1), ValueRefMut::RBTree(cursor_mut)); + return allocate_from_chosen_chunk(FrameRange::new(requested_frame, requested_frame + num_frames - 1), ValueRefMut::RBTree(cursor_mut), None); } else { // We found the chunk containing the requested address, but it was too small to cover all of the requested frames. // Let's try to merge the next-highest contiguous chunk to see if those two chunks together @@ -824,7 +819,7 @@ fn find_specific_chunk( // Requested address: {:?}, num_frames: {}, chunk: {:?}", // requested_frame, num_frames, chunk, // ); - let next_contiguous_chunk: Option> = { + let next_contiguous_chunk: Option = { cursor_mut.move_next();// cursor now points to the next chunk if let Some(next_chunk) = cursor_mut.get().map(|w| w.deref()) { if *chunk.end() + 1 == *next_chunk.start() { @@ -858,7 +853,7 @@ fn find_specific_chunk( // We would like to merge it into the initial chunk with just the reference (since we have a cursor pointing to it already), // but we can't get a mutable reference to the element the cursor is pointing to. // So both chunks will be removed and then merged. - return merge_contiguous_chunks_and_allocate(FrameRange::new(requested_frame, requested_frame + num_frames -1), ValueRefMut::RBTree(cursor_mut), next_chunk); + return allocate_from_chosen_chunk(FrameRange::new(requested_frame, requested_frame + num_frames -1), ValueRefMut::RBTree(cursor_mut), Some(next_chunk)); } } } @@ -872,7 +867,7 @@ fn find_specific_chunk( /// Searches the given `list` for any chunk large enough to hold at least `num_frames`. fn find_any_chunk( - list: &mut StaticArrayRBTree>, + list: &mut StaticArrayRBTree, num_frames: usize ) -> Result<(AllocatedFrames, DeferredAllocAction<'static>), AllocationError> { // During the first pass, we ignore designated regions. @@ -885,7 +880,7 @@ fn find_any_chunk( continue; } else { - return allocate_from_chosen_chunk(FrameRange::new(*chunk.start(), *chunk.start() + num_frames - 1), ValueRefMut::Array(elem)); + return allocate_from_chosen_chunk(FrameRange::new(*chunk.start(), *chunk.start() + num_frames - 1), ValueRefMut::Array(elem), None); } } } @@ -894,10 +889,10 @@ fn find_any_chunk( // Because we allocate new frames by peeling them off from the beginning part of a chunk, // it's MUCH faster to start the search for free frames from higher addresses moving down. // This results in an O(1) allocation time in the general case, until all address ranges are already in use. - let mut cursor = tree.upper_bound_mut(Bound::<&Frames<{FrameState::Unmapped}>>::Unbounded); + let mut cursor = tree.upper_bound_mut(Bound::<&FreeFrames>::Unbounded); while let Some(chunk) = cursor.get().map(|w| w.deref()) { if num_frames <= chunk.size_in_frames() && chunk.typ() == MemoryRegionType::Free { - return allocate_from_chosen_chunk(FrameRange::new(*chunk.start(), *chunk.start() + num_frames - 1), ValueRefMut::RBTree(cursor)); + return allocate_from_chosen_chunk(FrameRange::new(*chunk.start(), *chunk.start() + num_frames - 1), ValueRefMut::RBTree(cursor), None); } warn!("Frame allocator: inefficient scenario: had to search multiple chunks \ (skipping {:?}) while trying to allocate {} frames at any address.", @@ -918,7 +913,7 @@ fn find_any_chunk( /// Removes a `Frames` object from the RBTree. /// `frames_ref` is basically a wrapper over the cursor which stores the position of the frames. -fn retrieve_frames_from_ref(mut frames_ref: ValueRefMut>) -> Option> { +fn retrieve_frames_from_ref(mut frames_ref: ValueRefMut) -> Option { // Remove the chosen chunk from the free frame list. let removed_val = frames_ref.remove(); @@ -930,51 +925,36 @@ fn retrieve_frames_from_ref(mut frames_ref: ValueRefMut>, - next_chunk: Frames<{FrameState::Unmapped}>, + initial_chunk_ref: ValueRefMut, + next_chunk: Option, ) -> Result<(AllocatedFrames, DeferredAllocAction<'static>), AllocationError> { // Remove the initial chunk from the free frame list. - let mut chosen_chunk = retrieve_frames_from_ref(initial_chunk_ref).ok_or(AllocationError::ChunkRemovalFailed)?; - // This should always succeed, since we've already checked the conditions for a merge + let mut chosen_chunk = retrieve_frames_from_ref(initial_chunk_ref) + .expect("BUG: Failed to retrieve chunk from free list"); + + // This should always succeed, since we've already checked the conditions for a merge and split. // We should return the next_chunk back to the list, but a failure at this point implies a bug in the frame allocator. - chosen_chunk.merge(next_chunk).map_err(|_| AllocationError::ChunkOperationFailed)?; - let (new_allocation, before, after) = chosen_chunk.split(frames_to_allocate); - - // TODO: Re-use the allocated wrapper if possible, rather than allocate a new one entirely. - // if let RemovedValue::RBTree(Some(wrapper_adapter)) = _removed_chunk { ... } - Ok(( - new_allocation, - DeferredAllocAction::new(before, after), - )) - -} + if let Some(chunk) = next_chunk { + chosen_chunk.merge(chunk).expect("BUG: Failed to merge adjacent chunks"); + } -/// The final part of the main allocation routine that splits the given chosen chunk -/// into multiple smaller chunks, thereby "allocating" frames from it. -/// -/// This function breaks up that chunk into multiple ones and returns an `AllocatedFrames` -/// from (part of) that chunk, ranging in the frames represented by `frames_to_allocate`. -fn allocate_from_chosen_chunk( - frames_to_allocate: FrameRange, - chosen_chunk_ref: ValueRefMut>, -) -> Result<(AllocatedFrames, DeferredAllocAction<'static>), AllocationError> { - // Remove the chosen chunk from the free frame list. - let chosen_chunk = retrieve_frames_from_ref(chosen_chunk_ref).ok_or(AllocationError::ChunkRemovalFailed)?; - let (new_allocation, before, after) = chosen_chunk.split(frames_to_allocate); + let ( before, new_allocation, after) = chosen_chunk.extract_frames_with_range(frames_to_allocate) + .expect("BUG: Failed to split merged chunk") + .destucture(); // TODO: Re-use the allocated wrapper if possible, rather than allocate a new one entirely. // if let RemovedValue::RBTree(Some(wrapper_adapter)) = _removed_chunk { ... } Ok(( - new_allocation, + new_allocation.into_allocated_frames(), DeferredAllocAction::new(before, after), )) @@ -983,7 +963,7 @@ fn allocate_from_chosen_chunk( /// Returns `true` if the given list contains *any* of the given `frames`. fn contains_any( - list: &StaticArrayRBTree, + list: &StaticArrayRBTree, frames: &FrameRange, ) -> bool { match &list.0 { @@ -1023,7 +1003,7 @@ fn contains_any( /// overlap any existing regions at all. /// TODO: handle partially-overlapping regions by extending existing regions on either end. fn add_reserved_region_to_frames_list( - list: &mut StaticArrayRBTree>, + list: &mut StaticArrayRBTree, frames: FrameRange, ) -> Result { @@ -1077,7 +1057,7 @@ fn add_reserved_region_to_frames_list( /// /// TODO: combine both functions for adding reserved regions/ frames using a trait OR make one function which adds to both lists, since they are always called together. fn add_reserved_region_to_regions_list( - list: &mut StaticArrayRBTree, + list: &mut StaticArrayRBTree, frames: FrameRange, ) -> Result { @@ -1112,7 +1092,7 @@ fn add_reserved_region_to_regions_list( } } - list.insert(Region { + list.insert(PhysicalMemoryRegion { typ: MemoryRegionType::Reserved, frames: frames.clone(), }).map_err(|_c| "BUG: Failed to insert non-overlapping frames into list.")?; From 69943bb417a1ed64e4c456b7abbabf1d8e61554c Mon Sep 17 00:00:00 2001 From: ramla-i Date: Tue, 18 Jul 2023 18:25:33 -0400 Subject: [PATCH 11/40] tests --- kernel/frame_allocator/src/test.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/kernel/frame_allocator/src/test.rs b/kernel/frame_allocator/src/test.rs index 9cadaa7b79..ee6abb3c94 100644 --- a/kernel/frame_allocator/src/test.rs +++ b/kernel/frame_allocator/src/test.rs @@ -1,4 +1,4 @@ -//! Tests for the AllocatedFrames type, mainly the `split` method. +//! Tests for the `Frames` type, mainly the `split` method. extern crate std; @@ -6,8 +6,8 @@ use self::std::dbg; use super::*; -fn from_addr(start_addr: usize, end_addr: usize) -> AllocatedFrames { - AllocatedFrames::new( +fn from_addr(start_addr: usize, end_addr: usize) -> FreeFrames { + FreeFrames::new( MemoryRegionType::Free, FrameRange::new( Frame::containing_address(PhysicalAddress::new_canonical(start_addr)), From 21f2b385f40b5fe3d8b56487a39411e5c56716ab Mon Sep 17 00:00:00 2001 From: ramla-i Date: Tue, 18 Jul 2023 18:31:58 -0400 Subject: [PATCH 12/40] moved frame state to memory structs --- kernel/frame_allocator/src/lib.rs | 49 +++++++++++++------------------ kernel/memory_structs/src/lib.rs | 12 +++++++- 2 files changed, 32 insertions(+), 29 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index 5d01e6edca..f26f07af1a 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -34,7 +34,7 @@ use core::{borrow::Borrow, cmp::{Ordering, min, max}, ops::{Deref, DerefMut}, fm use intrusive_collections::Bound; use kernel_config::memory::*; use log::{error, warn, debug, trace}; -use memory_structs::{PhysicalAddress, Frame, FrameRange}; +use memory_structs::{PhysicalAddress, Frame, FrameRange, MemoryState}; use spin::Mutex; use static_array_rb_tree::*; use static_assertions::assert_not_impl_any; @@ -320,16 +320,9 @@ pub enum MemoryRegionType { Unknown, } -pub type FreeFrames = Frames<{FrameState::Free}>; -pub type AllocatedFrames = Frames<{FrameState::Allocated}>; -pub type MappedFrames = Frames<{FrameState::Mapped}>; - -#[derive(PartialEq, Eq, ConstParamTy)] -pub enum FrameState { - Free, - Allocated, - Mapped -} +pub type FreeFrames = Frames<{MemoryState::Free}>; +pub type AllocatedFrames = Frames<{MemoryState::Allocated}>; +pub type MappedFrames = Frames<{MemoryState::Mapped}>; /// A range of contiguous frames. /// Owning a `Frames` object gives ownership of the range of frames it references. @@ -351,7 +344,7 @@ pub enum FrameState { /// Thus, comparing two `Frames` with the `==` or `!=` operators may not work as expected. /// since it ignores their actual range of frames. #[derive(Eq)] -pub struct Frames { +pub struct Frames { /// The type of this memory chunk, e.g., whether it's in a free or reserved region. typ: MemoryRegionType, /// The Frames covered by this chunk, an inclusive range. @@ -359,9 +352,9 @@ pub struct Frames { } // Frames must not be Cloneable, and it must not expose its inner frames as mutable. -assert_not_impl_any!(Frames<{FrameState::Free}>: DerefMut, Clone); -assert_not_impl_any!(Frames<{FrameState::Allocated}>: DerefMut, Clone); -assert_not_impl_any!(Frames<{FrameState::Mapped}>: DerefMut, Clone); +assert_not_impl_any!(Frames<{MemoryState::Free}>: DerefMut, Clone); +assert_not_impl_any!(Frames<{MemoryState::Allocated}>: DerefMut, Clone); +assert_not_impl_any!(Frames<{MemoryState::Mapped}>: DerefMut, Clone); impl FreeFrames { @@ -428,10 +421,10 @@ pub(crate) fn into_allocated_frames(frames: FrameRange) -> AllocatedFrames { Frames{ typ, frames} } -impl Drop for Frames { +impl Drop for Frames { fn drop(&mut self) { match S { - FrameState::Free => { + MemoryState::Free => { if self.size_in_frames() == 0 { return; } let unmapped_frames: FreeFrames = Frames { @@ -459,8 +452,8 @@ impl Drop for Frames { // a requested address is in a chunk that needs to be merged. // Thus, for performance, we save that for those future situations. }, - FrameState::Allocated => { FreeFrames::new(self.typ, self.frames.clone()); }, //ToDo: Check drop handler correctness - FrameState::Mapped => panic!("We should never drop a mapped frame! It should be forgotten instead."), + MemoryState::Allocated => { FreeFrames::new(self.typ, self.frames.clone()); }, //ToDo: Check drop handler correctness + MemoryState::Mapped => panic!("We should never drop a mapped frame! It should be forgotten instead."), } } } @@ -517,19 +510,19 @@ impl<'f> Deref for AllocatedFrame<'f> { } assert_not_impl_any!(AllocatedFrame: DerefMut, Clone); -pub struct SplitFrames { +pub struct SplitFrames { before_start: Option>, start_to_end: Frames, after_end: Option>, } -impl SplitFrames { +impl SplitFrames { fn destucture(self) -> (Option>, Frames, Option>) { (self.before_start, self.start_to_end, self.after_end) } } -impl Frames { +impl Frames { #[allow(dead_code)] pub(crate) fn frames(&self) -> FrameRange { self.frames.clone() @@ -661,34 +654,34 @@ impl Frames { } } -impl Deref for Frames { +impl Deref for Frames { type Target = FrameRange; fn deref(&self) -> &FrameRange { &self.frames } } -impl Ord for Frames { +impl Ord for Frames { fn cmp(&self, other: &Self) -> Ordering { self.frames.start().cmp(other.frames.start()) } } -impl PartialOrd for Frames { +impl PartialOrd for Frames { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } } -impl PartialEq for Frames { +impl PartialEq for Frames { fn eq(&self, other: &Self) -> bool { self.frames.start() == other.frames.start() } } -impl Borrow for &'_ Frames { +impl Borrow for &'_ Frames { fn borrow(&self) -> &Frame { self.frames.start() } } -impl fmt::Debug for Frames { +impl fmt::Debug for Frames { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "Frames({:?}, {:?})", self.typ, self.frames) } diff --git a/kernel/memory_structs/src/lib.rs b/kernel/memory_structs/src/lib.rs index ec53bf4171..39c6bc14ed 100644 --- a/kernel/memory_structs/src/lib.rs +++ b/kernel/memory_structs/src/lib.rs @@ -7,12 +7,15 @@ #![no_std] #![feature(step_trait)] +#![allow(incomplete_features)] +#![feature(adt_const_params)] use core::{ cmp::{min, max}, fmt, iter::Step, - ops::{Add, AddAssign, Deref, DerefMut, Sub, SubAssign} + ops::{Add, AddAssign, Deref, DerefMut, Sub, SubAssign}, + marker::ConstParamTy }; use kernel_config::memory::{MAX_PAGE_NUMBER, PAGE_SIZE}; use zerocopy::FromBytes; @@ -20,6 +23,13 @@ use paste::paste; use derive_more::*; use range_inclusive::{RangeInclusive, RangeInclusiveIterator}; +#[derive(PartialEq, Eq, ConstParamTy)] +pub enum MemoryState { + Free, + Allocated, + Mapped +} + /// A macro for defining `VirtualAddress` and `PhysicalAddress` structs /// and implementing their common traits, which are generally identical. macro_rules! implement_address { From 69d246d01d3082075c8974a1a4a58f0b0756900c Mon Sep 17 00:00:00 2001 From: ramla-i Date: Wed, 19 Jul 2023 15:55:36 -0400 Subject: [PATCH 13/40] added unmapped frames --- kernel/frame_allocator/Cargo.toml | 4 +- kernel/frame_allocator/src/lib.rs | 137 +++++++++++++++++++++++------ kernel/memory/src/lib.rs | 1 + kernel/memory/src/paging/mapper.rs | 23 ++--- kernel/memory/src/paging/mod.rs | 8 +- kernel/memory_structs/src/lib.rs | 3 +- kernel/page_table_entry/src/lib.rs | 12 +-- 7 files changed, 138 insertions(+), 50 deletions(-) diff --git a/kernel/frame_allocator/Cargo.toml b/kernel/frame_allocator/Cargo.toml index db6a060ef4..6392004606 100644 --- a/kernel/frame_allocator/Cargo.toml +++ b/kernel/frame_allocator/Cargo.toml @@ -8,10 +8,12 @@ edition = "2021" [dependencies] log = "0.4.8" spin = "0.9.4" -intrusive-collections = "0.9.0" static_assertions = "1.1.0" range_inclusive = { path = "../../libs/range_inclusive" } kernel_config = { path = "../kernel_config" } memory_structs = { path = "../memory_structs" } + +[dependencies.intrusive-collections] +git = "https://github.com/Ramla-I/intrusive-rs" \ No newline at end of file diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index f26f07af1a..e1b2e348b9 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -30,7 +30,7 @@ mod test; mod static_array_rb_tree; // mod static_array_linked_list; -use core::{borrow::Borrow, cmp::{Ordering, min, max}, ops::{Deref, DerefMut}, fmt, marker::ConstParamTy, slice::Split}; +use core::{borrow::Borrow, cmp::{Ordering, min, max}, ops::{Deref, DerefMut}, fmt}; use intrusive_collections::Bound; use kernel_config::memory::*; use log::{error, warn, debug, trace}; @@ -72,11 +72,11 @@ static RESERVED_REGIONS: Mutex> = Mutex: /// ## Return /// Upon success, this function returns a callback function that allows the caller /// (the memory subsystem init function) to convert a range of unmapped frames -/// back into an [`AllocatedFrames`] object. +/// back into an [`UnmappedFrames`] object. pub fn init( free_physical_memory_areas: F, reserved_physical_memory_areas: R, -) -> Result AllocatedFrames, &'static str> +) -> Result UnmappedFrames, &'static str> where P: Borrow, F: IntoIterator, R: IntoIterator + Clone, @@ -178,7 +178,7 @@ pub fn init( *GENERAL_REGIONS.lock() = StaticArrayRBTree::new(free_list); *RESERVED_REGIONS.lock() = StaticArrayRBTree::new(reserved_list); - Ok(into_allocated_frames) + Ok(into_unmapped_frames) } @@ -323,6 +323,7 @@ pub enum MemoryRegionType { pub type FreeFrames = Frames<{MemoryState::Free}>; pub type AllocatedFrames = Frames<{MemoryState::Allocated}>; pub type MappedFrames = Frames<{MemoryState::Mapped}>; +pub type UnmappedFrames = Frames<{MemoryState::Unmapped}>; /// A range of contiguous frames. /// Owning a `Frames` object gives ownership of the range of frames it references. @@ -355,6 +356,7 @@ pub struct Frames { assert_not_impl_any!(Frames<{MemoryState::Free}>: DerefMut, Clone); assert_not_impl_any!(Frames<{MemoryState::Allocated}>: DerefMut, Clone); assert_not_impl_any!(Frames<{MemoryState::Mapped}>: DerefMut, Clone); +assert_not_impl_any!(Frames<{MemoryState::Unmapped}>: DerefMut, Clone); impl FreeFrames { @@ -403,22 +405,34 @@ impl AllocatedFrames { } } +impl UnmappedFrames { + /// Consumes the `Frames` in an unmapped state and converts them to `Frames` in an allocated state. + pub fn into_allocated_frames(self) -> AllocatedFrames { + let f = Frames { + typ: self.typ, + frames: self.frames.clone(), + }; + core::mem::forget(self); + f + } +} + -/// This function is a callback used to convert `UnmappedFrames` into `AllocatedFrames`. -/// `AllocatedFrames` represents frames that have been unmapped from a page that had +/// This function is a callback used to convert `UnmappedFramesInfo` into `UnmappedFrames`. +/// `UnmappedFrames` represents frames that have been unmapped from a page that had /// exclusively mapped them, indicating that no others pages have been mapped /// to those same frames, and thus, they can be safely deallocated. /// /// This exists to break the cyclic dependency cycle between this crate and /// the `page_table_entry` crate, since `page_table_entry` must depend on types /// from this crate in order to enforce safety when modifying page table entries. -pub(crate) fn into_allocated_frames(frames: FrameRange) -> AllocatedFrames { +pub(crate) fn into_unmapped_frames(frames: FrameRange) -> UnmappedFrames { let typ = if contains_any(&RESERVED_REGIONS.lock(), &frames) { MemoryRegionType::Reserved } else { MemoryRegionType::Free }; - Frames{ typ, frames} + Frames{ typ, frames } } impl Drop for Frames { @@ -427,33 +441,66 @@ impl Drop for Frames { MemoryState::Free => { if self.size_in_frames() == 0 { return; } - let unmapped_frames: FreeFrames = Frames { + let free_frames: FreeFrames = Frames { typ: self.typ, frames: self.frames.clone(), }; - let list = if unmapped_frames.typ == MemoryRegionType::Reserved { - &FREE_RESERVED_FRAMES_LIST + let mut list = if free_frames.typ == MemoryRegionType::Reserved { + FREE_RESERVED_FRAMES_LIST.lock() } else { - &FREE_GENERAL_FRAMES_LIST - }; - - // Simply add the newly-deallocated chunk to the free frames list. - let mut locked_list = list.lock(); - let res = locked_list.insert(unmapped_frames); - match res { - Ok(_inserted_free_chunk) => (), - Err(c) => error!("BUG: couldn't insert deallocated chunk {:?} into free frame list", c), + FREE_GENERAL_FRAMES_LIST.lock() + }; + + match &mut list.0 { + // For early allocations, just add the deallocated chunk to the free pages list. + Inner::Array(_) => { + if list.insert(free_frames).is_ok() { + return; + } + } + + // For full-fledged deallocations, use the entry API to efficiently determine if + // we can merge the deallocated pages with an existing contiguously-adjactent chunk + // or if we need to insert a new chunk. + Inner::RBTree(ref mut tree) => { + let mut cursor_mut = tree.lower_bound_mut(Bound::Included(free_frames.start())); + if let Some(next_frames) = cursor_mut.get_mut() { + if *free_frames.end() + 1 == *next_frames.start() { + trace!("Prepending {:?} onto beg of next {:?}", free_frames, next_frames.deref().deref()); + if next_frames.merge(free_frames).is_ok() { + return; + } else { + panic!("BUG: couldn't merge deallocated chunk into next chunk"); + } + } + } + if let Some(prev_frames) = cursor_mut.peek_prev().get() { + if *prev_frames.end() + 1 == *free_frames.start() { + trace!("Appending {:?} onto end of prev {:?}", free_frames, prev_frames.deref()); + cursor_mut.move_prev(); + if let Some(prev_frames) = cursor_mut.get_mut() { + if prev_frames.merge(free_frames).is_ok() { + return; + } else { + panic!("BUG: couldn't merge deallocated chunk into prev chunk"); + } + } + } + } + + trace!("Inserting new chunk for deallocated {:?} ", free_frames); + cursor_mut.insert(Wrapper::new_link(free_frames)); + return; + } } - - // Here, we could optionally use above `_inserted_free_chunk` to merge the adjacent (contiguous) chunks - // before or after the newly-inserted free chunk. - // However, there's no *need* to do so until we actually run out of address space or until - // a requested address is in a chunk that needs to be merged. - // Thus, for performance, we save that for those future situations. }, - MemoryState::Allocated => { FreeFrames::new(self.typ, self.frames.clone()); }, //ToDo: Check drop handler correctness + MemoryState::Allocated => { + trace!("Converting AllocatedFrames to FreeFrames. Drop handler should be called again {:?}", self.frames); + FreeFrames::new(self.typ, self.frames.clone()); + }, MemoryState::Mapped => panic!("We should never drop a mapped frame! It should be forgotten instead."), + MemoryState::Unmapped => { AllocatedFrames{ typ: self.typ, frames: self.frames.clone() }; }, } } } @@ -986,6 +1033,42 @@ fn contains_any( false } +trait IntoFrameRange { + fn into_frame_range(&self) -> FrameRange; +} + +// /// Returns `true` if the given list contains *any* of the given `frames`. +// fn contains_any_generic ( +// list: &StaticArrayRBTree, +// frames: &FrameRange, +// ) -> bool { +// match &list.0 { +// Inner::Array(ref arr) => { +// for chunk in arr.iter().flatten() { +// if chunk.into_frame_range().overlap(frames).is_some() { +// return true; +// } +// } +// } +// Inner::RBTree(ref tree) => { +// let mut cursor = tree.upper_bound(Bound::Included(frames.start())); +// while let Some(chunk) = cursor.get() { +// if chunk.into_frame_range().start() > frames.end() { +// // We're iterating in ascending order over a sorted tree, so we can stop +// // looking for overlapping regions once we pass the end of `frames`. +// break; +// } + +// if chunk.into_frame_range().overlap(frames).is_some() { +// return true; +// } +// cursor.move_next(); +// } +// } +// } +// false +// } + /// Adds the given `frames` to the given `list` as a Chunk of reserved frames. /// diff --git a/kernel/memory/src/lib.rs b/kernel/memory/src/lib.rs index 63753cadc7..fcbf0f8c2d 100644 --- a/kernel/memory/src/lib.rs +++ b/kernel/memory/src/lib.rs @@ -36,6 +36,7 @@ pub use page_allocator::{ }; pub use frame_allocator::{ AllocatedFrames, + UnmappedFrames, allocate_frames, allocate_frames_at, allocate_frames_by_bytes, diff --git a/kernel/memory/src/paging/mapper.rs b/kernel/memory/src/paging/mapper.rs index 08ffccd7b9..802d479b95 100644 --- a/kernel/memory/src/paging/mapper.rs +++ b/kernel/memory/src/paging/mapper.rs @@ -18,7 +18,7 @@ use core::{ slice, }; use log::{error, warn, debug, trace}; -use crate::{BROADCAST_TLB_SHOOTDOWN_FUNC, VirtualAddress, PhysicalAddress, Page, Frame, FrameRange, AllocatedPages, AllocatedFrames}; +use crate::{BROADCAST_TLB_SHOOTDOWN_FUNC, VirtualAddress, PhysicalAddress, Page, Frame, FrameRange, AllocatedPages, AllocatedFrames, UnmappedFrames}; use crate::paging::{ get_current_p4, table::{P4, UPCOMING_P4, Table, Level4}, @@ -34,23 +34,23 @@ use owned_borrowed_trait::{OwnedOrBorrowed, Owned, Borrowed}; #[cfg(target_arch = "x86_64")] use kernel_config::memory::ENTRIES_PER_PAGE_TABLE; -/// This is a private callback used to convert `UnmappedFrames` into `AllocatedFrames`. +/// This is a private callback used to convert `UnmappedFramesInfo` into `UnmappedFrames`. /// /// This exists to break the cyclic dependency cycle between `page_table_entry` and /// `frame_allocator`, which depend on each other as such: -/// * `frame_allocator` needs to `impl Into for UnmappedFrames` +/// * `frame_allocator` needs to `impl Into for UnmappedFramesInfo` /// in order to allow unmapped exclusive frames to be safely deallocated /// * `page_table_entry` needs to use the `AllocatedFrames` type in order to allow /// page table entry values to be set safely to a real physical frame that is owned and exists. /// /// To get around that, the `frame_allocator::init()` function returns a callback -/// to its function that allows converting a range of unmapped frames back into `AllocatedFrames`, +/// to its function that allows converting a range of unmapped frames back into `UnmappedFrames`, /// which then allows them to be dropped and thus deallocated. /// /// This is safe because the frame allocator can only be initialized once, and also because /// only this crate has access to that function callback and can thus guarantee -/// that it is only invoked for `UnmappedFrames`. -pub(super) static INTO_ALLOCATED_FRAMES_FUNC: Once AllocatedFrames> = Once::new(); +/// that it is only invoked for `UnmappedFramesInfo`. +pub(super) static INTO_UNMAPPED_FRAMES_FUNC: Once UnmappedFrames> = Once::new(); /// A convenience function to translate the given virtual address into a /// physical address using the currently-active page table. @@ -609,8 +609,8 @@ impl MappedPages { ); } - let mut first_frame_range: Option = None; // this is what we'll return - let mut current_frame_range: Option = None; + let mut first_frame_range: Option = None; // this is what we'll return + let mut current_frame_range: Option = None; for page in self.pages.range().clone() { let p1 = active_table_mapper.p4_mut() @@ -630,8 +630,8 @@ impl MappedPages { // freed from the newly-unmapped P1 PTE entry above. match unmapped_frames { UnmapResult::Exclusive(newly_unmapped_frames) => { - let newly_unmapped_frames = INTO_ALLOCATED_FRAMES_FUNC.get() - .ok_or("BUG: Mapper::unmap(): the `INTO_ALLOCATED_FRAMES_FUNC` callback was not initialized") + let newly_unmapped_frames = INTO_UNMAPPED_FRAMES_FUNC.get() + .ok_or("BUG: Mapper::unmap(): the `INTO_UNMAPPED_FRAMES_FUNC` callback was not initialized") .map(|into_func| into_func(newly_unmapped_frames.deref().clone()))?; if let Some(mut curr_frames) = current_frame_range.take() { @@ -680,7 +680,8 @@ impl MappedPages { } // Ensure that we return at least some frame range, even if we broke out of the above loop early. - Ok(first_frame_range.or(current_frame_range)) + Ok(first_frame_range.map(|f| f.into_allocated_frames()) + .or(current_frame_range.map(|f| f.into_allocated_frames()))) } diff --git a/kernel/memory/src/paging/mod.rs b/kernel/memory/src/paging/mod.rs index 1761c52cb9..700b1a6ef2 100644 --- a/kernel/memory/src/paging/mod.rs +++ b/kernel/memory/src/paging/mod.rs @@ -30,7 +30,7 @@ use core::{ use log::debug; use super::{ Frame, FrameRange, PageRange, VirtualAddress, PhysicalAddress, - AllocatedPages, allocate_pages, AllocatedFrames, PteFlags, + AllocatedPages, allocate_pages, AllocatedFrames, UnmappedFrames, PteFlags, InitialMemoryMappings, tlb_flush_all, tlb_flush_virt_addr, get_p4, find_section_memory_bounds, }; @@ -223,11 +223,11 @@ pub fn get_current_p4() -> Frame { pub fn init( boot_info: &impl BootInformation, stack_start_virt: VirtualAddress, - into_alloc_frames_fn: fn(FrameRange) -> AllocatedFrames, + into_unmapped_frames_fn: fn(FrameRange) -> UnmappedFrames, ) -> Result { // Store the callback from `frame_allocator::init()` that allows the `Mapper` to convert - // `page_table_entry::UnmappedFrames` back into `AllocatedFrames`. - mapper::INTO_ALLOCATED_FRAMES_FUNC.call_once(|| into_alloc_frames_fn); + // `page_table_entry::UnmappedFramesInfo` back into `UnmappedFrames`. + mapper::INTO_UNMAPPED_FRAMES_FUNC.call_once(|| into_unmapped_frames_fn); // bootstrap a PageTable from the currently-loaded page table let mut page_table = PageTable::from_current() diff --git a/kernel/memory_structs/src/lib.rs b/kernel/memory_structs/src/lib.rs index 39c6bc14ed..5b36dd9623 100644 --- a/kernel/memory_structs/src/lib.rs +++ b/kernel/memory_structs/src/lib.rs @@ -27,7 +27,8 @@ use range_inclusive::{RangeInclusive, RangeInclusiveIterator}; pub enum MemoryState { Free, Allocated, - Mapped + Mapped, + Unmapped } /// A macro for defining `VirtualAddress` and `PhysicalAddress` structs diff --git a/kernel/page_table_entry/src/lib.rs b/kernel/page_table_entry/src/lib.rs index a9116b6c3c..ed0abb0ac3 100644 --- a/kernel/page_table_entry/src/lib.rs +++ b/kernel/page_table_entry/src/lib.rs @@ -57,7 +57,7 @@ impl PageTableEntry { // to specify whether this is a 4KiB, 2MiB, or 1GiB PTE. let frame_range = FrameRange::new(frame, frame); if flags.is_exclusive() { - UnmapResult::Exclusive(UnmappedFrames(frame_range)) + UnmapResult::Exclusive(UnmappedFramesInfo(frame_range)) } else { UnmapResult::NonExclusive(frame_range) } @@ -110,26 +110,26 @@ impl PageTableEntry { /// The frames returned from the action of unmapping a page table entry. /// See the `PageTableEntry::set_unmapped()` function. /// -/// If exclusive, the contained `UnmappedFrames` can be used to deallocate frames. +/// If exclusive, the contained `UnmappedFramesInfo` can be used to deallocate frames. /// /// If non-exclusive, the contained `FrameRange` is provided just for debugging feedback. /// Note that we use `FrameRange` instead of `Frame` because a single page table entry /// can map many frames, e.g., using huge pages. #[must_use] pub enum UnmapResult { - Exclusive(UnmappedFrames), + Exclusive(UnmappedFramesInfo), NonExclusive(FrameRange) } /// A range of frames that have been unmapped from a `PageTableEntry` /// that previously mapped that frame exclusively (i.e., "owned it"). /// -/// These `UnmappedFrames` can be converted into `UnmappedAllocatedFrames` +/// `UnmappedFramesInfo` can be used to create an `UnmappedFrames` /// and then safely deallocated within the `frame_allocator`. /// /// See the `PageTableEntry::set_unmapped()` function. -pub struct UnmappedFrames(FrameRange); -impl Deref for UnmappedFrames { +pub struct UnmappedFramesInfo(FrameRange); +impl Deref for UnmappedFramesInfo { type Target = FrameRange; fn deref(&self) -> &FrameRange { &self.0 From 0dfbc985ba34c07d4d0e6693c28db7ed65cdb8a3 Mon Sep 17 00:00:00 2001 From: ramla-i Date: Thu, 20 Jul 2023 13:45:37 -0400 Subject: [PATCH 14/40] PR changes --- Cargo.lock | 23 +++- kernel/frame_allocator/src/lib.rs | 208 +++++++++--------------------- kernel/memory_structs/src/lib.rs | 12 ++ 3 files changed, 90 insertions(+), 153 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0914694399..ca082f0bdb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1183,7 +1183,7 @@ dependencies = [ name = "frame_allocator" version = "0.1.0" dependencies = [ - "intrusive-collections", + "intrusive-collections 0.9.5", "kernel_config", "log", "memory_structs", @@ -1602,6 +1602,14 @@ dependencies = [ "memoffset 0.5.6", ] +[[package]] +name = "intrusive-collections" +version = "0.9.5" +source = "git+https://github.com/Ramla-I/intrusive-rs#bfae69d5209c082b642a26e30c1345e5c50a1e88" +dependencies = [ + "memoffset 0.9.0", +] + [[package]] name = "io" version = "0.1.0" @@ -1979,6 +1987,15 @@ dependencies = [ "autocfg", ] +[[package]] +name = "memoffset" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5a634b1c61a95585bd15607c6ab0c4e5b226e695ff2800ba0cdccddf208c406c" +dependencies = [ + "autocfg", +] + [[package]] name = "memory" version = "0.1.0" @@ -2259,7 +2276,7 @@ dependencies = [ "cfg-if 0.1.10", "hashbrown", "heap", - "intrusive-collections", + "intrusive-collections 0.9.0", "irq_safety", "kernel_config", "log", @@ -2530,7 +2547,7 @@ dependencies = [ name = "page_allocator" version = "0.1.0" dependencies = [ - "intrusive-collections", + "intrusive-collections 0.9.0", "kernel_config", "log", "memory_structs", diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index e1b2e348b9..b7a87662c5 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -563,11 +563,6 @@ pub struct SplitFrames { after_end: Option>, } -impl SplitFrames { - fn destucture(self) -> (Option>, Frames, Option>) { - (self.before_start, self.start_to_end, self.after_end) - } -} impl Frames { #[allow(dead_code)] @@ -627,32 +622,31 @@ impl Frames { /// 3. The range of frames in the `self` that came after the end of the requested frame range. /// /// If `frames` is not contained within `self`, then `self` is not changed and we return it within an Err. - pub fn extract_frames_with_range( + pub fn split_range( self, - frames: FrameRange + frames_to_extract: FrameRange ) -> Result, Self> { - let start_frame = *frames.start(); - let num_frames = frames.size_in_frames(); - - if (start_frame < *self.start()) || (start_frame + (num_frames - 1) > *self.end()) || (num_frames == 0) { + + if !self.contains_range(&frames_to_extract) { return Err(self); } - - let new_allocation = Frames{ frames, ..self }; + + let start_frame = *frames_to_extract.start(); + let start_to_end = Frames{ frames: frames_to_extract, ..self }; let before_start = if start_frame == MIN_FRAME || start_frame == *self.start() { None } else { - Some(Frames{ frames: FrameRange::new(*self.start(), *new_allocation.start() - 1), ..self }) + Some(Frames{ frames: FrameRange::new(*self.start(), *start_to_end.start() - 1), ..self }) }; - let after_end = if *new_allocation.end() == MAX_FRAME || *new_allocation.end() == *self.end(){ + let after_end = if *start_to_end.end() == MAX_FRAME || *start_to_end.end() == *self.end(){ None } else { - Some(Frames{ frames: FrameRange::new(*new_allocation.end() + 1, *self.end()), ..self }) + Some(Frames{ frames: FrameRange::new(*start_to_end.end() + 1, *self.end()), ..self }) }; core::mem::forget(self); - Ok(SplitFrames{ before_start, start_to_end: new_allocation, after_end}) + Ok(SplitFrames{ before_start, start_to_end, after_end}) } /// Splits this `Frames` into two separate `Frames` objects: @@ -986,16 +980,15 @@ fn allocate_from_chosen_chunk( chosen_chunk.merge(chunk).expect("BUG: Failed to merge adjacent chunks"); } - let ( before, new_allocation, after) = chosen_chunk.extract_frames_with_range(frames_to_allocate) - .expect("BUG: Failed to split merged chunk") - .destucture(); + let SplitFrames{ before_start, start_to_end: new_allocation, after_end } = chosen_chunk.split_range(frames_to_allocate) + .expect("BUG: Failed to split merged chunk"); // TODO: Re-use the allocated wrapper if possible, rather than allocate a new one entirely. // if let RemovedValue::RBTree(Some(wrapper_adapter)) = _removed_chunk { ... } Ok(( new_allocation.into_allocated_frames(), - DeferredAllocAction::new(before, after), + DeferredAllocAction::new(before_start, after_end), )) } @@ -1033,146 +1026,65 @@ fn contains_any( false } -trait IntoFrameRange { - fn into_frame_range(&self) -> FrameRange; -} - -// /// Returns `true` if the given list contains *any* of the given `frames`. -// fn contains_any_generic ( -// list: &StaticArrayRBTree, -// frames: &FrameRange, -// ) -> bool { -// match &list.0 { -// Inner::Array(ref arr) => { -// for chunk in arr.iter().flatten() { -// if chunk.into_frame_range().overlap(frames).is_some() { -// return true; -// } -// } -// } -// Inner::RBTree(ref tree) => { -// let mut cursor = tree.upper_bound(Bound::Included(frames.start())); -// while let Some(chunk) = cursor.get() { -// if chunk.into_frame_range().start() > frames.end() { -// // We're iterating in ascending order over a sorted tree, so we can stop -// // looking for overlapping regions once we pass the end of `frames`. -// break; -// } - -// if chunk.into_frame_range().overlap(frames).is_some() { -// return true; -// } -// cursor.move_next(); -// } -// } -// } -// false -// } - - -/// Adds the given `frames` to the given `list` as a Chunk of reserved frames. +/// Adds the given `frames` to the given `regions_list` and `frames_list` as a Chunk of reserved frames. /// -/// Returns the range of **new** frames that were added to the list, +/// Returns the range of **new** frames that were added to the lists, /// which will be a subset of the given input `frames`. /// /// Currently, this function adds no new frames at all if any frames within the given `frames` list /// overlap any existing regions at all. /// TODO: handle partially-overlapping regions by extending existing regions on either end. -fn add_reserved_region_to_frames_list( - list: &mut StaticArrayRBTree, +fn add_reserved_region_to_lists( + regions_list: &mut StaticArrayRBTree, + frames_list: &mut StaticArrayRBTree, frames: FrameRange, ) -> Result { - // Check whether the reserved region overlaps any existing regions. - match &mut list.0 { - Inner::Array(ref mut arr) => { - for chunk in arr.iter().flatten() { - if let Some(_overlap) = chunk.overlap(&frames) { - // trace!("Failed to add reserved region {:?} due to overlap {:?} with existing chunk {:?}", - // frames, _overlap, chunk - // ); - return Err("Failed to add reserved region that overlapped with existing reserved regions (array)."); + if !contains_any(®ions_list, &frames){ + // Check whether the reserved region overlaps any existing regions. + match &mut frames_list.0 { + Inner::Array(ref mut arr) => { + for chunk in arr.iter().flatten() { + if let Some(_overlap) = chunk.overlap(&frames) { + // trace!("Failed to add reserved region {:?} due to overlap {:?} with existing chunk {:?}", + // frames, _overlap, chunk + // ); + return Err("Failed to add free franes that overlapped with existing frames (array)."); + } } } - } - Inner::RBTree(ref mut tree) => { - let mut cursor_mut = tree.upper_bound_mut(Bound::Included(frames.start())); - while let Some(chunk) = cursor_mut.get().map(|w| w.deref()) { - if chunk.start() > frames.end() { - // We're iterating in ascending order over a sorted tree, - // so we can stop looking for overlapping regions once we pass the end of the new frames to add. - break; - } - if let Some(_overlap) = chunk.overlap(&frames) { - // trace!("Failed to add reserved region {:?} due to overlap {:?} with existing chunk {:?}", - // frames, _overlap, chunk - // ); - return Err("Failed to add reserved region that overlapped with existing reserved regions (RBTree)."); + Inner::RBTree(ref mut tree) => { + let mut cursor_mut = tree.upper_bound_mut(Bound::Included(frames.start())); + while let Some(chunk) = cursor_mut.get().map(|w| w.deref()) { + if chunk.start() > frames.end() { + // We're iterating in ascending order over a sorted tree, + // so we can stop looking for overlapping regions once we pass the end of the new frames to add. + break; + } + if let Some(_overlap) = chunk.overlap(&frames) { + // trace!("Failed to add reserved region {:?} due to overlap {:?} with existing chunk {:?}", + // frames, _overlap, chunk + // ); + return Err("Failed to add free frames that overlapped with existing frames (RBTree)."); + } + cursor_mut.move_next(); } - cursor_mut.move_next(); } } - } - - list.insert(Frames::new( - MemoryRegionType::Reserved, - frames.clone(), - )).map_err(|_c| "BUG: Failed to insert non-overlapping frames into list.")?; - - Ok(frames) -} - -/// Adds the given `frames` to the given `list` as a Region of reserved frames. -/// -/// Returns the range of **new** frames that were added to the list, -/// which will be a subset of the given input `frames`. -/// -/// Currently, this function adds no new frames at all if any frames within the given `frames` list -/// overlap any existing regions at all. -/// TODO: handle partially-overlapping regions by extending existing regions on either end. -/// -/// TODO: combine both functions for adding reserved regions/ frames using a trait OR make one function which adds to both lists, since they are always called together. -fn add_reserved_region_to_regions_list( - list: &mut StaticArrayRBTree, - frames: FrameRange, -) -> Result { + + regions_list.insert(PhysicalMemoryRegion { + typ: MemoryRegionType::Reserved, + frames: frames.clone(), + }).map_err(|_c| "BUG: Failed to insert non-overlapping frames into list.")?; - // Check whether the reserved region overlaps any existing regions. - match &mut list.0 { - Inner::Array(ref mut arr) => { - for chunk in arr.iter().flatten() { - if let Some(_overlap) = chunk.overlap(&frames) { - // trace!("Failed to add reserved region {:?} due to overlap {:?} with existing chunk {:?}", - // frames, _overlap, chunk - // ); - return Err("Failed to add reserved region that overlapped with existing reserved regions (array)."); - } - } - } - Inner::RBTree(ref mut tree) => { - let mut cursor_mut = tree.upper_bound_mut(Bound::Included(frames.start())); - while let Some(chunk) = cursor_mut.get().map(|w| w.deref()) { - if chunk.start() > frames.end() { - // We're iterating in ascending order over a sorted tree, - // so we can stop looking for overlapping regions once we pass the end of the new frames to add. - break; - } - if let Some(_overlap) = chunk.overlap(&frames) { - // trace!("Failed to add reserved region {:?} due to overlap {:?} with existing chunk {:?}", - // frames, _overlap, chunk - // ); - return Err("Failed to add reserved region that overlapped with existing reserved regions (RBTree)."); - } - cursor_mut.move_next(); - } - } + frames_list.insert(Frames::new( + MemoryRegionType::Reserved, + frames.clone(), + )).map_err(|_c| "BUG: Failed to insert non-overlapping frames into list.")?; + } else { + return Err("Failed to add reserved region that overlapped with existing reserved regions."); } - - list.insert(PhysicalMemoryRegion { - typ: MemoryRegionType::Reserved, - frames: frames.clone(), - }).map_err(|_c| "BUG: Failed to insert non-overlapping frames into list.")?; - + Ok(frames) } @@ -1240,11 +1152,7 @@ pub fn allocate_frames_deferred( // but ONLY if those frames are *NOT* in the general-purpose region. let requested_frames = FrameRange::new(requested_start_frame, requested_start_frame + (requested_num_frames - 1)); if !contains_any(&GENERAL_REGIONS.lock(), &requested_frames) { - let new_reserved_frames = add_reserved_region_to_regions_list(&mut RESERVED_REGIONS.lock(), requested_frames)?; - // If we successfully added a new reserved region, - // then add those frames to the actual list of *available* reserved regions. - let _new_free_reserved_frames = add_reserved_region_to_frames_list(&mut free_reserved_frames_list, new_reserved_frames.clone())?; - assert_eq!(new_reserved_frames, _new_free_reserved_frames); + let _new_reserved_frames = add_reserved_region_to_lists(&mut RESERVED_REGIONS.lock(), &mut free_reserved_frames_list, requested_frames)?; find_specific_chunk(&mut free_reserved_frames_list, start_frame, num_frames) } else { diff --git a/kernel/memory_structs/src/lib.rs b/kernel/memory_structs/src/lib.rs index 5b36dd9623..f9a2c79d5c 100644 --- a/kernel/memory_structs/src/lib.rs +++ b/kernel/memory_structs/src/lib.rs @@ -481,6 +481,18 @@ macro_rules! implement_page_frame_range { None } } + + #[doc = "Returns true if `other` is contained within the `" $TypeName "`."] + pub fn contains_range(&self, other: &$TypeName) -> bool { + if (other.[]() != 0) + && (other.start() >= self.start()) + && (*other.start() + (other.[]() - 1) <= *self.end()) + { + true + } else { + false + } + } } impl fmt::Debug for $TypeName { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { From 45e3a30e598edb7158bab8d6620c6de46b8b534f Mon Sep 17 00:00:00 2001 From: ramla-i Date: Fri, 21 Jul 2023 15:02:08 -0400 Subject: [PATCH 15/40] comments --- kernel/frame_allocator/src/lib.rs | 69 ++++++++++++++++++------------- kernel/memory_structs/src/lib.rs | 5 +++ 2 files changed, 46 insertions(+), 28 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index b7a87662c5..3ddd8f625e 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -14,9 +14,10 @@ //! but there are several convenience functions that offer simpler interfaces for general usage. //! //! # Notes and Missing Features -//! This allocator currently does **not** merge freed chunks (de-fragmentation). -//! We don't need to do so until we actually run out of address space or until -//! a requested address is in a chunk that needs to be merged. +//! This allocator only makes one attempt to merge deallocated frames into existing +//! free chunks for de-fragmentation. It does not iteratively merge adjacent chunks in order to +//! maximally combine separate chunks into the biggest single chunk. +//! Instead, free chunks are merged only when they are dropped or when needed to fulfill a specific request. #![no_std] #![allow(clippy::blocks_in_if_conditions)] @@ -245,7 +246,7 @@ fn check_and_add_free_region( } -/// `PhysicalMemoryRegion` represents a range of contiguous frames in phsical memory for bookkeeping purposes. +/// `PhysicalMemoryRegion` represents a range of contiguous frames in physical memory for bookkeeping purposes. /// It does not give access to the underlying frames. /// /// # Ordering and Equality @@ -320,21 +321,23 @@ pub enum MemoryRegionType { Unknown, } -pub type FreeFrames = Frames<{MemoryState::Free}>; -pub type AllocatedFrames = Frames<{MemoryState::Allocated}>; -pub type MappedFrames = Frames<{MemoryState::Mapped}>; -pub type UnmappedFrames = Frames<{MemoryState::Unmapped}>; - /// A range of contiguous frames. /// Owning a `Frames` object gives ownership of the range of frames it references. /// -/// The frames can be in an unmapped or mapped state. In the unmapped state, the frames are not -/// immediately accessible because they're not yet mapped by any virtual memory pages. -/// They are converted into a mapped state once they are used to create a `MappedPages` object. +/// The frames can be in a free, allocated, mapped or unmapped state. +/// In the free state, frames are owned by the frame allocator and have not been allocated for a mapping. +/// In the allocated state, frames have been removed from the free list and can be used for a mapping. +/// In the mapped state, frames have been mapped to a virtual memory page and are in use. +/// In the unmapped state, frames have been unmapped and can be retured to the frame allocator. /// -/// When a `Frames` object in an unmapped state is dropped, it is deallocated and returned to the free frames list. -/// We expect that `Frames` in a mapped state will never be dropped, but instead will be forgotten. +/// When a `Frames` object in a free state is dropped, it will be added back to the free list. +/// When a `Frames` object in an allocated state is dropped, it is transitioned to a free state. +/// When a `Frames` obect in an unmapped state is dropped, it is transitioned to an allocated state. +/// We expect that `Frames` in a mapped state will never be dropped, but instead will be forgotten and then +/// recreated in an unmapped state when it's frames are cleared from the page table. /// +/// (Free) <--> (Allocated) -> (Mapped) -> (Unmapped) -> (Allocated) +/// /// # Ordering and Equality /// /// `Frames` implements the `Ord` trait, and its total ordering is ONLY based on @@ -352,6 +355,12 @@ pub struct Frames { frames: FrameRange } +// Type aliases for the Frames in each state +pub type FreeFrames = Frames<{MemoryState::Free}>; +pub type AllocatedFrames = Frames<{MemoryState::Allocated}>; +pub type MappedFrames = Frames<{MemoryState::Mapped}>; +pub type UnmappedFrames = Frames<{MemoryState::Unmapped}>; + // Frames must not be Cloneable, and it must not expose its inner frames as mutable. assert_not_impl_any!(Frames<{MemoryState::Free}>: DerefMut, Clone); assert_not_impl_any!(Frames<{MemoryState::Allocated}>: DerefMut, Clone); @@ -420,7 +429,7 @@ impl UnmappedFrames { /// This function is a callback used to convert `UnmappedFramesInfo` into `UnmappedFrames`. /// `UnmappedFrames` represents frames that have been unmapped from a page that had -/// exclusively mapped them, indicating that no others pages have been mapped +/// exclusively mapped to them, indicating that no others pages have been mapped /// to those same frames, and thus, they can be safely deallocated. /// /// This exists to break the cyclic dependency cycle between this crate and @@ -461,14 +470,15 @@ impl Drop for Frames { } // For full-fledged deallocations, use the entry API to efficiently determine if - // we can merge the deallocated pages with an existing contiguously-adjactent chunk + // we can merge the deallocated frames with an existing contiguously-adjacent chunk // or if we need to insert a new chunk. Inner::RBTree(ref mut tree) => { let mut cursor_mut = tree.lower_bound_mut(Bound::Included(free_frames.start())); if let Some(next_frames) = cursor_mut.get_mut() { if *free_frames.end() + 1 == *next_frames.start() { - trace!("Prepending {:?} onto beg of next {:?}", free_frames, next_frames.deref().deref()); + // trace!("Prepending {:?} onto beg of next {:?}", free_frames, next_frames.deref().deref()); if next_frames.merge(free_frames).is_ok() { + // trace!("newly merged next chunk: {:?}", next_frames); return; } else { panic!("BUG: couldn't merge deallocated chunk into next chunk"); @@ -477,10 +487,11 @@ impl Drop for Frames { } if let Some(prev_frames) = cursor_mut.peek_prev().get() { if *prev_frames.end() + 1 == *free_frames.start() { - trace!("Appending {:?} onto end of prev {:?}", free_frames, prev_frames.deref()); + // trace!("Appending {:?} onto end of prev {:?}", free_frames, prev_frames.deref()); cursor_mut.move_prev(); if let Some(prev_frames) = cursor_mut.get_mut() { if prev_frames.merge(free_frames).is_ok() { + // trace!("newly merged prev chunk: {:?}", prev_frames); return; } else { panic!("BUG: couldn't merge deallocated chunk into prev chunk"); @@ -489,14 +500,14 @@ impl Drop for Frames { } } - trace!("Inserting new chunk for deallocated {:?} ", free_frames); + // trace!("Inserting new chunk for deallocated {:?} ", free_frames); cursor_mut.insert(Wrapper::new_link(free_frames)); return; } } }, MemoryState::Allocated => { - trace!("Converting AllocatedFrames to FreeFrames. Drop handler should be called again {:?}", self.frames); + // trace!("Converting AllocatedFrames to FreeFrames. Drop handler should be called again {:?}", self.frames); FreeFrames::new(self.typ, self.frames.clone()); }, MemoryState::Mapped => panic!("We should never drop a mapped frame! It should be forgotten instead."), @@ -557,13 +568,13 @@ impl<'f> Deref for AllocatedFrame<'f> { } assert_not_impl_any!(AllocatedFrame: DerefMut, Clone); +/// The result of splitting a `Frames` object into multiple smaller `Frames` objects. pub struct SplitFrames { before_start: Option>, start_to_end: Frames, after_end: Option>, } - impl Frames { #[allow(dead_code)] pub(crate) fn frames(&self) -> FrameRange { @@ -616,12 +627,12 @@ impl Frames { /// Splits up the given `Frames` into multiple smaller `Frames`. /// - /// Returns a tuple of three `Frames`: + /// Returns a `SplitFrames` instance containing three `Frames`: /// 2. The range of frames in the `self` that came before the beginning of the requested frame range. - /// 1. The `Frames` containing the requested range of frames starting at `frames.start`. + /// 1. The `Frames` containing the requested range of frames, `frames_to_extract`. /// 3. The range of frames in the `self` that came after the end of the requested frame range. /// - /// If `frames` is not contained within `self`, then `self` is not changed and we return it within an Err. + /// If `frames_to_extract` is not contained within `self`, then `self` is not changed and we return it within an Err. pub fn split_range( self, frames_to_extract: FrameRange @@ -633,6 +644,7 @@ impl Frames { let start_frame = *frames_to_extract.start(); let start_to_end = Frames{ frames: frames_to_extract, ..self }; + let before_start = if start_frame == MIN_FRAME || start_frame == *self.start() { None } else { @@ -646,7 +658,7 @@ impl Frames { }; core::mem::forget(self); - Ok(SplitFrames{ before_start, start_to_end, after_end}) + Ok( SplitFrames{ before_start, start_to_end, after_end} ) } /// Splits this `Frames` into two separate `Frames` objects: @@ -974,7 +986,7 @@ fn allocate_from_chosen_chunk( .expect("BUG: Failed to retrieve chunk from free list"); // This should always succeed, since we've already checked the conditions for a merge and split. - // We should return the next_chunk back to the list, but a failure at this point implies a bug in the frame allocator. + // We should return the chunks back to the list, but a failure at this point implies a bug in the frame allocator. if let Some(chunk) = next_chunk { chosen_chunk.merge(chunk).expect("BUG: Failed to merge adjacent chunks"); @@ -1026,7 +1038,7 @@ fn contains_any( false } -/// Adds the given `frames` to the given `regions_list` and `frames_list` as a Chunk of reserved frames. +/// Adds the given `frames` to the given `regions_list` and `frames_list` as a chunk of reserved frames. /// /// Returns the range of **new** frames that were added to the lists, /// which will be a subset of the given input `frames`. @@ -1040,6 +1052,7 @@ fn add_reserved_region_to_lists( frames: FrameRange, ) -> Result { + // first check the regions list for overlaps and proceed only if there are none. if !contains_any(®ions_list, &frames){ // Check whether the reserved region overlaps any existing regions. match &mut frames_list.0 { @@ -1049,7 +1062,7 @@ fn add_reserved_region_to_lists( // trace!("Failed to add reserved region {:?} due to overlap {:?} with existing chunk {:?}", // frames, _overlap, chunk // ); - return Err("Failed to add free franes that overlapped with existing frames (array)."); + return Err("Failed to add free frames that overlapped with existing frames (array)."); } } } diff --git a/kernel/memory_structs/src/lib.rs b/kernel/memory_structs/src/lib.rs index f9a2c79d5c..64be497ff5 100644 --- a/kernel/memory_structs/src/lib.rs +++ b/kernel/memory_structs/src/lib.rs @@ -24,10 +24,15 @@ use derive_more::*; use range_inclusive::{RangeInclusive, RangeInclusiveIterator}; #[derive(PartialEq, Eq, ConstParamTy)] +/// The possible states of a page or frame in virtual and physical memory. pub enum MemoryState { + /// Memory is free and owned by the allocator Free, + /// Memory is allocated and can be used for a mapping Allocated, + /// Memory is mapped (PTE has been set) Mapped, + /// Memory has been unmapped (PTE has been cleared) Unmapped } From 4c8d2ed98a66fa7e4fcc9d50e9e95f0c775d6cc5 Mon Sep 17 00:00:00 2001 From: ramla-i Date: Fri, 21 Jul 2023 15:10:34 -0400 Subject: [PATCH 16/40] comments and cleanup --- kernel/frame_allocator/Cargo.toml | 2 +- kernel/memory_structs/src/lib.rs | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/kernel/frame_allocator/Cargo.toml b/kernel/frame_allocator/Cargo.toml index 6392004606..dcc7c3c865 100644 --- a/kernel/frame_allocator/Cargo.toml +++ b/kernel/frame_allocator/Cargo.toml @@ -16,4 +16,4 @@ kernel_config = { path = "../kernel_config" } memory_structs = { path = "../memory_structs" } [dependencies.intrusive-collections] -git = "https://github.com/Ramla-I/intrusive-rs" \ No newline at end of file +git = "https://github.com/Ramla-I/intrusive-rs" diff --git a/kernel/memory_structs/src/lib.rs b/kernel/memory_structs/src/lib.rs index 64be497ff5..e22b4a398d 100644 --- a/kernel/memory_structs/src/lib.rs +++ b/kernel/memory_structs/src/lib.rs @@ -489,9 +489,7 @@ macro_rules! implement_page_frame_range { #[doc = "Returns true if `other` is contained within the `" $TypeName "`."] pub fn contains_range(&self, other: &$TypeName) -> bool { - if (other.[]() != 0) - && (other.start() >= self.start()) - && (*other.start() + (other.[]() - 1) <= *self.end()) + if !other.is_empty() && (other.start() >= self.start()) && (other.end() <= self.end()) { true } else { From 906a7b3c469ef78932a913674a72ea06361d81a5 Mon Sep 17 00:00:00 2001 From: Ramla-I Date: Mon, 24 Jul 2023 11:38:31 -0400 Subject: [PATCH 17/40] Update kernel/frame_allocator/src/lib.rs Frames documentation Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> --- kernel/frame_allocator/src/lib.rs | 34 +++++++++++++++++++------------ 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index 3ddd8f625e..ea40648466 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -322,21 +322,29 @@ pub enum MemoryRegionType { } /// A range of contiguous frames. -/// Owning a `Frames` object gives ownership of the range of frames it references. -/// -/// The frames can be in a free, allocated, mapped or unmapped state. -/// In the free state, frames are owned by the frame allocator and have not been allocated for a mapping. -/// In the allocated state, frames have been removed from the free list and can be used for a mapping. -/// In the mapped state, frames have been mapped to a virtual memory page and are in use. -/// In the unmapped state, frames have been unmapped and can be retured to the frame allocator. +/// +/// Owning a `Frames` object implies globally-exclusive ownership of and access to +/// the range of frames it contains. /// -/// When a `Frames` object in a free state is dropped, it will be added back to the free list. -/// When a `Frames` object in an allocated state is dropped, it is transitioned to a free state. -/// When a `Frames` obect in an unmapped state is dropped, it is transitioned to an allocated state. -/// We expect that `Frames` in a mapped state will never be dropped, but instead will be forgotten and then -/// recreated in an unmapped state when it's frames are cleared from the page table. +/// A `Frames` object can be in one of four states: +/// * `Free`: frames are owned by the frame allocator and have not been allocated for any use. +/// * `Allocated`: frames have been removed from the allocator's free list and are owned elsewhere; +/// they can now be used for mapping purposes. +/// * `Mapped`: frames have been (and are currently) mapped by a range of virtual memory pages. +/// * `Unmapped`: frames have been unmapped and can be returned to the frame allocator. +/// +/// The drop behavior for a `Frames` object is based on its state: +/// * `Free`: the frames will be added back to the frame allocator's free list. +/// * `Allocated`: the frames will be transitioned into the `Free` state. +/// * `Unmapped`: the frames will be transitioned into the `Allocated` state. +/// * `Mapped`: currently, Theseus does not actually drop mapped `Frames`, but rather they are forgotten +/// when they are mapped by virtual pages, and then re-created in the `Unmapped` state +/// after being unmapped from the page tables. /// -/// (Free) <--> (Allocated) -> (Mapped) -> (Unmapped) -> (Allocated) +/// As such, one can visualize the `Frames` state diagram as such: +/// ``` +/// (Free) <---> (Allocated) --> (Mapped) --> (Unmapped) --> (Allocated) <---> (Free) +/// ``` /// /// # Ordering and Equality /// From 2d4987ea37585b9e4ef0dba65d2cb8042c2b35f3 Mon Sep 17 00:00:00 2001 From: Ramla-I Date: Mon, 24 Jul 2023 11:39:12 -0400 Subject: [PATCH 18/40] Update kernel/frame_allocator/src/lib.rs Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> --- kernel/frame_allocator/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index ea40648466..a3e521665c 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -363,10 +363,13 @@ pub struct Frames { frames: FrameRange } -// Type aliases for the Frames in each state +/// A type alias for `Frames` in the `Free` state. pub type FreeFrames = Frames<{MemoryState::Free}>; +/// A type alias for `Frames` in the `Allocated` state. pub type AllocatedFrames = Frames<{MemoryState::Allocated}>; +/// A type alias for `Frames` in the `Mapped` state. pub type MappedFrames = Frames<{MemoryState::Mapped}>; +/// A type alias for `Frames` in the `Unmapped` state. pub type UnmappedFrames = Frames<{MemoryState::Unmapped}>; // Frames must not be Cloneable, and it must not expose its inner frames as mutable. From 6abc6aa36cbca9ea35728dedb7a2b752c9d93ee5 Mon Sep 17 00:00:00 2001 From: Ramla-I Date: Mon, 24 Jul 2023 11:39:50 -0400 Subject: [PATCH 19/40] Update kernel/frame_allocator/src/lib.rs Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> --- kernel/frame_allocator/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index a3e521665c..201e823987 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -389,7 +389,8 @@ impl FreeFrames { } } - /// Consumes the `Frames` in an free state and converts them to `Frames` in a allocated state. + /// Consumes this `Frames` in the `Free` state and converts them into the `Allocated` state. + pub fn into_allocated_frames(self) -> AllocatedFrames { let f = Frames { typ: self.typ, From a0c471d98b9cfa63729ed006f564b9854815682c Mon Sep 17 00:00:00 2001 From: Ramla-I Date: Mon, 24 Jul 2023 11:40:09 -0400 Subject: [PATCH 20/40] Update kernel/frame_allocator/src/lib.rs Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> --- kernel/frame_allocator/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index 201e823987..01e62ce0d1 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -380,7 +380,7 @@ assert_not_impl_any!(Frames<{MemoryState::Unmapped}>: DerefMut, Clone); impl FreeFrames { - /// Creates a new `Frames` object in an free state. + /// Creates a new `Frames` object in the `Free` state. /// The frame allocator is reponsible for making sure no two `Frames` objects overlap. pub(crate) fn new(typ: MemoryRegionType, frames: FrameRange) -> Self { Frames { From bc69076f6f3a483ef26bc74494236b4b3b2810bb Mon Sep 17 00:00:00 2001 From: Ramla-I Date: Mon, 24 Jul 2023 11:40:45 -0400 Subject: [PATCH 21/40] Update kernel/frame_allocator/src/lib.rs Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> --- kernel/frame_allocator/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index 01e62ce0d1..c21194f122 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -402,7 +402,7 @@ impl FreeFrames { } impl AllocatedFrames { - /// Consumes the `Frames` in an allocated state and converts them to `Frames` in a mapped state. + /// Consumes this `Frames` in the `Allocated` state and converts them into the `Mapped` state. /// This should only be called once a `MappedPages` has been created from the `Frames`. pub fn into_mapped_frames(self) -> MappedFrames { let f = Frames { From c6ce7ce872198d62af7f6f7e696b8f8554d43fdd Mon Sep 17 00:00:00 2001 From: Ramla-I Date: Mon, 24 Jul 2023 11:41:05 -0400 Subject: [PATCH 22/40] Update kernel/memory_structs/src/lib.rs Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> --- kernel/memory_structs/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/memory_structs/src/lib.rs b/kernel/memory_structs/src/lib.rs index e22b4a398d..8bbf07c0a8 100644 --- a/kernel/memory_structs/src/lib.rs +++ b/kernel/memory_structs/src/lib.rs @@ -14,8 +14,8 @@ use core::{ cmp::{min, max}, fmt, iter::Step, + marker::ConstParamTy, ops::{Add, AddAssign, Deref, DerefMut, Sub, SubAssign}, - marker::ConstParamTy }; use kernel_config::memory::{MAX_PAGE_NUMBER, PAGE_SIZE}; use zerocopy::FromBytes; From dc40d1fd7b9c6e6e6c37b25305d5c17451a94e01 Mon Sep 17 00:00:00 2001 From: Ramla-I Date: Mon, 24 Jul 2023 11:42:40 -0400 Subject: [PATCH 23/40] Update kernel/frame_allocator/src/lib.rs Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> --- kernel/frame_allocator/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index c21194f122..9e79910b4b 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -1100,7 +1100,7 @@ fn add_reserved_region_to_lists( regions_list.insert(PhysicalMemoryRegion { typ: MemoryRegionType::Reserved, frames: frames.clone(), - }).map_err(|_c| "BUG: Failed to insert non-overlapping frames into list.")?; + }).map_err(|_c| "BUG: Failed to insert non-overlapping physical memory region into reserved regions list.")?; frames_list.insert(Frames::new( MemoryRegionType::Reserved, From 01fd3981efe9192ab4c284babe8abe60e01d20a6 Mon Sep 17 00:00:00 2001 From: Ramla-I Date: Mon, 24 Jul 2023 11:43:11 -0400 Subject: [PATCH 24/40] Update kernel/memory_structs/src/lib.rs Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> --- kernel/memory_structs/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/memory_structs/src/lib.rs b/kernel/memory_structs/src/lib.rs index 8bbf07c0a8..bc70eb0c9f 100644 --- a/kernel/memory_structs/src/lib.rs +++ b/kernel/memory_structs/src/lib.rs @@ -23,8 +23,8 @@ use paste::paste; use derive_more::*; use range_inclusive::{RangeInclusive, RangeInclusiveIterator}; +/// The possible states that a range of exclusively-owned pages or frames can be in. #[derive(PartialEq, Eq, ConstParamTy)] -/// The possible states of a page or frame in virtual and physical memory. pub enum MemoryState { /// Memory is free and owned by the allocator Free, From b786c22001d5fdae7fc14412bd23e3d88914d62f Mon Sep 17 00:00:00 2001 From: Ramla-I Date: Mon, 24 Jul 2023 11:43:41 -0400 Subject: [PATCH 25/40] Update kernel/frame_allocator/src/lib.rs Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> --- kernel/frame_allocator/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index 9e79910b4b..49ae1c9527 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -427,7 +427,7 @@ impl AllocatedFrames { } impl UnmappedFrames { - /// Consumes the `Frames` in an unmapped state and converts them to `Frames` in an allocated state. + /// Consumes this `Frames` in the `Unmapped` state and converts them into the `Allocated` state. pub fn into_allocated_frames(self) -> AllocatedFrames { let f = Frames { typ: self.typ, From ea87a4a01a159cf88941452dd263a37713c5f343 Mon Sep 17 00:00:00 2001 From: Ramla-I Date: Mon, 24 Jul 2023 11:44:35 -0400 Subject: [PATCH 26/40] Update kernel/frame_allocator/src/lib.rs Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> --- kernel/frame_allocator/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index 49ae1c9527..08ace04226 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -640,11 +640,11 @@ impl Frames { /// Splits up the given `Frames` into multiple smaller `Frames`. /// /// Returns a `SplitFrames` instance containing three `Frames`: - /// 2. The range of frames in the `self` that came before the beginning of the requested frame range. - /// 1. The `Frames` containing the requested range of frames, `frames_to_extract`. - /// 3. The range of frames in the `self` that came after the end of the requested frame range. + /// 1. The range of frames in `self` that are before the beginning of `frames_to_extract`. + /// 2. The `Frames` containing the requested range of frames, `frames_to_extract`. + /// 3. The range of frames in `self` that are after the end of `frames_to_extract`. /// - /// If `frames_to_extract` is not contained within `self`, then `self` is not changed and we return it within an Err. + /// If `frames_to_extract` is not contained within `self`, then `self` is returned unchanged within an `Err`. pub fn split_range( self, frames_to_extract: FrameRange From cc822c68cff8a1a5ee31070998cc5a0b70d226b6 Mon Sep 17 00:00:00 2001 From: Ramla-I Date: Mon, 24 Jul 2023 11:46:31 -0400 Subject: [PATCH 27/40] Update kernel/frame_allocator/src/lib.rs Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> --- kernel/frame_allocator/src/lib.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index 08ace04226..fbb006670c 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -655,22 +655,22 @@ impl Frames { } let start_frame = *frames_to_extract.start(); - let start_to_end = Frames{ frames: frames_to_extract, ..self }; + let start_to_end = Frames { frames: frames_to_extract, ..self }; let before_start = if start_frame == MIN_FRAME || start_frame == *self.start() { None } else { - Some(Frames{ frames: FrameRange::new(*self.start(), *start_to_end.start() - 1), ..self }) + Some(Frames { frames: FrameRange::new(*self.start(), *start_to_end.start() - 1), ..self }) }; - let after_end = if *start_to_end.end() == MAX_FRAME || *start_to_end.end() == *self.end(){ + let after_end = if *start_to_end.end() == MAX_FRAME || *start_to_end.end() == *self.end() { None } else { - Some(Frames{ frames: FrameRange::new(*start_to_end.end() + 1, *self.end()), ..self }) + Some(Frames { frames: FrameRange::new(*start_to_end.end() + 1, *self.end()), ..self }) }; core::mem::forget(self); - Ok( SplitFrames{ before_start, start_to_end, after_end} ) + Ok(SplitFrames { before_start, start_to_end, after_end }) } /// Splits this `Frames` into two separate `Frames` objects: From 58d25aa08d93351795bb283d75f670a4c60adf92 Mon Sep 17 00:00:00 2001 From: Ramla-I Date: Mon, 24 Jul 2023 11:47:13 -0400 Subject: [PATCH 28/40] Update kernel/frame_allocator/src/lib.rs Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> --- kernel/frame_allocator/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index fbb006670c..b25f017caa 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -748,7 +748,7 @@ impl Borrow for &'_ Frames { impl fmt::Debug for Frames { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "Frames({:?}, {:?})", self.typ, self.frames) + write!(f, "Frames({:?}, {:?})", self.frames, self.typ) } } From 66c0b26efd0000cc4c00a1e7be9a909112d5afd3 Mon Sep 17 00:00:00 2001 From: ramla-i Date: Mon, 24 Jul 2023 15:25:12 -0400 Subject: [PATCH 29/40] small changes --- Cargo.lock | 23 ++---------- kernel/frame_allocator/Cargo.toml | 3 +- kernel/frame_allocator/src/lib.rs | 61 ++++++++++++++++++++----------- kernel/memory_structs/src/lib.rs | 7 +--- 4 files changed, 45 insertions(+), 49 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ca082f0bdb..0914694399 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1183,7 +1183,7 @@ dependencies = [ name = "frame_allocator" version = "0.1.0" dependencies = [ - "intrusive-collections 0.9.5", + "intrusive-collections", "kernel_config", "log", "memory_structs", @@ -1602,14 +1602,6 @@ dependencies = [ "memoffset 0.5.6", ] -[[package]] -name = "intrusive-collections" -version = "0.9.5" -source = "git+https://github.com/Ramla-I/intrusive-rs#bfae69d5209c082b642a26e30c1345e5c50a1e88" -dependencies = [ - "memoffset 0.9.0", -] - [[package]] name = "io" version = "0.1.0" @@ -1987,15 +1979,6 @@ dependencies = [ "autocfg", ] -[[package]] -name = "memoffset" -version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5a634b1c61a95585bd15607c6ab0c4e5b226e695ff2800ba0cdccddf208c406c" -dependencies = [ - "autocfg", -] - [[package]] name = "memory" version = "0.1.0" @@ -2276,7 +2259,7 @@ dependencies = [ "cfg-if 0.1.10", "hashbrown", "heap", - "intrusive-collections 0.9.0", + "intrusive-collections", "irq_safety", "kernel_config", "log", @@ -2547,7 +2530,7 @@ dependencies = [ name = "page_allocator" version = "0.1.0" dependencies = [ - "intrusive-collections 0.9.0", + "intrusive-collections", "kernel_config", "log", "memory_structs", diff --git a/kernel/frame_allocator/Cargo.toml b/kernel/frame_allocator/Cargo.toml index dcc7c3c865..6f49678c46 100644 --- a/kernel/frame_allocator/Cargo.toml +++ b/kernel/frame_allocator/Cargo.toml @@ -9,11 +9,10 @@ edition = "2021" log = "0.4.8" spin = "0.9.4" static_assertions = "1.1.0" +intrusive-collections = "0.9.0" range_inclusive = { path = "../../libs/range_inclusive" } kernel_config = { path = "../kernel_config" } memory_structs = { path = "../memory_structs" } -[dependencies.intrusive-collections] -git = "https://github.com/Ramla-I/intrusive-rs" diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index b25f017caa..aadbd7575c 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -355,6 +355,11 @@ pub enum MemoryRegionType { /// both of which are also based ONLY on the **starting** `Frame` of the `Frames`. /// Thus, comparing two `Frames` with the `==` or `!=` operators may not work as expected. /// since it ignores their actual range of frames. +/// +/// Similarly, `Frames` implements the `Borrow` trait to return a `Frame`, +/// not a `FrameRange`. This is required so we can search for `Frames` in a sorted collection +/// using a `Frame` value. +/// It differs from the behavior of the `Deref` trait which returns a `FrameRange`. #[derive(Eq)] pub struct Frames { /// The type of this memory chunk, e.g., whether it's in a free or reserved region. @@ -462,7 +467,7 @@ impl Drop for Frames { MemoryState::Free => { if self.size_in_frames() == 0 { return; } - let free_frames: FreeFrames = Frames { + let mut free_frames: FreeFrames = Frames { typ: self.typ, frames: self.frames.clone(), }; @@ -486,25 +491,45 @@ impl Drop for Frames { // or if we need to insert a new chunk. Inner::RBTree(ref mut tree) => { let mut cursor_mut = tree.lower_bound_mut(Bound::Included(free_frames.start())); - if let Some(next_frames) = cursor_mut.get_mut() { - if *free_frames.end() + 1 == *next_frames.start() { - // trace!("Prepending {:?} onto beg of next {:?}", free_frames, next_frames.deref().deref()); + if let Some(next_frames_ref) = cursor_mut.get() { + if *free_frames.end() + 1 == *next_frames_ref.start() { + // extract the next chunk from the list + let mut next_frames = cursor_mut + .replace_with(Wrapper::new_link(Frames::empty())) + .expect("BUG: couldn't remove next frames from free list in drop handler") + .into_inner(); + + // trace!("Prepending {:?} onto beg of next {:?}", free_frames, next_frames); if next_frames.merge(free_frames).is_ok() { // trace!("newly merged next chunk: {:?}", next_frames); - return; + // now return newly merged chunk into list + match cursor_mut.replace_with(Wrapper::new_link(next_frames)) { + Ok(_) => { return; } + Err(f) => free_frames = f.into_inner(), + } } else { panic!("BUG: couldn't merge deallocated chunk into next chunk"); } } } - if let Some(prev_frames) = cursor_mut.peek_prev().get() { - if *prev_frames.end() + 1 == *free_frames.start() { + if let Some(prev_frames_ref) = cursor_mut.peek_prev().get() { + if *prev_frames_ref.end() + 1 == *free_frames.start() { // trace!("Appending {:?} onto end of prev {:?}", free_frames, prev_frames.deref()); cursor_mut.move_prev(); - if let Some(prev_frames) = cursor_mut.get_mut() { + if let Some(_prev_frames_ref) = cursor_mut.get() { + // extract the next chunk from the list + let mut prev_frames = cursor_mut + .replace_with(Wrapper::new_link(Frames::empty())) + .expect("BUG: couldn't remove next frames from free list in drop handler") + .into_inner(); + if prev_frames.merge(free_frames).is_ok() { // trace!("newly merged prev chunk: {:?}", prev_frames); - return; + // now return newly merged chunk into list + match cursor_mut.replace_with(Wrapper::new_link(prev_frames)) { + Ok(_) => { return; } + Err(f) => free_frames = f.into_inner(), + } } else { panic!("BUG: couldn't merge deallocated chunk into prev chunk"); } @@ -582,17 +607,12 @@ assert_not_impl_any!(AllocatedFrame: DerefMut, Clone); /// The result of splitting a `Frames` object into multiple smaller `Frames` objects. pub struct SplitFrames { - before_start: Option>, - start_to_end: Frames, - after_end: Option>, + before_start: Option>, + start_to_end: Frames, + after_end: Option>, } impl Frames { - #[allow(dead_code)] - pub(crate) fn frames(&self) -> FrameRange { - self.frames.clone() - } - pub(crate) fn typ(&self) -> MemoryRegionType { self.typ } @@ -1065,7 +1085,9 @@ fn add_reserved_region_to_lists( ) -> Result { // first check the regions list for overlaps and proceed only if there are none. - if !contains_any(®ions_list, &frames){ + if contains_any(®ions_list, &frames){ + return Err("Failed to add reserved region that overlapped with existing reserved regions."); + } else { // Check whether the reserved region overlaps any existing regions. match &mut frames_list.0 { Inner::Array(ref mut arr) => { @@ -1106,10 +1128,7 @@ fn add_reserved_region_to_lists( MemoryRegionType::Reserved, frames.clone(), )).map_err(|_c| "BUG: Failed to insert non-overlapping frames into list.")?; - } else { - return Err("Failed to add reserved region that overlapped with existing reserved regions."); } - Ok(frames) } diff --git a/kernel/memory_structs/src/lib.rs b/kernel/memory_structs/src/lib.rs index bc70eb0c9f..b048ae3f19 100644 --- a/kernel/memory_structs/src/lib.rs +++ b/kernel/memory_structs/src/lib.rs @@ -489,12 +489,7 @@ macro_rules! implement_page_frame_range { #[doc = "Returns true if `other` is contained within the `" $TypeName "`."] pub fn contains_range(&self, other: &$TypeName) -> bool { - if !other.is_empty() && (other.start() >= self.start()) && (other.end() <= self.end()) - { - true - } else { - false - } + !other.is_empty() && (other.start() >= self.start()) && (other.end() <= self.end()) } } impl fmt::Debug for $TypeName { From a9ac710cc270f42957308ff9a9534611d6b4d9bc Mon Sep 17 00:00:00 2001 From: ramla-i Date: Mon, 24 Jul 2023 16:03:57 -0400 Subject: [PATCH 30/40] changed UnmappedFramesInfo to UnmappedFrameRange --- kernel/frame_allocator/src/lib.rs | 10 +++++----- kernel/memory/src/paging/mapper.rs | 6 +++--- kernel/memory/src/paging/mod.rs | 2 +- kernel/page_table_entry/src/lib.rs | 12 ++++++------ 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index aadbd7575c..ef8532d3c6 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -444,7 +444,7 @@ impl UnmappedFrames { } -/// This function is a callback used to convert `UnmappedFramesInfo` into `UnmappedFrames`. +/// This function is a callback used to convert `UnmappedFrameRange` into `UnmappedFrames`. /// `UnmappedFrames` represents frames that have been unmapped from a page that had /// exclusively mapped to them, indicating that no others pages have been mapped /// to those same frames, and thus, they can be safely deallocated. @@ -461,6 +461,7 @@ pub(crate) fn into_unmapped_frames(frames: FrameRange) -> UnmappedFrames { Frames{ typ, frames } } + impl Drop for Frames { fn drop(&mut self) { match S { @@ -481,8 +482,8 @@ impl Drop for Frames { match &mut list.0 { // For early allocations, just add the deallocated chunk to the free pages list. Inner::Array(_) => { - if list.insert(free_frames).is_ok() { - return; + if list.insert(free_frames).is_err() { + panic!("BUG: Failed to insert frames into list(array) in the drop handler"); } } @@ -539,7 +540,6 @@ impl Drop for Frames { // trace!("Inserting new chunk for deallocated {:?} ", free_frames); cursor_mut.insert(Wrapper::new_link(free_frames)); - return; } } }, @@ -1085,7 +1085,7 @@ fn add_reserved_region_to_lists( ) -> Result { // first check the regions list for overlaps and proceed only if there are none. - if contains_any(®ions_list, &frames){ + if contains_any(regions_list, &frames){ return Err("Failed to add reserved region that overlapped with existing reserved regions."); } else { // Check whether the reserved region overlaps any existing regions. diff --git a/kernel/memory/src/paging/mapper.rs b/kernel/memory/src/paging/mapper.rs index 802d479b95..af7ad288b4 100644 --- a/kernel/memory/src/paging/mapper.rs +++ b/kernel/memory/src/paging/mapper.rs @@ -34,11 +34,11 @@ use owned_borrowed_trait::{OwnedOrBorrowed, Owned, Borrowed}; #[cfg(target_arch = "x86_64")] use kernel_config::memory::ENTRIES_PER_PAGE_TABLE; -/// This is a private callback used to convert `UnmappedFramesInfo` into `UnmappedFrames`. +/// This is a private callback used to convert `UnmappedFrameRange` into `UnmappedFrames`. /// /// This exists to break the cyclic dependency cycle between `page_table_entry` and /// `frame_allocator`, which depend on each other as such: -/// * `frame_allocator` needs to `impl Into for UnmappedFramesInfo` +/// * `frame_allocator` needs to `impl Into for UnmappedFrameRange` /// in order to allow unmapped exclusive frames to be safely deallocated /// * `page_table_entry` needs to use the `AllocatedFrames` type in order to allow /// page table entry values to be set safely to a real physical frame that is owned and exists. @@ -49,7 +49,7 @@ use kernel_config::memory::ENTRIES_PER_PAGE_TABLE; /// /// This is safe because the frame allocator can only be initialized once, and also because /// only this crate has access to that function callback and can thus guarantee -/// that it is only invoked for `UnmappedFramesInfo`. +/// that it is only invoked for `UnmappedFrameRange`. pub(super) static INTO_UNMAPPED_FRAMES_FUNC: Once UnmappedFrames> = Once::new(); /// A convenience function to translate the given virtual address into a diff --git a/kernel/memory/src/paging/mod.rs b/kernel/memory/src/paging/mod.rs index 700b1a6ef2..92093c1f49 100644 --- a/kernel/memory/src/paging/mod.rs +++ b/kernel/memory/src/paging/mod.rs @@ -226,7 +226,7 @@ pub fn init( into_unmapped_frames_fn: fn(FrameRange) -> UnmappedFrames, ) -> Result { // Store the callback from `frame_allocator::init()` that allows the `Mapper` to convert - // `page_table_entry::UnmappedFramesInfo` back into `UnmappedFrames`. + // `page_table_entry::UnmappedFrameRange` back into `UnmappedFrames`. mapper::INTO_UNMAPPED_FRAMES_FUNC.call_once(|| into_unmapped_frames_fn); // bootstrap a PageTable from the currently-loaded page table diff --git a/kernel/page_table_entry/src/lib.rs b/kernel/page_table_entry/src/lib.rs index ed0abb0ac3..2606ed3039 100644 --- a/kernel/page_table_entry/src/lib.rs +++ b/kernel/page_table_entry/src/lib.rs @@ -57,7 +57,7 @@ impl PageTableEntry { // to specify whether this is a 4KiB, 2MiB, or 1GiB PTE. let frame_range = FrameRange::new(frame, frame); if flags.is_exclusive() { - UnmapResult::Exclusive(UnmappedFramesInfo(frame_range)) + UnmapResult::Exclusive(UnmappedFrameRange(frame_range)) } else { UnmapResult::NonExclusive(frame_range) } @@ -110,26 +110,26 @@ impl PageTableEntry { /// The frames returned from the action of unmapping a page table entry. /// See the `PageTableEntry::set_unmapped()` function. /// -/// If exclusive, the contained `UnmappedFramesInfo` can be used to deallocate frames. +/// If exclusive, the contained `UnmappedFrameRange` can be used to deallocate frames. /// /// If non-exclusive, the contained `FrameRange` is provided just for debugging feedback. /// Note that we use `FrameRange` instead of `Frame` because a single page table entry /// can map many frames, e.g., using huge pages. #[must_use] pub enum UnmapResult { - Exclusive(UnmappedFramesInfo), + Exclusive(UnmappedFrameRange), NonExclusive(FrameRange) } /// A range of frames that have been unmapped from a `PageTableEntry` /// that previously mapped that frame exclusively (i.e., "owned it"). /// -/// `UnmappedFramesInfo` can be used to create an `UnmappedFrames` +/// `UnmappedFrameRange` can be used to create an `UnmappedFrames` /// and then safely deallocated within the `frame_allocator`. /// /// See the `PageTableEntry::set_unmapped()` function. -pub struct UnmappedFramesInfo(FrameRange); -impl Deref for UnmappedFramesInfo { +pub struct UnmappedFrameRange(FrameRange); +impl Deref for UnmappedFrameRange { type Target = FrameRange; fn deref(&self) -> &FrameRange { &self.0 From 8639f12c7882c5d198d4556adb5c442062cecdd0 Mon Sep 17 00:00:00 2001 From: ramla-i Date: Mon, 24 Jul 2023 16:13:01 -0400 Subject: [PATCH 31/40] drop handler log statements --- kernel/frame_allocator/src/lib.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index ef8532d3c6..cacf46f328 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -482,8 +482,8 @@ impl Drop for Frames { match &mut list.0 { // For early allocations, just add the deallocated chunk to the free pages list. Inner::Array(_) => { - if list.insert(free_frames).is_err() { - panic!("BUG: Failed to insert frames into list(array) in the drop handler"); + if list.insert(free_frames).is_ok() { + return; } } @@ -540,8 +540,10 @@ impl Drop for Frames { // trace!("Inserting new chunk for deallocated {:?} ", free_frames); cursor_mut.insert(Wrapper::new_link(free_frames)); + return; } } + log::error!("BUG: couldn't insert deallocated {:?} into free frames list", self.frames); }, MemoryState::Allocated => { // trace!("Converting AllocatedFrames to FreeFrames. Drop handler should be called again {:?}", self.frames); From 7a6e458886d4a2eeabff7b0d5d9825bad121d4ff Mon Sep 17 00:00:00 2001 From: ramla-i Date: Mon, 24 Jul 2023 16:18:30 -0400 Subject: [PATCH 32/40] pr comments --- kernel/frame_allocator/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/frame_allocator/Cargo.toml b/kernel/frame_allocator/Cargo.toml index 6f49678c46..1f9340f6ba 100644 --- a/kernel/frame_allocator/Cargo.toml +++ b/kernel/frame_allocator/Cargo.toml @@ -8,8 +8,8 @@ edition = "2021" [dependencies] log = "0.4.8" spin = "0.9.4" -static_assertions = "1.1.0" intrusive-collections = "0.9.0" +static_assertions = "1.1.0" range_inclusive = { path = "../../libs/range_inclusive" } From 53a14b3af585b24dcd626d63d2df211cde334cb0 Mon Sep 17 00:00:00 2001 From: ramla-i Date: Mon, 24 Jul 2023 16:19:17 -0400 Subject: [PATCH 33/40] spacing --- kernel/frame_allocator/Cargo.toml | 1 - 1 file changed, 1 deletion(-) diff --git a/kernel/frame_allocator/Cargo.toml b/kernel/frame_allocator/Cargo.toml index 1f9340f6ba..db6a060ef4 100644 --- a/kernel/frame_allocator/Cargo.toml +++ b/kernel/frame_allocator/Cargo.toml @@ -15,4 +15,3 @@ range_inclusive = { path = "../../libs/range_inclusive" } kernel_config = { path = "../kernel_config" } memory_structs = { path = "../memory_structs" } - From 6f75236585ffa2d5c9dac36f47ef8a17405d25ab Mon Sep 17 00:00:00 2001 From: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> Date: Mon, 24 Jul 2023 14:21:41 -0700 Subject: [PATCH 34/40] improve formatting --- kernel/memory_structs/src/lib.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/kernel/memory_structs/src/lib.rs b/kernel/memory_structs/src/lib.rs index b048ae3f19..d7511fcd80 100644 --- a/kernel/memory_structs/src/lib.rs +++ b/kernel/memory_structs/src/lib.rs @@ -487,9 +487,11 @@ macro_rules! implement_page_frame_range { } } - #[doc = "Returns true if `other` is contained within the `" $TypeName "`."] + #[doc = "Returns `true` if the `other` `" $TypeName "` is fully contained within this `" $TypeName "`."] pub fn contains_range(&self, other: &$TypeName) -> bool { - !other.is_empty() && (other.start() >= self.start()) && (other.end() <= self.end()) + !other.is_empty() + && (other.start() >= self.start()) + && (other.end() <= self.end()) } } impl fmt::Debug for $TypeName { From a77b8b9693d5cb677623b1a3a215408e0b1f96cd Mon Sep 17 00:00:00 2001 From: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> Date: Mon, 24 Jul 2023 14:43:33 -0700 Subject: [PATCH 35/40] clarify `Frames` docs --- kernel/frame_allocator/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index cacf46f328..a5a829c4ff 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -321,10 +321,10 @@ pub enum MemoryRegionType { Unknown, } -/// A range of contiguous frames. +/// A range of contiguous frames in physical memory. /// -/// Owning a `Frames` object implies globally-exclusive ownership of and access to -/// the range of frames it contains. +/// Each `Frames` object is globally unique, meaning that the owner of a `Frames` object +/// has globally-exclusive access to the range of frames it contains. /// /// A `Frames` object can be in one of four states: /// * `Free`: frames are owned by the frame allocator and have not been allocated for any use. From 00cc97f2e3c3e3550b053265441736861300eb9a Mon Sep 17 00:00:00 2001 From: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> Date: Mon, 24 Jul 2023 15:03:06 -0700 Subject: [PATCH 36/40] improve clarity of docs / drop handler --- kernel/frame_allocator/src/lib.rs | 34 +++++++++++++++++-------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index a5a829c4ff..73e4d691e8 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -386,7 +386,8 @@ assert_not_impl_any!(Frames<{MemoryState::Unmapped}>: DerefMut, Clone); impl FreeFrames { /// Creates a new `Frames` object in the `Free` state. - /// The frame allocator is reponsible for making sure no two `Frames` objects overlap. + /// + /// The frame allocator logic is responsible for ensuring that no two `Frames` objects overlap. pub(crate) fn new(typ: MemoryRegionType, frames: FrameRange) -> Self { Frames { typ, @@ -395,7 +396,6 @@ impl FreeFrames { } /// Consumes this `Frames` in the `Free` state and converts them into the `Allocated` state. - pub fn into_allocated_frames(self) -> AllocatedFrames { let f = Frames { typ: self.typ, @@ -419,7 +419,7 @@ impl AllocatedFrames { } /// Returns an `AllocatedFrame` if this `AllocatedFrames` object contains only one frame. - /// + /// /// ## Panic /// Panics if this `AllocatedFrame` contains multiple frames or zero frames. pub fn as_allocated_frame(&self) -> AllocatedFrame { @@ -445,11 +445,12 @@ impl UnmappedFrames { /// This function is a callback used to convert `UnmappedFrameRange` into `UnmappedFrames`. -/// `UnmappedFrames` represents frames that have been unmapped from a page that had -/// exclusively mapped to them, indicating that no others pages have been mapped -/// to those same frames, and thus, they can be safely deallocated. -/// -/// This exists to break the cyclic dependency cycle between this crate and +/// +/// `UnmappedFrames` represents frames that have been unmapped by a page that had +/// previously exclusively mapped them, indicating that no others pages have been mapped +/// to those same frames, and thus, those frames can be safely deallocated. +/// +/// This exists to break the cyclic dependency chain between this crate and /// the `page_table_entry` crate, since `page_table_entry` must depend on types /// from this crate in order to enforce safety when modifying page table entries. pub(crate) fn into_unmapped_frames(frames: FrameRange) -> UnmappedFrames { @@ -544,13 +545,15 @@ impl Drop for Frames { } } log::error!("BUG: couldn't insert deallocated {:?} into free frames list", self.frames); - }, + } MemoryState::Allocated => { - // trace!("Converting AllocatedFrames to FreeFrames. Drop handler should be called again {:?}", self.frames); - FreeFrames::new(self.typ, self.frames.clone()); - }, + // trace!("Converting AllocatedFrames to FreeFrames. Drop handler will be called again {:?}", self.frames); + let _to_drop = FreeFrames::new(self.typ, self.frames.clone()); + } MemoryState::Mapped => panic!("We should never drop a mapped frame! It should be forgotten instead."), - MemoryState::Unmapped => { AllocatedFrames{ typ: self.typ, frames: self.frames.clone() }; }, + MemoryState::Unmapped => { + let _to_drop = AllocatedFrames { typ: self.typ, frames: self.frames.clone() }; + } } } } @@ -628,8 +631,9 @@ impl Frames { } } - /// Merges the given `Frames` object `other` into this `Frames` object (`self`). - /// This is just for convenience and usability purposes, it performs no allocation or remapping. + /// Merges the given `other` `Frames` object into this `Frames` object (`self`). + /// + /// This function performs no allocation or re-mapping, it exists for convenience and usability purposes. /// /// The given `other` must be physically contiguous with `self`, i.e., come immediately before or after `self`. /// That is, either `self.start == other.end + 1` or `self.end + 1 == other.start` must be true. From 42f7d8463a83fccb58fb1d93b63df50476de3523 Mon Sep 17 00:00:00 2001 From: Kevin Boos Date: Tue, 25 Jul 2023 12:42:47 -0700 Subject: [PATCH 37/40] fix formatting, use proper early return pattern --- kernel/frame_allocator/src/lib.rs | 78 ++++++++++++++++--------------- 1 file changed, 40 insertions(+), 38 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index 73e4d691e8..f4b6f9b3af 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -937,7 +937,7 @@ fn find_specific_chunk( // We would like to merge it into the initial chunk with just the reference (since we have a cursor pointing to it already), // but we can't get a mutable reference to the element the cursor is pointing to. // So both chunks will be removed and then merged. - return allocate_from_chosen_chunk(FrameRange::new(requested_frame, requested_frame + num_frames -1), ValueRefMut::RBTree(cursor_mut), Some(next_chunk)); + return allocate_from_chosen_chunk(FrameRange::new(requested_frame, requested_frame + num_frames - 1), ValueRefMut::RBTree(cursor_mut), Some(next_chunk)); } } } @@ -1030,7 +1030,8 @@ fn allocate_from_chosen_chunk( chosen_chunk.merge(chunk).expect("BUG: Failed to merge adjacent chunks"); } - let SplitFrames{ before_start, start_to_end: new_allocation, after_end } = chosen_chunk.split_range(frames_to_allocate) + let SplitFrames { before_start, start_to_end: new_allocation, after_end } = chosen_chunk + .split_range(frames_to_allocate) .expect("BUG: Failed to split merged chunk"); // TODO: Re-use the allocated wrapper if possible, rather than allocate a new one entirely. @@ -1093,48 +1094,49 @@ fn add_reserved_region_to_lists( // first check the regions list for overlaps and proceed only if there are none. if contains_any(regions_list, &frames){ return Err("Failed to add reserved region that overlapped with existing reserved regions."); - } else { - // Check whether the reserved region overlaps any existing regions. - match &mut frames_list.0 { - Inner::Array(ref mut arr) => { - for chunk in arr.iter().flatten() { - if let Some(_overlap) = chunk.overlap(&frames) { - // trace!("Failed to add reserved region {:?} due to overlap {:?} with existing chunk {:?}", - // frames, _overlap, chunk - // ); - return Err("Failed to add free frames that overlapped with existing frames (array)."); - } + } + + // Check whether the reserved region overlaps any existing regions. + match &mut frames_list.0 { + Inner::Array(ref mut arr) => { + for chunk in arr.iter().flatten() { + if let Some(_overlap) = chunk.overlap(&frames) { + // trace!("Failed to add reserved region {:?} due to overlap {:?} with existing chunk {:?}", + // frames, _overlap, chunk + // ); + return Err("Failed to add free frames that overlapped with existing frames (array)."); } } - Inner::RBTree(ref mut tree) => { - let mut cursor_mut = tree.upper_bound_mut(Bound::Included(frames.start())); - while let Some(chunk) = cursor_mut.get().map(|w| w.deref()) { - if chunk.start() > frames.end() { - // We're iterating in ascending order over a sorted tree, - // so we can stop looking for overlapping regions once we pass the end of the new frames to add. - break; - } - if let Some(_overlap) = chunk.overlap(&frames) { - // trace!("Failed to add reserved region {:?} due to overlap {:?} with existing chunk {:?}", - // frames, _overlap, chunk - // ); - return Err("Failed to add free frames that overlapped with existing frames (RBTree)."); - } - cursor_mut.move_next(); + } + Inner::RBTree(ref mut tree) => { + let mut cursor_mut = tree.upper_bound_mut(Bound::Included(frames.start())); + while let Some(chunk) = cursor_mut.get().map(|w| w.deref()) { + if chunk.start() > frames.end() { + // We're iterating in ascending order over a sorted tree, + // so we can stop looking for overlapping regions once we pass the end of the new frames to add. + break; } + if let Some(_overlap) = chunk.overlap(&frames) { + // trace!("Failed to add reserved region {:?} due to overlap {:?} with existing chunk {:?}", + // frames, _overlap, chunk + // ); + return Err("Failed to add free frames that overlapped with existing frames (RBTree)."); + } + cursor_mut.move_next(); } } - - regions_list.insert(PhysicalMemoryRegion { - typ: MemoryRegionType::Reserved, - frames: frames.clone(), - }).map_err(|_c| "BUG: Failed to insert non-overlapping physical memory region into reserved regions list.")?; - - frames_list.insert(Frames::new( - MemoryRegionType::Reserved, - frames.clone(), - )).map_err(|_c| "BUG: Failed to insert non-overlapping frames into list.")?; } + + regions_list.insert(PhysicalMemoryRegion { + typ: MemoryRegionType::Reserved, + frames: frames.clone(), + }).map_err(|_c| "BUG: Failed to insert non-overlapping physical memory region into reserved regions list.")?; + + frames_list.insert(Frames::new( + MemoryRegionType::Reserved, + frames.clone(), + )).map_err(|_c| "BUG: Failed to insert non-overlapping frames into list.")?; + Ok(frames) } From 43ecf9fecb0dd3d039bfcb5203b1b5953d444897 Mon Sep 17 00:00:00 2001 From: ramla-i Date: Tue, 25 Jul 2023 16:20:34 -0400 Subject: [PATCH 38/40] comments --- kernel/frame_allocator/src/lib.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index f4b6f9b3af..c41d0a9cc2 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -485,12 +485,13 @@ impl Drop for Frames { Inner::Array(_) => { if list.insert(free_frames).is_ok() { return; + } else { + error!("Failed to insert deallocated frames into the list (array). The initial static array should be created with a larger size."); } } - // For full-fledged deallocations, use the entry API to efficiently determine if - // we can merge the deallocated frames with an existing contiguously-adjacent chunk - // or if we need to insert a new chunk. + // For full-fledged deallocations, determine if we can merge the deallocated frames + // with an existing contiguously-adjacent chunk or if we need to insert a new chunk. Inner::RBTree(ref mut tree) => { let mut cursor_mut = tree.lower_bound_mut(Bound::Included(free_frames.start())); if let Some(next_frames_ref) = cursor_mut.get() { From 0d8ede6749cfc9bd62eca0037005f8bbd989162d Mon Sep 17 00:00:00 2001 From: ramla-i Date: Thu, 27 Jul 2023 15:47:35 -0400 Subject: [PATCH 39/40] make sure two frames objects never have the same range within the Frames methods --- kernel/frame_allocator/src/lib.rs | 62 ++++++++++++++++++------------- 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index c41d0a9cc2..103ac6e7fc 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -396,26 +396,28 @@ impl FreeFrames { } /// Consumes this `Frames` in the `Free` state and converts them into the `Allocated` state. - pub fn into_allocated_frames(self) -> AllocatedFrames { - let f = Frames { + pub fn into_allocated_frames(mut self) -> AllocatedFrames { + let frames = core::mem::replace(&mut self.frames, FrameRange::empty()); + let af = Frames { typ: self.typ, - frames: self.frames.clone(), + frames, }; core::mem::forget(self); - f + af } } impl AllocatedFrames { /// Consumes this `Frames` in the `Allocated` state and converts them into the `Mapped` state. /// This should only be called once a `MappedPages` has been created from the `Frames`. - pub fn into_mapped_frames(self) -> MappedFrames { - let f = Frames { + pub fn into_mapped_frames(mut self) -> MappedFrames { + let frames = core::mem::replace(&mut self.frames, FrameRange::empty()); + let mf = Frames { typ: self.typ, - frames: self.frames.clone(), + frames, }; core::mem::forget(self); - f + mf } /// Returns an `AllocatedFrame` if this `AllocatedFrames` object contains only one frame. @@ -433,13 +435,14 @@ impl AllocatedFrames { impl UnmappedFrames { /// Consumes this `Frames` in the `Unmapped` state and converts them into the `Allocated` state. - pub fn into_allocated_frames(self) -> AllocatedFrames { - let f = Frames { + pub fn into_allocated_frames(mut self) -> AllocatedFrames { + let frames = core::mem::replace(&mut self.frames, FrameRange::empty()); + let af = Frames { typ: self.typ, - frames: self.frames.clone(), + frames }; core::mem::forget(self); - f + af } } @@ -469,10 +472,8 @@ impl Drop for Frames { MemoryState::Free => { if self.size_in_frames() == 0 { return; } - let mut free_frames: FreeFrames = Frames { - typ: self.typ, - frames: self.frames.clone(), - }; + let frames = core::mem::replace(&mut self.frames, FrameRange::empty()); + let mut free_frames: FreeFrames = Frames { typ: self.typ, frames }; let mut list = if free_frames.typ == MemoryRegionType::Reserved { FREE_RESERVED_FRAMES_LIST.lock() @@ -549,11 +550,13 @@ impl Drop for Frames { } MemoryState::Allocated => { // trace!("Converting AllocatedFrames to FreeFrames. Drop handler will be called again {:?}", self.frames); - let _to_drop = FreeFrames::new(self.typ, self.frames.clone()); + let frames = core::mem::replace(&mut self.frames, FrameRange::empty()); + let _to_drop = FreeFrames { typ: self.typ, frames }; } MemoryState::Mapped => panic!("We should never drop a mapped frame! It should be forgotten instead."), MemoryState::Unmapped => { - let _to_drop = AllocatedFrames { typ: self.typ, frames: self.frames.clone() }; + let frames = core::mem::replace(&mut self.frames, FrameRange::empty()); + let _to_drop = AllocatedFrames { typ: self.typ, frames }; } } } @@ -646,21 +649,22 @@ impl Frames { return Err(other); } - if *self.start() == *other.end() + 1 { + let frames = if *self.start() == *other.end() + 1 { // `other` comes contiguously before `self` - self.frames = FrameRange::new(*other.start(), *self.end()); + FrameRange::new(*other.start(), *self.end()) } else if *self.end() + 1 == *other.start() { // `self` comes contiguously before `other` - self.frames = FrameRange::new(*self.start(), *other.end()); + FrameRange::new(*self.start(), *other.end()) } else { // non-contiguous return Err(other); - } + }; // ensure the now-merged Frames doesn't run its drop handler core::mem::forget(other); + self.frames = frames; Ok(()) } @@ -682,22 +686,28 @@ impl Frames { } let start_frame = *frames_to_extract.start(); - let start_to_end = Frames { frames: frames_to_extract, ..self }; + let start_to_end = frames_to_extract; let before_start = if start_frame == MIN_FRAME || start_frame == *self.start() { None } else { - Some(Frames { frames: FrameRange::new(*self.start(), *start_to_end.start() - 1), ..self }) + Some(FrameRange::new(*self.start(), *start_to_end.start() - 1)) }; let after_end = if *start_to_end.end() == MAX_FRAME || *start_to_end.end() == *self.end() { None } else { - Some(Frames { frames: FrameRange::new(*start_to_end.end() + 1, *self.end()), ..self }) + Some(FrameRange::new(*start_to_end.end() + 1, *self.end())) }; + let typ = self.typ; + // ensure the original Frames doesn't run its drop handler and free its frames. core::mem::forget(self); - Ok(SplitFrames { before_start, start_to_end, after_end }) + Ok(SplitFrames { + before_start: before_start.map(|frames| Frames { typ, frames }), + start_to_end: Frames { typ, frames: start_to_end }, + after_end: after_end.map(|frames| Frames { typ, frames }), + }) } /// Splits this `Frames` into two separate `Frames` objects: From cb1ec40556907403830bf2edfd3b520af030a1cc Mon Sep 17 00:00:00 2001 From: ramla-i Date: Thu, 27 Jul 2023 16:08:41 -0400 Subject: [PATCH 40/40] drop handler uses remove rather than replace_with --- kernel/frame_allocator/src/lib.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/kernel/frame_allocator/src/lib.rs b/kernel/frame_allocator/src/lib.rs index 103ac6e7fc..89488479b1 100644 --- a/kernel/frame_allocator/src/lib.rs +++ b/kernel/frame_allocator/src/lib.rs @@ -473,7 +473,7 @@ impl Drop for Frames { if self.size_in_frames() == 0 { return; } let frames = core::mem::replace(&mut self.frames, FrameRange::empty()); - let mut free_frames: FreeFrames = Frames { typ: self.typ, frames }; + let free_frames: FreeFrames = Frames { typ: self.typ, frames }; let mut list = if free_frames.typ == MemoryRegionType::Reserved { FREE_RESERVED_FRAMES_LIST.lock() @@ -499,7 +499,7 @@ impl Drop for Frames { if *free_frames.end() + 1 == *next_frames_ref.start() { // extract the next chunk from the list let mut next_frames = cursor_mut - .replace_with(Wrapper::new_link(Frames::empty())) + .remove() .expect("BUG: couldn't remove next frames from free list in drop handler") .into_inner(); @@ -507,10 +507,8 @@ impl Drop for Frames { if next_frames.merge(free_frames).is_ok() { // trace!("newly merged next chunk: {:?}", next_frames); // now return newly merged chunk into list - match cursor_mut.replace_with(Wrapper::new_link(next_frames)) { - Ok(_) => { return; } - Err(f) => free_frames = f.into_inner(), - } + cursor_mut.insert_before(Wrapper::new_link(next_frames)); + return; } else { panic!("BUG: couldn't merge deallocated chunk into next chunk"); } @@ -523,17 +521,15 @@ impl Drop for Frames { if let Some(_prev_frames_ref) = cursor_mut.get() { // extract the next chunk from the list let mut prev_frames = cursor_mut - .replace_with(Wrapper::new_link(Frames::empty())) - .expect("BUG: couldn't remove next frames from free list in drop handler") + .remove() + .expect("BUG: couldn't remove previous frames from free list in drop handler") .into_inner(); if prev_frames.merge(free_frames).is_ok() { // trace!("newly merged prev chunk: {:?}", prev_frames); // now return newly merged chunk into list - match cursor_mut.replace_with(Wrapper::new_link(prev_frames)) { - Ok(_) => { return; } - Err(f) => free_frames = f.into_inner(), - } + cursor_mut.insert_before(Wrapper::new_link(prev_frames)); + return; } else { panic!("BUG: couldn't merge deallocated chunk into prev chunk"); }