Merge #468 #469

468: Use `List` for `Kmem` r=jeehoonkang a=Medowhill

원래 `Kmem`은 자체적인 intrusive linked list를 사용하도록 구현되어 있었는데, 이를 이미 rv6가 가지고 있는 instrusive linked list 타입인 `List`를 사용하도록 수정했습니다. 원래 `Kmem`에서는 singly linked list를 사용하지만 `List` 타입은 doubly linked list인 것 등의 사소한 차이가 있고, 이로 인해 약간의 runtime overhead가 추가될 수는 있겠으나 무시할 수 있을 정도라고 생각합니다. 어차피 핵심은 `Kmem`이 intrusive linked list로 구현되어야 한다는 점이니, `Kmem`을 이미 존재하는 `List` 타입을 사용해 구현하는 것이 더 바람직한 방향인 것 같습니다.

469: [Closes #371] Use `const Default` trait in arena. r=jeehoonkang a=travis1829

Closes #371 
### Changes
`ArrayArena`/`MruArena`의 `new` method를 변경했습니다.
기존:
```Rust
pub const fn new(entries: [RcCell<T>; CAPACITY]) -> Self; // ArrayArena
pub const fn new(entries: [MruEntry<T>; CAPACITY]) -> Self; // MruArena
```

변경 후:
```Rust
pub const fn new<D: Default>() -> Self; // ArrayArena
pub const fn new<D: Default>() -> Self; // MruArena
```
-----
* 기존에는 caller가 직접 초기화된 배열을 제공하는 형태였기 때문에, caller가 malicious한 배열을 제공하게 되면 문제가 생길 수 있었습니다.
* 변경 후에는 `ArrayArena`/`MruArena`가 직접 배열을 만들고 초기화합니다.
  * 이를 위해서, `Default` trait을 요구하도록 변경했습니다. (`const fn`안에서는 closure나 function pointer를 사용할 수 없는 것으로 보입니다.)
  * 특히, `Default` trait을 `const fn`안에서 사용하기 위해서, `#![feature(const_impl_trait)]`을 사용했습니다.

Co-authored-by: Jaemin Hong <hjm0901@gmail.com>
Co-authored-by: travis1829 <travis1829@naver.com>

kernel-rs/Cargo.toml

@@ -26,7 +26,7 @@ scopeguard = { version = "1.1.0", default-features = false }
 arrayvec = { version = "0.5.2", default-features = false }
 cstr_core = { version = "0.2.2", default-features = false }
 spin = { version = "0.7.1", default-features = false }
-array-macro = "2.0.0"
+array-macro = "2.1.0"
 static_assertions = "1.1.0"
 itertools = { version = "0.10.0", default-features = false }
 pin-project = "1"

kernel-rs/src/arena.rs

@@ -3,6 +3,7 @@ use core::mem::{self, ManuallyDrop};
 use core::ops::Deref;
 use core::pin::Pin;
 
+use array_macro::array;
 use pin_project::pin_project;
 
 use crate::list::*;
@@ -124,9 +125,21 @@ pub struct Rc<A: Arena> {
 unsafe impl<T: Sync, A: Arena<Data = T>> Send for Rc<A> {}
 
 impl<T, const CAPACITY: usize> ArrayArena<T, CAPACITY> {
-    // TODO(https://github.com/kaist-cp/rv6/issues/371): unsafe...
-    pub const fn new(entries: [RcCell<T>; CAPACITY]) -> Self {
-        Self { entries }
+    /// Returns an `ArrayArena` of size `CAPACITY` that is filled with `D`'s const default value.
+    /// Note that `D` must `impl const Default`.
+    ///
+    /// # Examples
+    ///
+    /// ```rust,no_run
+    /// let arr_arena = ArrayArena::<D, 100>::new();
+    /// ```
+    // Note: We cannot use the generic `T` in the following function, since we need to only allow
+    // types that `impl const Default`, not just `impl Default`.
+    #[allow(clippy::new_ret_no_self)]
+    pub const fn new<D: Default>() -> ArrayArena<D, CAPACITY> {
+        ArrayArena {
+            entries: array![_ => RcCell::new(Default::default()); CAPACITY],
+        }
     }
 }
 
@@ -246,10 +259,20 @@ unsafe impl<T> ListNode for MruEntry<T> {
 }
 
 impl<T, const CAPACITY: usize> MruArena<T, CAPACITY> {
-    // TODO(https://github.com/kaist-cp/rv6/issues/371): unsafe...
-    pub const fn new(entries: [MruEntry<T>; CAPACITY]) -> Self {
-        Self {
-            entries,
+    /// Returns an `MruArena` of size `CAPACITY` that is filled with `D`'s const default value.
+    /// Note that `D` must `impl const Default`.
+    ///
+    /// # Examples
+    ///
+    /// ```rust,no_run
+    /// let mru_arena = MruArena::<D, 100>::new();
+    /// ```
+    // Note: We cannot use the generic `T` in the following function, since we need to only allow
+    // types that `impl const Default`, not just `impl Default`.
+    #[allow(clippy::new_ret_no_self)]
+    pub const fn new<D: Default>() -> MruArena<D, CAPACITY> {
+        MruArena {
+            entries: array![_ => MruEntry::new(Default::default()); CAPACITY],
             list: unsafe { List::new() },
         }
     }

kernel-rs/src/bio.rs

@@ -14,10 +14,8 @@
 use core::mem::{self, ManuallyDrop};
 use core::ops::{Deref, DerefMut};
 
-use array_macro::array;
-
 use crate::{
-    arena::{Arena, ArenaObject, MruArena, MruEntry, Rc},
+    arena::{Arena, ArenaObject, MruArena, Rc},
     lock::{Sleeplock, Spinlock},
     param::{BSIZE, NBUF},
     proc::WaitChannel,
@@ -44,6 +42,13 @@ impl BufEntry {
     }
 }
 
+#[rustfmt::skip] // Need this if lower than rustfmt 1.4.34
+impl const Default for BufEntry {
+    fn default() -> Self {
+        Self::zero()
+    }
+}
+
 impl ArenaObject for BufEntry {
     fn finalize<'s, A: Arena>(&'s mut self, _guard: &'s mut A::Guard<'_>) {
         // The buffer contents should have been written. Does nothing.
@@ -149,12 +154,7 @@ impl Bcache {
     ///
     /// The caller should make sure that `Bcache` never gets moved.
     pub const unsafe fn zero() -> Self {
-        unsafe {
-            Spinlock::new(
-                "BCACHE",
-                MruArena::new(array![_ => MruEntry::new(BufEntry::zero()); NBUF]),
-            )
-        }
+        Spinlock::new("BCACHE", MruArena::<BufEntry, NBUF>::new())
     }
 
     /// Return a unlocked buf with the contents of the indicated block.

kernel-rs/src/file.rs

@@ -2,8 +2,6 @@
 
 use core::{cell::UnsafeCell, cmp, mem, ops::Deref, ops::DerefMut};
 
-use array_macro::array;
-
 use crate::{
     arena::{Arena, ArenaObject, ArrayArena, Rc},
     fs::{FileSystem, InodeGuard, RcInode},
@@ -12,7 +10,6 @@ use crate::{
     param::{BSIZE, MAXOPBLOCKS, NFILE},
     pipe::AllocatedPipe,
     proc::CurrentProc,
-    rc_cell::RcCell,
     vm::UVAddr,
 };
 
@@ -201,6 +198,13 @@ impl File {
     }
 }
 
+#[rustfmt::skip] // Need this if lower than rustfmt 1.4.34
+impl const Default for File {
+    fn default() -> Self {
+        Self::zero()
+    }
+}
+
 impl ArenaObject for File {
     fn finalize<'s, A: Arena>(&'s mut self, guard: &'s mut A::Guard<'_>) {
         // SAFETY: `FileTable` does not use `Arena::find_or_alloc`.
@@ -235,10 +239,7 @@ impl ArenaObject for File {
 
 impl FileTable {
     pub const fn zero() -> Self {
-        Spinlock::new(
-            "FTABLE",
-            ArrayArena::new(array![_ => RcCell::new(File::zero()); NFILE]),
-        )
+        Spinlock::new("FTABLE", ArrayArena::<File, NFILE>::new())
     }
 
     /// Allocate a file structure.

kernel-rs/src/fs/inode.rs

@@ -74,7 +74,6 @@ use core::{
     ptr,
 };
 
-use array_macro::array;
 use static_assertions::const_assert;
 
 use super::{FileName, IPB, MAXFILE, NDIRECT, NINDIRECT};
@@ -87,7 +86,6 @@ use crate::{
     param::ROOTDEV,
     param::{BSIZE, NINODE},
     proc::CurrentProc,
-    rc_cell::RcCell,
     stat::Stat,
     vm::UVAddr,
 };
@@ -675,6 +673,13 @@ impl InodeGuard<'_> {
     }
 }
 
+#[rustfmt::skip] // Need this if lower than rustfmt 1.4.34
+impl const Default for Inode {
+    fn default() -> Self {
+        Self::zero()
+    }
+}
+
 impl ArenaObject for Inode {
     /// Drop a reference to an in-memory inode.
     /// If that was the last reference, the inode table entry can
@@ -810,10 +815,7 @@ impl Inode {
 
 impl Itable {
     pub const fn zero() -> Self {
-        Spinlock::new(
-            "ITABLE",
-            ArrayArena::new(array![_ => RcCell::new(Inode::zero()); NINODE]),
-        )
+        Spinlock::new("ITABLE", ArrayArena::<Inode, NINODE>::new())
     }
 
     /// Find the inode with number inum on device dev

kernel-rs/src/kalloc.rs

@@ -1,10 +1,12 @@
 //! Physical memory allocator, for user processes,
 //! kernel stacks, page-table pages,
 //! and pipe buffers. Allocates whole 4096-byte pages.
-use core::mem;
-use core::ptr;
+use core::{mem, pin::Pin};
+
+use pin_project::pin_project;
 
 use crate::{
+    list::{List, ListEntry, ListNode},
     lock::Spinlock,
     memlayout::PHYSTOP,
     page::Page,
@@ -17,23 +19,61 @@ extern "C" {
     pub static mut end: [u8; 0];
 }
 
+#[repr(transparent)]
+#[pin_project]
 struct Run {
-    next: *mut Run,
+    #[pin]
+    entry: ListEntry,
+}
+
+impl Run {
+    /// # Safety
+    ///
+    /// It must be used only after initializing it with `Run::init`.
+    unsafe fn new() -> Self {
+        Self {
+            entry: unsafe { ListEntry::new() },
+        }
+    }
+
+    fn init(self: Pin<&mut Self>) {
+        self.project().entry.init();
+    }
+}
+
+// SAFETY: `Run` owns a `ListEntry`.
+unsafe impl ListNode for Run {
+    fn get_list_entry(&self) -> &ListEntry {
+        &self.entry
+    }
+
+    fn from_list_entry(list_entry: *const ListEntry) -> *const Self {
+        list_entry as _
+    }
 }
 
 /// # Safety
 ///
-/// - This singly linked list does not have a cycle.
-/// - If head is null, then it is an empty list. Ohterwise, it is nonempty, and
-///   head is its first element, which is a valid page.
+/// The address of each `Run` in `runs` can become a `Page` by `Page::from_usize`.
+// This implementation defers from xv6. Kmem of xv6 uses intrusive singly linked list, while this
+// Kmem uses List, which is a intrusive doubly linked list type of rv6. In a intrusive singly
+// linked list, it is impossible to automatically remove an entry from a list when it is dropped.
+// Therefore, it is nontrivial to make a general intrusive singly linked list type in a safe way.
+// For this reason, we use a doubly linked list instead. It adds runtime overhead, but the overhead
+// seems negligible.
+#[pin_project]
 pub struct Kmem {
-    head: *mut Run,
+    #[pin]
+    runs: List<Run>,
 }
 
 impl Kmem {
-    pub const fn new() -> Self {
+    /// # Safety
+    ///
+    /// It must be used only after initializing it with `Kmem::init`.
+    pub const unsafe fn new() -> Self {
         Self {
-            head: ptr::null_mut(),
+            runs: unsafe { List::new() },
         }
     }
 
@@ -43,7 +83,9 @@ impl Kmem {
     ///
     /// There must be no existing pages. It implies that this method should be
     /// called only once.
-    pub unsafe fn init(&mut self) {
+    pub unsafe fn init(mut self: Pin<&mut Self>) {
+        self.as_mut().project().runs.init();
+
         // SAFETY: safe to acquire only the address of a static variable.
         let pa_start = pgroundup(unsafe { end.as_ptr() as usize });
         let pa_end = pgrounddown(PHYSTOP);
@@ -53,35 +95,31 @@ impl Kmem {
             // * end <= pa < PHYSTOP
             // * the safety condition of this method guarantees that the
             //   created page does not overlap with existing pages
-            self.free(unsafe { Page::from_usize(pa) });
+            self.as_ref()
+                .get_ref()
+                .free(unsafe { Page::from_usize(pa) });
         }
     }
 
-    pub fn free(&mut self, mut page: Page) {
+    pub fn free(&self, mut page: Page) {
         // Fill with junk to catch dangling refs.
         page.write_bytes(1);
-        let pa = page.into_usize();
-        debug_assert!(
-            // SAFETY: safe to acquire only the address of a static variable.
-            pa % PGSIZE == 0 && (unsafe { end.as_ptr() as usize }..PHYSTOP).contains(&pa),
-            "Kmem::free"
-        );
-        let mut r = pa as *mut Run;
-        // SAFETY: By the invariant of Page, it does not create a cycle in this list and
-        // thus is safe.
-        unsafe { (*r).next = self.head };
-        self.head = r;
+
+        let run = page.as_uninit_mut();
+        // SAFETY: `run` will be initialized by the following `init`.
+        let run = run.write(unsafe { Run::new() });
+        let mut run = unsafe { Pin::new_unchecked(run) };
+        run.as_mut().init();
+        self.runs.push_front(run.as_ref().get_ref());
+
+        // Since the page has returned to the list, forget the page.
+        mem::forget(page);
     }
 
-    pub fn alloc(&mut self) -> Option<Page> {
-        if self.head.is_null() {
-            return None;
-        }
-        // SAFETY: head is not null and the structure of this list
-        // is maintained by the invariant.
-        let next = unsafe { (*self.head).next };
-        // SAFETY: the first element is a valid page by the invariant.
-        let mut page = unsafe { Page::from_usize(mem::replace(&mut self.head, next) as _) };
+    pub fn alloc(&self) -> Option<Page> {
+        let run = self.runs.pop_front()?;
+        // SAFETY: the invariant of `Kmem`.
+        let mut page = unsafe { Page::from_usize(run as _) };
         // fill with junk
         page.write_bytes(5);
         Some(page)