From f0c83e4f2356984f63235b3a00c2e17cb962cf54 Mon Sep 17 00:00:00 2001 From: travis1829 Date: Wed, 20 Jan 2021 08:33:22 +0000 Subject: [PATCH 1/5] Fix WaitChannel's API --- kernel-rs/src/proc.rs | 38 ++++++++++------------------------ kernel-rs/src/sleepablelock.rs | 16 +++++++------- kernel-rs/src/spinlock.rs | 22 +++++++++++++------- kernel-rs/src/virtio_disk.rs | 2 +- 4 files changed, 34 insertions(+), 44 deletions(-) diff --git a/kernel-rs/src/proc.rs b/kernel-rs/src/proc.rs index 17e36b8fd..94a908ef2 100644 --- a/kernel-rs/src/proc.rs +++ b/kernel-rs/src/proc.rs @@ -19,11 +19,9 @@ use crate::{ param::{MAXPROCNAME, NOFILE, NPROC, ROOTDEV}, println, riscv::{intr_get, intr_on, r_tp, PGSIZE}, - sleepablelock::SleepablelockGuard, some_or, spinlock::{ - pop_off, push_off, RawSpinlock, Spinlock, SpinlockGuard, SpinlockProtected, - SpinlockProtectedGuard, + pop_off, push_off, Guard, RawSpinlock, Spinlock, SpinlockProtected, SpinlockProtectedGuard, }, trap::usertrapret, vm::{PageTable, UVAddr, VAddr}, @@ -222,23 +220,11 @@ impl WaitChannel { /// Atomically release lock and sleep on waitchannel. /// Reacquires lock when awakened. - pub unsafe fn sleep(&self, guard: &mut SpinlockGuard<'_, T>) { - self.sleep_raw(guard.raw()); - } - - /// Atomically release lock and sleep on waitchannel. - /// Reacquires lock when awakened. - pub unsafe fn sleep_sleepable(&self, guard: &mut SleepablelockGuard<'_, T>) { - self.sleep_raw(guard.raw()); - } - - /// Atomically release lock and sleep on waitchannel. - /// Reacquires lock when awakened. - // TODO(@kimjungwow): lk is not SpinlockGuard yet because - // 1. Some static mut variables are still not Spinlock but RawSpinlock - // 2. Sleeplock doesn't have Spinlock - pub unsafe fn sleep_raw(&self, lk: *const RawSpinlock) { - let p: *mut Proc = myproc(); + /// # Safety + /// + /// Make sure `lk` is the only lock we currently hold. + pub unsafe fn sleep(&self, lk: &mut dyn Guard) { + let p = &*myproc(); // Must acquire p->lock in order to // change p->state and then call sched. @@ -248,22 +234,20 @@ impl WaitChannel { // so it's okay to release lk. //DOC: sleeplock1 - mem::forget((*p).info.lock()); - (*lk).release(); + let mut guard = p.lock(); + lk.get_raw().release(); // Go to sleep. - let mut guard = ProcGuard::from_raw(p); guard.deref_mut_info().waitchannel = self; guard.deref_mut_info().state = Procstate::SLEEPING; guard.sched(); // Tidy up. guard.deref_mut_info().waitchannel = ptr::null(); - mem::forget(guard); // Reacquire original lock. - (*p).info.unlock(); - (*lk).acquire(); + drop(guard); + lk.get_raw().acquire(); } /// Wake up all processes sleeping on waitchannel. @@ -801,7 +785,7 @@ impl ProcessSystem { // Wait for a child to exit. //DOC: wait-sleep - ((*p).info.get_mut_unchecked().child_waitchannel).sleep_raw(parent_guard.raw()); + ((*p).info.get_mut_unchecked().child_waitchannel).sleep(&mut parent_guard); } } diff --git a/kernel-rs/src/sleepablelock.rs b/kernel-rs/src/sleepablelock.rs index c1e04c840..4f3ad9610 100644 --- a/kernel-rs/src/sleepablelock.rs +++ b/kernel-rs/src/sleepablelock.rs @@ -1,6 +1,6 @@ //! Sleepable locks use crate::proc::WaitChannel; -use crate::spinlock::RawSpinlock; +use crate::spinlock::{Guard, RawSpinlock}; use core::cell::UnsafeCell; use core::marker::PhantomData; use core::ops::{Deref, DerefMut}; @@ -60,15 +60,9 @@ impl Sleepablelock { } impl SleepablelockGuard<'_, T> { - pub fn raw(&self) -> *const RawSpinlock { - &self.lock.lock as *const _ - } - pub fn sleep(&mut self) { unsafe { - self.lock - .waitchannel - .sleep_raw(&self.lock.lock as *const _ as *mut RawSpinlock); + self.lock.waitchannel.sleep(self); } } @@ -77,6 +71,12 @@ impl SleepablelockGuard<'_, T> { } } +impl Guard for SleepablelockGuard<'_, T> { + fn get_raw(&self) -> &RawSpinlock { + &self.lock.lock + } +} + impl Drop for SleepablelockGuard<'_, T> { fn drop(&mut self) { self.lock.lock.release(); diff --git a/kernel-rs/src/spinlock.rs b/kernel-rs/src/spinlock.rs index 32b775008..9062d28eb 100644 --- a/kernel-rs/src/spinlock.rs +++ b/kernel-rs/src/spinlock.rs @@ -101,6 +101,11 @@ impl RawSpinlock { } } +pub trait Guard { + /// Returns a reference to the inner `RawSpinlock`. + fn get_raw(&self) -> &RawSpinlock; +} + pub struct SpinlockGuard<'s, T> { lock: &'s Spinlock, _marker: PhantomData<*const ()>, @@ -187,10 +192,6 @@ impl Spinlock { } impl SpinlockGuard<'_, T> { - pub fn raw(&self) -> *const RawSpinlock { - &self.lock.lock as *const _ - } - pub fn reacquire_after(&mut self, f: F) -> R where F: FnOnce() -> R, @@ -202,6 +203,12 @@ impl SpinlockGuard<'_, T> { } } +impl Guard for SpinlockGuard<'_, T> { + fn get_raw(&self) -> &RawSpinlock { + &self.lock.lock + } +} + impl Drop for SpinlockGuard<'_, T> { fn drop(&mut self) { self.lock.lock.release(); @@ -262,10 +269,9 @@ impl SpinlockProtected { } } -impl SpinlockProtectedGuard<'_> { - /// Returns the inner `RawSpinlock`. - pub fn raw(&self) -> *const RawSpinlock { - self.lock as *const _ +impl Guard for SpinlockProtectedGuard<'_> { + fn get_raw(&self) -> &RawSpinlock { + &self.lock } } diff --git a/kernel-rs/src/virtio_disk.rs b/kernel-rs/src/virtio_disk.rs index d29cd65e8..6f67283bf 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_sleepable(this); + (*b).vdisk_request_waitchannel.sleep(this); } this.info[desc[0].idx].b = ptr::null_mut(); IntoIter::new(desc).for_each(|desc| this.desc.free(desc)); From b6db5290145987281b4e7a0690cfb930794a44ef Mon Sep 17 00:00:00 2001 From: travis1829 Date: Wed, 20 Jan 2021 12:06:57 +0000 Subject: [PATCH 2/5] Change Guard trait -> WaitableGuard trait --- kernel-rs/src/proc.rs | 10 ++++++++-- kernel-rs/src/sleepablelock.rs | 6 +++--- kernel-rs/src/spinlock.rs | 11 +++-------- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/kernel-rs/src/proc.rs b/kernel-rs/src/proc.rs index 94a908ef2..1846c4d41 100644 --- a/kernel-rs/src/proc.rs +++ b/kernel-rs/src/proc.rs @@ -21,7 +21,7 @@ use crate::{ riscv::{intr_get, intr_on, r_tp, PGSIZE}, some_or, spinlock::{ - pop_off, push_off, Guard, RawSpinlock, Spinlock, SpinlockProtected, SpinlockProtectedGuard, + pop_off, push_off, RawSpinlock, Spinlock, SpinlockProtected, SpinlockProtectedGuard, }, trap::usertrapret, vm::{PageTable, UVAddr, VAddr}, @@ -207,6 +207,11 @@ pub enum Procstate { USED, } +pub trait WaitableGuard { + /// Returns a reference to the inner `RawSpinlock`. + fn get_raw(&self) -> &RawSpinlock; +} + pub struct WaitChannel { /// Required to make this type non-zero-sized. If it were zero-sized, multiple wait channels may /// have the same address, spuriously waking up more threads. @@ -220,10 +225,11 @@ impl WaitChannel { /// Atomically release lock and sleep on waitchannel. /// Reacquires lock when awakened. + /// /// # Safety /// /// Make sure `lk` is the only lock we currently hold. - pub unsafe fn sleep(&self, lk: &mut dyn Guard) { + pub unsafe fn sleep(&self, lk: &mut T) { let p = &*myproc(); // Must acquire p->lock in order to diff --git a/kernel-rs/src/sleepablelock.rs b/kernel-rs/src/sleepablelock.rs index 4f3ad9610..45cbc9403 100644 --- a/kernel-rs/src/sleepablelock.rs +++ b/kernel-rs/src/sleepablelock.rs @@ -1,6 +1,6 @@ //! Sleepable locks -use crate::proc::WaitChannel; -use crate::spinlock::{Guard, RawSpinlock}; +use crate::proc::{WaitChannel, WaitableGuard}; +use crate::spinlock::RawSpinlock; use core::cell::UnsafeCell; use core::marker::PhantomData; use core::ops::{Deref, DerefMut}; @@ -71,7 +71,7 @@ impl SleepablelockGuard<'_, T> { } } -impl Guard for SleepablelockGuard<'_, T> { +impl WaitableGuard for SleepablelockGuard<'_, T> { fn get_raw(&self) -> &RawSpinlock { &self.lock.lock } diff --git a/kernel-rs/src/spinlock.rs b/kernel-rs/src/spinlock.rs index 9062d28eb..f970754fa 100644 --- a/kernel-rs/src/spinlock.rs +++ b/kernel-rs/src/spinlock.rs @@ -1,6 +1,6 @@ use crate::{ kernel::kernel, - proc::Cpu, + proc::{Cpu, WaitableGuard}, riscv::{intr_get, intr_off, intr_on}, }; use core::cell::UnsafeCell; @@ -101,11 +101,6 @@ impl RawSpinlock { } } -pub trait Guard { - /// Returns a reference to the inner `RawSpinlock`. - fn get_raw(&self) -> &RawSpinlock; -} - pub struct SpinlockGuard<'s, T> { lock: &'s Spinlock, _marker: PhantomData<*const ()>, @@ -203,7 +198,7 @@ impl SpinlockGuard<'_, T> { } } -impl Guard for SpinlockGuard<'_, T> { +impl WaitableGuard for SpinlockGuard<'_, T> { fn get_raw(&self) -> &RawSpinlock { &self.lock.lock } @@ -269,7 +264,7 @@ impl SpinlockProtected { } } -impl Guard for SpinlockProtectedGuard<'_> { +impl WaitableGuard for SpinlockProtectedGuard<'_> { fn get_raw(&self) -> &RawSpinlock { &self.lock } From 96b54833e12a48282084b9e78009935e82350fbd Mon Sep 17 00:00:00 2001 From: travis1829 Date: Wed, 20 Jan 2021 15:48:19 +0000 Subject: [PATCH 3/5] Mark `get_raw` as unsafe. --- kernel-rs/src/proc.rs | 12 +++++++++--- kernel-rs/src/sleepablelock.rs | 6 +++--- kernel-rs/src/spinlock.rs | 10 +++++----- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/kernel-rs/src/proc.rs b/kernel-rs/src/proc.rs index 1846c4d41..5c3f0be43 100644 --- a/kernel-rs/src/proc.rs +++ b/kernel-rs/src/proc.rs @@ -207,9 +207,15 @@ pub enum Procstate { USED, } -pub trait WaitableGuard { +/// Represents lock guards that can be slept in a `WaitChannel`. +pub trait Waitable { /// Returns a reference to the inner `RawSpinlock`. - fn get_raw(&self) -> &RawSpinlock; + /// + /// # Safety + /// + /// You should manually prove the correctness when directly accessing + /// the inner `RawSpinlock` instead of using the lock's API. + unsafe fn get_raw(&self) -> &RawSpinlock; } pub struct WaitChannel { @@ -229,7 +235,7 @@ impl WaitChannel { /// # Safety /// /// Make sure `lk` is the only lock we currently hold. - pub unsafe fn sleep(&self, lk: &mut T) { + pub unsafe fn sleep(&self, lk: &mut T) { let p = &*myproc(); // Must acquire p->lock in order to diff --git a/kernel-rs/src/sleepablelock.rs b/kernel-rs/src/sleepablelock.rs index 45cbc9403..6397f6216 100644 --- a/kernel-rs/src/sleepablelock.rs +++ b/kernel-rs/src/sleepablelock.rs @@ -1,5 +1,5 @@ //! Sleepable locks -use crate::proc::{WaitChannel, WaitableGuard}; +use crate::proc::{WaitChannel, Waitable}; use crate::spinlock::RawSpinlock; use core::cell::UnsafeCell; use core::marker::PhantomData; @@ -71,8 +71,8 @@ impl SleepablelockGuard<'_, T> { } } -impl WaitableGuard for SleepablelockGuard<'_, T> { - fn get_raw(&self) -> &RawSpinlock { +impl Waitable for SleepablelockGuard<'_, T> { + unsafe fn get_raw(&self) -> &RawSpinlock { &self.lock.lock } } diff --git a/kernel-rs/src/spinlock.rs b/kernel-rs/src/spinlock.rs index f970754fa..7df42130e 100644 --- a/kernel-rs/src/spinlock.rs +++ b/kernel-rs/src/spinlock.rs @@ -1,6 +1,6 @@ use crate::{ kernel::kernel, - proc::{Cpu, WaitableGuard}, + proc::{Cpu, Waitable}, riscv::{intr_get, intr_off, intr_on}, }; use core::cell::UnsafeCell; @@ -198,8 +198,8 @@ impl SpinlockGuard<'_, T> { } } -impl WaitableGuard for SpinlockGuard<'_, T> { - fn get_raw(&self) -> &RawSpinlock { +impl Waitable for SpinlockGuard<'_, T> { + unsafe fn get_raw(&self) -> &RawSpinlock { &self.lock.lock } } @@ -264,8 +264,8 @@ impl SpinlockProtected { } } -impl WaitableGuard for SpinlockProtectedGuard<'_> { - fn get_raw(&self) -> &RawSpinlock { +impl Waitable for SpinlockProtectedGuard<'_> { + unsafe fn get_raw(&self) -> &RawSpinlock { &self.lock } } From 488155e8876ad0ca779db333becd27d6cb5de393 Mon Sep 17 00:00:00 2001 From: travis1829 Date: Thu, 21 Jan 2021 05:28:18 +0000 Subject: [PATCH 4/5] Remove Waitable::get_raw() and add raw_release(), raw_acquire(). --- kernel-rs/src/proc.rs | 46 ++++++++++++++++++++++++---------- kernel-rs/src/sleepablelock.rs | 11 ++++---- kernel-rs/src/spinlock.rs | 14 ++++++++--- 3 files changed, 49 insertions(+), 22 deletions(-) diff --git a/kernel-rs/src/proc.rs b/kernel-rs/src/proc.rs index 5c3f0be43..9b9f7a954 100644 --- a/kernel-rs/src/proc.rs +++ b/kernel-rs/src/proc.rs @@ -209,13 +209,22 @@ pub enum Procstate { /// Represents lock guards that can be slept in a `WaitChannel`. pub trait Waitable { - /// Returns a reference to the inner `RawSpinlock`. + /// Releases the inner `RawSpinlock`. /// /// # Safety /// - /// You should manually prove the correctness when directly accessing - /// the inner `RawSpinlock` instead of using the lock's API. - unsafe fn get_raw(&self) -> &RawSpinlock; + /// `raw_release()` and `raw_acquire` must always be used as a pair. + /// Use these only for temporarily releasing (and then acquiring) the lock. + /// Also, do not access `self` until re-acquiring the lock with `raw_acquire()`. + unsafe fn raw_release(&self); + + /// Acquires the inner `RawSpinlock`. + /// + /// # Safety + /// + /// `raw_release()` and `raw_acquire` must always be used as a pair. + /// Use these only for temporarily releasing (and then acquiring) the lock. + unsafe fn raw_acquire(&self); } pub struct WaitChannel { @@ -231,12 +240,11 @@ impl WaitChannel { /// Atomically release lock and sleep on waitchannel. /// Reacquires lock when awakened. - /// - /// # Safety - /// - /// Make sure `lk` is the only lock we currently hold. - pub unsafe fn sleep(&self, lk: &mut T) { - let p = &*myproc(); + pub fn sleep(&self, lk: &mut T) { + let p = unsafe { + // TODO: Remove this unsafe part after resolving #258 + &*myproc() + }; // Must acquire p->lock in order to // change p->state and then call sched. @@ -247,19 +255,31 @@ impl WaitChannel { //DOC: sleeplock1 let mut guard = p.lock(); - lk.get_raw().release(); + unsafe { + // Temporarily release the inner `RawSpinlock`. + // This is safe, since we don't access `lk` until re-acquiring the lock + // at `lk.raw_acquire()`. + lk.raw_release(); + } // Go to sleep. guard.deref_mut_info().waitchannel = self; guard.deref_mut_info().state = Procstate::SLEEPING; - guard.sched(); + unsafe { + // Safe since we hold `p.lock()`, changed the process's state, + // and device interrupts are disabled by `push_off()` in `p.lock()`. + guard.sched(); + } // Tidy up. guard.deref_mut_info().waitchannel = ptr::null(); // Reacquire original lock. drop(guard); - lk.get_raw().acquire(); + unsafe { + // Safe since this is paired with a previous `lk.raw_release()`. + lk.raw_acquire(); + } } /// Wake up all processes sleeping on waitchannel. diff --git a/kernel-rs/src/sleepablelock.rs b/kernel-rs/src/sleepablelock.rs index 6397f6216..2d8d6f0aa 100644 --- a/kernel-rs/src/sleepablelock.rs +++ b/kernel-rs/src/sleepablelock.rs @@ -61,9 +61,7 @@ impl Sleepablelock { impl SleepablelockGuard<'_, T> { pub fn sleep(&mut self) { - unsafe { - self.lock.waitchannel.sleep(self); - } + self.lock.waitchannel.sleep(self); } pub fn wakeup(&self) { @@ -72,8 +70,11 @@ impl SleepablelockGuard<'_, T> { } impl Waitable for SleepablelockGuard<'_, T> { - unsafe fn get_raw(&self) -> &RawSpinlock { - &self.lock.lock + unsafe fn raw_release(&self) { + self.lock.lock.release(); + } + unsafe fn raw_acquire(&self) { + self.lock.lock.acquire(); } } diff --git a/kernel-rs/src/spinlock.rs b/kernel-rs/src/spinlock.rs index 7df42130e..2e5bb73f9 100644 --- a/kernel-rs/src/spinlock.rs +++ b/kernel-rs/src/spinlock.rs @@ -199,8 +199,11 @@ impl SpinlockGuard<'_, T> { } impl Waitable for SpinlockGuard<'_, T> { - unsafe fn get_raw(&self) -> &RawSpinlock { - &self.lock.lock + unsafe fn raw_release(&self) { + self.lock.lock.release(); + } + unsafe fn raw_acquire(&self) { + self.lock.lock.acquire(); } } @@ -265,8 +268,11 @@ impl SpinlockProtected { } impl Waitable for SpinlockProtectedGuard<'_> { - unsafe fn get_raw(&self) -> &RawSpinlock { - &self.lock + unsafe fn raw_release(&self) { + self.lock.release(); + } + unsafe fn raw_acquire(&self) { + self.lock.acquire(); } } From dcfd735d59baf5a7e7c392042062527fa3d9155f Mon Sep 17 00:00:00 2001 From: travis1829 Date: Thu, 21 Jan 2021 05:44:38 +0000 Subject: [PATCH 5/5] Change to mutable reference. --- kernel-rs/src/proc.rs | 4 ++-- kernel-rs/src/sleepablelock.rs | 4 ++-- kernel-rs/src/spinlock.rs | 8 ++++---- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/kernel-rs/src/proc.rs b/kernel-rs/src/proc.rs index 9b9f7a954..ce9442892 100644 --- a/kernel-rs/src/proc.rs +++ b/kernel-rs/src/proc.rs @@ -216,7 +216,7 @@ pub trait Waitable { /// `raw_release()` and `raw_acquire` must always be used as a pair. /// Use these only for temporarily releasing (and then acquiring) the lock. /// Also, do not access `self` until re-acquiring the lock with `raw_acquire()`. - unsafe fn raw_release(&self); + unsafe fn raw_release(&mut self); /// Acquires the inner `RawSpinlock`. /// @@ -224,7 +224,7 @@ pub trait Waitable { /// /// `raw_release()` and `raw_acquire` must always be used as a pair. /// Use these only for temporarily releasing (and then acquiring) the lock. - unsafe fn raw_acquire(&self); + unsafe fn raw_acquire(&mut self); } pub struct WaitChannel { diff --git a/kernel-rs/src/sleepablelock.rs b/kernel-rs/src/sleepablelock.rs index 2d8d6f0aa..65c51f40c 100644 --- a/kernel-rs/src/sleepablelock.rs +++ b/kernel-rs/src/sleepablelock.rs @@ -70,10 +70,10 @@ impl SleepablelockGuard<'_, T> { } impl Waitable for SleepablelockGuard<'_, T> { - unsafe fn raw_release(&self) { + unsafe fn raw_release(&mut self) { self.lock.lock.release(); } - unsafe fn raw_acquire(&self) { + unsafe fn raw_acquire(&mut self) { self.lock.lock.acquire(); } } diff --git a/kernel-rs/src/spinlock.rs b/kernel-rs/src/spinlock.rs index 2e5bb73f9..330931102 100644 --- a/kernel-rs/src/spinlock.rs +++ b/kernel-rs/src/spinlock.rs @@ -199,10 +199,10 @@ impl SpinlockGuard<'_, T> { } impl Waitable for SpinlockGuard<'_, T> { - unsafe fn raw_release(&self) { + unsafe fn raw_release(&mut self) { self.lock.lock.release(); } - unsafe fn raw_acquire(&self) { + unsafe fn raw_acquire(&mut self) { self.lock.lock.acquire(); } } @@ -268,10 +268,10 @@ impl SpinlockProtected { } impl Waitable for SpinlockProtectedGuard<'_> { - unsafe fn raw_release(&self) { + unsafe fn raw_release(&mut self) { self.lock.release(); } - unsafe fn raw_acquire(&self) { + unsafe fn raw_acquire(&mut self) { self.lock.acquire(); } }