-
Notifications
You must be signed in to change notification settings - Fork 768
[SYCL][Coverity] Fix MISSING_LOCK coverity hits in the program manager #18121
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
Conversation
@aelovikov-intel Sergey's on vacation and wont be able to review in time, is there someone that can review it sooner? |
Maybe @KseniyaTikhomirova or @steffenlarsen ... I definitely don't think that going with a hack is the right thing to do here. Also, I don't know if there might be some other locks up the call stack that might make this a false positive. |
Would you prefer an additional mutex? I thought about adding another mutex, but decided against it since every other access to these maps has used I checked the call stack up to |
#16836 seems to be related. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, you need to reword your description then. Be more assertive that this is the right change that follows what pre-existing codes does and drop any hesitation from it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine, but please open a tracker to improve the locking. Also, I agree with @aelovikov-intel that the locking in removeImages
should be done outside the top-level loop, with a check that there are binaries before locking (or an early exit.)
Co-authored-by: Steffen Larsen <[email protected]>
Tracker created in #18165 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@intel/llvm-gatekeepers PR ready for merge, thanks! |
This PR guards accesses to
m_DeviceImage
andm_VFSet2BinImage
caches/maps in the Program Manager usingm_KernelIDsMutex
in situations where multiple threads may be accessing the maps at the same time.I am opting to use
m_KernelIDsMutex
here as both maps are commonly accessed while holding said mutex: In fact, a lot of maps/caches in the program manager usem_KernelIDsMutex
(This is not the most optimal solution, but that is out of the scope of this PR, see #18165)This PR addresses Coverity hits that needs to have fixes submitted before the code cutoff.