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.

Sign up for GitHub

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

Fix ETW trace logging crash in multithreading situations #21566

Merged
merged 10 commits into from
Jul 31, 2024
+28 −6
Merged

Fix ETW trace logging crash in multithreading situations #21566

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8b71d89
Move ETW initialization check assignment to after initialization has …
Jul 22, 2024
afd6858
Add initialization check to prevent re-entry into the lock during Tra…
Jul 22, 2024
dbac3b9
Update logging to honor initialization checks
Jul 23, 2024
5c2d1bd
change initialization vars to enum, and handle the failed case to avo…
Jul 30, 2024
d828441
typo
Jul 30, 2024
9362dc6
more typo
Jul 30, 2024
fff6c96
typo
Jul 31, 2024
a076c97
lint
Jul 31, 2024
71ecd52
lint 2
Jul 31, 2024
822c03a
lint 3
Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions onnxruntime/core/platform/windows/logging/etw_sink.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,28 +137,46 @@
EtwRegistrationManager::~EtwRegistrationManager() {
std::lock_guard<OrtMutex> lock(callbacks_mutex_);
callbacks_.clear();
::TraceLoggingUnregister(etw_provider_handle);
if (initialization_status_ == InitializationStatus::Intialized ||
smk2007 marked this conversation as resolved.
Show resolved Hide resolved
initialization_status_ == InitializationStatus::Initializing) {
std::lock_guard<OrtMutex> init_lock(init_mutex_);
assert(initialization_status_ != InitializationStatus::Initializing);
if (initialization_status_ == InitializationStatus::Intialized) {
::TraceLoggingUnregister(etw_provider_handle);
initialization_status_ = InitializationStatus::NotInitialized;
}
}
}

EtwRegistrationManager::EtwRegistrationManager() {
}

void EtwRegistrationManager::LazyInitialize() {
if (!initialized_) {
void EtwRegistrationManager::LazyInitialize() try {
if (initialization_status_ == InitializationStatus::NoitIntialized) {
std::lock_guard<OrtMutex> lock(init_mutex_);
if (!initialized_) { // Double-check locking pattern
initialized_ = true;
if (initialization_status_ == InitializationStatus::NotInitialized) { // Double-check locking pattern
initialization_status_ == InitializationStatus::Initializing;
etw_status_ = ::TraceLoggingRegisterEx(etw_provider_handle, ORT_TL_EtwEnableCallback, nullptr);
if (FAILED(etw_status_)) {
ORT_THROW("ETW registration failed. Logging will be broken: " + std::to_string(etw_status_));
}
initialization_status_ == InitializationStatus::Initialized;
}
}
} catch (...)
{

Check warning on line 167 in onnxruntime/core/platform/windows/logging/etw_sink.cc

View workflow job for this annotation

GitHub Actions / Optional Lint C++ 

[cpplint] reported by reviewdog 🐶
 { should almost always be at the end of the previous line  [whitespace/braces] [4]

Raw Output:
onnxruntime/core/platform/windows/logging/etw_sink.cc:167:  { should almost always be at the end of the previous line  [whitespace/braces] [4]
initialization_status_ == InitializationStatus::Failed;
throw;
}

void EtwRegistrationManager::InvokeCallbacks(LPCGUID SourceId, ULONG IsEnabled, UCHAR Level, ULONGLONG MatchAnyKeyword,
ULONGLONG MatchAllKeyword, PEVENT_FILTER_DESCRIPTOR FilterData,
PVOID CallbackContext) {
if (initialization_status_ != InitializationStatus::Initialized) {
// Drop messages until manager is fully initialized.
return;
}

std::lock_guard<OrtMutex> lock(callbacks_mutex_);
for (const auto& callback : callbacks_) {
(*callback)(SourceId, IsEnabled, Level, MatchAnyKeyword, MatchAllKeyword, FilterData, CallbackContext);
Expand Down
4 changes: 3 additions & 1 deletion onnxruntime/core/platform/windows/logging/etw_sink.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ class EtwSink : public ISink {
};

class EtwRegistrationManager {
enum class InitializationStatus { NotInitialized, Initializing, Initialized, Failed};

public:
using EtwInternalCallback = std::function<void(LPCGUID SourceId, ULONG IsEnabled, UCHAR Level,
ULONGLONG MatchAnyKeyword, ULONGLONG MatchAllKeyword,
Expand Down Expand Up @@ -96,7 +98,7 @@ class EtwRegistrationManager {
OrtMutex callbacks_mutex_;
mutable OrtMutex provider_change_mutex_;
OrtMutex init_mutex_;
bool initialized_ = false;
InitializationStatus initialization_status_ = InitializationStatus::NotInitialized;
bool is_enabled_;
UCHAR level_;
ULONGLONG keyword_;
Expand Down
Loading