From d3c3283d2e50142a1630c605a7f6cec85726da3b Mon Sep 17 00:00:00 2001 From: ChenRuiwei <1982833213@qq.com> Date: Sat, 27 Jul 2024 16:53:25 +0800 Subject: [PATCH] fix(task): task should always call do_exit to become zombie state Parent can call wait4 to clean zombie children, however, in previous design, task could be zombie state without calling do_exit function, which will cause bugs, e.g. children not reparented. --- Cargo.lock | 8 ++++ crates/range-map/Cargo.toml | 9 +++++ .../range-map/src/lib.rs | 6 +++ kernel/Cargo.toml | 1 + kernel/src/mm/memory_space/mod.rs | 4 +- kernel/src/syscall/process.rs | 6 +-- kernel/src/task/schedule.rs | 10 ++--- kernel/src/task/signal.rs | 3 +- kernel/src/task/task.rs | 40 ++++++++++++++----- kernel/src/trap/user_trap.rs | 5 +-- modules/memory/src/page_table.rs | 4 +- user/src/bin/final_tests.rs | 4 +- 12 files changed, 72 insertions(+), 28 deletions(-) create mode 100644 crates/range-map/Cargo.toml rename kernel/src/mm/memory_space/range_map.rs => crates/range-map/src/lib.rs (98%) diff --git a/Cargo.lock b/Cargo.lock index 73b5e413..0cbfa62b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -588,6 +588,7 @@ dependencies = [ "numeric-enum-macro", "page", "paste", + "range-map", "recycle-allocator", "riscv", "sbi-print", @@ -869,6 +870,13 @@ dependencies = [ "getrandom", ] +[[package]] +name = "range-map" +version = "0.1.0" +dependencies = [ + "log", +] + [[package]] name = "recycle-allocator" version = "0.1.0" diff --git a/crates/range-map/Cargo.toml b/crates/range-map/Cargo.toml new file mode 100644 index 00000000..2b7678b9 --- /dev/null +++ b/crates/range-map/Cargo.toml @@ -0,0 +1,9 @@ +[package] +name = "range-map" +version = "0.1.0" +edition = "2021" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] +log = "0.4" diff --git a/kernel/src/mm/memory_space/range_map.rs b/crates/range-map/src/lib.rs similarity index 98% rename from kernel/src/mm/memory_space/range_map.rs rename to crates/range-map/src/lib.rs index c76d0106..2a40069b 100644 --- a/kernel/src/mm/memory_space/range_map.rs +++ b/crates/range-map/src/lib.rs @@ -1,6 +1,12 @@ +#![no_std] +#![no_main] +#![feature(map_try_insert)] + use alloc::collections::BTreeMap; use core::ops::{Add, Range}; +extern crate alloc; + #[derive(Clone, Debug)] struct Node { pub end: U, diff --git a/kernel/Cargo.toml b/kernel/Cargo.toml index bee5fc39..7a9fc4f5 100644 --- a/kernel/Cargo.toml +++ b/kernel/Cargo.toml @@ -23,6 +23,7 @@ net = { path = "../modules/net/" } recycle-allocator = { path = "../crates/recycle-allocator/" } async-utils = { path = "../crates/async-utils/" } sbi-print = { path = "../crates/sbi-print/" } +range-map = { path = "../crates/range-map/" } backtrace = { path = "../crates/backtrace/" } cfg-if = "1.0" diff --git a/kernel/src/mm/memory_space/mod.rs b/kernel/src/mm/memory_space/mod.rs index 3b9c25c5..e6c6ada2 100644 --- a/kernel/src/mm/memory_space/mod.rs +++ b/kernel/src/mm/memory_space/mod.rs @@ -12,6 +12,7 @@ use core::{ ops::{self, Range}, }; +use ::range_map::RangeMap; use arch::memory::{sfence_vma_all, sfence_vma_vaddr}; use async_utils::block_on; use config::{ @@ -33,7 +34,7 @@ use systype::{SysError, SysResult}; use vfs_core::{Dentry, File}; use xmas_elf::ElfFile; -use self::{range_map::RangeMap, vm_area::VmArea}; +use self::vm_area::VmArea; use super::{kernel_page_table, PageFaultAccessType}; use crate::{ mm::{ @@ -48,7 +49,6 @@ use crate::{ }, }; -mod range_map; pub mod vm_area; /// Virtual memory space for kernel and user. diff --git a/kernel/src/syscall/process.rs b/kernel/src/syscall/process.rs index 13eea1ff..0f86c95e 100644 --- a/kernel/src/syscall/process.rs +++ b/kernel/src/syscall/process.rs @@ -88,7 +88,7 @@ impl Syscall<'_> { /// group. pub fn sys_exit(&self, exit_code: i32) -> SyscallResult { let task = self.task; - task.set_zombie(); + task.set_terminated(); // non-leader thread are detached (see CLONE_THREAD flag in manual page clone.2) if task.is_leader() { task.set_exit_code((exit_code & 0xFF) << 8); @@ -102,8 +102,8 @@ impl Syscall<'_> { let task = self.task; task.with_thread_group(|tg| { for t in tg.iter() { - t.set_zombie(); - log::info!("tid {} set zombie", t.tid()); + t.set_terminated(); + log::info!("tid {} set terminated", t.tid()); } }); task.set_exit_code((exit_code & 0xFF) << 8); diff --git a/kernel/src/task/schedule.rs b/kernel/src/task/schedule.rs index 30f10b4a..6de47af8 100644 --- a/kernel/src/task/schedule.rs +++ b/kernel/src/task/schedule.rs @@ -80,17 +80,17 @@ pub async fn task_loop(task: Arc) { *task.waker() = Some(get_waker().await); loop { match task.state() { - Zombie => break, + Terminated => break, Stopped => suspend_now().await, _ => {} } trap::user_trap::trap_return(&task); - // task may be set to zombie by other task, e.g. execve will kill other tasks in - // the same thread group + // task may be set to terminated by other task, e.g. execve will kill other + // tasks in the same thread group match task.state() { - Zombie => break, + Terminated => break, Stopped => suspend_now().await, _ => {} } @@ -98,7 +98,7 @@ pub async fn task_loop(task: Arc) { let intr = trap::user_trap::trap_handler(&task).await; match task.state() { - Zombie => break, + Terminated => break, Stopped => suspend_now().await, _ => {} } diff --git a/kernel/src/task/signal.rs b/kernel/src/task/signal.rs index 23257fd8..4fec2c02 100644 --- a/kernel/src/task/signal.rs +++ b/kernel/src/task/signal.rs @@ -256,12 +256,13 @@ fn terminate(task: &Arc, sig: Sig) { // exit all the memers of a thread group task.with_thread_group(|tg| { for t in tg.iter() { - t.set_zombie(); + t.set_terminated(); } }); // 将信号放入低7位 (第8位是core dump标志,在gdb调试崩溃程序中用到) task.set_exit_code(sig.raw() as i32 & 0x7F); } + fn stop(task: &Arc, sig: Sig) { log::warn!("[do_signal] task stopped!"); task.with_mut_thread_group(|tg| { diff --git a/kernel/src/task/task.rs b/kernel/src/task/task.rs index f0d168b1..e8cfaeaa 100644 --- a/kernel/src/task/task.rs +++ b/kernel/src/task/task.rs @@ -146,8 +146,13 @@ pub enum TaskState { /// The task is currently running or ready to run, occupying the CPU and /// executing its code. Running, - /// The task has terminated, but its process control block (PCB) still - /// exists for the parent process to read its exit status. + /// The task has been terminated for user mode, which means it will + /// never return back to user anymore. All it left to do is to call + /// `do_exit` to end its life in kernel mode. + Terminated, + /// The task has has called `do_exit` function, but its process + /// control block (PCB) still exists for the parent process to read its + /// exit status. Zombie, /// The task has been stopped, usually due to receiving a stop signal (e.g., /// SIGSTOP). It can be resumed with a continue signal (e.g., SIGCONT). @@ -169,7 +174,14 @@ pub enum TaskState { impl Task { // you can use is_running() / set_running()、 is_zombie() / set_zombie() - generate_state_methods!(Running, Zombie, Stopped, Interruptable, UnInterruptable); + generate_state_methods!( + Running, + Zombie, + Stopped, + Terminated, + Interruptable, + UnInterruptable + ); generate_accessors!( waker: Option, tid_address: TidAddress, @@ -469,7 +481,7 @@ impl Task { let mut pid = 0; for t in tg.iter() { if !t.is_leader() { - t.set_zombie(); + t.set_terminated(); } else { pid = t.tid(); } @@ -523,7 +535,7 @@ impl Task { // NOTE: After all of the threads in a thread group is terminated, the parent // process of the thread group is sent a SIGCHLD (or other termination) signal. // WARN: do not call this function directly if a task should be terminated, - // instead, call `set_zombie` + // instead, call `set_terminated` pub fn do_exit(self: &Arc) { log::info!("thread {} do exit", self.tid()); assert_ne!( @@ -551,11 +563,11 @@ impl Task { let mut tg = self.thread_group.lock(); - if (!self.leader().is_zombie()) + if (!self.leader().is_terminated()) || (self.is_leader && tg.len() > 1) || (!self.is_leader && tg.len() > 2) { - if !self.is_leader { + if !self.is_leader() { // NOTE: leader will be removed by parent calling `sys_wait4` tg.remove(self); TASK_MANAGER.remove(self.tid()); @@ -563,10 +575,10 @@ impl Task { return; } - if self.is_leader { - debug_assert!(tg.len() == 1); + if self.is_leader() { + assert!(tg.len() == 1); } else { - debug_assert!(tg.len() == 2); + assert!(tg.len() == 2); // NOTE: leader will be removed by parent calling `sys_wait4` tg.remove(self); TASK_MANAGER.remove(self.tid()); @@ -628,6 +640,14 @@ impl Task { // TODO: drop most resources here instead of wait4 function parent // called + + if self.is_leader() { + self.set_zombie(); + } else { + self.leader().set_zombie(); + } + // When the task is not leader, which means its is not a process, it + // will get dropped when hart leaves this task. } /// The dirfd argument is used in conjunction with the pathname argument as diff --git a/kernel/src/trap/user_trap.rs b/kernel/src/trap/user_trap.rs index f9a2092a..b03459e8 100644 --- a/kernel/src/trap/user_trap.rs +++ b/kernel/src/trap/user_trap.rs @@ -98,14 +98,13 @@ pub async fn trap_handler(task: &Arc) -> bool { }, false, ); - // task.set_zombie(); } } Exception::IllegalInstruction => { log::warn!( "[trap_handler] detected illegal instruction, stval {stval:#x}, sepc {sepc:#x}", ); - task.set_zombie(); + task.set_terminated(); } e => { log::warn!("Unknown user exception: {:?}", e); @@ -163,7 +162,7 @@ pub fn trap_return(task: &Arc) { task.trap_context_mut().user_fx.restore(); task.trap_context_mut().sstatus.set_fs(FS::Clean); assert!(!task.trap_context_mut().sstatus.sie()); - assert!(!task.is_zombie()); + assert!(!task.is_terminated() && !task.is_zombie()); unsafe { __return_to_user(task.trap_context_mut()); // NOTE: next time when user traps into kernel, it will come back here diff --git a/modules/memory/src/page_table.rs b/modules/memory/src/page_table.rs index 3fc614ff..e5c4f723 100644 --- a/modules/memory/src/page_table.rs +++ b/modules/memory/src/page_table.rs @@ -73,8 +73,8 @@ impl PageTable { frames: Vec::new(), }; let leaf_pte = page_table.find_pte(vaddr.floor()).unwrap(); - let paddr = (leaf_pte.ppn().bits() << PAGE_SIZE_BITS) + (vaddr.bits() & PAGE_MASK); - paddr.into() + let paddr = leaf_pte.ppn().to_pa() + vaddr.page_offset(); + paddr } /// Switch to this pagetable diff --git a/user/src/bin/final_tests.rs b/user/src/bin/final_tests.rs index 9a75843d..35e18a1a 100644 --- a/user/src/bin/final_tests.rs +++ b/user/src/bin/final_tests.rs @@ -10,17 +10,17 @@ use user_lib::{execve, fork, wait, waitpid}; #[macro_use] extern crate user_lib; -const TESTCASES: [&str; 9] = [ +const TESTCASES: [&str; 10] = [ "time-test", "busybox_testcode.sh", "lua_testcode.sh", "netperf_testcode.sh", - // "cyclictest_testcode.sh", "libc-bench", "libctest_testcode.sh", "iozone_testcode.sh", "unixbench_testcode.sh", "lmbench_testcode.sh", + "cyclictest_testcode.sh", ]; fn run_cmd(cmd: &str) {