From af83913188b6b0a06240374f3902926129b0dddb Mon Sep 17 00:00:00 2001 From: Runji Wang Date: Sat, 30 Nov 2019 18:11:03 +0800 Subject: [PATCH] fix data racing on thread crate The key point is that all methods on 'Processor' must be called with interrupt disabled. Otherwise if an interrupt happened inside a method, and then the thread is switched to other CPUs, it will touch other 'Processor'. --- kernel/Cargo.lock | 6 +++--- kernel/Cargo.toml | 2 +- kernel/src/logging.rs | 8 ++++---- kernel/src/process/mod.rs | 21 +++++---------------- kernel/src/process/structs.rs | 4 ++-- kernel/src/shell.rs | 6 +++--- kernel/src/sync/condvar.rs | 8 ++++---- kernel/src/syscall/mod.rs | 2 +- kernel/src/syscall/proc.rs | 22 +++++++++++----------- kernel/src/trap.rs | 4 ++-- 10 files changed, 36 insertions(+), 47 deletions(-) diff --git a/kernel/Cargo.lock b/kernel/Cargo.lock index ec6c046a..b80c93f8 100644 --- a/kernel/Cargo.lock +++ b/kernel/Cargo.lock @@ -356,7 +356,7 @@ dependencies = [ "rcore-fs-ramfs 0.1.0 (git+https://github.com/rcore-os/rcore-fs?rev=d8d6119)", "rcore-fs-sfs 0.1.0 (git+https://github.com/rcore-os/rcore-fs?rev=d8d6119)", "rcore-memory 0.1.0", - "rcore-thread 0.1.0 (git+https://github.com/rcore-os/rcore-thread?rev=95e716a2)", + "rcore-thread 0.1.0 (git+https://github.com/rcore-os/rcore-thread?rev=d727949b)", "riscv 0.5.0 (git+https://github.com/rcore-os/riscv)", "smoltcp 0.5.0 (git+https://github.com/rcore-os/smoltcp?rev=5bd87c7c)", "spin 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -436,7 +436,7 @@ dependencies = [ [[package]] name = "rcore-thread" version = "0.1.0" -source = "git+https://github.com/rcore-os/rcore-thread?rev=95e716a2#95e716a2d3c315b19dda787cfe7302957b66f80b" +source = "git+https://github.com/rcore-os/rcore-thread?rev=d727949b#d727949b8ea20c2e3a94f8ba015652c9eff8b53c" dependencies = [ "deque 0.3.2 (git+https://github.com/rcore-os/deque.git?branch=no_std)", "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", @@ -707,7 +707,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" "checksum rcore-fs-mountfs 0.1.0 (git+https://github.com/rcore-os/rcore-fs?rev=d8d6119)" = "" "checksum rcore-fs-ramfs 0.1.0 (git+https://github.com/rcore-os/rcore-fs?rev=d8d6119)" = "" "checksum rcore-fs-sfs 0.1.0 (git+https://github.com/rcore-os/rcore-fs?rev=d8d6119)" = "" -"checksum rcore-thread 0.1.0 (git+https://github.com/rcore-os/rcore-thread?rev=95e716a2)" = "" +"checksum rcore-thread 0.1.0 (git+https://github.com/rcore-os/rcore-thread?rev=d727949b)" = "" "checksum register 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)" = "e10f31b6d2299e5620986ad9fcdd66463e125ad72af4f403f9aedf7592d5ccdb" "checksum riscv 0.5.0 (git+https://github.com/rcore-os/riscv)" = "" "checksum rustc_version 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)" = "138e3e0acb6c9fb258b19b67cb8abd63c00679d2851805ea151465464fe9030a" diff --git a/kernel/Cargo.toml b/kernel/Cargo.toml index 8cac82d8..fbaaa241 100644 --- a/kernel/Cargo.toml +++ b/kernel/Cargo.toml @@ -62,7 +62,7 @@ smoltcp = { git = "https://github.com/rcore-os/smoltcp", rev = "5bd87c7c", defau bitmap-allocator = { git = "https://github.com/rcore-os/bitmap-allocator" } rcore-console = { git = "https://github.com/rcore-os/rcore-console", rev = "b7bacf9", default-features = false } rcore-memory = { path = "../crate/memory" } -rcore-thread = { git = "https://github.com/rcore-os/rcore-thread", rev = "95e716a2" } +rcore-thread = { git = "https://github.com/rcore-os/rcore-thread", rev = "d727949b" } rcore-fs = { git = "https://github.com/rcore-os/rcore-fs", rev = "d8d6119" } rcore-fs-sfs = { git = "https://github.com/rcore-os/rcore-fs", rev = "d8d6119" } rcore-fs-ramfs = { git = "https://github.com/rcore-os/rcore-fs", rev = "d8d6119" } diff --git a/kernel/src/logging.rs b/kernel/src/logging.rs index 6cabb431..1c43c65c 100644 --- a/kernel/src/logging.rs +++ b/kernel/src/logging.rs @@ -69,10 +69,10 @@ impl Log for SimpleLogger { if let Some(tid) = processor().tid_option() { print_in_color( format_args!( - "[{:>5}][{}][{}] {}\n", + "[{:>5}][{},{}] {}\n", record.level(), + crate::arch::cpu::id(), tid, - record.target(), record.args() ), level_to_color_code(record.level()), @@ -80,9 +80,9 @@ impl Log for SimpleLogger { } else { print_in_color( format_args!( - "[{:>5}][-][{}] {}\n", + "[{:>5}][{},-] {}\n", record.level(), - record.target(), + crate::arch::cpu::id(), record.args() ), level_to_color_code(record.level()), diff --git a/kernel/src/process/mod.rs b/kernel/src/process/mod.rs index 2f00b1df..62b647fd 100644 --- a/kernel/src/process/mod.rs +++ b/kernel/src/process/mod.rs @@ -90,22 +90,6 @@ static PROCESSORS: [Processor; MAX_CPU_NUM] = [ Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), - // Processor::new(), Processor::new(), Processor::new(), Processor::new(), ]; /// Get current thread @@ -118,6 +102,11 @@ pub unsafe fn current_thread() -> &'static mut Thread { process } +/// Get global thread manager. +pub fn thread_manager() -> &'static ThreadPool { + processor().manager() +} + // Implement dependencies for std::thread #[no_mangle] diff --git a/kernel/src/process/structs.rs b/kernel/src/process/structs.rs index 2e0eb411..17427e62 100644 --- a/kernel/src/process/structs.rs +++ b/kernel/src/process/structs.rs @@ -21,7 +21,7 @@ use crate::memory::{ use crate::sync::{Condvar, SpinNoIrqLock as Mutex}; use super::abi::{self, ProcInitInfo}; -use crate::processor; +use crate::process::thread_manager; use core::mem::MaybeUninit; use rcore_fs::vfs::INode; @@ -420,7 +420,7 @@ impl Process { pub fn exit(&mut self, exit_code: usize) { // quit all threads for tid in self.threads.iter() { - processor().manager().exit(*tid, 1); + thread_manager().exit(*tid, 1); } // notify parent and fill exit code if let Some(parent) = self.parent.upgrade() { diff --git a/kernel/src/shell.rs b/kernel/src/shell.rs index f811969c..1e967b7d 100644 --- a/kernel/src/shell.rs +++ b/kernel/src/shell.rs @@ -38,7 +38,7 @@ pub fn add_user_shell() { .manager() .add(Thread::new_user(&inode, init_shell, init_args, init_envs)); } else { - processor().manager().add(Thread::new_kernel(shell, 0)); + thread_manager().add(Thread::new_kernel(shell, 0)); } } @@ -47,7 +47,7 @@ pub fn add_user_shell() { use crate::drivers::CMDLINE; let cmdline = CMDLINE.read(); let inode = ROOT_INODE.lookup(&cmdline).unwrap(); - processor().manager().add(Thread::new_user( + thread_manager().add(Thread::new_user( &inode, &cmdline, cmdline.split(' ').map(|s| s.into()).collect(), @@ -68,7 +68,7 @@ pub extern "C" fn shell(_arg: usize) -> ! { } let name = cmd.trim().split(' ').next().unwrap(); if let Ok(inode) = ROOT_INODE.lookup(name) { - let _tid = processor().manager().add(Thread::new_user( + let _tid = thread_manager().add(Thread::new_user( &inode, &cmd, cmd.split(' ').map(|s| s.into()).collect(), diff --git a/kernel/src/sync/condvar.rs b/kernel/src/sync/condvar.rs index 8b140b6b..22ca8f63 100644 --- a/kernel/src/sync/condvar.rs +++ b/kernel/src/sync/condvar.rs @@ -1,6 +1,6 @@ use super::*; use crate::process::Process; -use crate::process::{current_thread, processor}; +use crate::process::{current_thread, thread_manager}; use crate::thread; use alloc::collections::VecDeque; use alloc::sync::Arc; @@ -63,19 +63,19 @@ impl Condvar { let mut lock = condvar.wait_queue.lock(); locks.push(lock); } - processor().manager().sleep(tid, 0); + thread_manager().sleep(tid, 0); locks.clear(); if let Some(res) = condition() { let _ = FlagsGuard::no_irq_region(); - processor().manager().cancel_sleeping(tid); + thread_manager().cancel_sleeping(tid); for condvar in condvars { let mut lock = condvar.wait_queue.lock(); lock.retain(|t| !Arc::ptr_eq(t, &token)); } return res; } - processor().yield_now(); + thread::yield_now(); } } diff --git a/kernel/src/syscall/mod.rs b/kernel/src/syscall/mod.rs index 37bc431b..7d801158 100644 --- a/kernel/src/syscall/mod.rs +++ b/kernel/src/syscall/mod.rs @@ -80,7 +80,7 @@ impl Syscall<'_> { let begin_time = unsafe { core::arch::x86_64::_rdtsc() }; let cid = cpu::id(); let pid = self.process().pid.clone(); - let tid = processor().tid(); + let tid = thread::current().id(); if !pid.is_init() { // we trust pid 0 process debug!("{}:{}:{} syscall id {} begin", cid, pid, tid, id); diff --git a/kernel/src/syscall/proc.rs b/kernel/src/syscall/proc.rs index a20fc4fd..6a79aabd 100644 --- a/kernel/src/syscall/proc.rs +++ b/kernel/src/syscall/proc.rs @@ -7,8 +7,8 @@ impl Syscall<'_> { pub fn sys_fork(&mut self) -> SysResult { let new_thread = self.thread.fork(self.tf); let pid = new_thread.proc.lock().pid.get(); - let tid = processor().manager().add(new_thread); - processor().manager().detach(tid); + let tid = thread_manager().add(new_thread); + thread_manager().detach(tid); info!("fork: {} -> {}", thread::current().id(), pid); Ok(pid) } @@ -53,8 +53,8 @@ impl Syscall<'_> { let new_thread = self .thread .clone(self.tf, newsp, newtls, child_tid as usize); - let tid = processor().manager().add(new_thread); - processor().manager().detach(tid); + let tid = thread_manager().add(new_thread); + thread_manager().detach(tid); info!("clone: {} -> {}", thread::current().id(), tid); *parent_tid_ref = tid as u32; *child_tid_ref = tid as u32; @@ -172,10 +172,10 @@ impl Syscall<'_> { // Kill other threads proc.threads.retain(|&tid| { - if tid != processor().tid() { - processor().manager().exit(tid, 1); + if tid != thread::current().id() { + thread_manager().exit(tid, 1); } - tid == processor().tid() + tid == thread::current().id() }); // Read program file @@ -280,8 +280,8 @@ impl Syscall<'_> { drop(proc); - processor().manager().exit(tid, exit_code as usize); - processor().yield_now(); + thread_manager().exit(tid, exit_code as usize); + thread::yield_now(); unreachable!(); } @@ -292,7 +292,7 @@ impl Syscall<'_> { proc.exit(exit_code); - processor().yield_now(); + thread::yield_now(); unreachable!(); } @@ -306,7 +306,7 @@ impl Syscall<'_> { pub fn sys_set_priority(&mut self, priority: usize) -> SysResult { let pid = thread::current().id(); - processor().manager().set_priority(pid, priority as u8); + thread_manager().set_priority(pid, priority as u8); Ok(0) } diff --git a/kernel/src/trap.rs b/kernel/src/trap.rs index fe76b03a..2b0a7a9c 100644 --- a/kernel/src/trap.rs +++ b/kernel/src/trap.rs @@ -3,7 +3,7 @@ use crate::arch::interrupt::TrapFrame; use crate::consts::INFORM_PER_MSEC; use crate::process::*; use crate::sync::Condvar; -use log::*; +use rcore_thread::std_thread as thread; pub static mut TICK: usize = 0; @@ -33,7 +33,7 @@ pub fn error(tf: &TrapFrame) -> ! { let mut proc = current_thread().proc.lock(); proc.exit(0x100); } - processor().yield_now(); + thread::yield_now(); unreachable!(); }