From 1b23182dccba8c9c0a943b5abc1785fe456c2ea6 Mon Sep 17 00:00:00 2001 From: Zhang Junyang Date: Sun, 29 Sep 2024 10:29:27 +0800 Subject: [PATCH] Fix the thread status ordering by limiting the API --- kernel/src/process/kill.rs | 2 +- kernel/src/process/posix_thread/exit.rs | 2 +- kernel/src/process/posix_thread/mod.rs | 2 +- kernel/src/process/signal/mod.rs | 15 +---- kernel/src/thread/kernel_thread.rs | 2 +- kernel/src/thread/mod.rs | 76 ++++++++++++++++--------- kernel/src/thread/task.rs | 6 +- 7 files changed, 58 insertions(+), 47 deletions(-) diff --git a/kernel/src/process/kill.rs b/kernel/src/process/kill.rs index 29cfaf63f..658d774ab 100644 --- a/kernel/src/process/kill.rs +++ b/kernel/src/process/kill.rs @@ -71,7 +71,7 @@ pub fn tgkill(tid: Tid, tgid: Pid, signal: Option, ctx: &Context) -> let thread = thread_table::get_thread(tid) .ok_or_else(|| Error::with_message(Errno::ESRCH, "target thread does not exist"))?; - if thread.status().is_exited() { + if thread.is_exited() { return Ok(()); } diff --git a/kernel/src/process/posix_thread/exit.rs b/kernel/src/process/posix_thread/exit.rs index 0664cd37e..211e2310d 100644 --- a/kernel/src/process/posix_thread/exit.rs +++ b/kernel/src/process/posix_thread/exit.rs @@ -14,7 +14,7 @@ use crate::{ /// /// If the thread is not a POSIX thread, this method will panic. pub fn do_exit(thread: &Thread, posix_thread: &PosixThread, term_status: TermStatus) -> Result<()> { - if thread.status().is_exited() { + if thread.is_exited() { return Ok(()); } thread.exit(); diff --git a/kernel/src/process/posix_thread/mod.rs b/kernel/src/process/posix_thread/mod.rs index 186a4ada8..ac10c5a3a 100644 --- a/kernel/src/process/posix_thread/mod.rs +++ b/kernel/src/process/posix_thread/mod.rs @@ -277,7 +277,7 @@ impl PosixThread { let tasks = process.tasks().lock(); tasks .iter() - .all(|task| Thread::borrow_from_task(task).status().is_exited()) + .all(|task| Thread::borrow_from_task(task).is_exited()) } /// Gets the read-only credentials of the thread. diff --git a/kernel/src/process/signal/mod.rs b/kernel/src/process/signal/mod.rs index c2529dbb3..3a6ec3873 100644 --- a/kernel/src/process/signal/mod.rs +++ b/kernel/src/process/signal/mod.rs @@ -31,7 +31,6 @@ use crate::{ get_current_userspace, prelude::*, process::{do_exit_group, TermStatus}, - thread::status::ThreadStatus, }; pub trait SignalContext { @@ -107,20 +106,10 @@ pub fn handle_pending_signal(user_ctx: &mut UserContext, ctx: &Context) -> Resul } SigDefaultAction::Ign => {} SigDefaultAction::Stop => { - let _ = ctx.thread.atomic_status().compare_exchange( - ThreadStatus::Running, - ThreadStatus::Stopped, - Ordering::AcqRel, - Ordering::Relaxed, - ); + let _ = ctx.thread.stop(); } SigDefaultAction::Cont => { - let _ = ctx.thread.atomic_status().compare_exchange( - ThreadStatus::Stopped, - ThreadStatus::Running, - Ordering::AcqRel, - Ordering::Relaxed, - ); + let _ = ctx.thread.resume(); } } } diff --git a/kernel/src/thread/kernel_thread.rs b/kernel/src/thread/kernel_thread.rs index 1334ed43a..477d64e6d 100644 --- a/kernel/src/thread/kernel_thread.rs +++ b/kernel/src/thread/kernel_thread.rs @@ -32,7 +32,7 @@ impl KernelThreadExt for Thread { fn join(&self) { loop { - if self.status().is_exited() { + if self.is_exited() { return; } else { Thread::yield_now(); diff --git a/kernel/src/thread/mod.rs b/kernel/src/thread/mod.rs index cb765fc92..329ebfd0e 100644 --- a/kernel/src/thread/mod.rs +++ b/kernel/src/thread/mod.rs @@ -86,27 +86,59 @@ impl Thread { /// Runs this thread at once. pub fn run(&self) { - self.set_status(ThreadStatus::Running); + self.status.store(ThreadStatus::Running, Ordering::Release); self.task.upgrade().unwrap().run(); } + /// Returns whether the thread is exited. + pub fn is_exited(&self) -> bool { + self.status.load(Ordering::Acquire).is_exited() + } + + /// Returns whether the thread is stopped. + pub fn is_stopped(&self) -> bool { + self.status.load(Ordering::Acquire).is_stopped() + } + + /// Stops the thread if it is running. + /// + /// If the previous status is not [`ThreadStatus::Running`], this function + /// returns [`Err`] with the previous state. Otherwise, it sets the status + /// to [`ThreadStatus::Stopped`] and returns [`Ok`] with the previous state. + /// + /// This function only sets the status to [`ThreadStatus::Stopped`], + /// without initiating a reschedule. + pub fn stop(&self) -> core::result::Result { + self.status.compare_exchange( + ThreadStatus::Running, + ThreadStatus::Stopped, + Ordering::AcqRel, + Ordering::Acquire, + ) + } + + /// Resumes running the thread if it is stopped. + /// + /// If the previous status is not [`ThreadStatus::Stopped`], this function + /// returns [`None`]. Otherwise, it sets the status to + /// [`ThreadStatus::Running`] and returns [`Some(())`]. + /// + /// This function only sets the status to [`ThreadStatus::Running`], + /// without initiating a reschedule. + pub fn resume(&self) -> Option<()> { + self.status + .compare_exchange( + ThreadStatus::Stopped, + ThreadStatus::Running, + Ordering::AcqRel, + Ordering::Acquire, + ) + .ok() + .map(|_| ()) + } + pub(super) fn exit(&self) { - self.set_status(ThreadStatus::Exited); - } - - /// Returns the reference to the atomic status. - pub fn atomic_status(&self) -> &AtomicThreadStatus { - &self.status - } - - /// Returns the current status. - pub fn status(&self) -> ThreadStatus { - self.status.load(Ordering::Acquire) - } - - /// Updates the status with the new value. - pub fn set_status(&self, new_status: ThreadStatus) { - self.status.store(new_status, Ordering::Release); + self.status.store(ThreadStatus::Exited, Ordering::Release); } /// Returns the reference to the atomic priority. @@ -114,16 +146,6 @@ impl Thread { &self.priority } - /// Returns the current priority. - pub fn priority(&self) -> Priority { - self.priority.load(Ordering::Relaxed) - } - - /// Updates the priority with the new value. - pub fn set_priority(&self, new_priority: Priority) { - self.priority.store(new_priority, Ordering::Relaxed) - } - /// Returns the reference to the atomic CPU affinity. pub fn atomic_cpu_affinity(&self) -> &AtomicCpuSet { &self.cpu_affinity diff --git a/kernel/src/thread/task.rs b/kernel/src/thread/task.rs index 2e816e073..86646e424 100644 --- a/kernel/src/thread/task.rs +++ b/kernel/src/thread/task.rs @@ -71,17 +71,17 @@ pub fn create_new_user_task(user_space: Arc, thread_ref: Arc) ReturnReason::KernelEvent => {} }; - if current_thread.status().is_exited() { + if current_thread.is_exited() { break; } handle_pending_signal(user_ctx, &ctx).unwrap(); // If current is suspended, wait for a signal to wake up self - while current_thread.status().is_stopped() { + while current_thread.is_stopped() { Thread::yield_now(); debug!("{} is suspended.", current_posix_thread.tid()); handle_pending_signal(user_ctx, &ctx).unwrap(); } - if current_thread.status().is_exited() { + if current_thread.is_exited() { debug!("exit due to signal"); break; }