From fcf53ef6a563fc06dbb623e17544dccc243c2fca Mon Sep 17 00:00:00 2001 From: apio Date: Thu, 27 Oct 2022 17:05:42 +0200 Subject: [PATCH] Kernel: Make waitpid() block by default unless WNOHANG is specified --- apps/src/init.c | 5 +- apps/src/sh.c | 3 +- kernel/include/thread/Task.h | 37 +++++++++-- kernel/src/sys/stdio.cpp | 1 + kernel/src/thread/Scheduler.cpp | 110 ++++++++++++++++++++++++++++---- kernel/src/thread/Task.cpp | 29 ++++++++- libs/libc/include/sys/wait.h | 3 + 7 files changed, 163 insertions(+), 25 deletions(-) diff --git a/apps/src/init.c b/apps/src/init.c index 6f1b56c3..cb054e0c 100644 --- a/apps/src/init.c +++ b/apps/src/init.c @@ -60,10 +60,7 @@ int main() for (;;) { - while ((result = wait(NULL)) == 0) // No child has exited yet - { - msleep(100); - } + result = wait(NULL); if (result == child) return 0; } } diff --git a/apps/src/sh.c b/apps/src/sh.c index cdf85167..e212b29c 100644 --- a/apps/src/sh.c +++ b/apps/src/sh.c @@ -169,8 +169,7 @@ void command_execute(command* cmd) perror(argv[0]); exit(127); } - pid_t result; - while ((result = waitpid(child, &status, 0)) == 0) { msleep(20); } + pid_t result = waitpid(child, &status, 0); if (result < 0) { perror("waitpid"); diff --git a/kernel/include/thread/Task.h b/kernel/include/thread/Task.h index c0655535..797daf90 100644 --- a/kernel/include/thread/Task.h +++ b/kernel/include/thread/Task.h @@ -7,6 +7,13 @@ #define TASK_MAX_FDS 32 +enum class BlockReason +{ + None, + Reading, + Waiting, +}; + struct Task { enum TaskState @@ -69,16 +76,32 @@ struct Task char name[128]; - struct - { - size_t size; - int fd; - char* buf; - } blocking_read_info; + BlockReason block_reason; - void resume_read(); + union { + struct + { + size_t size; + int fd; + char* buf; + } blocking_read_info; + struct + { + int64_t wait_pid; + int* wstatus; + } blocking_wait_info; + }; + + void resume(); bool is_still_blocking(); Descriptor* descriptor_from_fd(int fd, int& error); + + private: + void resume_read(); + void resume_wait(); + + bool is_read_still_blocking(); + bool is_wait_still_blocking(); }; \ No newline at end of file diff --git a/kernel/src/sys/stdio.cpp b/kernel/src/sys/stdio.cpp index a1be546b..df1276d1 100644 --- a/kernel/src/sys/stdio.cpp +++ b/kernel/src/sys/stdio.cpp @@ -255,6 +255,7 @@ void sys_read(Context* context, int fd, size_t size, char* buffer) } Task* current_task = Scheduler::current_task(); current_task->state = current_task->Blocking; + current_task->block_reason = BlockReason::Reading; current_task->blocking_read_info.fd = fd; current_task->blocking_read_info.buf = (char*)VMM::get_physical((uint64_t)buffer); // FIXME: Handle errors. current_task->blocking_read_info.size = size; diff --git a/kernel/src/thread/Scheduler.cpp b/kernel/src/thread/Scheduler.cpp index 8b31ce4a..4565786f 100644 --- a/kernel/src/thread/Scheduler.cpp +++ b/kernel/src/thread/Scheduler.cpp @@ -404,11 +404,7 @@ void Scheduler::task_yield(Context* context) sched_current_task = sched_current_task->next_task; if (sched_current_task->state == sched_current_task->Blocking) { - if (!sched_current_task->is_still_blocking()) - { - sched_current_task->resume_read(); - sched_current_task->state = sched_current_task->Running; - } + if (!sched_current_task->is_still_blocking()) sched_current_task->resume(); } if (sched_current_task->state == sched_current_task->Running) { @@ -464,8 +460,10 @@ Task* Scheduler::current_task() return sched_current_task; } +#define WNOHANG 1 + void sys_waitpid(Context* context, long pid, int* wstatus, - int) // FIXME: Use the value in options and block if WNOHANG has not been specified. + int options) // FIXME: only allow waiting for child processes when specifying a PID. { Task* child = nullptr; if (pid == -1) @@ -480,8 +478,29 @@ void sys_waitpid(Context* context, long pid, int* wstatus, }); if (!child) { - context->rax = 0; // No child has exited, let's return 0. - return; + if (options & WNOHANG) + { + context->rax = 0; // No child has exited, let's return 0. + return; + } + int* kwstatus; + if (wstatus) + { + kwstatus = obtain_user_ref(wstatus); + if (!kwstatus) + { + context->rax = -EFAULT; + return; + } + } + kdbgln("blocking wait on any child"); + sched_current_task->state = sched_current_task->Blocking; + sched_current_task->block_reason = BlockReason::Waiting; + sched_current_task->blocking_wait_info.wait_pid = -1; + if (wstatus) sched_current_task->blocking_wait_info.wstatus = kwstatus; + else + sched_current_task->blocking_wait_info.wstatus = nullptr; + return Scheduler::task_yield(context); } } else @@ -493,10 +512,31 @@ void sys_waitpid(Context* context, long pid, int* wstatus, return; } } - if (child->state != child->Dying) // FIXME: This should block if WNOHANG has not been specified. + if (child->state != child->Dying) { - context->rax = 0; - return; + if (options & WNOHANG) + { + context->rax = 0; // No child has exited, let's return 0. + return; + } + int* kwstatus; + if (wstatus) + { + kwstatus = obtain_user_ref(wstatus); + if (!kwstatus) + { + context->rax = -EFAULT; + return; + } + } + kdbgln("blocking wait on pid %ld", pid); + sched_current_task->state = sched_current_task->Blocking; + sched_current_task->block_reason = BlockReason::Waiting; + sched_current_task->blocking_wait_info.wait_pid = pid; + if (wstatus) sched_current_task->blocking_wait_info.wstatus = kwstatus; + else + sched_current_task->blocking_wait_info.wstatus = nullptr; + return Scheduler::task_yield(context); } if (wstatus) { @@ -518,6 +558,54 @@ void sys_waitpid(Context* context, long pid, int* wstatus, context->rax = (long)child->id; } +bool Task::is_wait_still_blocking() +{ + Task* child = nullptr; + if (blocking_wait_info.wait_pid == -1) + { + sched_for_each_child(sched_current_task, [&](Task* task) { + if (task->state == task->Dying) + { + child = task; + return false; + } + return true; + }); + if (!child) return true; + else + { + blocking_wait_info.wait_pid = child->id; // We're committed to this child now. + return false; + } + } + else + { + child = Scheduler::find_by_pid(blocking_wait_info.wait_pid); + if (child->state != child->Dying) return true; + else + return false; + } +} + +void Task::resume_wait() +{ + ASSERT(blocking_wait_info.wait_pid != -1); // is_wait_still_blocking should have chosen a child for us if the user + // process told us to wait for any child. + Task* child = Scheduler::find_by_pid(blocking_wait_info.wait_pid); + ASSERT(child); // This should also already have been validated. + + kdbgln("resuming wait on child %ld", child->id); + + if (blocking_wait_info.wstatus) + { + *blocking_wait_info.wstatus = (int)(child->exit_status & 0xff); + release_user_ref(blocking_wait_info.wstatus); + } + + child->state = child->Exited; + regs.rax = (long)child->id; +} + struct pstat { long pt_pid; diff --git a/kernel/src/thread/Task.cpp b/kernel/src/thread/Task.cpp index 392f352c..8d2fb115 100644 --- a/kernel/src/thread/Task.cpp +++ b/kernel/src/thread/Task.cpp @@ -3,6 +3,7 @@ #include "thread/Task.h" #include "log/Log.h" #include "memory/VMM.h" +#include "std/assert.h" #include "std/errno.h" #include "std/string.h" @@ -80,11 +81,37 @@ void Task::resume_read() VMM::apply_address_space(); } -bool Task::is_still_blocking() +bool Task::is_read_still_blocking() { return VFS::would_block(files[blocking_read_info.fd].node()); } +void Task::resume() +{ + switch (block_reason) + { + case BlockReason::None: ASSERT(false); + case BlockReason::Reading: resume_read(); break; + case BlockReason::Waiting: resume_wait(); break; + + default: ASSERT(false); + } + block_reason = BlockReason::None; + state = Running; +} + +bool Task::is_still_blocking() +{ + switch (block_reason) + { + case BlockReason::None: ASSERT(false); + case BlockReason::Reading: return is_read_still_blocking(); + case BlockReason::Waiting: return is_wait_still_blocking(); + + default: ASSERT(false); + } +} + Descriptor* Task::descriptor_from_fd(int fd, int& error) { if (fd < 0 || fd >= TASK_MAX_FDS) diff --git a/libs/libc/include/sys/wait.h b/libs/libc/include/sys/wait.h index 458d00bd..792ad645 100644 --- a/libs/libc/include/sys/wait.h +++ b/libs/libc/include/sys/wait.h @@ -9,6 +9,9 @@ /* What was the child's exit status? */ #define WEXITSTATUS(status) (char)((status)&0xff) +/* Do not block the current process if no child has exited. */ +#define WNOHANG 1 + #ifdef __cplusplus extern "C" {