diff --git a/kernel-rs/Cargo.lock b/kernel-rs/Cargo.lock index 9183b7802..9a0c0e016 100644 --- a/kernel-rs/Cargo.lock +++ b/kernel-rs/Cargo.lock @@ -91,6 +91,44 @@ dependencies = [ "autocfg", ] +[[package]] +name = "pin-project" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "95b70b68509f17aa2857863b6fa00bf21fc93674c7a8893de2f469f6aa7ca2f2" +dependencies = [ + "pin-project-internal", +] + +[[package]] +name = "pin-project-internal" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "caa25a6393f22ce819b0f50e0be89287292fda8d425be38ee0ca14c4931d9e71" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "proc-macro2" +version = "1.0.24" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1e0704ee1a7e00d7bb417d0770ea303c1bccbabf0ef1667dae92b5967f5f8a71" +dependencies = [ + "unicode-xid", +] + +[[package]] +name = "quote" +version = "1.0.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "991431c3519a3f36861882da93630ce66b52918dcf1b8e2fd66b397fc96f28df" +dependencies = [ + "proc-macro2", +] + [[package]] name = "rv6-kernel" version = "0.1.0" @@ -101,6 +139,7 @@ dependencies = [ "cstr_core", "itertools", "num-iter", + "pin-project", "scopeguard", "spin", "static_assertions", @@ -123,3 +162,20 @@ name = "static_assertions" version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a2eb9349b6444b326872e140eb1cf5e7c522154d69e7a0ffb0fb81c06b37543f" + +[[package]] +name = "syn" +version = "1.0.60" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c700597eca8a5a762beb35753ef6b94df201c81cca676604f547495a0d7f0081" +dependencies = [ + "proc-macro2", + "quote", + "unicode-xid", +] + +[[package]] +name = "unicode-xid" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f7fe0bb3479651439c9112f72b6c505038574c9fbb575ed1bf3b797fa39dd564" diff --git a/kernel-rs/Cargo.toml b/kernel-rs/Cargo.toml index c47f86296..ecd8304ba 100644 --- a/kernel-rs/Cargo.toml +++ b/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. diff --git a/kernel-rs/src/bio.rs b/kernel-rs/src/bio.rs index 6f2107bd5..b7b96b0af 100644 --- a/kernel-rs/src/bio.rs +++ b/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, } @@ -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( diff --git a/kernel-rs/src/file.rs b/kernel-rs/src/file.rs index 4da2c6c65..16cddeed9 100644 --- a/kernel-rs/src/file.rs +++ b/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 diff --git a/kernel-rs/src/kernel.rs b/kernel-rs/src/kernel.rs index b0816c671..0807d0f05 100644 --- a/kernel-rs/src/kernel.rs +++ b/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], diff --git a/kernel-rs/src/lib.rs b/kernel-rs/src/lib.rs index 986be91b1..cdc546468 100644 --- a/kernel-rs/src/lib.rs +++ b/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; diff --git a/kernel-rs/src/pipe.rs b/kernel-rs/src/pipe.rs index 965d73fb2..303fbb0c6 100644 --- a/kernel-rs/src/pipe.rs +++ b/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,13 +27,16 @@ struct PipeInner { writeopen: bool, } +#[pin_project] pub struct Pipe { inner: Spinlock, /// 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, } @@ -41,18 +44,19 @@ 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 { - let mut inner = self.inner.lock(); + pub unsafe fn read(self: Pin<&Self>, addr: UVAddr, n: usize) -> Result { + 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,22 +65,23 @@ 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 { + pub unsafe fn write(self: Pin<&Self>, addr: UVAddr, n: usize) -> Result { 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(()), @@ -84,33 +89,31 @@ impl Pipe { } } - 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 { @@ -118,7 +121,7 @@ impl AllocatedPipe { 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::() <= 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 _)); + } } } } diff --git a/kernel-rs/src/proc.rs b/kernel-rs/src/proc.rs index ce9442892..0595e262b 100644 --- a/kernel-rs/src/proc.rs +++ b/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(&self, lk: &mut T) { + pub fn sleep(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); } } diff --git a/kernel-rs/src/sleepablelock.rs b/kernel-rs/src/sleepablelock.rs index 65c51f40c..a774c622e 100644 --- a/kernel-rs/src/sleepablelock.rs +++ b/kernel-rs/src/sleepablelock.rs @@ -4,9 +4,10 @@ use crate::spinlock::RawSpinlock; use core::cell::UnsafeCell; use core::marker::PhantomData; use core::ops::{Deref, DerefMut}; +use core::pin::Pin; pub struct SleepablelockGuard<'s, T> { - lock: &'s Sleepablelock, + lock: Pin<&'s Sleepablelock>, _marker: PhantomData<*const ()>, } @@ -14,9 +15,11 @@ pub struct SleepablelockGuard<'s, T> { unsafe impl<'s, T: Sync> Sync for SleepablelockGuard<'s, T> {} /// Sleepable locks +#[pin_project] pub struct Sleepablelock { lock: RawSpinlock, /// WaitChannel saying spinlock is released. + #[pin] waitchannel: WaitChannel, data: UnsafeCell, } @@ -40,7 +43,12 @@ impl Sleepablelock { self.lock.acquire(); SleepablelockGuard { - lock: self, + lock: unsafe { + // Safe since we maintain the `Pin` as long as the + // `SleepablelockGuard` is alive. + // TODO: Obtain a `Pin` from `self` instead of creating a new one. + Pin::new_unchecked(self) + }, _marker: PhantomData, } } @@ -61,7 +69,8 @@ impl Sleepablelock { impl SleepablelockGuard<'_, T> { pub fn sleep(&mut self) { - self.lock.waitchannel.sleep(self); + let lock = self.lock.project_ref(); + lock.waitchannel.sleep(self); } pub fn wakeup(&self) { diff --git a/kernel-rs/src/spinlock.rs b/kernel-rs/src/spinlock.rs index 330931102..328756c0a 100644 --- a/kernel-rs/src/spinlock.rs +++ b/kernel-rs/src/spinlock.rs @@ -159,6 +159,7 @@ impl Spinlock { } } + // TODO: This should be removed. pub unsafe fn unlock(&self) { self.lock.release(); } diff --git a/kernel-rs/src/virtio_disk.rs b/kernel-rs/src/virtio_disk.rs index 6f67283bf..e8fe1ee32 100644 --- a/kernel-rs/src/virtio_disk.rs +++ b/kernel-rs/src/virtio_disk.rs @@ -317,7 +317,7 @@ impl Disk { // Wait for virtio_disk_intr() to say request has finished. while b.deref_mut_inner().disk { - (*b).vdisk_request_waitchannel.sleep(this); + (*b).get_waitchannel().sleep(this); } this.info[desc[0].idx].b = ptr::null_mut(); IntoIter::new(desc).for_each(|desc| this.desc.free(desc)); @@ -348,7 +348,7 @@ impl Disk { // disk is done with buf buf.deref_mut_inner().disk = false; - buf.vdisk_request_waitchannel.wakeup(); + buf.get_waitchannel().wakeup(); self.used_idx += 1; }