[Closes #241] Use Pin API for unmovable structs

kernel-rs/Cargo.toml

@@ -29,6 +29,7 @@ spin = { version = "0.7.0", default-features = false }
 array-macro = "2.0.0"
 static_assertions = "1.1.0"
 itertools = { version = "0.9.0", default-features = false }
+pin-project = "1.0.4"
 
 # Compiler options for sysroot packages.
 # Cargo currently warns following packages are not dependencies.

kernel-rs/src/bio.rs

@@ -21,13 +21,14 @@ use crate::{
 
 use core::mem;
 use core::ops::{Deref, DerefMut};
+use core::pin::Pin;
 
 pub struct BufEntry {
     dev: u32,
     pub blockno: u32,
 
     /// WaitChannel saying virtio_disk request is done.
-    pub vdisk_request_waitchannel: WaitChannel,
+    vdisk_request_waitchannel: WaitChannel,
 
     pub inner: Sleeplock<BufInner>,
 }
@@ -41,6 +42,13 @@ impl BufEntry {
             inner: Sleeplock::new("buffer", BufInner::zero()),
         }
     }
+
+    /// Returns the `WaitChannel` saying virtio_disk request is done.
+    pub fn get_waitchannel(&self) -> Pin<&WaitChannel> {
+        // TODO: change `&self` -> `self: Pin<&Self>`,
+        // and obtain a pin from `self` instead of making a new pin with `new_unchecked()`.
+        unsafe { Pin::new_unchecked(&self.vdisk_request_waitchannel) }
+    }
 }
 
 impl ArenaObject for BufEntry {
@@ -145,7 +153,7 @@ impl Bcache {
         )
     }
 
