From 1f58b052553b0ca008a4c051bcc6fccd4c9c9b7f Mon Sep 17 00:00:00 2001 From: Alexander Boettcher Date: Tue, 23 Nov 2021 10:17:30 +0100 Subject: [PATCH] cpu_balancer: limit mem increase on config update The commits avoids reading in and allocating memory for all potentially threads, which are potentially currently not existent (but configured in the policy beforehand). Instead the policy is read in and evaluated when a thread is created and policy changes are solely applied to existing/running threads. By this the commit avoids the increase of memory consumption during the evaluation of policies during config ROM updates. Issue #4333 --- repos/os/run/cpu_balancer.run | 67 ++++++++++++++----- repos/os/src/server/cpu_balancer/component.cc | 20 +++--- repos/os/src/server/cpu_balancer/config.cc | 43 ++++++++++-- repos/os/src/server/cpu_balancer/config.h | 2 + repos/os/src/server/cpu_balancer/session.cc | 49 ++++++++++++-- repos/os/src/server/cpu_balancer/session.h | 36 +++++++--- 6 files changed, 174 insertions(+), 43 deletions(-) diff --git a/repos/os/run/cpu_balancer.run b/repos/os/run/cpu_balancer.run index fb9f0316ba..6fb6dd2eb0 100644 --- a/repos/os/run/cpu_balancer.run +++ b/repos/os/run/cpu_balancer.run @@ -1,4 +1,4 @@ -build "core init timer server/cpu_balancer app/cpu_burner app/top" +build "core init timer server/cpu_balancer app/cpu_burner app/top server/dynamic_rom" if {![have_include "power_on/qemu"]} { puts "Run script is not supported on this platform" @@ -63,6 +63,53 @@ append_if [expr $report_config eq "yes"] config { } +append config { + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + } + append config { @@ -70,20 +117,10 @@ append config { - - - - - - - - + + + @@ -125,7 +162,7 @@ append config { install_config $config -build_boot_image { core ld.lib.so init timer cpu_balancer cpu_burner top } +build_boot_image { core ld.lib.so init timer cpu_balancer cpu_burner top dynamic_rom } append qemu_args " -nographic" append qemu_args " -smp [expr $cpu_width * $cpu_height],cores=$cpu_width,threads=$cpu_height" diff --git a/repos/os/src/server/cpu_balancer/component.cc b/repos/os/src/server/cpu_balancer/component.cc index 631b1f1654..8223963c4c 100644 --- a/repos/os/src/server/cpu_balancer/component.cc +++ b/repos/os/src/server/cpu_balancer/component.cc @@ -120,7 +120,13 @@ namespace Cpu { Arg_string::set_arg(argbuf, sizeof(argbuf), "ram_quota", remaining_ram_quota.value); Arg_string::set_arg(argbuf, sizeof(argbuf), "cap_quota", remaining_cap_quota.value); - return fn(argbuf); + try { + return fn(argbuf); + } catch (Out_of_ram) { + throw Insufficient_ram_quota(); + } catch (Out_of_caps) { + throw Insufficient_cap_quota(); + } } } @@ -244,15 +250,9 @@ struct Cpu::Balancer : Rpc_object> Mutex::Guard guard(list_mutex); - auto * session = new (slice) Registered(list, env, - affinity, - session_args, - list, verbose); - - /* check for config of new session */ - Cpu::Config::apply(config.xml(), list); - - return session->cap(); + return (new (slice) Registered(list, env, affinity, + session_args, list, config, + verbose))->cap(); }); } diff --git a/repos/os/src/server/cpu_balancer/config.cc b/repos/os/src/server/cpu_balancer/config.cc index 01e5ab7088..c745d33ade 100644 --- a/repos/os/src/server/cpu_balancer/config.cc +++ b/repos/os/src/server/cpu_balancer/config.cc @@ -18,9 +18,6 @@ void Cpu::Config::apply(Xml_node const &start, Child_list &sessions) { - using Genode::Session_label; - using Genode::String; - typedef String Label; start.for_each_sub_node("component", [&](Xml_node const &node) { @@ -54,8 +51,46 @@ void Cpu::Config::apply(Xml_node const &start, Child_list &sessions) thread.attribute_value("ypos", 0U), 1, 1); - session.config(name, policy, location); + session.update_if_active(name, policy, location); }); }); }); } + +void Cpu::Config::apply_for_thread(Xml_node const &start, Cpu::Session &session, + Thread::Name const &target_thread) +{ + typedef String Label; + + start.for_each_sub_node("component", [&](Xml_node const &node) { + if (!node.has_attribute("label")) + return; + + Label const label = node.attribute_value("label", Label("")); + + if (!session.match(label)) + return; + + node.for_each_sub_node("thread", [&](Xml_node const &thread) { + if (!thread.has_attribute("name") || !thread.has_attribute("policy")) + return; + + Thread::Name const name = thread.attribute_value("name", Thread::Name()); + Cpu::Policy::Name const policy = thread.attribute_value("policy", Cpu::Policy::Name()); + + if (target_thread != name) + return; + + /* explicitly create invalid width/height */ + /* used during thread construction in policy static case */ + Affinity::Location location { 0, 0, 0, 0}; + + if (thread.has_attribute("xpos") && thread.has_attribute("ypos")) + location = Affinity::Location(thread.attribute_value("xpos", 0U), + thread.attribute_value("ypos", 0U), + 1, 1); + + session.update(name, policy, location); + }); + }); +} diff --git a/repos/os/src/server/cpu_balancer/config.h b/repos/os/src/server/cpu_balancer/config.h index 0cdce6a025..98b94de38d 100644 --- a/repos/os/src/server/cpu_balancer/config.h +++ b/repos/os/src/server/cpu_balancer/config.h @@ -29,6 +29,8 @@ class Cpu::Config { public: static void apply(Xml_node const &, Child_list &); + static void apply_for_thread(Xml_node const &, Cpu::Session &, + Thread::Name const &); }; #endif /* _CONFIG_H_ */ diff --git a/repos/os/src/server/cpu_balancer/session.cc b/repos/os/src/server/cpu_balancer/session.cc index 509102b766..b72929ec48 100644 --- a/repos/os/src/server/cpu_balancer/session.cc +++ b/repos/os/src/server/cpu_balancer/session.cc @@ -12,6 +12,7 @@ */ #include "session.h" +#include "config.h" using namespace Genode; using namespace Cpu; @@ -37,6 +38,9 @@ Cpu::Session::create_thread(Pd_session_capability const pd, throw Out_of_caps(); } + /* read in potential existing policy for thread */ + Cpu::Config::apply_for_thread(_config.xml(), *this, name); + lookup(name, [&](Thread_capability &store_cap, Cpu::Policy &policy) { @@ -126,13 +130,13 @@ Dataspace_capability Cpu::Session::trace_control() return _parent.trace_control(); } -Cpu::Session::Session(Env &env, - Affinity const &affinity, - char const * args, - Child_list &list, bool const verbose) +Cpu::Session::Session(Env &env, Affinity const &affinity, char const * args, + Child_list &list, Attached_rom_dataspace &config, + bool const verbose) : _list(list), _env(env), + _config(config), _ram_guard(ram_quota_from_args(args)), _cap_guard(cap_quota_from_args(args)), _parent(_env.session(_id.id(), _withdraw_quota(args), affinity)), @@ -141,6 +145,17 @@ Cpu::Session::Session(Env &env, _verbose(verbose) { try { + /* warm up the heap, we need space for at least one thread object */ + construct(_default_policy, [&](Thread_capability &, Name &, + Cpu::Policy &) {}); + + _threads.for_each([&](Thread_client &thread) { + if (thread._policy) + destroy(_md_alloc, thread._policy); + destroy(_md_alloc, &thread); + }); + + /* finally, make object available via rpc */ _env.ep().rpc_ep().manage(this); } catch (...) { env.close(_id.id()); @@ -209,7 +224,7 @@ bool Cpu::Session::report_state(Xml_generator &xml) const return _report; } -void Cpu::Session::config(Thread::Name const &thread, +void Cpu::Session::update(Thread::Name const &thread, Cpu::Policy::Name const &policy_name, Affinity::Location const &relativ) { @@ -227,3 +242,27 @@ void Cpu::Session::config(Thread::Name const &thread, _report = true; } + +void Cpu::Session::update_if_active(Thread::Name const &thread, + Cpu::Policy::Name const &policy_name, + Affinity::Location const &location) +{ + /* + * Policies for existing threads are updated. + */ + bool found = reconstruct_if_active(policy_name, thread, + [&](Thread_capability const &, + Cpu::Policy &policy) + { + policy.config(location); + + if (_verbose) { + String<12> const loc { policy.location.xpos(), "x", policy.location.ypos() }; + log("[", _label, "] name='", thread, "' " + "update policy to '", policy, "' ", loc); + } + }); + + if (found) + _report = true; +} diff --git a/repos/os/src/server/cpu_balancer/session.h b/repos/os/src/server/cpu_balancer/session.h index 48c1c3dd33..27f6847d9a 100644 --- a/repos/os/src/server/cpu_balancer/session.h +++ b/repos/os/src/server/cpu_balancer/session.h @@ -15,6 +15,7 @@ #define _SESSION_H_ /* Genode includes */ +#include #include #include #include @@ -53,8 +54,9 @@ class Cpu::Session : public Rpc_object { private: - Child_list &_list; - Env &_env; + Child_list &_list; + Env &_env; + Attached_rom_dataspace &_config; Ram_quota_guard _ram_guard; Cap_quota_guard _cap_guard; @@ -195,12 +197,12 @@ class Cpu::Session : public Rpc_object } template - void reconstruct(Cpu::Policy::Name const &policy_name, - Thread::Name const &thread_name, - FUNC const &fn) + bool reconstruct_if_active(Cpu::Policy::Name const &policy_name, + Thread::Name const &thread_name, + FUNC const &fn) { if (!thread_name.valid()) - return; + return false; bool done = false; @@ -227,7 +229,18 @@ class Cpu::Session : public Rpc_object return true; }); - if (done) + return done; + } + + template + void reconstruct(Cpu::Policy::Name const &policy_name, + Thread::Name const &thread_name, + FUNC const &fn) + { + if (!thread_name.valid()) + return; + + if (reconstruct_if_active(policy_name, thread_name, fn)) return; construct(policy_name, [&](Thread_capability const &cap, @@ -317,7 +330,8 @@ class Cpu::Session : public Rpc_object public: - Session(Env &, Affinity const &, char const *, Child_list &, bool); + Session(Env &, Affinity const &, char const *, Child_list &, + Attached_rom_dataspace &, bool); ~Session(); /*************************** @@ -341,8 +355,12 @@ class Cpu::Session : public Rpc_object ** internal interface ** ************************/ bool match(Label const &label) const { return _label == label; }; - void config(Thread::Name const &, Cpu::Policy::Name const &, + + void update(Thread::Name const &, Cpu::Policy::Name const &, Affinity::Location const &); + void update_if_active(Thread::Name const &, Cpu::Policy::Name const &, + Affinity::Location const &); + void update_threads(); void update_threads(Trace &, Session_label const &); bool report_state(Xml_generator &) const;