[SYCL] Reset command lists in the synchronization points #6168
Conversation
Don't reset command lists each time we try to get an available command list. We should check for signalled command lists in synchronization points like piQueueFinish, piQueueRelease, piEventsWait instead.
| // signalled, then the command list & fence are reset and command list is | ||
| // returned to the command list cache. All events associated with command list | ||
| // are cleaned up if command list was reset. | ||
| std::scoped_lock Lock(this->Mutex); |
There was a problem hiding this comment.
can we make this function non-locking (but request callers to hold a lock on the queue)?
There was a problem hiding this comment.
I am not sure why it is better to do so. In my understanding it is better to lock a mutex for smaller amount of time because it is easier to control it this way. Otherwise we keep enabling the strategy of locking mutexes for very long time and doing everything under one lock which will result in nested locks/deadlocks.
Also, as the next step I was planning to move loop which does event->cleanup (it is currently inside resetCommandList()) out of this queue lock. Because event->cleanup locks queue itself to update it's state (update last command event and refcount) and since event->cleanup triggers cleanup of all dependent events we need to lock their queues as well which can result in deadlock situation if we have an outer queue lock (because dependent events may be from different queue). Actually, we have this deadlock situation now in the code and it needs to be fixed.
There was a problem hiding this comment.
OK, but please explain in function comment that we expect no locks on entry
Don't reset command lists each time we try to get an available command list. We should check for signaled command lists in synchronization points like piQueueFinish, piQueueRelease, piEventsWait instead.
3 participants
Don't reset command lists each time we try to get an available command
list. We should check for signalled command lists in synchronization
points like piQueueFinish, piQueueRelease, piEventsWait instead.