Skip to content

[SYCL][Graph] Fix lock order inversion during native recording host task submission - #22758

Open
mmichel11 wants to merge 2 commits into
intel:syclfrom
adamfidel:matt/native_recording_lock_fix
Open

[SYCL][Graph] Fix lock order inversion during native recording host task submission#22758
mmichel11 wants to merge 2 commits into
intel:syclfrom
adamfidel:matt/native_recording_lock_fix

Conversation

@mmichel11

@mmichel11 mmichel11 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

If a user concurrently ends capture while submitting a native host task, then the thread ending capture acquires the graph lock -> queue lock and the host task submission thread acquires queue lock -> graph lock through the handler. This was introduced after we added locking around graph capture in #22604.

The graph lock is acquired during host task submission to extend the lifetime of the user's callback. This container can be given its own mutex as it is just a lifetime extender that does not require locking the full graph and prevents deadlock.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes a potential deadlock during SYCL graph native recording by removing a graph_impl::MMutex acquisition from the restricted host-task submission path (which runs under the queue mutex), eliminating a queue -> graph lock order that could invert with end_recording()’s graph -> queue order.

Changes:

  • Add a new e2e regression test that races Graph.end_recording() against concurrent restricted host task submissions.
  • Introduce a dedicated mutex guarding the native host-task callback lifetime container in graph_impl.
  • Switch graph_impl::addNativeHostTaskCallback() to use the new leaf mutex instead of the graph-wide mutex.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
sycl/test-e2e/Graph/RecordReplay/NativeRecording/Threading/host_task_concurrent_end_recording.cpp Adds a concurrency regression test intended to reproduce the prior lock-order inversion/deadlock scenario.
sycl/source/detail/graph/graph_impl.hpp Adds a dedicated mutex for the native host-task callback container to avoid taking the graph-wide lock in the submit path.
sycl/source/detail/graph/graph_impl.cpp Updates addNativeHostTaskCallback() to lock the new mutex instead of MMutex.

Comment thread sycl/source/detail/graph/graph_impl.hpp
@mmichel11
mmichel11 marked this pull request as ready for review July 24, 2026 18:33
@mmichel11
mmichel11 requested a review from a team as a code owner July 24, 2026 18:33
@mmichel11
mmichel11 requested a review from adamfidel July 24, 2026 18: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.

2 participants