[UR] [V2] Fix synchronization between command_list_manager usages #17297
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pbalcer
reviewed
Mar 5, 2025
unified-runtime/source/adapters/level_zero/v2/queue_immediate_in_order.cpp
Show resolved
Hide resolved
igchor
reviewed
Mar 5, 2025
| @@ -140,20 +142,20 @@ ur_result_t ur_queue_immediate_in_order_t::queueGetNativeHandle( | |||
| ur_queue_native_desc_t *pDesc, ur_native_handle_t *phNativeQueue) { | |||
| std::ignore = pDesc; | |||
| *phNativeQueue = reinterpret_cast<ur_native_handle_t>( | |||
| this->commandListManager.getZeCommandList()); | |||
| this->commandListManager.get_no_lock()->getZeCommandList()); | |||
| return UR_RESULT_SUCCESS; | |||
| } | |||
|
|
|||
| ur_result_t ur_queue_immediate_in_order_t::queueFinish() { | |||
| TRACK_SCOPE_LATENCY("ur_queue_immediate_in_order_t::queueFinish"); | |||
|
|
|||
| std::unique_lock<ur_shared_mutex> lock(this->Mutex); | |||
There was a problem hiding this comment.
why do we still use this->mutex (here and in other places)? shouldn't we remove it and use command list locking exclusively?
unified-runtime/source/adapters/level_zero/v2/queue_immediate_in_order.cpp
Show resolved
Hide resolved
konradkusiak97
approved these changes
Mar 6, 2025
Xewar313
force-pushed
the
add-mutex-to-command-manager
branch
from
cfe33c4 to
53e2c7a
Compare
pbalcer
approved these changes
Mar 10, 2025
igchor
approved these changes
Mar 11, 2025
|
@intel/llvm-gatekeepers please merge. One test fails on unrelated changes and it has an issue #17392 |
|
@intel/llvm-gatekeepers please merge, and if there is any issue with merge, I would be really grateful if someone could tell how to solve it |
|
@intel/llvm-gatekeepers bump |
|
Unlikely ESIMD fail is related |
EwanC
reviewed
Mar 14, 2025
Comment on lines
+497
to
+506
| ur_result_t urCommandBufferEnqueueExp( | ||
| ur_exp_command_buffer_handle_t CommandBuffer, ur_queue_handle_t UrQueue, | ||
| uint32_t NumEventsInWaitList, const ur_event_handle_t *EventWaitList, | ||
| ur_event_handle_t *Event) try { | ||
| return UrQueue->get().enqueueCommandBufferExp( | ||
| CommandBuffer, NumEventsInWaitList, EventWaitList, Event); | ||
| } catch (...) { | ||
| return exceptionToResult(std::current_exception()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why was this function reintroduced? I think it might be a rebase error
There was a problem hiding this comment.
whoops, let me know if i should revert this
There was a problem hiding this comment.
Opened a PR to fix this #17476 (not worth a revert, it's dead code but not breaking anything)
uditagarwal97
pushed a commit
that referenced
this pull request
Mar 14, 2025
kbenzie
pushed a commit
to oneapi-src/unified-runtime
that referenced
this pull request
Mar 17, 2025
`urCommandBufferEnqueueExp` was changed in intel/llvm#16984 to `urEnqueueCommandBufferExp` but added back accidentally to the v2 L0 adapter in intel/llvm#17297
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes:
Command_list_manager no longer synchronize its calls, instead the responsibility to ensure exclusivity belongs to the caller.
To add synchronization I implemented the mechanism similar to rust lock as suggested in #17061 (comment).