From a0a112ffe70f359a5ec1c25edd547f79a52b9ba4 Mon Sep 17 00:00:00 2001 From: Christian Helmuth Date: Thu, 3 Sep 2020 14:07:41 +0200 Subject: [PATCH] libc: use monitor for fork Issue #3874 --- repos/libports/src/lib/libc/fork.cc | 177 ++++++++---------- repos/libports/src/lib/libc/internal/init.h | 4 +- repos/libports/src/lib/libc/internal/kernel.h | 33 +--- .../src/lib/libc/internal/kernel_routine.h | 46 ----- repos/libports/src/lib/libc/kernel.cc | 5 +- 5 files changed, 82 insertions(+), 183 deletions(-) delete mode 100644 repos/libports/src/lib/libc/internal/kernel_routine.h diff --git a/repos/libports/src/lib/libc/fork.cc b/repos/libports/src/lib/libc/fork.cc index e98968ede3..19ab8de016 100644 --- a/repos/libports/src/lib/libc/fork.cc +++ b/repos/libports/src/lib/libc/fork.cc @@ -12,7 +12,6 @@ */ /* Genode includes */ -#include #include #include #include @@ -34,9 +33,7 @@ /* libc-internal includes */ #include #include -#include -#include -#include +#include #include namespace Libc { @@ -58,15 +55,15 @@ namespace Libc { using namespace Libc; +namespace { using Fn = Monitor::Function_result; } + static pid_t fork_result; static Env *_env_ptr; static Allocator *_alloc_ptr; -static Suspend *_suspend_ptr; -static Resume *_resume_ptr; +static Monitor *_monitor_ptr; static Libc::Signal *_signal_ptr; -static Kernel_routine_scheduler *_kernel_routine_scheduler_ptr; static Heap *_malloc_heap_ptr; static void *_user_stack_base_ptr; static size_t _user_stack_size; @@ -77,6 +74,15 @@ static Binary_name const *_binary_name_ptr; static Forked_children *_forked_children_ptr; +static Libc::Monitor & monitor() +{ + struct Missing_call_of_init_fork : Genode::Exception { }; + if (!_monitor_ptr) + throw Missing_call_of_init_fork(); + return *_monitor_ptr; +} + + struct Libc::Child_config { Constructible _ds { }; @@ -348,44 +354,35 @@ struct Libc::Local_clone_service : Noncopyable typedef Local_service Service; - Child_ready &_child_ready; - - Resume &_resume; - - Io_signal_handler _child_ready_handler; - - void _handle_child_ready() - { - _child_ready.child_ready(); - - _resume.resume_all(); - } - struct Factory : Local_service::Factory { - Session &_session; - Signal_context_capability _started_sigh; + Session &_session; + Child_ready &_child_ready; - Factory(Session &session, Signal_context_capability started_sigh) - : _session(session), _started_sigh(started_sigh) { } + Factory(Session &session, Child_ready &child_ready) + : _session(session), _child_ready(child_ready) { } Session &create(Args const &, Affinity) override { return _session; } void upgrade(Session &, Args const &) override { } - void destroy(Session &) override { Signal_transmitter(_started_sigh).submit(); } + void destroy(Session &) override + { + Mutex mutex { }; + mutex.acquire(); + + monitor().monitor(mutex, [&] { + _child_ready.child_ready(); + return Fn::COMPLETE; + }); + } } _factory; Service service { _factory }; - Local_clone_service(Env &env, Entrypoint &ep, Child_ready &child_ready, - Resume &resume) - : - _session(env, ep), _child_ready(child_ready), _resume(resume), - _child_ready_handler(env.ep(), *this, &Local_clone_service::_handle_child_ready), - _factory(_session, _child_ready_handler) - { } + Local_clone_service(Env &env, Entrypoint &ep, Child_ready &child_ready) + : _session(env, ep), _factory(_session, child_ready) { } }; @@ -395,8 +392,6 @@ struct Libc::Forked_child : Child_policy, Child_ready Binary_name const _binary_name; - Resume &_resume; - Signal &_signal; pid_t const _pid; @@ -408,8 +403,8 @@ struct Libc::Forked_child : Child_policy, Child_ready Name const _name { _pid }; /* - * Signal handler triggered at the main entrypoint, waking up the libc - * suspend mechanism. + * Signal handler triggered at the main entrypoint, charging 'SIGCHILD' + * and waking up the libc monitor mechanism. */ Io_signal_handler _exit_handler { _env.ep(), *this, &Forked_child::_handle_exit }; @@ -417,7 +412,7 @@ struct Libc::Forked_child : Child_policy, Child_ready void _handle_exit() { _signal.charge(SIGCHLD); - _resume.resume_all(); + monitor().trigger_monitor_examination(); } Child_config _child_config; @@ -427,20 +422,6 @@ struct Libc::Forked_child : Child_policy, Child_ready Local_clone_service _local_clone_service; Local_rom_service _config_rom_service; - struct Wait_fork_ready : Kernel_routine - { - Forked_child const &child; - - Wait_fork_ready(Forked_child const &child) : child(child) { } - - void execute_in_kernel() override - { - /* keep executing this kernel routine until child is running */ - if (!child.running() && !child.exited()) - _kernel_routine_scheduler_ptr->register_kernel_routine(*this); - } - } wait_fork_ready { *this }; - pid_t pid() const { return _pid; } bool running() const { return _state == State::RUNNING; } @@ -459,7 +440,7 @@ struct Libc::Forked_child : Child_policy, Child_ready /* * Don't modify the state if the child already exited. * This can happen for short-lived children where the asynchronous - * notification for '_handle_exit' arrives before '_handle_child_ready' + * notification for 'Child_exit' arrives before 'Child_ready' * (while the parent is still blocking in the fork call). */ if (_state == State::STARTING_UP) @@ -544,6 +525,10 @@ struct Libc::Forked_child : Child_policy, Child_ready _exit_code = code; _state = State::EXITED; + /* + * We can't destroy the child object in a monitor from this RPC + * function as this would deadlock in an 'Entrypoint::dissolve()'. + */ Signal_transmitter(_exit_handler).submit(); } @@ -553,7 +538,6 @@ struct Libc::Forked_child : Child_policy, Child_ready Entrypoint &fork_ep, Allocator &alloc, Binary_name const &binary_name, - Resume &resume, Signal &signal, pid_t pid, Config_accessor const &config_accessor, @@ -561,11 +545,11 @@ struct Libc::Forked_child : Child_policy, Child_ready Local_rom_services &local_rom_services) : _env(env), _binary_name(binary_name), - _resume(resume), _signal(signal), _pid(pid), + _signal(signal), _pid(pid), _child_config(env, config_accessor, pid), _parent_services(parent_services), _local_rom_services(local_rom_services), - _local_clone_service(env, fork_ep, *this, resume), + _local_clone_service(env, fork_ep, *this), _config_rom_service(fork_ep, "config", _child_config.ds_cap()), _child(env.rm(), fork_ep.rpc_ep(), *this) { } @@ -574,7 +558,7 @@ struct Libc::Forked_child : Child_policy, Child_ready }; -static void fork_kernel_routine() +static Forked_child * fork_kernel_routine() { fork_result = 0; @@ -585,7 +569,6 @@ static void fork_kernel_routine() Env &env = *_env_ptr; Allocator &alloc = *_alloc_ptr; - Resume &resume = *_resume_ptr; Libc::Signal &signal = *_signal_ptr; pid_t const child_pid = ++_pid_cnt; @@ -597,15 +580,15 @@ static void fork_kernel_routine() static Local_rom_services local_rom_services(env, fork_ep, alloc); - Registered &child = *new (alloc) + Registered *child = new (alloc) Registered(*_forked_children_ptr, env, fork_ep, alloc, - *_binary_name_ptr, resume, + *_binary_name_ptr, signal, child_pid, *_config_accessor_ptr, parent_services, local_rom_services); fork_result = child_pid; - _kernel_routine_scheduler_ptr->register_kernel_routine(child.wait_fork_ready); + return child; } @@ -627,25 +610,28 @@ extern "C" pid_t __sys_fork(void) _user_stack_base_ptr = (void *)mystack.base; _user_stack_size = mystack.top - mystack.base; - struct Fork_kernel_routine : Kernel_routine - { - void execute_in_kernel() override { fork_kernel_routine(); } + Mutex mutex { }; + mutex.acquire(); - } kernel_routine { }; + enum class Stage { FORK, WAIT_FORK_READY }; + + Stage stage { Stage::FORK }; + Forked_child *child { nullptr }; - struct Missing_call_of_init_fork : Exception { }; - if (!_kernel_routine_scheduler_ptr || !_suspend_ptr) - throw Missing_call_of_init_fork(); + monitor().monitor(mutex, [&] { + switch (stage) { + case Stage::FORK: + child = fork_kernel_routine(); + stage = Stage::WAIT_FORK_READY; [[ fallthrough ]] + case Stage::WAIT_FORK_READY: + if (child->running() || child->exited()) { + return Fn::COMPLETE; + } + break; + } - _kernel_routine_scheduler_ptr->register_kernel_routine(kernel_routine); - - struct Suspend_functor_impl : Suspend_functor - { - bool suspend() override { return false; } - - } suspend_functor { }; - - _suspend_ptr->suspend(suspend_functor, 0); + return Fn::INCOMPLETE; + }); return fork_result; } @@ -669,15 +655,15 @@ pid_t getpid(void) __attribute__((weak, alias("__sys_getpid"))); ** wait4 ** ***********/ -namespace Libc { struct Wait4_suspend_functor; } +namespace Libc { struct Wait4_functor; } -struct Libc::Wait4_suspend_functor : Suspend_functor +struct Libc::Wait4_functor { Forked_children &_children; pid_t const _pid; - Wait4_suspend_functor(pid_t pid, Forked_children &children) + Wait4_functor(pid_t pid, Forked_children &children) : _children(children), _pid(pid) { } template @@ -700,14 +686,6 @@ struct Libc::Wait4_suspend_functor : Suspend_functor fn(*child_ptr); return true; } - - bool suspend() override - { - bool const any_child_exited = - with_exited_child([] (Forked_child const &) { }); - - return !any_child_exited; - } }; @@ -724,25 +702,23 @@ extern "C" pid_t __sys_wait4(pid_t pid, int *status, int options, rusage *rusage return -1; } - struct Missing_call_of_init_fork : Exception { }; - if (!_suspend_ptr) - throw Missing_call_of_init_fork(); + Wait4_functor functor { pid, *_forked_children_ptr }; - Wait4_suspend_functor suspend_functor { pid, *_forked_children_ptr }; + Mutex mutex { }; + mutex.acquire(); - for (;;) { - - suspend_functor.with_exited_child([&] (Registered &child) { + monitor().monitor(mutex, [&] { + functor.with_exited_child([&] (Registered &child) { result = child.pid(); exit_code = child.exit_code(); destroy(*_alloc_ptr, &child); }); if (result >= 0 || (options & WNOHANG)) - break; + return Fn::COMPLETE; - _suspend_ptr->suspend(suspend_functor, 0); - } + return Fn::INCOMPLETE; + }); /* * The libc expects status information in bits 0..6 and the exit value @@ -761,16 +737,13 @@ extern "C" pid_t wait4(pid_t, int *, int, rusage *) __attribute__((weak, alias(" void Libc::init_fork(Env &env, Config_accessor const &config_accessor, Allocator &alloc, Heap &malloc_heap, pid_t pid, - Suspend &suspend, Resume &resume, Signal &signal, - Kernel_routine_scheduler &kernel_routine_scheduler, + Monitor &monitor, Signal &signal, Binary_name const &binary_name) { _env_ptr = &env; _alloc_ptr = &alloc; - _suspend_ptr = &suspend; - _resume_ptr = &resume; + _monitor_ptr = &monitor; _signal_ptr = &signal; - _kernel_routine_scheduler_ptr = &kernel_routine_scheduler; _malloc_heap_ptr = &malloc_heap; _config_accessor_ptr = &config_accessor; _pid = pid; diff --git a/repos/libports/src/lib/libc/internal/init.h b/repos/libports/src/lib/libc/internal/init.h index afb5b09ae6..ecec26be92 100644 --- a/repos/libports/src/lib/libc/internal/init.h +++ b/repos/libports/src/lib/libc/internal/init.h @@ -35,7 +35,6 @@ namespace Libc { struct Current_time; struct Current_real_time; struct Clone_connection; - struct Kernel_routine_scheduler; struct Watch; struct Signal; struct File_descriptor_allocator; @@ -126,8 +125,7 @@ namespace Libc { */ void init_fork(Genode::Env &, Config_accessor const &, Genode::Allocator &heap, Heap &malloc_heap, int pid, - Suspend &, Resume &, Signal &, Kernel_routine_scheduler &, - Binary_name const &); + Monitor &, Signal &, Binary_name const &); struct Reset_malloc_heap : Interface { diff --git a/repos/libports/src/lib/libc/internal/kernel.h b/repos/libports/src/lib/libc/internal/kernel.h index 1bb1db151c..258903bbc2 100644 --- a/repos/libports/src/lib/libc/internal/kernel.h +++ b/repos/libports/src/lib/libc/internal/kernel.h @@ -34,7 +34,6 @@ #include #include #include -#include #include #include #include @@ -108,7 +107,6 @@ struct Libc::Kernel final : Vfs::Io_response_handler, Suspend, Monitor, Select, - Kernel_routine_scheduler, Current_time, Current_real_time, Watch, @@ -236,8 +234,6 @@ struct Libc::Kernel final : Vfs::Io_response_handler, Select_handler_base *_scheduled_select_handler = nullptr; - Kernel_routine *_kernel_routine = nullptr; - void _resume_main() { _resume_main_once = true; } Kernel_timer_accessor _timer_accessor { _env }; @@ -356,7 +352,7 @@ struct Libc::Kernel final : Vfs::Io_response_handler, exit(1); } - if (!check.suspend() && !_kernel_routine) + if (!check.suspend()) return timeout_ms; if (timeout_ms > 0) @@ -425,6 +421,7 @@ struct Libc::Kernel final : Vfs::Io_response_handler, /* _setjmp() returned directly -> switch to user stack and call application code */ if (_cloned) { + _main_monitor_job->complete(); _switch_to_user(); } else { _state = USER; @@ -438,22 +435,6 @@ struct Libc::Kernel final : Vfs::Io_response_handler, while ((!_app_returned)) { - if (_kernel_routine) { - Kernel_routine &routine = *_kernel_routine; - - /* the 'kernel_routine' may install another kernel routine */ - _kernel_routine = nullptr; - routine.execute_in_kernel(); - - if (!_kernel_routine) { - _switch_to_user(); - } - - if (_kernel_routine) { - _env.ep().wait_and_dispatch_one_io_signal(); - } - } - /* * Dispatch all pending I/O signals at once and execute * monitors that may now become able to complete. @@ -521,7 +502,7 @@ struct Libc::Kernel final : Vfs::Io_response_handler, /* * Return to the application */ - if (!_kernel_routine && _resume_main_once && !_setjmp(_kernel_context)) { + if (_resume_main_once && !_setjmp(_kernel_context)) { _switch_to_user(); } } @@ -701,14 +682,6 @@ struct Libc::Kernel final : Vfs::Io_response_handler, void handle_io_progress() override; - /** - * Kernel_routine_scheduler interface - */ - void register_kernel_routine(Kernel_routine &kernel_routine) override - { - _kernel_routine = &kernel_routine; - } - /******************************** ** Access to kernel singleton ** diff --git a/repos/libports/src/lib/libc/internal/kernel_routine.h b/repos/libports/src/lib/libc/internal/kernel_routine.h deleted file mode 100644 index 78ad5edba9..0000000000 --- a/repos/libports/src/lib/libc/internal/kernel_routine.h +++ /dev/null @@ -1,46 +0,0 @@ -/* - * \brief Interface executing code in the context of the libc kernel - * \author Norman Feske - * \date 2019-09-18 - */ - -/* - * Copyright (C) 2019 Genode Labs GmbH - * - * This file is part of the Genode OS framework, which is distributed - * under the terms of the GNU Affero General Public License version 3. - */ - -#ifndef _LIBC__INTERNAL__KERNEL_ROUTINE_H_ -#define _LIBC__INTERNAL__KERNEL_ROUTINE_H_ - -/* libc-internal includes */ -#include - -namespace Libc { - - /** - * Base class to be implemented by a kernel routine - */ - struct Kernel_routine : Interface - { - virtual void execute_in_kernel() = 0; - }; - - struct Kernel_routine_scheduler : Interface - { - /** - * Register routine to be called once on the next libc-kernel activation - * - * The specified routine is executed only once. For a repeated execution, - * the routine must call 'register_kernel_routine' with itself as - * argument. - * - * This mechanism is used by 'fork' to implement the blocking for the - * startup of a new child and for 'wait4'. - */ - virtual void register_kernel_routine(Kernel_routine &) = 0; - }; -} - -#endif /* _LIBC__INTERNAL__KERNEL_ROUTINE_H_ */ diff --git a/repos/libports/src/lib/libc/kernel.cc b/repos/libports/src/lib/libc/kernel.cc index 81c7b98258..f5d0a5c176 100644 --- a/repos/libports/src/lib/libc/kernel.cc +++ b/repos/libports/src/lib/libc/kernel.cc @@ -348,6 +348,7 @@ void Libc::Kernel::_clone_state_from_parent() /* fetch user contex of the parent's application */ _clone_connection->memory_content(&_user_context, sizeof(_user_context)); + _clone_connection->memory_content(&_main_monitor_job, sizeof(_main_monitor_job)); _valid_user_context = true; _libc_env.libc_config().for_each_sub_node([&] (Xml_node node) { @@ -466,8 +467,8 @@ Libc::Kernel::Kernel(Genode::Env &env, Genode::Allocator &heap) init_malloc(*_malloc_heap); } - init_fork(_env, _libc_env, _heap, *_malloc_heap, _pid, *this, *this, _signal, - *this, _binary_name); + init_fork(_env, _libc_env, _heap, *_malloc_heap, _pid, *this, _signal, + _binary_name); init_execve(_env, _heap, _user_stack, *this, _binary_name, *file_descriptor_allocator()); init_plugin(*this);