trace session: fix double quota accounting

Fixes #4707.

repos/base/include/base/trace/types.h

@@ -29,7 +29,6 @@ namespace Genode { namespace Trace {
 
 	struct Policy_too_large        : Exception { };
 	struct Nonexistent_subject     : Exception { };
-	struct Already_traced          : Exception { };
 	struct Source_is_dead          : Exception { };
 	struct Nonexistent_policy      : Exception { };
 	struct Traced_by_other_session : Exception { };

repos/base/include/trace_session/trace_session.h

@@ -56,7 +56,6 @@ struct Genode::Trace::Session : Genode::Session
 	 *
 	 * \throw Out_of_ram
 	 * \throw Out_of_caps
-	 * \throw Already_traced
 	 * \throw Source_is_dead
 	 * \throw Nonexistent_policy
 	 * \throw Nonexistent_subject
@@ -113,7 +112,7 @@ struct Genode::Trace::Session : Genode::Session
 	GENODE_RPC_THROW(Rpc_unload_policy, void, unload_policy,
 	                 GENODE_TYPE_LIST(Nonexistent_policy), Policy_id);
 	GENODE_RPC_THROW(Rpc_trace, void, trace,
-	                 GENODE_TYPE_LIST(Out_of_ram, Out_of_caps, Already_traced,
+	                 GENODE_TYPE_LIST(Out_of_ram, Out_of_caps,
 	                                  Source_is_dead, Nonexistent_subject,
 	                                  Nonexistent_policy,
 	                                  Traced_by_other_session),

repos/base/src/core/include/trace/root.h

@@ -41,7 +41,7 @@ class Genode::Trace::Root : public Genode::Root_component<Session_component>
 			unsigned parent_levels = (unsigned)Arg_string::find_arg(args, "parent_levels").ulong_value(0);
 
 			if (arg_buffer_size > ram_quota)
-				throw Service_denied();
+				throw Insufficient_ram_quota();
 
 			return new (md_alloc())
 			       Session_component(*this->ep(),

repos/base/src/core/include/trace/subject_registry.h

@@ -96,18 +96,15 @@ class Genode::Trace::Subject
 				/**
 				 * Clone dataspace into newly allocated dataspace
 				 */
-				bool setup(Ram_allocator &ram, Region_map &local_rm,
+				void setup(Ram_allocator &ram, Region_map &local_rm,
 				           Dataspace_capability &from_ds, size_t size)
 				{
-					if (!from_ds.valid())
-						return false;
-
 					if (_size)
 						flush();
 
+					_ds      = ram.alloc(size); /* may throw */
 					_ram_ptr = &ram;
 					_size    = size;
-					_ds      = ram.alloc(_size);
 
 					/* copy content */
 					void *src = local_rm.attach(from_ds),
@@ -117,8 +114,6 @@ class Genode::Trace::Subject
 
 					local_rm.detach(src);
 					local_rm.detach(dst);
-
-					return true;
 				}
 
 				/**
@@ -146,7 +141,6 @@ class Genode::Trace::Subject
 		Ram_dataspace       _buffer { };
 		Ram_dataspace       _policy { };
 		Policy_id           _policy_id { };
-		size_t              _allocated_memory { 0 };
 
 		Subject_info::State _state()
 		{
@@ -217,9 +211,6 @@ class Genode::Trace::Subject
 		 */
 		bool has_source_id(unsigned id) const { return id == _source_id; }
 
-		size_t allocated_memory() const { return _allocated_memory; }
-		void   reset_allocated_memory() { _allocated_memory = 0; }
-
 		/**
 		 * Start tracing
 		 *
@@ -227,7 +218,6 @@ class Genode::Trace::Subject
 		 *
 		 * \throw Out_of_ram
 		 * \throw Out_of_caps
-		 * \throw Already_traced
 		 * \throw Source_is_dead
 		 * \throw Traced_by_other_session
 		 */
@@ -238,26 +228,31 @@ class Genode::Trace::Subject
 			/* check state and throw error in case subject is not traceable */
 			_traceable_or_throw();
 
-			_buffer.setup(ram, size);
-			if(!_policy.setup(ram, local_rm, policy_ds, policy_size))
-				throw Already_traced();
+			_buffer.setup(ram, size); /* may throw */
+
+			try {
+				_policy.setup(ram, local_rm, policy_ds, policy_size);
+			} catch (...) {
+				_buffer.flush();
+				throw;
+			}
 
 			/* inform trace source about the new buffer */
 			Locked_ptr<Source> source(_source);
 
-			if (!source->try_acquire(*this))
+			if (!source->try_acquire(*this)) {
+				_policy.flush();
+				_buffer.flush();
 				throw Traced_by_other_session();
+			}
 
 			_policy_id = policy_id;
 
-			_allocated_memory = policy_size + size;
-
 			source->trace(_policy.dataspace(), _buffer.dataspace());
 		}
 
 		void pause()
 		{
-			/* inform trace source about the new buffer */
 			Locked_ptr<Source> source(_source);
 
 			if (source.valid())
@@ -271,7 +266,6 @@ class Genode::Trace::Subject
 		 */
 		void resume()
 		{
-			/* inform trace source about the new buffer */
 			Locked_ptr<Source> source(_source);
 
 			if (!source.valid())
@@ -301,16 +295,16 @@ class Genode::Trace::Subject
 
 		Dataspace_capability buffer() const { return _buffer.dataspace(); }
 
-		size_t release()
+		void release()
 		{
-			/* inform trace source about the new buffer */
 			Locked_ptr<Source> source(_source);
 
 			/* source vanished */
 			if (!source.valid())
-				return 0;
+				return;
 
-			return _buffer.flush() + _policy.flush();
+			_buffer.flush();
+			_policy.flush();
 		}
 };
 
@@ -375,18 +369,14 @@ class Genode::Trace::Subject_registry
 
 		/**
 		 * Destroy subject, and release policy and trace buffers
-		 *
-		 * \return RAM resources released during destruction
 		 */
-		size_t _unsynchronized_destroy(Subject &s)
+		void _unsynchronized_destroy(Subject &s)
 		{
 			_entries.remove(&s);
 
-			size_t const released_ram = s.release();
+			s.release();
 
 			destroy(&_md_alloc, &s);
-
-			return released_ram;
 		};
 
 		/**
@@ -471,18 +461,13 @@ class Genode::Trace::Subject_registry
 
 		/**
 		 * Remove subject and release resources
-		 *
-		 * \return  RAM resources released as a side effect for removing the
-		 *          subject (i.e., if the subject held a trace buffer or
-		 *          policy dataspace). The value does not account for
-		 *          memory allocated from the metadata allocator.
 		 */
-		size_t release(Subject_id subject_id)
+		void release(Subject_id subject_id)
 		{
 			Mutex::Guard guard(_mutex);
 
 			Subject &subject = _unsynchronized_lookup_by_id(subject_id);
-			return _unsynchronized_destroy(subject);
+			_unsynchronized_destroy(subject);
 		}
 
 		Subject &lookup_by_id(Subject_id id)

repos/base/src/core/trace_session_component.cc

@@ -59,21 +59,12 @@ Policy_id Session_component::alloc_policy(size_t size)
 	 */
 	Policy_id const id(++_policy_cnt);
 
-	Ram_quota const amount { size };
-
-	/*
-	 * \throw Out_of_ram
-	 */
-	withdraw(amount);
+	Ram_dataspace_capability ds_cap = _ram.alloc(size); /* may throw */
 
 	try {
-		Dataspace_capability ds_cap = _ram.alloc(size);
 		_policies.insert(*this, id, _policies_slab, ds_cap, size);
-
 	} catch (...) {
-
-		/* revert withdrawal or quota */
-		replenish(amount);
+		_ram.free(ds_cap);
 		throw;
 	}
 
@@ -89,7 +80,11 @@ Dataspace_capability Session_component::policy(Policy_id id)
 
 void Session_component::unload_policy(Policy_id id)
 {
-	_policies.remove(*this, id);
+	try {
+		Dataspace_capability ds_cap = _policies.dataspace(*this, id);
+		_policies.remove(*this, id);
+		_ram.free(static_cap_cast<Ram_dataspace>(ds_cap));
+	} catch (Nonexistent_policy) { }
 }
 
 
@@ -98,32 +93,10 @@ void Session_component::trace(Subject_id subject_id, Policy_id policy_id,
 {
 	size_t const policy_size  = _policies.size(*this, policy_id);
 
-	Ram_quota const required_ram { buffer_size + policy_size };
-
 	Trace::Subject &subject = _subjects.lookup_by_id(subject_id);
 
-	/* revert quota from previous call to trace */
-	if (subject.allocated_memory()) {
-		replenish(Ram_quota{subject.allocated_memory()});
-		subject.reset_allocated_memory();
-	}
-
-	/*
-	 * Account RAM needed for trace buffer and policy buffer to the trace
-	 * session.
-	 *
-	 * \throw Out_of_ram
-	 */
-	withdraw(required_ram);
-
-	try {
-		subject.trace(policy_id, _policies.dataspace(*this, policy_id),
-		              policy_size, _ram, _local_rm, buffer_size);
-	} catch (...) {
-		/* revert withdrawal or quota */
-		replenish(required_ram);
-		throw;
-	}
+	subject.trace(policy_id, _policies.dataspace(*this, policy_id),
+	              policy_size, _ram, _local_rm, buffer_size);
 }
 
 
@@ -147,9 +120,7 @@ Dataspace_capability Session_component::buffer(Subject_id subject_id)
 
 void Session_component::free(Subject_id subject_id)
 {
-	Ram_quota const released_ram { _subjects.release(subject_id) };
-
-	replenish(released_ram);
+	_subjects.release(subject_id);
 }
 
 

repos/os/src/app/trace_logger/main.cc

@@ -192,7 +192,6 @@ class Main
 				}
 				monitors.insert(new (_heap) Monitor(_trace, _env.rm(), id));
 			}
-			catch (Trace::Already_traced         ) { warn_msg("Already_traced"         ); return; }
 			catch (Trace::Source_is_dead         ) { warn_msg("Source_is_dead"         ); return; }
 			catch (Trace::Nonexistent_policy     ) { warn_msg("Nonexistent_policy"     ); return; }
 			catch (Trace::Traced_by_other_session) { warn_msg("Traced_by_other_session"); return; }