-    /// Return a unlocked buf with the contents of the indicated block.
+    /// Returns an unlocked buf with the contents of the indicated block.
     pub fn get_buf(&self, dev: u32, blockno: u32) -> BufUnlocked<'_> {
         let inner = self
             .find_or_alloc(

kernel-rs/src/file.rs

@@ -95,7 +95,7 @@ impl File {
         }
 
         match &self.typ {
-            FileType::Pipe { pipe } => pipe.read(addr, usize::try_from(n).unwrap_or(0)),
+            FileType::Pipe { pipe } => pipe.inner().read(addr, usize::try_from(n).unwrap_or(0)),
             FileType::Inode { ip, off } => {
                 let mut ip = ip.deref().lock();
                 let curr_off = *off.get();
@@ -122,7 +122,7 @@ impl File {
         }
 
         match &self.typ {
-            FileType::Pipe { pipe } => pipe.write(addr, usize::try_from(n).unwrap_or(0)),
+            FileType::Pipe { pipe } => pipe.inner().write(addr, usize::try_from(n).unwrap_or(0)),
             FileType::Inode { ip, off } => {
                 // write a few blocks at a time to avoid exceeding
                 // the maximum log transaction size, including
@@ -178,7 +178,7 @@ impl ArenaObject for File {
         A::reacquire_after(guard, || {
             let typ = mem::replace(&mut self.typ, FileType::None);
             match typ {
-                FileType::Pipe { mut pipe } => unsafe { pipe.close(self.writable) },
+                FileType::Pipe { pipe } => pipe.inner().close(self.writable),
                 FileType::Inode { ip, .. } | FileType::Device { ip, .. } => {
                     // TODO(rv6)
                     // The inode ip will be dropped by drop(ip). Deallocation

kernel-rs/src/kernel.rs

@@ -60,8 +60,6 @@ pub struct Kernel {
 
     /// Memory for virtio descriptors `&c` for queue 0.
     ///
-    /// This is a global instead of allocated because it must be multiple contiguous pages, which
-    /// `kernel().alloc()` doesn't support, and page aligned.
     // TODO(efenniht): I moved out pages from Disk. Did I changed semantics (pointer indirection?)
     virtqueue: [RawPage; 2],
 

kernel-rs/src/lib.rs

@@ -61,3 +61,5 @@ extern crate array_macro;
 extern crate static_assertions;
 #[macro_use]
 extern crate itertools;
+#[macro_use]
+extern crate pin_project;

kernel-rs/src/pipe.rs

@@ -7,7 +7,7 @@ use crate::{
     spinlock::Spinlock,
     vm::UVAddr,
 };
-use core::{mem, ops::Deref, ptr};
+use core::{mem, pin::Pin, ptr};
 
 const PIPESIZE: usize = 512;
 
@@ -27,32 +27,36 @@ struct PipeInner {
     writeopen: bool,
 }
 
+#[pin_project]
 pub struct Pipe {
     inner: Spinlock<PipeInner>,
 
     /// WaitChannel for saying there are unread bytes in Pipe.data.
+    #[pin]
     read_waitchannel: WaitChannel,
 
     /// WaitChannel for saying all bytes in Pipe.data are already read.
+    #[pin]
     write_waitchannel: WaitChannel,
 }
 
 impl Pipe {
     /// PipeInner::try_read() tries to read as much as possible.
     /// Pipe::read() executes try_read() until all bytes in pipe are read.
     //TODO : `n` should be u32
-    pub unsafe fn read(&self, addr: UVAddr, n: usize) -> Result<usize, ()> {
-        let mut inner = self.inner.lock();
+    pub unsafe fn read(self: Pin<&Self>, addr: UVAddr, n: usize) -> Result<usize, ()> {
+        let this = self.project_ref();
+        let mut inner = this.inner.lock();
         loop {
             match inner.try_read(addr, n) {
                 Ok(r) => {
                     //DOC: piperead-wakeup
-                    self.write_waitchannel.wakeup();
+                    this.write_waitchannel.wakeup();
                     return Ok(r);
                 }
                 Err(PipeError::WaitForIO) => {
                     //DOC: piperead-sleep
-                    self.read_waitchannel.sleep(&mut inner);
+                    this.read_waitchannel.sleep(&mut inner);
                 }
                 _ => return Err(()),
             }
@@ -61,64 +65,63 @@ impl Pipe {
 
     /// PipeInner::try_write() tries to write as much as possible.
     /// Pipe::write() executes try_write() until `n` bytes are written.
-    pub unsafe fn write(&self, addr: UVAddr, n: usize) -> Result<usize, ()> {
+    pub unsafe fn write(self: Pin<&Self>, addr: UVAddr, n: usize) -> Result<usize, ()> {
         let mut written = 0;
-        let mut inner = self.inner.lock();
+        let this = self.project_ref();
+        let mut inner = this.inner.lock();
         loop {
             match inner.try_write(addr + written, n - written) {
                 Ok(r) => {
                     written += r;
-                    self.read_waitchannel.wakeup();
+                    this.read_waitchannel.wakeup();
                     if written < n {
-                        self.write_waitchannel.sleep(&mut inner);
+                        this.write_waitchannel.sleep(&mut inner);
                     } else {
                         return Ok(written);
                     }
                 }
                 Err(PipeError::InvalidCopyin(i)) => {
-                    self.read_waitchannel.wakeup();
+                    this.read_waitchannel.wakeup();
                     return Ok(written + i);
                 }
                 _ => return Err(()),
             }
         }
     }
 
-    unsafe fn close(&self, writable: bool) -> bool {
-        let mut inner = self.inner.lock();
+    pub fn close(self: Pin<&Self>, writable: bool) {
+        let this = self.project_ref();
+        let mut inner = this.inner.lock();
 
         if writable {
             inner.writeopen = false;
-            self.read_waitchannel.wakeup();
+            this.read_waitchannel.wakeup();
         } else {
             inner.readopen = false;
-            self.write_waitchannel.wakeup();
+            this.write_waitchannel.wakeup();
         }
+    }
+
+    fn closed(self: Pin<&Self>) -> bool {
+        let this = self.project_ref();
+        let inner = this.inner.lock();
 
         // Return whether pipe would be freed or not
         !inner.readopen && !inner.writeopen
     }
 }
 
-// TODO: Remove Copy and Clone
-#[derive(Copy, Clone)]
 pub struct AllocatedPipe {
-    ptr: *mut Pipe,
-}
-
-impl Deref for AllocatedPipe {
-    type Target = Pipe;
-    fn deref(&self) -> &Self::Target {
-        unsafe { &*self.ptr }
-    }
+    // `Pipe` must not be movable, since `WaitChannel` must not be movable.
+    pin: Pin<&'static Pipe>,
 }
 
 impl AllocatedPipe {
     pub fn alloc() -> Result<(RcFile<'static>, RcFile<'static>), ()> {
         let page = kernel().alloc().ok_or(())?;
         let ptr = page.into_usize() as *mut Pipe;
 
-        // `Pipe` must be align with `Page`
+        // `Pipe` must be aligned with `Page`.
         const_assert!(mem::size_of::<Pipe>() <= PGSIZE);
 
         //TODO(rv6): Since Pipe is a huge struct, need to check whether stack is used to fill `*ptr`
@@ -143,27 +146,39 @@ impl AllocatedPipe {
                 },
             );
         };
+        let pin = unsafe {
+            // Safe because after this `alloc()` function ends, we can access the `Pipe`
+            // only through `AllocatedPipe::pin`, which is `Pin<&Pipe>` type.
+            // Hence, the `Pipe` cannot be moved.
+            Pin::new_unchecked(&*ptr)
+        };
         let f0 = kernel()
             .ftable
-            .alloc_file(FileType::Pipe { pipe: Self { ptr } }, true, false)
+            .alloc_file(FileType::Pipe { pipe: Self { pin } }, true, false)
             // It is safe because ptr is an address of page, which obtained by alloc()
             .map_err(|_| kernel().free(unsafe { Page::from_usize(ptr as _) }))?;
         let f1 = kernel()
             .ftable
-            .alloc_file(FileType::Pipe { pipe: Self { ptr } }, false, true)
+            .alloc_file(FileType::Pipe { pipe: Self { pin } }, false, true)
             // It is safe because ptr is an address of page, which obtained by alloc()
             .map_err(|_| kernel().free(unsafe { Page::from_usize(ptr as _) }))?;
 
         Ok((f0, f1))
     }
 
-    // TODO: use `Drop` instead of `close`
-    // TODO: use `self` instead of `&mut self`
-    // `&mut self` is used because `Drop` of `File` uses AllocatedPipe inside File.
-    // https://github.com/kaist-cp/rv6/pull/211#discussion_r491671723
-    pub unsafe fn close(&mut self, writable: bool) {
-        if (*self.ptr).close(writable) {
-            kernel().free(Page::from_usize(self.ptr as *mut Pipe as _));
+    pub fn inner(&self) -> Pin<&Pipe> {
+        self.pin
+    }
+}
+
+impl Drop for AllocatedPipe {
+    fn drop(&mut self) {
+        if self.pin.closed() {
+            let ptr = self.pin.get_ref() as *const Pipe;
+            unsafe {
+                // Safe since this is a page that was allocated through `kernel().alloc()`.
+                kernel().free(Page::from_usize(ptr as _));
+            }
         }
     }
 }

kernel-rs/src/proc.rs

@@ -5,6 +5,7 @@ use core::{
     cmp,
     mem::{self, MaybeUninit},
     ops::{Deref, DerefMut},
+    pin::Pin,
     ptr, slice, str,
     sync::atomic::{AtomicBool, AtomicI32, Ordering},
 };
@@ -240,7 +241,7 @@ impl WaitChannel {
 
     /// Atomically release lock and sleep on waitchannel.
     /// Reacquires lock when awakened.
-    pub fn sleep<T: Waitable>(&self, lk: &mut T) {
+    pub fn sleep<T: Waitable>(self: Pin<&Self>, lk: &mut T) {
         let p = unsafe {
             // TODO: Remove this unsafe part after resolving #258
             &*myproc()
@@ -263,7 +264,7 @@ impl WaitChannel {
         }
 
         // Go to sleep.
-        guard.deref_mut_info().waitchannel = self;
+        guard.deref_mut_info().waitchannel = self.get_ref();
         guard.deref_mut_info().state = Procstate::SLEEPING;
         unsafe {
             // Safe since we hold `p.lock()`, changed the process's state,
@@ -284,6 +285,7 @@ impl WaitChannel {
 
     /// Wake up all processes sleeping on waitchannel.
     /// Must be called without any p->lock.
+    /// TODO: `&self` -> `self: Pin<&Self>`.
     pub fn wakeup(&self) {
         kernel().procs.wakeup_pool(self)
     }
@@ -817,7 +819,9 @@ impl ProcessSystem {
 
             // Wait for a child to exit.
             //DOC: wait-sleep
-            ((*p).info.get_mut_unchecked().child_waitchannel).sleep(&mut parent_guard);
+            // TODO: Obtain a pin with `project_ref()` instead of making a new pin with `new_unchecked()`.
+            let waitchannel = Pin::new_unchecked(&(*p).info.get_mut_unchecked().child_waitchannel);
+            waitchannel.sleep(&mut parent_guard);
         }
     }