From d370f56a77fa744c6a39208eafb5f31df5b964a2 Mon Sep 17 00:00:00 2001 From: Norman Feske Date: Wed, 15 Dec 2021 17:19:10 +0100 Subject: [PATCH] Remove obsolete Trace::Session::subject_info RPC Issue #3610 Fixes #4349 --- repos/base-okl4/src/lib/base/ipc.cc | 2 +- repos/base/include/trace_session/client.h | 3 - .../include/trace_session/trace_session.h | 11 +-- .../core/include/trace/session_component.h | 1 - .../base/src/core/trace_session_component.cc | 6 -- repos/base/src/test/migrate/main.cc | 52 +++++------ repos/os/src/app/trace_logger/main.cc | 36 ++++---- repos/os/src/app/trace_logger/monitor.cc | 20 ++--- repos/os/src/app/trace_logger/monitor.h | 13 +-- .../os/src/app/trace_subject_reporter/main.cc | 12 +-- repos/os/src/server/cpu_balancer/trace.cc | 86 ++++--------------- repos/os/src/server/cpu_balancer/trace.h | 20 +++-- repos/os/src/test/trace/main.cc | 26 +----- 13 files changed, 103 insertions(+), 185 deletions(-) diff --git a/repos/base-okl4/src/lib/base/ipc.cc b/repos/base-okl4/src/lib/base/ipc.cc index f38b8913c9..f28bce6321 100644 --- a/repos/base-okl4/src/lib/base/ipc.cc +++ b/repos/base-okl4/src/lib/base/ipc.cc @@ -111,7 +111,7 @@ static void copy_msg_to_utcb(Msgbuf_base const &snd_msg, L4_Word_t local_name) uint8_t const num_msg_words = num_data_words + num_header_words; if (num_msg_words >= L4_GetMessageRegisters()) { - raw("Message does not fit into UTCB message registers"); + raw("Message does not fit into UTCB message registers, num_msg_words=", num_msg_words); L4_LoadMR(0, 0); return; } diff --git a/repos/base/include/trace_session/client.h b/repos/base/include/trace_session/client.h index 683d5214eb..42276e607e 100644 --- a/repos/base/include/trace_session/client.h +++ b/repos/base/include/trace_session/client.h @@ -126,9 +126,6 @@ struct Genode::Trace::Session_client : Genode::Rpc_client(subject); } - Subject_info subject_info(Subject_id subject) override { - return call(subject); } - Dataspace_capability buffer(Subject_id subject) override { return call(subject); } diff --git a/repos/base/include/trace_session/trace_session.h b/repos/base/include/trace_session/trace_session.h index b180edb2e1..1657aad55b 100644 --- a/repos/base/include/trace_session/trace_session.h +++ b/repos/base/include/trace_session/trace_session.h @@ -85,13 +85,6 @@ struct Genode::Trace::Session : Genode::Session */ virtual void resume(Subject_id) = 0; - /** - * Obtain details about tracing subject - * - * \throw Nonexistent_subject - */ - virtual Subject_info subject_info(Subject_id) = 0; - /** * Obtain trace buffer of given subject * @@ -144,8 +137,6 @@ struct Genode::Trace::Session : Genode::Session GENODE_TYPE_LIST(Out_of_ram, Out_of_caps)); GENODE_RPC_THROW(Rpc_subject_infos, size_t, subject_infos, GENODE_TYPE_LIST(Out_of_ram, Out_of_caps)); - GENODE_RPC_THROW(Rpc_subject_info, Subject_info, subject_info, - GENODE_TYPE_LIST(Nonexistent_subject), Subject_id); GENODE_RPC_THROW(Rpc_buffer, Dataspace_capability, buffer, GENODE_TYPE_LIST(Nonexistent_subject, Subject_not_traced), Subject_id); @@ -154,7 +145,7 @@ struct Genode::Trace::Session : Genode::Session GENODE_RPC_INTERFACE(Rpc_dataspace, Rpc_alloc_policy, Rpc_policy, Rpc_unload_policy, Rpc_trace, Rpc_rule, Rpc_pause, - Rpc_resume, Rpc_subjects, Rpc_subject_info, Rpc_buffer, + Rpc_resume, Rpc_subjects, Rpc_buffer, Rpc_free, Rpc_subject_infos); }; diff --git a/repos/base/src/core/include/trace/session_component.h b/repos/base/src/core/include/trace/session_component.h index c15cc696e8..d115784ff6 100644 --- a/repos/base/src/core/include/trace/session_component.h +++ b/repos/base/src/core/include/trace/session_component.h @@ -81,7 +81,6 @@ class Genode::Trace::Session_component void rule(Session_label const &, Thread_name const &, Policy_id, size_t) override; void pause(Subject_id) override; void resume(Subject_id) override; - Subject_info subject_info(Subject_id) override; Dataspace_capability buffer(Subject_id) override; void free(Subject_id) override; }; diff --git a/repos/base/src/core/trace_session_component.cc b/repos/base/src/core/trace_session_component.cc index fd86783754..58308242ef 100644 --- a/repos/base/src/core/trace_session_component.cc +++ b/repos/base/src/core/trace_session_component.cc @@ -146,12 +146,6 @@ void Session_component::resume(Subject_id subject_id) } -Subject_info Session_component::subject_info(Subject_id subject_id) -{ - return _subjects.lookup_by_id(subject_id).info(); -} - - Dataspace_capability Session_component::buffer(Subject_id subject_id) { return _subjects.lookup_by_id(subject_id).buffer(); diff --git a/repos/base/src/test/migrate/main.cc b/repos/base/src/test/migrate/main.cc index 79d4dbac74..440c1e4c8b 100644 --- a/repos/base/src/test/migrate/main.cc +++ b/repos/base/src/test/migrate/main.cc @@ -65,7 +65,6 @@ struct Migrate Signal_handler timer_handler { env.ep(), *this, &Migrate::check_traces }; - Trace::Subject_id trace_id { }; Affinity::Location location { }; unsigned loc_same { 0 }; unsigned loc_pos { 0 }; @@ -95,13 +94,12 @@ struct Migrate switch (state) { case LOOKUP_TRACE_ID: { - auto count = trace.for_each_subject_info([&](Trace::Subject_id const &id, + auto count = trace.for_each_subject_info([&](Trace::Subject_id const &, Trace::Subject_info const &info) { if (info.thread_name() != "migrate") return; - trace_id = id; location = info.affinity(); state = CHECK_AFFINITY; @@ -116,32 +114,38 @@ struct Migrate } case CHECK_AFFINITY: { - Trace::Subject_info const info = trace.subject_info(trace_id); - - loc_same ++; - - if ((location.xpos() == info.affinity().xpos()) && - (location.ypos() == info.affinity().ypos()) && - (location.width() == info.affinity().width()) && - (location.height() == info.affinity().height())) + trace.for_each_subject_info([&](Trace::Subject_id const &, + Trace::Subject_info const &info) { - if (loc_same >= 1) { - loc_same = 0; - state = MIGRATE; + if (info.thread_name() != "migrate") + return; + + loc_same ++; + + if ((location.xpos() == info.affinity().xpos()) && + (location.ypos() == info.affinity().ypos()) && + (location.width() == info.affinity().width()) && + (location.height() == info.affinity().height())) + { + if (loc_same >= 1) { + loc_same = 0; + state = MIGRATE; + } + log ("[ep] ."); + return; } - log ("[ep] ."); - break; - } - loc_same = 0; - location = info.affinity(); + loc_same = 0; + location = info.affinity(); - log("[ep] thread '", info.thread_name(), "' migrated,", - " location=", location.xpos(), "x", location.ypos()); + log("[ep] thread '", info.thread_name(), "' migrated,", + " location=", location.xpos(), "x", location.ypos()); + + test_rounds ++; + if (test_rounds == 4) + log("--- test completed successfully ---"); + }); - test_rounds ++; - if (test_rounds == 4) - log("--- test completed successfully ---"); break; } case MIGRATE: diff --git a/repos/os/src/app/trace_logger/main.cc b/repos/os/src/app/trace_logger/main.cc index 94c8da9ca9..1030133d52 100644 --- a/repos/os/src/app/trace_logger/main.cc +++ b/repos/os/src/app/trace_logger/main.cc @@ -90,23 +90,22 @@ class Main _monitors_switch = !_monitors_switch; /* update available subject IDs and iterate over them */ - try { _num_subjects = _trace.subjects(_subjects, MAX_SUBJECTS); } - catch (Out_of_ram ) { warning("Cannot list subjects: Out_of_ram" ); return; } - catch (Out_of_caps) { warning("Cannot list subjects: Out_of_caps"); return; } - for (unsigned i = 0; i < _num_subjects; i++) { + _trace.for_each_subject_info([&] (Trace::Subject_id const id, + Trace::Subject_info const &info) { - Trace::Subject_id const id = _subjects[i]; try { /* skip dead subjects */ - if (_trace.subject_info(id).state() == Trace::Subject_info::DEAD) - continue; + if (info.state() == Trace::Subject_info::DEAD) + return; /* check if there is a matching policy in the XML config */ - Session_policy session_policy = _session_policy(id); + Session_policy session_policy = _session_policy(info); try { /* lookup monitor by subject ID */ Monitor &monitor = old_monitors.find_by_subject_id(id); + monitor.update_info(info); + /* move monitor from old to new tree */ old_monitors.remove(&monitor); new_monitors.insert(&monitor); @@ -114,12 +113,13 @@ class Main } catch (Monitor_tree::No_match) { /* create monitor for subject in the new tree */ - _new_monitor(new_monitors, id, session_policy); + _new_monitor(new_monitors, id, info, session_policy); } } - catch (Trace::Nonexistent_subject ) { continue; } - catch (Session_policy::No_policy_defined) { continue; } - } + catch (Trace::Nonexistent_subject ) { return; } + catch (Session_policy::No_policy_defined) { return; } + }); + /* all monitors in the old tree are deprecated, destroy them */ while (Monitor *monitor = old_monitors.first()) _destroy_monitor(old_monitors, *monitor); @@ -144,9 +144,10 @@ class Main _num_monitors--; } - void _new_monitor(Monitor_tree &monitors, - Trace::Subject_id id, - Session_policy &session_policy) + void _new_monitor(Monitor_tree &monitors, + Trace::Subject_id const id, + Trace::Subject_info const &info, + Session_policy const &session_policy) { try { Number_of_bytes const buffer_sz = session_policy.attribute_value("buffer", _default_buf_sz); @@ -158,7 +159,7 @@ class Main _policies.insert(policy); _trace.trace(id.id, policy.id(), buffer_sz); } - monitors.insert(new (_heap) Monitor(_trace, _env.rm(), id)); + monitors.insert(new (_heap) Monitor(_trace, _env.rm(), id, info)); } catch (Trace::Already_traced ) { warning("Cannot activate tracing: Already_traced" ); return; } catch (Trace::Source_is_dead ) { warning("Cannot activate tracing: Source_is_dead" ); return; } @@ -173,9 +174,8 @@ class Main log("new monitor: subject ", id.id); } - Session_policy _session_policy(Trace::Subject_id id) + Session_policy _session_policy(Trace::Subject_info const &info) { - Trace::Subject_info info = _trace.subject_info(id); Session_label const label(info.session_label()); Session_policy policy(label, _config); if (policy.has_attribute("thread")) diff --git a/repos/os/src/app/trace_logger/monitor.cc b/repos/os/src/app/trace_logger/monitor.cc index 568e844d81..87ed250869 100644 --- a/repos/os/src/app/trace_logger/monitor.cc +++ b/repos/os/src/app/trace_logger/monitor.cc @@ -58,38 +58,36 @@ Monitor &Monitor::find_by_subject_id(Trace::Subject_id const subject_id) } -Monitor::Monitor(Trace::Connection &trace, - Region_map &rm, - Trace::Subject_id subject_id) +Monitor::Monitor(Trace::Connection &trace, + Region_map &rm, + Trace::Subject_id const subject_id, + Trace::Subject_info const &info) : Monitor_base(trace, rm, subject_id), _subject_id(subject_id), _buffer(_buffer_raw) { - _update_info(); + update_info(info); } -void Monitor::_update_info() +void Monitor::update_info(Trace::Subject_info const &info) { try { - Trace::Subject_info const &info = - _trace.subject_info(_subject_id); - uint64_t const last_execution_time = _info.execution_time().thread_context; _info = info; + _recent_exec_time = _info.execution_time().thread_context - last_execution_time; } - catch (Trace::Nonexistent_subject) { warning("Cannot update subject info: Nonexistent_subject"); } + catch (Trace::Nonexistent_subject) { + warning("Cannot update subject info: Nonexistent_subject"); } } void Monitor::print(bool activity, bool affinity) { - _update_info(); - /* print general subject information */ typedef Trace::Subject_info Subject_info; Subject_info::State const state = _info.state(); diff --git a/repos/os/src/app/trace_logger/monitor.h b/repos/os/src/app/trace_logger/monitor.h index fdef7fabca..5c6d9b29b8 100644 --- a/repos/os/src/app/trace_logger/monitor.h +++ b/repos/os/src/app/trace_logger/monitor.h @@ -56,17 +56,16 @@ class Monitor : public Monitor_base, Genode::Trace::Subject_id const _subject_id; Trace_buffer _buffer; unsigned long _report_id { 0 }; - Genode::Trace::Subject_info _info { }; + Genode::Trace::Subject_info _info { }; unsigned long long _recent_exec_time { 0 }; char _curr_entry_data[MAX_ENTRY_LENGTH]; - void _update_info(); - public: - Monitor(Genode::Trace::Connection &trace, - Genode::Region_map &rm, - Genode::Trace::Subject_id subject_id); + Monitor(Genode::Trace::Connection &trace, + Genode::Region_map &rm, + Genode::Trace::Subject_id subject_id, + Genode::Trace::Subject_info const &info); void print(bool activity, bool affinity); @@ -86,6 +85,8 @@ class Monitor : public Monitor_base, Genode::Trace::Subject_id subject_id() const { return _subject_id; } Genode::Trace::Subject_info const &info() const { return _info; } + + void update_info(Genode::Trace::Subject_info const &); }; diff --git a/repos/os/src/app/trace_subject_reporter/main.cc b/repos/os/src/app/trace_subject_reporter/main.cc index d2e2946e5a..a8d4c1daf9 100644 --- a/repos/os/src/app/trace_subject_reporter/main.cc +++ b/repos/os/src/app/trace_subject_reporter/main.cc @@ -93,12 +93,8 @@ struct Trace_subject_registry void update(Genode::Trace::Connection &trace, Genode::Allocator &alloc) { - unsigned const num_subjects = update_subjects(trace); - - /* add and update existing entries */ - for (unsigned i = 0; i < num_subjects; i++) { - - Genode::Trace::Subject_id const id = _subjects[i]; + trace.for_each_subject_info([&] (Genode::Trace::Subject_id id, + Genode::Trace::Subject_info const &info) { Entry *e = _lookup(id); if (!e) { @@ -106,7 +102,7 @@ struct Trace_subject_registry _entries.insert(e); } - e->update(trace.subject_info(id)); + e->update(info); /* purge dead threads */ if (e->info.state() == Genode::Trace::Subject_info::DEAD) { @@ -114,7 +110,7 @@ struct Trace_subject_registry _entries.remove(e); Genode::destroy(alloc, e); } - } + }); _sort_by_recent_execution_time(); } diff --git a/repos/os/src/server/cpu_balancer/trace.cc b/repos/os/src/server/cpu_balancer/trace.cc index 936d291b99..a3128ea671 100644 --- a/repos/os/src/server/cpu_balancer/trace.cc +++ b/repos/os/src/server/cpu_balancer/trace.cc @@ -6,7 +6,7 @@ */ /* - * Copyright (C) 2020 Genode Labs GmbH + * Copyright (C) 2020-2021 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. @@ -21,38 +21,27 @@ void Cpu::Trace::_read_idle_times(bool skip_max_idle) _idle_slot = (_idle_slot + 1) % HISTORY; + auto count = _trace->for_each_subject_info([&](Subject_id const &, + Subject_info const &info) + { + if (info.session_label() != "kernel" || info.thread_name() != "idle") + return; + + if (info.affinity().xpos() < MAX_CORES && info.affinity().ypos() < MAX_THREADS) + _idle_times[info.affinity().xpos()][info.affinity().ypos()][_idle_slot] = info.execution_time(); + }); + + if (count.count == count.limit) { + Genode::log("reconstruct trace session, subject_count=", count.count); + _reconstruct(); + } + + if (skip_max_idle) + return; + for (unsigned x = 0; x < _space.width(); x++) { for (unsigned y = 0; y < _space.height(); y++) { Affinity::Location const idle_location(x,y); - Subject_id const &subject_id = _idle_id[x][y]; - - if (!subject_id.id || _subject_id_reread) - _lookup_missing_idle_id(idle_location); - - if (!subject_id.id) { - _idle_times[x][y][_idle_slot] = Execution_time(0, 0); - continue; - } - - Subject_info const info = _trace->subject_info(subject_id); - - Affinity::Location location = info.affinity(); - - if (location.xpos() != int(x) || location.ypos() != int(y)) { - Subject_id const subject_id_old = subject_id; - - _lookup_missing_idle_id(idle_location); - - Genode::warning("idle location mismatch ", x, "x", y, " vs ", - location.xpos(), "x", location.ypos(), " subject id=", - subject_id_old.id, " vs ", _idle_id[x][y].id); - } - - _idle_times[x][y][_idle_slot] = info.execution_time(); - - if (skip_max_idle) - continue; - /* determine max available execution time by monitoring idle */ auto const time = diff_idle_times(idle_location); auto &max = _idle_max[x][y]; @@ -63,43 +52,6 @@ void Cpu::Trace::_read_idle_times(bool skip_max_idle) } } -void Cpu::Trace::_lookup_missing_idle_id(Affinity::Location const &location) -{ - Subject_id found_id { }; - - do { - auto count = _trace->for_each_subject_info([&](Subject_id const &id, - Subject_info const &info) - { - if (found_id.id) - return; - - if (info.affinity().xpos() != location.xpos() || - info.affinity().ypos() != location.ypos()) - return; - - if (info.session_label() != "kernel" || info.thread_name() != "idle") - return; - - _idle_id[location.xpos()][location.ypos()] = id; - found_id = id; - }); - - - if (count.count == count.limit) { - Genode::log("reconstruct trace session, subject_count=", count.count); - _reconstruct(); - found_id = Subject_id(); - continue; - } - - if (!found_id.id) { - Genode::error("idle trace id missing"); - break; - } - } while (!found_id.id); -} - Genode::Trace::Subject_id Cpu::Trace::lookup_missing_id(Session_label const &label, Thread_name const &thread) diff --git a/repos/os/src/server/cpu_balancer/trace.h b/repos/os/src/server/cpu_balancer/trace.h index f716fb3dd9..500cedcd02 100644 --- a/repos/os/src/server/cpu_balancer/trace.h +++ b/repos/os/src/server/cpu_balancer/trace.h @@ -6,7 +6,7 @@ */ /* - * Copyright (C) 2020 Genode Labs GmbH + * Copyright (C) 2020-2021 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. @@ -45,15 +45,12 @@ class Cpu::Trace enum { MAX_CORES = 64, MAX_THREADS = 2, HISTORY = 4 }; - Subject_id _idle_id [MAX_CORES][MAX_THREADS]; Execution_time _idle_times[MAX_CORES][MAX_THREADS][HISTORY]; Execution_time _idle_max [MAX_CORES][MAX_THREADS]; unsigned _idle_slot { HISTORY - 1 }; unsigned _subject_id_reread { 0 }; - void _lookup_missing_idle_id(Affinity::Location const &); - void _reconstruct(Genode::size_t const upgrade = 4 * 4096) { _ram_quota += upgrade; @@ -124,14 +121,21 @@ class Cpu::Trace Session_label lookup_my_label(); template - void retrieve(Subject_id const id, FUNC const &fn) + void retrieve(Subject_id const target_id, FUNC const &fn) { if (!_trace.constructed()) return; - /* Ieegs, XXX, avoid copying whole object */ - Subject_info info = _trace->subject_info(id); - fn(info.execution_time(), info.affinity()); + auto count = _trace->for_each_subject_info([&](Subject_id const &id, + Subject_info const &info) { + if (id == target_id) + fn(info.execution_time(), info.affinity()); + }); + + if (count.count == count.limit) { + Genode::log("reconstruct trace session, subject_count=", count.count); + _reconstruct(); + } } Execution_time abs_idle_times(Affinity::Location const &location) diff --git a/repos/os/src/test/trace/main.cc b/repos/os/src/test/trace/main.cc index 23a1265307..23a8cf7e9c 100644 --- a/repos/os/src/test/trace/main.cc +++ b/repos/os/src/test/trace/main.cc @@ -221,16 +221,6 @@ struct Test_tracing return "undefined"; } - template - void for_each_subject(Trace::Subject_id subjects[], - size_t max_subjects, FUNC const &func) - { - for (size_t i = 0; i < max_subjects; i++) { - Trace::Subject_info info = trace.subject_info(subjects[i]); - func(subjects[i].id, info); - } - } - struct Failed : Genode::Exception { }; Test_tracing(Env &env) : env(env) @@ -273,14 +263,6 @@ struct Test_tracing /* wait some time before querying the subjects */ timer.msleep(1500); - Trace::Subject_id subjects[MAX_SUBJECTS]; - size_t num_subjects = trace.subjects(subjects, MAX_SUBJECTS); - - log(num_subjects, " tracing subjects present"); - - if (num_subjects == MAX_SUBJECTS) - error("Seems we reached the maximum number of subjects."); - auto print_info = [this] (Trace::Subject_id id, Trace::Subject_info info) { log("ID:", id.id, " " @@ -294,7 +276,7 @@ struct Test_tracing "quantum:", info.execution_time().quantum); }; - for_each_subject(subjects, num_subjects, print_info); + trace.for_each_subject_info(print_info); auto check_untraced = [this] (Trace::Subject_id id, Trace::Subject_info info) { @@ -302,7 +284,7 @@ struct Test_tracing error("Subject ", id.id, " is not UNTRACED"); }; - for_each_subject(subjects, num_subjects, check_untraced); + trace.for_each_subject_info(check_untraced); /* enable tracing for test-thread */ auto enable_tracing = [this, &env] (Trace::Subject_id id, @@ -329,12 +311,12 @@ struct Test_tracing } }; - for_each_subject(subjects, num_subjects, enable_tracing); + trace.for_each_subject_info(enable_tracing); /* give the test thread some time to run */ timer.msleep(1000); - for_each_subject(subjects, num_subjects, print_info); + trace.for_each_subject_info(print_info); /* read events from trace buffer */ if (test_monitor.constructed()) {