Skip to content

Ensure is_done is set for unmanaged commands#9719

Open
richard-byron wants to merge 1 commit intoXilinx:masterfrom
richard-byron:master
Open

Ensure is_done is set for unmanaged commands#9719
richard-byron wants to merge 1 commit intoXilinx:masterfrom
richard-byron:master

Conversation

@richard-byron
Copy link
Copy Markdown

Problem solved by the commit

  • Fixes hitting assert in run_impl destructor when unmanaged have actually finished but their state has not been updated
  • Resolves potential deadlock by removing lock from is_done. The lock here should not be necessary, and is_done is called from set_dtrace_control_file while it already holds the mutex
  • Fixes potential race conditions with m_callbacks being accessed outside of locks, which could theoretically results in callbacks being called multiple times, or not at all.

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered

Introduced in #9649

How problem was solved, alternative solutions (if any) and why they were rejected

There are simpler solutions. Just calling get_state() before is_done() works, as long as you can convince the compiler it actually needs to update m_done. That would leave the other issues unresolved, but I'm not aware of them actually causing problems.

Risks (if any) associated the changes in the commit

Might change behavior with multiple threads waiting on runs to complete.

What has been tested and how, request additional testing if necessary

Tested on several designs, including ones that were hitting the assert and ones that weren't. More testing would definitely be desirable.

@richard-byron richard-byron requested a review from stsoe as a code owner April 8, 2026 22:00
@xrt-pr-bot
Copy link
Copy Markdown

xrt-pr-bot bot commented Apr 8, 2026

⚠️ Authorization Failed

@richard-byron is not a repository collaborator.

To proceed:

  • XRT Admins: Add the build label to authorize this PR build
  • OR Add @richard-byron as a repository collaborator

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.

1 participant