[UR] Add remaining calls shared with queue in level-zero v2 adapter #17061
Conversation
Xewar313
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Xewar313
changed the title
Xewar313
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| ur_event_handle_t *phEvent) { | ||
| TRACK_SCOPE_LATENCY("ur_command_list_manager::appendUSMAdvise"); | ||
|
|
||
| std::scoped_lock<ur_shared_mutex> lock(this->Mutex); |
There was a problem hiding this comment.
This isn;t related to this PR in particular but to the whole command_list_manager implementation. I didn't notice this before but now, we have 2 different mutexes: this one (from command_list_manager) and a separate one in queue_immediate_in_order class.
Some functions now lock this mutex, while others lock the one from queue_immediate_in_order which means there's no synchronization.
This should be fixed, probably by always using lock from ur_command_list_manager (in every queue_immediate_in_order and command_buffer function).
There was a problem hiding this comment.
Makes sense - should I add it to this PR, or these changes should be in separate one?
There was a problem hiding this comment.
You can do that in a separate PR.
There was a problem hiding this comment.
What we are really trying to protect is the state inside of the command list manager. Maybe we should create some sort of scoped locking abstraction for objects (like mutexes in Rust), e.g.,:
class CommandBuffer {
Mutex<CommandListManager> cmdListMgr;
}
{
auto mgr = cmdbuf->cmdListMgr.lock();
mgr->AppendFoo();
}
This way it will be impossible to make the same mistake and we don't have to extend the scope of a lock to the outside of its member functions.
There was a problem hiding this comment.
Hm, that's actually a pretty good idea, and it should be enough to just create a structure that holds a reference to the cmdListMgr and implements command_list_manager operator->(), then mgr->whatever() should work automatically.
|
The CI failure is a known issue - #16877 |
…7297) 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).
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 intel/llvm#17061 (comment).
Adds implements calls shared between command buffer and queue in unified-runtime level-zero v2 adapter and moves the shared code to
command_list_manager.cpp