[EventPipe] Remove thread lock and cleanup ThreadSessionState lifecycle

src/native/eventpipe/ep-block.c

@@ -906,10 +906,10 @@ ep_sequence_point_block_init (
 	const uint32_t thread_count = dn_umap_size (map);
 	ep_write_buffer_uint32_t (&sequence_point_block->block.write_pointer, thread_count);
 
-	DN_UMAP_FOREACH_BEGIN (const EventPipeThreadSessionState *, key, uint32_t, sequence_number, map) {
+	DN_UMAP_FOREACH_BEGIN (EventPipeThread *, thread, uint32_t, sequence_number, map) {
 		ep_write_buffer_uint64_t (
 			&sequence_point_block->block.write_pointer,
-			ep_thread_get_os_thread_id (ep_thread_session_state_get_thread (key)));
+			ep_thread_get_os_thread_id (thread));
 
 		ep_write_buffer_uint32_t (
 			&sequence_point_block->block.write_pointer,

src/native/eventpipe/ep-buffer-manager.h

@@ -21,8 +21,6 @@ struct _EventPipeBufferList {
 #else
 struct _EventPipeBufferList_Internal {
 #endif
-	// The thread which writes to the buffers in this list.
-	EventPipeThreadHolder thread_holder;
 	// The buffer manager that owns this list.
 	EventPipeBufferManager *manager;
 	// Buffers are stored in an intrusive linked-list from oldest to newest.
@@ -31,9 +29,6 @@ struct _EventPipeBufferList_Internal {
 	EventPipeBuffer *tail_buffer;
 	// The number of buffers in the list.
 	uint32_t buffer_count;
-	// The sequence number of the last event that was read, only
-	// updated/read by the reader thread.
-	uint32_t last_read_sequence_number;
 };
 
 #if !defined(EP_INLINE_GETTER_SETTER) && !defined(EP_IMPL_BUFFER_MANAGER_GETTER_SETTER)
@@ -42,26 +37,13 @@ struct _EventPipeBufferList {
 };
 #endif
 
-EP_DEFINE_GETTER_REF(EventPipeBufferList *, buffer_list, EventPipeThreadHolder *, thread_holder);
-
-static
-inline
-EventPipeThread *
-ep_buffer_list_get_thread (EventPipeBufferList *buffer_list)
-{
-	return ep_thread_holder_get_thread (ep_buffer_list_get_thread_holder_cref (buffer_list));
-}
-
 EventPipeBufferList *
-ep_buffer_list_alloc (
-	EventPipeBufferManager *manager,
-	EventPipeThread *thread);
+ep_buffer_list_alloc (EventPipeBufferManager *manager);
 
 EventPipeBufferList *
 ep_buffer_list_init (
 	EventPipeBufferList *buffer_list,
-	EventPipeBufferManager *manager,
-	EventPipeThread *thread);
+	EventPipeBufferManager *manager);
 
 void
 ep_buffer_list_fini (EventPipeBufferList *buffer_list);
@@ -108,11 +90,22 @@ struct _EventPipeBufferManager_Internal {
 	ep_rt_spin_lock_handle_t rt_lock;
 	// The session this buffer manager belongs to.
 	EventPipeSession *session;
-	// Iterator state for reader thread.
+	// ---------------------------- READER-THREAD ONLY BEGIN ----------------------------
 	// These are not protected by rt_lock and expected to only be used on the reader thread.
+	// Iterator state for reader thread.
 	EventPipeEventInstance *current_event;
 	EventPipeBuffer *current_buffer;
-	EventPipeBufferList *current_buffer_list;
+	EventPipeThreadSessionState *current_thread_session_state;
+	// A snapshot of the thead_session_state_list only used by the reader thread to optimize event iteration.
+	// As existing and new threads write events, this represents the subset of EventPipeThreadSessionStates
+	// containing events before the snapshot timestamp. As buffer_manager_move_next_event_any_snapshot_thread
+	// and buffer_manager_move_next_event_same_thread iterate over events, entries are removed from the list
+	// when no more events are available before the snapshot timestamp. So once there are no more events
+	// before the snapshot timestamp, the list will be empty.
+	dn_list_t *thread_session_state_list_snapshot;
+	// The timestamp representing the cut-off for the snapshot list.
+	ep_timestamp_t snapshot_timestamp;
+	// ---------------------------- READER-THREAD ONLY END ------------------------------
 	// The total allocation size of buffers under management.
 	volatile size_t size_of_all_buffers;
 	// The maximum allowable size of buffers under management.
@@ -190,20 +183,6 @@ ep_buffer_manager_write_event (
 	ep_rt_thread_handle_t event_thread,
 	EventPipeStackContents *stack);
 
-// READ_ONLY state and no new EventPipeBuffers or EventPipeBufferLists can be created. Calls to
-// write_event that start during the suspension period or were in progress but hadn't yet recorded
-// their event into a buffer before the start of the suspension period will return false and the
-// event will not be recorded. Any events that not recorded as a result of this suspension will be
-// treated the same as events that were not recorded due to configuration.
-// EXPECTED USAGE: First the caller will disable all events via configuration, then call
-// suspend_write_event () to force any write_event calls that may still be in progress to either
-// finish or cancel. After that all BufferLists and Buffers can be safely drained and/or deleted.
-// _Requires_lock_held (ep)
-void
-ep_buffer_manager_suspend_write_event (
-	EventPipeBufferManager *buffer_manager,
-	uint32_t session_index);
-
 // Write the contents of the managed buffers to the specified file.
 // The stop_timeStamp is used to determine when tracing was stopped to ensure that we
 // skip any events that might be partially written due to races when tracing is stopped.
@@ -236,7 +215,7 @@ ep_buffer_manager_get_next_event (EventPipeBufferManager *buffer_manager);
 // threads can be in the middle of a write operation and get blocked, and we may not get an opportunity
 // to free their buffer for a very long time.
 void
-ep_buffer_manager_deallocate_buffers (EventPipeBufferManager *buffer_manager);
+ep_buffer_manager_close (EventPipeBufferManager *buffer_manager);
 
 #ifdef EP_CHECKED_BUILD
 bool

src/native/eventpipe/ep-buffer.c

@@ -56,8 +56,9 @@ ep_buffer_free (EventPipeBuffer *buffer)
 {
 	ep_return_void_if_nok (buffer != NULL);
 
-	// We should never be deleting a buffer that a writer thread might still try to write to
-	EP_ASSERT (ep_rt_volatile_load_uint32_t (&buffer->state) == (uint32_t)EP_BUFFER_STATE_READ_ONLY);
+	// buffers are not guaranteed to be read-only at this point as allocation may have failed,
+	// a Listener EventPipe Session may not have finished reading all events by disable,
+	// or an EventPipe session was abruptly disconnected.
 
 	ep_rt_vfree (buffer->buffer, buffer->limit - buffer->buffer);
 	ep_rt_object_free (buffer);
@@ -191,8 +192,6 @@ ep_buffer_convert_to_read_only (EventPipeBuffer *buffer)
 	EP_ASSERT (buffer != NULL);
 	EP_ASSERT (buffer->current_read_event == NULL);
 
-	ep_thread_requires_lock_held (buffer->writer_thread);
-
 	ep_rt_volatile_store_uint32_t (&buffer->state, (uint32_t)EP_BUFFER_STATE_READ_ONLY);
 
 	// If this buffer contains an event, select it.

src/native/eventpipe/ep-buffer.h

@@ -24,9 +24,9 @@
 // isn't writable and read-related methods will assert if it isn't readable. Methods that have no asserts should have immutable results that
 // can be used at any point during the buffer's lifetime. The buffer has no internal locks so it is the caller's responsibility to synchronize
 // their usage.
-// Writing into the buffer and calling convert_to_read_only() is always done with EventPipeThread rt_lock held. The eventual reader thread can do
+// Calling convert_to_read_only() is always done with EventPipeBufferManager lock held, and it yields to concurrent writes. The eventual reader thread can do
 // a few different things to ensure it sees a consistent state:
-// 1) Take the writer's EventPipeThread rt_lock at least once after the last time the writer writes events
+// 1) Take the buffer manager's lock at least once after the last time the writer writes events
 // 2) Use a memory barrier that prevents reader loads from being re-ordered earlier, such as the one that will occur implicitly by evaluating
 //    ep_buffer_get_volatile_state ()
 
@@ -57,7 +57,7 @@ struct _EventPipeBuffer_Internal {
 	// The linked list is invasive, thus we declare the pointers here.
 	EventPipeBuffer *prev_buffer;
 	EventPipeBuffer *next_buffer;
-	// State transition WRITABLE -> READ_ONLY only occurs while holding the writer_thread->rt_lock;
+	// State transition WRITABLE -> READ_ONLY only occurs while holding the buffer_manager lock;
 	// It can be read at any time
 	volatile uint32_t state;
 	// The sequence number corresponding to current_read_event
@@ -74,7 +74,6 @@ struct _EventPipeBuffer {
 EP_DEFINE_GETTER(EventPipeBuffer *, buffer, ep_timestamp_t, creation_timestamp)
 EP_DEFINE_GETTER(EventPipeBuffer *, buffer, uint8_t *, buffer)
 EP_DEFINE_GETTER(EventPipeBuffer *, buffer, uint8_t *, limit)
-EP_DEFINE_GETTER_REF(EventPipeBuffer *, buffer, volatile uint32_t *, state)
 EP_DEFINE_GETTER(EventPipeBuffer *, buffer, EventPipeBuffer *, prev_buffer)
 EP_DEFINE_SETTER(EventPipeBuffer *, buffer, EventPipeBuffer *, prev_buffer)
 EP_DEFINE_GETTER(EventPipeBuffer *, buffer, EventPipeBuffer *, next_buffer)
@@ -146,7 +145,7 @@ EventPipeBufferState
 ep_buffer_get_volatile_state (const EventPipeBuffer *buffer);
 
 // Convert the buffer writable to readable.
-// _Requires_lock_held (thread)
+// Should only be called while holding the buffer manager lock.
 void
 ep_buffer_convert_to_read_only (EventPipeBuffer *buffer);
 

src/native/eventpipe/ep-event-instance.c

@@ -231,8 +231,8 @@ sequence_point_fini (EventPipeSequencePoint *sequence_point)
 
 	// Each entry in the map owns a ref-count on the corresponding thread
 	if (dn_umap_size (sequence_point->thread_sequence_numbers) != 0) {
-		DN_UMAP_FOREACH_KEY_BEGIN (EventPipeThreadSessionState *, key, sequence_point->thread_sequence_numbers) {
-			ep_thread_release (ep_thread_session_state_get_thread (key));
+		DN_UMAP_FOREACH_KEY_BEGIN (EventPipeThread *, key, sequence_point->thread_sequence_numbers) {
+			ep_thread_release (key);
 		} DN_UMAP_FOREACH_END;
 	}
 

src/native/eventpipe/ep-session.c

@@ -40,10 +40,6 @@ static
 void
 session_create_streaming_thread (EventPipeSession *session);
 
-static
-void
-ep_session_remove_dangling_session_states (EventPipeSession *session);
-
 static
 bool
 session_user_events_tracepoints_init (
@@ -380,48 +376,6 @@ ep_session_alloc (
 	ep_exit_error_handler ();
 }
 
-void
-ep_session_remove_dangling_session_states (EventPipeSession *session)
-{
-	ep_return_void_if_nok (session != NULL);
-
-	DN_DEFAULT_LOCAL_ALLOCATOR (allocator, dn_vector_ptr_default_local_allocator_byte_size);
-
-	dn_vector_ptr_custom_init_params_t params = {0, };
-	params.allocator = (dn_allocator_t *)&allocator;
-	params.capacity = dn_vector_ptr_default_local_allocator_capacity_size;
-
-	dn_vector_ptr_t threads;
-
-	if (dn_vector_ptr_custom_init (&threads, &params)) {
-		ep_thread_get_threads (&threads);
-		DN_VECTOR_PTR_FOREACH_BEGIN (EventPipeThread *, thread, &threads) {
-			if (thread) {
-				EP_SPIN_LOCK_ENTER (ep_thread_get_rt_lock_ref (thread), section1);
-				EventPipeThreadSessionState *session_state = ep_thread_get_session_state(thread, session);
-				if (session_state) {
-					// If a buffer tries to write event(s) but never gets a buffer because the maximum total buffer size
-					// has been exceeded, we can leak the EventPipeThreadSessionState* and crash later trying to access 
-					// the session from the thread session state. Whenever we terminate a session we check to make sure
-					// we haven't leaked any thread session states.
-					ep_thread_delete_session_state(thread, session);
-				}
-				EP_SPIN_LOCK_EXIT (ep_thread_get_rt_lock_ref (thread), section1);
-
-				ep_thread_release (thread);
-			}
-		} DN_VECTOR_PTR_FOREACH_END;
-
-		dn_vector_ptr_dispose (&threads);
-	}
-
-ep_on_exit:
-	return;
-
-ep_on_error:
-	ep_exit_error_handler ();
-}
-
 void
 ep_session_inc_ref (EventPipeSession *session)
 {
@@ -445,11 +399,18 @@ ep_session_dec_ref (EventPipeSession *session)
 	ep_buffer_manager_free (session->buffer_manager);
 	ep_file_free (session->file);
 
-	ep_session_remove_dangling_session_states (session);
-
 	ep_rt_object_free (session);
 }
 
+void
+ep_session_close (EventPipeSession *session)
+{
+	EP_ASSERT (session != NULL);
+
+	if (ep_session_type_uses_buffer_manager (session->session_type))
+		ep_buffer_manager_close (session->buffer_manager);
+}
+
 EventPipeSessionProvider *
 ep_session_get_session_provider (
 	const EventPipeSession *session,
@@ -524,14 +485,23 @@ ep_session_execute_rundown (
 	ep_rt_execute_rundown (execution_checkpoints);
 }
 
+// Coordinates an EventPipeSession being freed in disable_holding_lock with
+// threads operating on session pointer slots in the _ep_sessions session array.
+// It assumes 1) callers have already cleared the session pointer slot from the
+// _ep_sessions so all threads that already loaded the session pointer slot will
+// be properly awaited and 2) the config lock is held to prevent a new session
+// being allocated and inheriting the session pointer slot index, which would
+// cause threads to mistakenly operate on the wrong session.
 void
-ep_session_suspend_write_event (EventPipeSession *session)
+ep_session_wait_for_inflight_thread_ops (EventPipeSession *session)
 {
 	EP_ASSERT (session != NULL);
 
 	// Need to disable the session before calling this method.
 	EP_ASSERT (!ep_is_session_enabled ((EventPipeSessionID)session));
 
+	ep_requires_lock_held ();
+
 	DN_DEFAULT_LOCAL_ALLOCATOR (allocator, dn_vector_ptr_default_local_allocator_byte_size);
 
 	dn_vector_ptr_custom_init_params_t params = {0, };
@@ -543,9 +513,13 @@ ep_session_suspend_write_event (EventPipeSession *session)
 	if (dn_vector_ptr_custom_init (&threads, &params)) {
 		ep_thread_get_threads (&threads);
 		DN_VECTOR_PTR_FOREACH_BEGIN (EventPipeThread *, thread, &threads) {
+			// The session slot must remain cleared from the _ep_sessions array from holding the config lock.
+			// Otherwise, if the config lock is removed, a newly allocated session could inherit the same slot
+			// and cause operating threads to mistake the new session for this one.
+			EP_ASSERT (!ep_is_session_enabled ((EventPipeSessionID)session));
 			if (thread) {
-				// Wait for the thread to finish any writes to this session
-				EP_YIELD_WHILE (ep_thread_get_session_write_in_progress (thread) == session->index);
+				// The session is disabled, so wait for any in-progress writes to complete.
+				EP_YIELD_WHILE (ep_thread_get_session_use_in_progress (thread) == session->index);
 
 				// Since we've already disabled the session, the thread won't call back in to this
 				// session once its done with the current write
@@ -556,9 +530,7 @@ ep_session_suspend_write_event (EventPipeSession *session)
 		dn_vector_ptr_dispose (&threads);
 	}
 
-	if (session->buffer_manager)
-		// Convert all buffers to read only to ensure they get flushed
-		ep_buffer_manager_suspend_write_event (session->buffer_manager, session->index);
+	ep_requires_lock_held ();
 }
 
 void

src/native/eventpipe/ep-session.h

@@ -86,6 +86,7 @@ EP_DEFINE_GETTER(EventPipeSession *, session, uint32_t, index)
 EP_DEFINE_GETTER(EventPipeSession *, session, EventPipeSessionProviderList *, providers)
 EP_DEFINE_GETTER(EventPipeSession *, session, EventPipeBufferManager *, buffer_manager)
 EP_DEFINE_GETTER_REF(EventPipeSession *, session, volatile uint32_t *, rundown_enabled)
+EP_DEFINE_GETTER(EventPipeSession *, session, EventPipeSessionType, session_type)
 EP_DEFINE_GETTER(EventPipeSession *, session, uint64_t, rundown_keyword)
 EP_DEFINE_GETTER(EventPipeSession *, session, ep_timestamp_t, session_start_time)
 EP_DEFINE_GETTER(EventPipeSession *, session, ep_timestamp_t, session_start_timestamp)
@@ -114,6 +115,9 @@ ep_session_inc_ref (EventPipeSession *session);
 void
 ep_session_dec_ref (EventPipeSession *session);
 
+void
+ep_session_close (EventPipeSession *session);
+
 // _Requires_lock_held (ep)
 EventPipeSessionProvider *
 ep_session_get_session_provider (
@@ -130,11 +134,12 @@ ep_session_execute_rundown (
 	EventPipeSession *session,
 	dn_vector_ptr_t *execution_checkpoints);
 
-// Force all in-progress writes to either finish or cancel
-// This is required to ensure we can safely flush and delete the buffers
+// Wait for all in-progress threads that depend on this session's pointer slot
+// in _ep_sessions remaining valid throughout its operation.
+// This is required to ensure we can safely flush, close, and free the session.
 // _Requires_lock_held (ep)
 void
-ep_session_suspend_write_event (EventPipeSession *session);
+ep_session_wait_for_inflight_thread_ops (EventPipeSession *session);
 
 // Write a sequence point into the output stream synchronously.
 void