Skip to content

Prevent unbounded lock holds in BufferManager of EventPipe #48435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Mar 16, 2021
2 changes: 1 addition & 1 deletion src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h
Original file line number Diff line number Diff line change
Expand Up @@ -2865,7 +2865,7 @@ ep_rt_thread_set_activity_id (
* ThreadSequenceNumberMap.
*/

EP_RT_DEFINE_HASH_MAP(thread_sequence_number_map, ep_rt_thread_sequence_number_hash_map_t, EventPipeThreadSessionState *, uint32_t)
EP_RT_DEFINE_HASH_MAP_REMOVE(thread_sequence_number_map, ep_rt_thread_sequence_number_hash_map_t, EventPipeThreadSessionState *, uint32_t)
EP_RT_DEFINE_HASH_MAP_ITERATOR(thread_sequence_number_map, ep_rt_thread_sequence_number_hash_map_t, ep_rt_thread_sequence_number_hash_map_iterator_t, EventPipeThreadSessionState *, uint32_t)

/*
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/eventing/eventpipe/ep-rt-types-coreclr.h
Original file line number Diff line number Diff line change
Expand Up @@ -325,10 +325,10 @@ typedef struct _rt_coreclr_thread_params_t {
*/

#undef ep_rt_thread_sequence_number_hash_map_t
typedef struct _rt_coreclr_table_default_internal_t<EventPipeThreadSessionState *, uint32_t> ep_rt_thread_sequence_number_hash_map_t;
typedef struct _rt_coreclr_table_remove_internal_t<EventPipeThreadSessionState *, uint32_t> ep_rt_thread_sequence_number_hash_map_t;

#undef ep_rt_thread_sequence_number_hash_map_iterator_t
typedef class _rt_coreclr_table_default_internal_t<EventPipeThreadSessionState *, uint32_t>::table_type_t::Iterator ep_rt_thread_sequence_number_hash_map_iterator_t;
typedef class _rt_coreclr_table_remove_internal_t<EventPipeThreadSessionState *, uint32_t>::table_type_t::Iterator ep_rt_thread_sequence_number_hash_map_iterator_t;

#endif /* ENABLE_PERFTRACING */
#endif /* __EVENTPIPE_RT_TYPES_CORECLR_H__ */
2 changes: 1 addition & 1 deletion src/mono/mono/eventpipe/ep-rt-mono.h
Original file line number Diff line number Diff line change
Expand Up @@ -2155,7 +2155,7 @@ ep_rt_mono_thread_yield (void)
* ThreadSequenceNumberMap.
*/

EP_RT_DEFINE_HASH_MAP(thread_sequence_number_map, ep_rt_thread_sequence_number_hash_map_t, EventPipeThreadSessionState *, uint32_t)
EP_RT_DEFINE_HASH_MAP_REMOVE(thread_sequence_number_map, ep_rt_thread_sequence_number_hash_map_t, EventPipeThreadSessionState *, uint32_t)
EP_RT_DEFINE_HASH_MAP_ITERATOR(thread_sequence_number_map, ep_rt_thread_sequence_number_hash_map_t, ep_rt_thread_sequence_number_hash_map_iterator_t, EventPipeThreadSessionState *, uint32_t)

/*
Expand Down
79 changes: 76 additions & 3 deletions src/native/eventpipe/ep-buffer-manager.c
Original file line number Diff line number Diff line change
Expand Up @@ -1145,8 +1145,11 @@ ep_buffer_manager_write_all_buffers_to_file_v4 (

*events_written = false;

EP_RT_DECLARE_LOCAL_THREAD_SESSION_STATE_ARRAY(session_states_to_delete);
ep_rt_thread_session_state_array_init(&session_states_to_delete);
EventPipeSequencePoint *sequence_point = NULL;
ep_timestamp_t current_timestamp_boundary = stop_timestamp;

EP_SPIN_LOCK_ENTER (&buffer_manager->rt_lock, section1)
if (buffer_manager_try_peek_sequence_point (buffer_manager, &sequence_point))
current_timestamp_boundary = EP_MIN (current_timestamp_boundary, ep_sequence_point_get_timestamp (sequence_point));
Expand Down Expand Up @@ -1197,8 +1200,8 @@ ep_buffer_manager_write_all_buffers_to_file_v4 (
// the sequence point captured a lower bound for sequence number on each thread, but iterating
// through the events we may have observed that a higher numbered event was recorded. If so we
// should adjust the sequence numbers upwards to ensure the data in the stream is consistent.
ep_rt_thread_session_state_list_iterator_t thread_session_state_list_iterator = ep_rt_thread_session_state_list_iterator_begin (&buffer_manager->thread_session_state_list);
EP_SPIN_LOCK_ENTER (&buffer_manager->rt_lock, section2)
ep_rt_thread_session_state_list_iterator_t thread_session_state_list_iterator = ep_rt_thread_session_state_list_iterator_begin (&buffer_manager->thread_session_state_list);
while (!ep_rt_thread_session_state_list_iterator_end (&buffer_manager->thread_session_state_list, &thread_session_state_list_iterator)) {
EventPipeThreadSessionState * session_state = ep_rt_thread_session_state_list_iterator_value (&thread_session_state_list_iterator);
uint32_t thread_sequence_number = 0;
Expand All @@ -1209,11 +1212,28 @@ ep_buffer_manager_write_all_buffers_to_file_v4 (
// miscategorize it, but that seems unlikely.
uint32_t last_read_delta = last_read_sequence_number - thread_sequence_number;
if (0 < last_read_delta && last_read_delta < 0x80000000) {
ep_rt_thread_sequence_number_map_add_or_replace (ep_sequence_point_get_thread_sequence_numbers_ref (sequence_point), session_state, last_read_sequence_number);
if (!exists)
if (exists) {
ep_rt_thread_sequence_number_map_remove (ep_sequence_point_get_thread_sequence_numbers_ref (sequence_point), session_state);
} else {
ep_thread_addref (ep_thread_holder_get_thread (ep_thread_session_state_get_thread_holder_ref (session_state)));
}
ep_rt_thread_sequence_number_map_add (ep_sequence_point_get_thread_sequence_numbers_ref (sequence_point), session_state, last_read_sequence_number);
}

ep_rt_thread_session_state_list_iterator_next (&thread_session_state_list_iterator);

// if a session_state was exhausted during this sequence point, mark it for deletion
if (ep_thread_session_state_get_buffer_list (session_state)->head_buffer == NULL) {

// We don't hold the thread lock here, so it technically races with a thread getting unregistered. This is okay,
// because we will either not have passed the above if statement (there were events still in the buffers) or we
// will catch it at the next sequence point.
if (ep_rt_volatile_load_uint32_t_without_barrier (ep_thread_get_unregistered_ref (ep_thread_session_state_get_thread (session_state))) > 0) {

ep_rt_thread_session_state_array_append (&session_states_to_delete, session_state);
ep_rt_thread_session_state_list_remove (&buffer_manager->thread_session_state_list, session_state);
}
}
}
EP_SPIN_LOCK_EXIT (&buffer_manager->rt_lock, section2)

Expand All @@ -1231,6 +1251,59 @@ ep_buffer_manager_write_all_buffers_to_file_v4 (
}
}

// There are sequence points created during this flush and we've marked session states for deletion.
// We need to remove these from the internal maps of the subsequent Sequence Points
if (ep_rt_thread_session_state_array_size (&session_states_to_delete) > 0) {
EP_SPIN_LOCK_ENTER (&buffer_manager->rt_lock, section4)
if (buffer_manager_try_peek_sequence_point (buffer_manager, &sequence_point)) {
// foreach (sequence_point in buffer_manager->sequence_point_list)
for (ep_rt_sequence_point_list_iterator_t sequence_point_list_iterator = ep_rt_sequence_point_list_iterator_begin (&buffer_manager->sequence_points);
!ep_rt_sequence_point_list_iterator_end (&buffer_manager->sequence_points, &sequence_point_list_iterator);
ep_rt_sequence_point_list_iterator_next (&sequence_point_list_iterator)) {

sequence_point = ep_rt_sequence_point_list_iterator_value (&sequence_point_list_iterator);

// foreach (session_state in session_states_to_delete)
for (ep_rt_thread_session_state_array_iterator_t thread_session_state_array_iterator = ep_rt_thread_session_state_array_iterator_begin (&session_states_to_delete);
!ep_rt_thread_session_state_array_iterator_end (&session_states_to_delete, &thread_session_state_array_iterator);
ep_rt_thread_session_state_array_iterator_next (&thread_session_state_array_iterator)) {

EventPipeThreadSessionState * thread_session_state = ep_rt_thread_session_state_array_iterator_value (&thread_session_state_array_iterator);
uint32_t unused_thread_sequence_number = 0;
bool exists = ep_rt_thread_sequence_number_map_lookup (ep_sequence_point_get_thread_sequence_numbers_cref (sequence_point), thread_session_state, &unused_thread_sequence_number);
if (exists) {
ep_rt_thread_sequence_number_map_remove (ep_sequence_point_get_thread_sequence_numbers_ref (sequence_point), thread_session_state);
// every entry of this map was holding an extra ref to the thread (see: ep-event-instance.{h|c})
ep_thread_release (ep_thread_session_state_get_thread (thread_session_state));
}
}
}
}

EP_SPIN_LOCK_EXIT (&buffer_manager->rt_lock, section4)
}

// foreach (session_state in session_states_to_delete)
for (ep_rt_thread_session_state_array_iterator_t thread_session_state_array_iterator = ep_rt_thread_session_state_array_iterator_begin (&session_states_to_delete);
!ep_rt_thread_session_state_array_iterator_end (&session_states_to_delete, &thread_session_state_array_iterator);
ep_rt_thread_session_state_array_iterator_next (&thread_session_state_array_iterator)) {

EventPipeThreadSessionState * thread_session_state = ep_rt_thread_session_state_array_iterator_value (&thread_session_state_array_iterator);
EP_ASSERT (thread_session_state != NULL);
// This may be the last reference to a given EventPipeThread, so make a ref to keep it around till we're done
EventPipeThreadHolder thread_holder;
if (ep_thread_holder_init (&thread_holder, ep_thread_session_state_get_thread (thread_session_state))) {

ep_rt_spin_lock_handle_t *thread_lock = ep_thread_get_rt_lock_ref (ep_thread_holder_get_thread (&thread_holder));
EP_SPIN_LOCK_ENTER (thread_lock, section5)

EP_ASSERT(ep_rt_volatile_load_uint32_t_without_barrier (ep_thread_get_unregistered_ref (ep_thread_session_state_get_thread (thread_session_state))) > 0);
ep_thread_delete_session_state (ep_thread_session_state_get_thread (thread_session_state), ep_thread_session_state_get_session (thread_session_state));
EP_SPIN_LOCK_EXIT (thread_lock, section5)
ep_thread_holder_fini (&thread_holder);
}
}

ep_on_exit:
return;
ep_on_error:
Expand Down
1 change: 1 addition & 0 deletions src/native/eventpipe/ep-buffer-manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ struct _EventPipeBufferManager_Internal {
// The total amount of allocations we can do after one sequence
// point before triggering the next one
size_t sequence_point_alloc_budget;

#ifdef EP_CHECKED_BUILD
volatile int64_t num_events_stored;
volatile int64_t num_events_dropped;
Expand Down
2 changes: 1 addition & 1 deletion src/native/eventpipe/ep-rt.h
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,7 @@ ep_rt_thread_set_activity_id (
* ThreadSequenceNumberMap.
*/

EP_RT_DECLARE_HASH_MAP(thread_sequence_number_map, ep_rt_thread_sequence_number_hash_map_t, EventPipeThreadSessionState *, uint32_t)
EP_RT_DECLARE_HASH_MAP_REMOVE(thread_sequence_number_map, ep_rt_thread_sequence_number_hash_map_t, EventPipeThreadSessionState *, uint32_t)
EP_RT_DECLARE_HASH_MAP_ITERATOR(thread_sequence_number_map, ep_rt_thread_sequence_number_hash_map_t, ep_rt_thread_sequence_number_hash_map_iterator_t, EventPipeThreadSessionState *, uint32_t)


Expand Down
2 changes: 2 additions & 0 deletions src/native/eventpipe/ep-thread.c
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ ep_thread_alloc (void)
memset (instance->session_state, 0, sizeof (instance->session_state));

instance->writing_event_in_progress = UINT32_MAX;
instance->unregistered = 0;

ep_on_exit:
return instance;
Expand Down Expand Up @@ -141,6 +142,7 @@ ep_thread_unregister (EventPipeThread *thread)
while (!ep_rt_thread_list_iterator_end (&_ep_threads, &iterator)) {
if (ep_rt_thread_list_iterator_value (&iterator) == thread) {
ep_rt_thread_list_remove (&_ep_threads, thread);
ep_rt_volatile_store_uint32_t(&thread->unregistered, 1);
ep_thread_release (thread);
found = true;
break;
Expand Down
6 changes: 6 additions & 0 deletions src/native/eventpipe/ep-thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ struct _EventPipeThread_Internal {
// that pointer will be protected from deletion. See ep_disable () and
// ep_write () for more detail.
volatile uint32_t writing_event_in_progress;
// This is set to non-zero when the thread is unregistered from the global list of EventPipe threads.
// This should happen when a physical thread is ending.
// This is a convenience marker to prevent us from having to search the global list.
// defaults to false.
volatile uint32_t unregistered;
};

#if !defined(EP_INLINE_GETTER_SETTER) && !defined(EP_IMPL_THREAD_GETTER_SETTER)
Expand All @@ -70,6 +75,7 @@ EP_DEFINE_SETTER(EventPipeThread *, thread, EventPipeSession *, rundown_session)
EP_DEFINE_GETTER_REF(EventPipeThread *, thread, ep_rt_spin_lock_handle_t *, rt_lock);
EP_DEFINE_GETTER(EventPipeThread *, thread, uint64_t, os_thread_id);
EP_DEFINE_GETTER_REF(EventPipeThread *, thread, int32_t *, ref_count);
EP_DEFINE_GETTER_REF(EventPipeThread *, thread, volatile uint32_t *, unregistered);

EventPipeThread *
ep_thread_alloc (void);
Expand Down