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

Fix ETW trace logging crash in multithreading situations (#21566) #21603

Merged
merged 4 commits into from
Aug 9, 2024

Conversation

smk2007
Copy link
Member

@smk2007 smk2007 commented Aug 2, 2024

ETW trace logger is fakely registered as initialized_ is marked as true before the registration is done, causing crashing issue for Lenovo camera application.

A prior attempt to address was made here:
#21226 It was reverted here:
#21360

The problem is that during initialization of TraceLoggingRegisterEx, it will reinvoke the callback and attempt reinitialization, which is not allowed. TraceLoggingRegisterEx however can be initialized concurrently when initialization happens on multiple threads. For these reasons it needs to be protected by a lock, but the lock cannot naively block because the callback's reinvocation will cause a deadlock.

To solve this problem another tracking variable is added : "initializing" which protects against reinitialization during the first initialization.


Description

A few CI pipelines are disabled because they are no longer compatible with this legacy GE branch.

Motivation and Context

ETW trace logger is fakely registered as initialized_ is marked as true
before the registration is done, causing crashing issue for Lenovo
camera application.

A prior attempt to address was made here:
#21226
It was reverted here:
#21360

The problem is that during initialization of TraceLoggingRegisterEx, it
will reinvoke the callback and attempt reinitialization, which is not
allowed. TraceLoggingRegisterEx however can be initialized concurrently
when initialization happens on multiple threads. For these reasons it
needs to be protected by a lock, but the lock cannot naively block
because the callback's reinvocation will cause a deadlock.

To solve this problem another tracking variable is added :
"initializing" which protects against reinitialization during the first
initialization.

---------

Co-authored-by: Sheil Kumar <[email protected]>
fdwr
fdwr previously approved these changes Aug 2, 2024
Copy link
Contributor

@fdwr fdwr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

zhangxiang1993
zhangxiang1993 previously approved these changes Aug 6, 2024
@zhangxiang1993 zhangxiang1993 dismissed stale reviews from fdwr and themself via 5ce7e77 August 8, 2024 15:32
@zhangxiang1993 zhangxiang1993 requested a review from a team as a code owner August 8, 2024 15:32
@zhangxiang1993 zhangxiang1993 self-requested a review August 8, 2024 17:43
Copy link
Contributor

@fdwr fdwr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The C++ parts still look good to me. Deferring to ORT folks for the pipeline config aspects.

zhangxiang1993
zhangxiang1993 previously approved these changes Aug 8, 2024
yf711
yf711 previously approved these changes Aug 8, 2024
fdwr
fdwr previously approved these changes Aug 8, 2024
@fdwr
Copy link
Contributor

fdwr commented Aug 8, 2024

Merging can be performed automatically with 2 approving reviews.

Xiang: I'm unsure who you need for this 🤔. Yifan Li and my approval are obviously inadequate.

baijumeswani
baijumeswani previously approved these changes Aug 8, 2024
@zhangxiang1993 zhangxiang1993 dismissed stale reviews from baijumeswani, fdwr, yf711, and themself via e7d1be0 August 8, 2024 21:55
@snnn snnn closed this Aug 8, 2024
@snnn snnn reopened this Aug 8, 2024
@snnn snnn closed this Aug 8, 2024
@snnn snnn reopened this Aug 8, 2024
@zhangxiang1993 zhangxiang1993 merged commit ce6951d into rel-os-1.17.0 Aug 9, 2024
63 of 72 checks passed
@zhangxiang1993 zhangxiang1993 deleted the user/sheilk/cp-b341c44 branch August 9, 2024 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants