refactor(kernel): wrap running_task with CpuLockCell instead of AtomicRef

`::atomic_ref` doesn't build for an Armv6-M target (because of atomics
limitations), so making this change is necessary to support Armv6-M.

Author: yvt

Cargo.lock

@@ -20,15 +20,6 @@ tokenlock = { version = "0.3.0", default-features = false }
 bitflags = { version = "1.2.1" }
 chrono = { version = "0.4.13", optional = true, default-features = false }
 
-[dependencies.atomic_ref]
-# - “Make the crate `no_std`”
-#   <https://github.com/mystor/atomic_ref/pull/1>
-# - “Make `AtomicRef::new` `const fn` (nightly-only)”
-#   <https://github.com/mystor/atomic_ref/pull/2>
-git = "https://github.com/yvt/atomic_ref.git"
-rev = "37523c17a2a535f4d04c204d65d2742647abed25"
-features = ["nightly"]
-
 [dev-dependencies]
 quickcheck_macros = "0.9.1"
 env_logger = "0.7.1"

src/constance/Cargo.toml

@@ -1,5 +1,4 @@
 //! The RTOS kernel
-use atomic_ref::AtomicRef;
 use core::{
     borrow::BorrowMut,
     fmt,
@@ -634,7 +633,7 @@ pub trait PortToKernel {
     // TODO: Explain phases
     unsafe fn boot() -> !;
 
-    /// Determine the next task to run and store it in [`State::running_task_ref`].
+    /// Determine the next task to run and store it in [`State::running_task_ptr`].
     ///
     /// Precondition: CPU Lock active / Postcondition: CPU Lock active
     unsafe fn choose_running_task();
@@ -800,7 +799,8 @@ pub struct State<
 > {
     /// The currently or recently running task. Can be in a Running or Waiting
     /// state.
-    running_task: AtomicRef<'static, TaskCb<System, PortTaskState, TaskPriority>>,
+    running_task:
+        utils::CpuLockCell<System, Option<&'static TaskCb<System, PortTaskState, TaskPriority>>>,
 
     /// The task ready bitmap, in which each bit indicates whether the
     /// task ready queue corresponding to that bit contains a task or not.
@@ -831,7 +831,7 @@ impl<
     for State<System, PortTaskState, TaskReadyBitmap, TaskReadyQueue, TaskPriority, TimeoutHeap>
 {
     const INIT: Self = Self {
-        running_task: AtomicRef::new(None),
+        running_task: utils::CpuLockCell::new(None),
         task_ready_bitmap: Init::INIT,
         task_ready_queue: Init::INIT,
         priority_boost: AtomicBool::new(false),
@@ -862,16 +862,28 @@ impl<
 
 impl<System: KernelCfg2> State<System> {
     /// Get the currently running task.
-    pub fn running_task(&self) -> Option<&'static TaskCb<System>> {
-        self.running_task.load(Ordering::Relaxed)
+    #[inline]
+    fn running_task(
+        &self,
+        lock: utils::CpuLockGuardBorrowMut<System>,
+    ) -> Option<&'static TaskCb<System>> {
+        *self.running_task.read(&*lock)
     }
 
-    /// Get a reference to the variable storing the currently running task.
-    ///
-    /// # Safety
-    ///
-    /// Modifying the stored value is not allowed.
-    pub unsafe fn running_task_ref(&self) -> &AtomicRef<'static, TaskCb<System>> {
-        &self.running_task
+    /// Get a pointer to the variable storing the currently running task.
+    ///
+    /// Reading the variable is safe as long as the read is free of data race.
+    /// Note that only the dispatcher (that calls
+    /// [`PortToKernel::choose_running_task`]) can modify the variable
+    /// asynchonously. For example, it's safe to read it in a task context. It's
+    /// also safe to read it in the dispatcher. On the other hand, reading it in
+    /// a non-task context (except for the dispatcher, of course) may lead to
+    /// an undefined behavior unless CPU Lock is activated while reading the
+    /// variable.
+    ///
+    /// Writing the variable is not allowed.
+    #[inline]
+    pub fn running_task_ptr(&self) -> *mut Option<&'static TaskCb<System>> {
+        self.running_task.as_ptr()
     }
 }

src/constance/src/kernel.rs

@@ -135,8 +135,8 @@ impl<System: Kernel> Task<System> {
     /// handler could be interrupted by another interrrupt, which might do
     /// scheduling on return (whether this happens or not is unspecified).
     pub fn current() -> Result<Option<Self>, GetCurrentTaskError> {
-        let _lock = utils::lock_cpu::<System>()?;
-        let task_cb = if let Some(cb) = System::state().running_task() {
+        let mut lock = utils::lock_cpu::<System>()?;
+        let task_cb = if let Some(cb) = System::state().running_task(lock.borrow_mut()) {
             cb
         } else {
             return Ok(None);
@@ -425,12 +425,12 @@ pub(super) unsafe fn exit_current_task<System: Kernel>() -> Result<!, ExitTaskEr
         .store(false, Ordering::Release);
 
     // Transition the current task to Dormant
-    let running_task = System::state().running_task().unwrap();
+    let running_task = System::state().running_task(lock.borrow_mut()).unwrap();
     assert_eq!(*running_task.st.read(&*lock), TaskSt::Running);
     running_task.st.replace(&mut *lock, TaskSt::Dormant);
 
     // Erase `running_task`
-    System::state().running_task.store(None, Ordering::Relaxed);
+    System::state().running_task.replace(&mut *lock, None);
 
     core::mem::forget(lock);
 
@@ -551,22 +551,29 @@ pub(super) unsafe fn make_ready<System: Kernel>(
 ///
 /// System services that transition a task into the Ready state should call
 /// this before returning to the caller.
-pub(super) fn unlock_cpu_and_check_preemption<System: Kernel>(lock: utils::CpuLockGuard<System>) {
+pub(super) fn unlock_cpu_and_check_preemption<System: Kernel>(
+    mut lock: utils::CpuLockGuard<System>,
+) {
     // If Priority Boost is active, treat the currently running task as the
     // highest-priority task.
     if System::is_priority_boost_active() {
         debug_assert_eq!(
-            *System::state().running_task().unwrap().st.read(&*lock),
+            *System::state()
+                .running_task(lock.borrow_mut())
+                .unwrap()
+                .st
+                .read(&*lock),
             TaskSt::Running
         );
         return;
     }
 
-    let prev_task_priority = if let Some(running_task) = System::state().running_task() {
-        running_task.priority.read(&*lock).to_usize().unwrap()
-    } else {
-        usize::MAX
-    };
+    let prev_task_priority =
+        if let Some(running_task) = System::state().running_task(lock.borrow_mut()) {
+            running_task.priority.read(&*lock).to_usize().unwrap()
+        } else {
+            usize::MAX
+        };
 
     // The priority of the next task to run
     let next_task_priority = System::state()
@@ -593,14 +600,18 @@ pub(super) fn choose_next_running_task<System: Kernel>(
     if System::is_priority_boost_active() {
         // Blocking system calls aren't allowed when Priority Boost is active
         debug_assert_eq!(
-            *System::state().running_task().unwrap().st.read(&*lock),
+            *System::state()
+                .running_task(lock.borrow_mut())
+                .unwrap()
+                .st
+                .read(&*lock),
             TaskSt::Running
         );
         return;
     }
 
     // The priority of `running_task`
-    let prev_running_task = System::state().running_task();
+    let prev_running_task = System::state().running_task(lock.borrow_mut());
     let prev_task_priority = if let Some(running_task) = prev_running_task {
         if *running_task.st.read(&*lock) == TaskSt::Running {
             running_task.priority.read(&*lock).to_usize().unwrap()
@@ -675,7 +686,7 @@ pub(super) fn choose_next_running_task<System: Kernel>(
 
     System::state()
         .running_task
-        .store(next_running_task, Ordering::Relaxed);
+        .replace(&mut *lock, next_running_task);
 }
 
 /// Transition the currently running task into the Waiting state. Returns when
@@ -691,7 +702,7 @@ pub(super) fn wait_until_woken_up<System: Kernel>(
     debug_assert_eq!(state::expect_waitable_context::<System>(), Ok(()));
 
     // Transition the current task to Waiting
-    let running_task = System::state().running_task().unwrap();
+    let running_task = System::state().running_task(lock.borrow_mut()).unwrap();
     assert_eq!(*running_task.st.read(&*lock), TaskSt::Running);
     running_task.st.replace(&mut *lock, TaskSt::Waiting);
 
@@ -720,7 +731,7 @@ pub(super) fn park_current_task<System: Kernel>() -> Result<(), ParkError> {
     let mut lock = utils::lock_cpu::<System>()?;
     state::expect_waitable_context::<System>()?;
 
-    let running_task = System::state().running_task().unwrap();
+    let running_task = System::state().running_task(lock.borrow_mut()).unwrap();
 
     // If the task already has a park token, return immediately
     if running_task.park_token.replace(&mut *lock, false) {
@@ -741,7 +752,7 @@ pub(super) fn park_current_task_timeout<System: Kernel>(
     let mut lock = utils::lock_cpu::<System>()?;
     state::expect_waitable_context::<System>()?;
 
-    let running_task = System::state().running_task().unwrap();
+    let running_task = System::state().running_task(lock.borrow_mut()).unwrap();
 
     // If the task already has a park token, return immediately
     if running_task.park_token.replace(&mut *lock, false) {

src/constance/src/kernel/task.rs

@@ -220,10 +220,10 @@ impl<System: Kernel> WaitQueue<System> {
     #[inline]
     pub(super) fn wait(
         &'static self,
-        lock: CpuLockGuardBorrowMut<'_, System>,
+        mut lock: CpuLockGuardBorrowMut<'_, System>,
         payload: WaitPayload<System>,
     ) -> Result<WaitPayload<System>, WaitError> {
-        let task = System::state().running_task().unwrap();
+        let task = System::state().running_task(lock.borrow_mut()).unwrap();
         let wait = Wait {
             task,
             link: CpuLockCell::new(None),
@@ -252,7 +252,7 @@ impl<System: Kernel> WaitQueue<System> {
         payload: WaitPayload<System>,
         duration_time32: timeout::Time32,
     ) -> Result<WaitPayload<System>, WaitTimeoutError> {
-        let task = System::state().running_task().unwrap();
+        let task = System::state().running_task(lock.borrow_mut()).unwrap();
         let wait = Wait {
             task,
             link: CpuLockCell::new(None),
@@ -283,7 +283,7 @@ impl<System: Kernel> WaitQueue<System> {
 
         debug_assert!(core::ptr::eq(
             wait.task,
-            System::state().running_task().unwrap()
+            System::state().running_task(lock.borrow_mut()).unwrap()
         ));
         debug_assert!(core::ptr::eq(wait.wait_queue.unwrap(), self));
 
@@ -517,10 +517,10 @@ pub(super) fn reorder_wait_of_task<System: Kernel>(
 /// [waitable]: crate#contexts
 #[inline]
 pub(super) fn wait_no_queue<System: Kernel>(
-    lock: CpuLockGuardBorrowMut<'_, System>,
+    mut lock: CpuLockGuardBorrowMut<'_, System>,
     payload: WaitPayload<System>,
 ) -> Result<WaitPayload<System>, WaitError> {
-    let task = System::state().running_task().unwrap();
+    let task = System::state().running_task(lock.borrow_mut()).unwrap();
     let wait = Wait {
         task,
         link: CpuLockCell::new(None),
@@ -550,7 +550,7 @@ pub(super) fn wait_no_queue_timeout<System: Kernel>(
     payload: WaitPayload<System>,
     duration_time32: timeout::Time32,
 ) -> Result<WaitPayload<System>, WaitTimeoutError> {
-    let task = System::state().running_task().unwrap();
+    let task = System::state().running_task(lock.borrow_mut()).unwrap();
     let wait = Wait {
         task,
         link: CpuLockCell::new(None),
@@ -580,7 +580,7 @@ fn wait_no_queue_inner<System: Kernel>(
 
     debug_assert!(core::ptr::eq(
         wait.task,
-        System::state().running_task().unwrap()
+        System::state().running_task(lock.borrow_mut()).unwrap()
     ));
     debug_assert!(wait.wait_queue.is_none());
     debug_assert!(wait.link.read(&*lock).is_none());

src/constance/src/kernel/wait.rs

@@ -204,7 +204,7 @@ impl State {
         //  - This is the top-level function in the PendSV handler. That is,
         //    the compiler must really inline `handle_pend_sv`.
 
-        let running_task_ref = unsafe { System::state().running_task_ref() };
+        let running_task_ptr = System::state().running_task_ptr();
 
         extern "C" fn choose_next_task<System: PortInstance>() {
             // Choose the next task to run
@@ -289,7 +289,7 @@ impl State {
             msr control, r0
         "
         :
-        :   "{r0}"(running_task_ref)
+        :   "{r0}"(running_task_ptr)
         ,   "X"(choose_next_task::<System> as extern fn())
         :
         :   "volatile");

src/constance_port_arm_m/src/threading.rs

@@ -342,7 +342,10 @@ impl State {
         let mut lock = self.thread_group.get().unwrap().lock();
 
         // Tell the scheduler which task to run next
-        lock.scheduler().task_thread = if let Some(task) = System::state().running_task() {
+        // Safety: `running_task` is only modified by `choose_running_task`, so
+        //         there's no data race
+        let running_task = unsafe { *System::state().running_task_ptr() };
+        lock.scheduler().task_thread = if let Some(task) = running_task {
             log::trace!("dispatching task {:p}", task);
 
             let mut tsm = task.port_task_state.tsm.lock();