From bfe88307de73e33c730601216bb91f743af50671 Mon Sep 17 00:00:00 2001 From: Norman Feske Date: Fri, 1 Dec 2023 17:52:57 +0100 Subject: [PATCH] core: filter trace subjects by TRACE session label This patch changes core's TRACE service to expose trace subjects only if their PD label matches the label of the TRACE monitor. Hence, by default, a trace monitor can only observe itself and its child components. Only if the trace monitor's parent rewrites the trace-session's label, the view of trace monitor can become broader. For example, when rewriting the trace label to an empty string "", the trace monitor becomes able to observe the sibling components hosted in the same init instance as the trace monitor. To grant a trace session the special privilege of obtaining a global system view (including the kernel's trace subjects), the top-level init has to rewrite the session's label to an empty string. At core, this specific label "init -> " is handled as a special case that discharges the filtering/namespacing of trace subjects. Note that the trace-subject label as reported as subject info is now given relative to the label of the trace session. As a nice side effect of this change, the pkg/test-trace_logger works now when executed by the depot_autopilot as well as via the test.run script. Issue #847 --- repos/base/src/core/cpu_session_component.cc | 4 +- repos/base/src/core/include/trace/root.h | 3 +- .../core/include/trace/session_component.h | 12 +- .../src/core/include/trace/source_registry.h | 19 ++- .../src/core/include/trace/subject_registry.h | 110 ++++++++---------- .../base/src/core/trace_session_component.cc | 3 +- repos/os/recipes/pkg/test-trace/runtime | 7 +- .../os/recipes/pkg/test-trace_logger/runtime | 16 ++- 8 files changed, 86 insertions(+), 88 deletions(-) diff --git a/repos/base/src/core/cpu_session_component.cc b/repos/base/src/core/cpu_session_component.cc index 7e05c113cd..e487029c78 100644 --- a/repos/base/src/core/cpu_session_component.cc +++ b/repos/base/src/core/cpu_session_component.cc @@ -384,11 +384,11 @@ size_t Cpu_session_component::_weight_to_quota(size_t const weight) const ** Trace::Source_registry ** ****************************/ -unsigned Core::Trace::Source::_alloc_unique_id() +Core::Trace::Source::Id Core::Trace::Source::_alloc_unique_id() { static Mutex lock; static unsigned cnt; Mutex::Guard guard(lock); - return cnt++; + return { cnt++ }; } diff --git a/repos/base/src/core/include/trace/root.h b/repos/base/src/core/include/trace/root.h index fd52cedd65..5818bbae11 100644 --- a/repos/base/src/core/include/trace/root.h +++ b/repos/base/src/core/include/trace/root.h @@ -38,7 +38,6 @@ class Core::Trace::Root : public Root_component { size_t ram_quota = Arg_string::find_arg(args, "ram_quota").ulong_value(0); size_t arg_buffer_size = Arg_string::find_arg(args, "arg_buffer_size").ulong_value(0); - unsigned parent_levels = (unsigned)Arg_string::find_arg(args, "parent_levels").ulong_value(0); if (arg_buffer_size > ram_quota) throw Insufficient_ram_quota(); @@ -49,7 +48,7 @@ class Core::Trace::Root : public Root_component session_label_from_args(args), session_diag_from_args(args), _ram, _local_rm, - arg_buffer_size, parent_levels, + arg_buffer_size, _sources, _policies); } diff --git a/repos/base/src/core/include/trace/session_component.h b/repos/base/src/core/include/trace/session_component.h index 9859c14941..451de90a4b 100644 --- a/repos/base/src/core/include/trace/session_component.h +++ b/repos/base/src/core/include/trace/session_component.h @@ -47,6 +47,17 @@ class Core::Trace::Session_component unsigned _policy_cnt { 0 }; Attached_ram_dataspace _argument_buffer; + /* + * Whenever a trace session is deliberately labeled as empty by the + * top-level init instance, the session is granted global reach. + * Otherwise, the label is taken a prefix filter for the visibility + * of trace subjects within the session. + */ + Filter _filter() const + { + return (_label == "init -> ") ? Filter("") : Filter(_label); + } + public: /** @@ -59,7 +70,6 @@ class Core::Trace::Session_component Ram_allocator &ram, Region_map &local_rm, size_t arg_buffer_size, - unsigned parent_levels, Source_registry &sources, Policy_registry &policies); diff --git a/repos/base/src/core/include/trace/source_registry.h b/repos/base/src/core/include/trace/source_registry.h index 8a5f1e3969..124f3cccd6 100644 --- a/repos/base/src/core/include/trace/source_registry.h +++ b/repos/base/src/core/include/trace/source_registry.h @@ -30,6 +30,8 @@ namespace Core { namespace Trace { using namespace Genode::Trace; + using Filter = String; + class Source; class Source_owner; class Source_registry; @@ -71,16 +73,18 @@ class Core::Trace::Source virtual Info trace_source_info() const = 0; }; + struct Id { unsigned value; }; + private: - unsigned const _unique_id; + Id const _unique_id; Info_accessor const &_info; Control &_control; Dataspace_capability _policy { }; Dataspace_capability _buffer { }; Source_owner const *_owner_ptr = nullptr; - static unsigned _alloc_unique_id(); + static Id _alloc_unique_id(); /* * Noncopyable @@ -144,7 +148,7 @@ class Core::Trace::Source Dataspace_capability buffer() const { return _buffer; } Dataspace_capability policy() const { return _policy; } - unsigned unique_id() const { return _unique_id; } + Id id() const { return _unique_id; } }; @@ -184,16 +188,11 @@ class Core::Trace::Source_registry ** Interface used by TRACE service ** *************************************/ - template - void export_sources(TEST &test, INSERT &insert) + void for_each_source(auto const &fn) { for (Source *s = _entries.first(); s; s = s->next()) - if (!test(s->unique_id())) { - Source::Info const info = s->info(); - insert(s->unique_id(), s->weak_ptr(), info.label, info.name); - } + fn(*s); } - }; #endif /* _CORE__INCLUDE__TRACE__SOURCE_REGISTRY_H_ */ diff --git a/repos/base/src/core/include/trace/subject_registry.h b/repos/base/src/core/include/trace/subject_registry.h index e97284bfa5..ee7be41d26 100644 --- a/repos/base/src/core/include/trace/subject_registry.h +++ b/repos/base/src/core/include/trace/subject_registry.h @@ -137,7 +137,7 @@ class Core::Trace::Subject friend class Subject_registry; Subject_id const _id; - unsigned const _source_id; + Source::Id const _source_id; Weak_ptr _source; Session_label const _label; Thread_name const _name; @@ -184,7 +184,7 @@ class Core::Trace::Subject /** * Constructor, called from 'Subject_registry' only */ - Subject(Subject_id id, unsigned source_id, Weak_ptr &source, + Subject(Subject_id id, Source::Id source_id, Weak_ptr &source, Session_label const &label, Thread_name const &name) : _id(id), _source_id(source_id), _source(source), @@ -212,7 +212,7 @@ class Core::Trace::Subject /** * Test if subject belongs to the specified unique source ID */ - bool has_source_id(unsigned id) const { return id == _source_id; } + bool has_source_id(Source::Id id) const { return id.value == _source_id.value; } /** * Start tracing @@ -325,51 +325,11 @@ class Core::Trace::Subject_registry Allocator &_md_alloc; Source_registry &_sources; + Filter const _filter; unsigned _id_cnt { 0 }; Mutex _mutex { }; Subjects _entries { }; - /** - * Functor for testing the existance of subjects for a given source - * - * This functor is invoked by 'Source_registry::export'. - */ - struct Tester - { - Subjects &subjects; - - Tester(Subjects &subjects) : subjects(subjects) { } - - bool operator () (unsigned source_id) - { - for (Subject *s = subjects.first(); s; s = s->next()) - if (s->has_source_id(source_id)) - return true; - return false; - } - } _tester { _entries }; - - /** - * Functor for inserting new subjects into the registry - * - * This functor is invoked by 'Source_registry::export'. - */ - struct Inserter - { - Subject_registry ®istry; - - Inserter(Subject_registry ®istry) : registry(registry) { } - - void operator () (unsigned source_id, Weak_ptr source, - Session_label const &label, Thread_name const &name) - { - Subject *subject = new (®istry._md_alloc) - Subject(Subject_id(++registry._id_cnt), source_id, source, label, name); - - registry._entries.insert(subject); - } - } _inserter { *this }; - /** * Destroy subject, and release policy and trace buffers */ @@ -398,23 +358,11 @@ class Core::Trace::Subject_registry public: - /** - * Constructor - * - * \param md_alloc meta-data allocator used for allocating 'Subject' - * objects. - * \param ram allocator used for the allocation of trace - * buffers and policy dataspaces. - */ - Subject_registry(Allocator &md_alloc, - Source_registry &sources) + Subject_registry(Allocator &md_alloc, Source_registry &sources, Filter const &filter) : - _md_alloc(md_alloc), _sources(sources) + _md_alloc(md_alloc), _sources(sources), _filter(filter) { } - /** - * Destructor - */ ~Subject_registry() { Mutex::Guard guard(_mutex); @@ -430,7 +378,32 @@ class Core::Trace::Subject_registry { Mutex::Guard guard(_mutex); - _sources.export_sources(_tester, _inserter); + auto already_known = [&] (Source::Id const unique_id) + { + for (Subject *s = _entries.first(); s; s = s->next()) + if (s->has_source_id(unique_id)) + return true; + return false; + }; + + auto filter_matches = [&] (Session_label const &label) + { + return strcmp(_filter.string(), label.string(), _filter.length() - 1) == 0; + }; + + _sources.for_each_source([&] (Source &source) { + + Source::Info const info = source.info(); + + if (!filter_matches(info.label) || already_known(source.id())) + return; + + Weak_ptr source_ptr = source.weak_ptr(); + + _entries.insert(new (_md_alloc) + Subject(Subject_id(++_id_cnt), source.id(), + source_ptr, info.label, info.name)); + }); } /** @@ -453,12 +426,25 @@ class Core::Trace::Subject_registry { Mutex::Guard guard(_mutex); + auto filtered = [&] (Session_label const &label) -> Session_label + { + return (label.length() <= _filter.length() || !_filter.length()) + ? Session_label("") /* this cannot happen */ + : Session_label(label.string() + _filter.length() - 1); + }; + unsigned i = 0; for (Subject *s = _entries.first(); s && i < len; s = s->next()) { - ids[i] = s->id(); - dst[i++] = s->info(); - } + ids[i] = s->id(); + Subject_info const info = s->info(); + + /* strip filter prefix from reported trace-subject label */ + dst[i++] = { + filtered(info.session_label()), info.thread_name(), info.state(), + info.policy_id(), info.execution_time(), info.affinity() + }; + } return i; } diff --git a/repos/base/src/core/trace_session_component.cc b/repos/base/src/core/trace_session_component.cc index e46950cc66..919e45c45c 100644 --- a/repos/base/src/core/trace_session_component.cc +++ b/repos/base/src/core/trace_session_component.cc @@ -132,7 +132,6 @@ Session_component::Session_component(Rpc_entrypoint &ep, Ram_allocator &ram, Region_map &local_rm, size_t arg_buffer_size, - unsigned /* parent_levels */, Source_registry &sources, Policy_registry &policies) : @@ -143,7 +142,7 @@ Session_component::Session_component(Rpc_entrypoint &ep, _policies_slab(&_md_alloc), _sources(sources), _policies(policies), - _subjects(_subjects_slab, _sources), + _subjects(_subjects_slab, _sources, _filter()), _argument_buffer(_ram, local_rm, arg_buffer_size) { } diff --git a/repos/os/recipes/pkg/test-trace/runtime b/repos/os/recipes/pkg/test-trace/runtime index 81cb89108e..b46a8d65b4 100644 --- a/repos/os/recipes/pkg/test-trace/runtime +++ b/repos/os/recipes/pkg/test-trace/runtime @@ -28,6 +28,7 @@ + @@ -37,21 +38,21 @@ - + - + - + diff --git a/repos/os/recipes/pkg/test-trace_logger/runtime b/repos/os/recipes/pkg/test-trace_logger/runtime index 51dbf17007..0eddfb62d3 100644 --- a/repos/os/recipes/pkg/test-trace_logger/runtime +++ b/repos/os/recipes/pkg/test-trace_logger/runtime @@ -5,11 +5,11 @@ [init -> trace_logger] Report * - [init -> trace_logger] PD "init -> dynamic -> test-trace_logger -> cpu_burner.*"* + [init -> trace_logger] PD "cpu_burner.*"* [init -> trace_logger] Thread "ep" at (0,0) TRACED total:* recent:* - [init -> trace_logger] PD "init -> dynamic -> test-trace_logger -> dynamic_rom"* + [init -> trace_logger] PD "dynamic_rom"* [init -> trace_logger] Thread "ep" at (0,0) TRACED total:* recent:* - [init -> trace_logger] PD "init -> dynamic -> test-trace_logger -> test-trace_logger"* + [init -> trace_logger] PD "test-trace_logger"* [init -> trace_logger] Thread "ep" at (0,0) TRACED total:* recent:* [init -> trace_logger] 100 * [init -> trace_logger] trigger_once @@ -49,12 +49,16 @@ - + - + - + + + + +