Skip to content
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

Eventpipe block unregister for callbacks #105734

Closed

Conversation

mdh1418
Copy link
Member

@mdh1418 mdh1418 commented Jul 31, 2024

Fixes #80666

mdh1418 added 5 commits July 31, 2024 13:04
Introduce a new CrstType to add a new CrstStatic lock
in EventPipe. The new lock specifically aims to match
EventPipe behavior with ETW, by blocking EventPipe's
EventProvider Disposal until in flight callbacks complete.
@mdh1418 mdh1418 force-pushed the eventpipe_block_unregister_for_callbacks branch from 0733de8 to b862d5b Compare July 31, 2024 20:00
@mdh1418
Copy link
Member Author

mdh1418 commented Aug 1, 2024

This PR attempted to fix #80666 by adding a new lock to help coordinate provider deletion while in-flight callbacks were running.

This PR tried to add new lock since the global eventpipe config lock ep_rt_config_acquire from EP_LOCK_ENTER was deliberately avoided when invoking the provider callback in provider_invoke_callback from ep_requires_lock_not_held() (original reason not completely clear).

Since there is a callback might be able to callback into various eventpipe functions, the global eventpipe config lock might be acquired during the callback, so the convention for using the new lock would be to always acquire the new lock before acquiring the global eventpipe config lock, to prevent deadlocks.

However, after implementing the new lock, we discovered a deadlock between this new eventpipe callback_dispatch lock and the EventListenersLock. It looks like by adding this new lock to coordinate provider_invoke_callback with ep_delete_provider, we unintentionally break the scenario where multiple threads could have callbacks running concurrently. So that is probably why locks should be avoided in provider_invoke_callback.

Deadlock scenario

callback_dispatch lock held -> try to acquire EventListenersLock in EventSource::SendCommand

09a8e804 6f09c8ed coreclr!JIT_MonEnter_Helper+0xf4
09a8e940 6c7473d9 coreclr!JIT_MonReliableEnter_Portable+0x40d
09a8e970 6c74c1d2 System_Private_CoreLib!System.Diagnostics.Tracing.EventSource::SendCommand(System.Diagnostics.Tracing.EventListener, System.Diagnostics.Tracing.EventProviderType, System.Int32, System.Diagnostics.Tracing.EventCommand, System.Boolean, System.Diagnostics.Tracing.EventLevel, System.Diagnostics.Tracing.EventKeywords, System.Collections.Generic.IDictionary`2[System.String, System.String])$##6008F46+0x99
09a8e998 6c743588 System_Private_CoreLib!System.Diagnostics.Tracing.EventSource+OverrideEventProvider::OnControllerCommand(System.Diagnostics.Tracing.ControllerCommand, System.Collections.Generic.IDictionary`2[System.String, System.String], System.Int32)$##6008F90+0x32
09a8e9c8 6c73dbaf System_Private_CoreLib!System.Diagnostics.Tracing.EventProviderImpl::ProviderCallback(System.Diagnostics.Tracing.EventProvider, System.Byte*, System.Int32, System.Byte, System.Int64, System.Int64, Interop+Advapi32+EVENT_FILTER_DESCRIPTOR*)$##6008EF7+0x88
09a8ea0c 6f2c0c4d System_Private_CoreLib!System.Diagnostics.Tracing.EventPipeEventProvider::Callback(System.Byte*, System.Int32, System.Byte, System.Int64, System.Int64, Interop+Advapi32+EVENT_FILTER_DESCRIPTOR*, System.Void*)$##6008EA6+0x6f
09a8ea90 6f2c63e4 coreclr!ep_rt_provider_invoke_callback+0x97
09a8eb04 6f2b7d74 coreclr!provider_invoke_callback+0x146
09a8eba4 6f2bd3b0 coreclr!disable_helper+0xc0

EventListenersLock held -> try to acquire eventpipe callback_dispatch lock by invoking callback

(Inline) -------- coreclr!ep_rt_callback_dispatch_acquire+0xa
00f7e720 6f2bd74e coreclr!provider_invoke_callback+0x50
00f7e76c 6f2bd3f6 coreclr!ep_enable_3+0xb4
00f7e7b0 6f03fe60 coreclr!ep_enable+0x3b
00f7e850 6f040c74 coreclr!EventPipeAdapter::Enable+0xb6
*** WARNING: Unable to verify checksum for System.Private.CoreLib.dll
00f7e918 6c73cd7e coreclr!EventPipeInternal_Enable+0x104
00f7e9bc 6c73d1af System_Private_CoreLib!System.UInt64 System.Diagnostics.Tracing.EventPipeInternal::Enable(System.String, System.Diagnostics.Tracing.EventPipeSerializationFormat, System.UInt32, System.Diagnostics.Tracing.EventPipeProviderConfiguration[])$##6008E81+0xfe
00f7ea80 6c73d036 System_Private_CoreLib!System.Diagnostics.Tracing.EventPipeEventDispatcher::CommitDispatchConfiguration()$##6008E99+0x13f
00f7eab0 6c74c6dd System_Private_CoreLib!System.Diagnostics.Tracing.EventPipeEventDispatcher::SendCommand(System.Diagnostics.Tracing.EventListener, System.Diagnostics.Tracing.EventCommand, System.Boolean, System.Diagnostics.Tracing.EventLevel, System.Diagnostics.Tracing.EventKeywords)$##6008E98+0xc6

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tracing/eventpipe/eventsourceerror/eventsourceerror/eventsourceerror failure
1 participant