Use atomic flag for buffer memory-attribute initialization#2216
Merged
Conversation
Store and read Buffer._mem_attrs_inited via std::atomic using acquire/release semantics to avoid races during lazy pointer memory- attribute initialization under free-threaded execution. Signed-off-by: Sebastian Berg <sebastianb@nvidia.com>
Contributor
github-actions
Bot
added
the
cuda.core
Contributor
Author
|
/ok to test 7c1c215 |
This comment has been minimized.
This comment has been minimized.
leofang
approved these changes
Jun 17, 2026
leofang
left a comment
leofang
left a comment
Member
There was a problem hiding this comment.
The change seems fine to me. Though I wonder if there'd be a significant perf hit. Originally Buffer was meant to be a thin wrapper over the raw pointer, but we've grown the complexity over time.
Might be a good idea checking with @danielfrg and see if the micro benchmarks can tell us the perf difference. I dunno if/where it runs TBH.
In any case, approving. Thanks, Sebastian!
Contributor
Author
I doubt this'll even register, I think the critical sections may be measurable but still so fast that it shouldn't really matter. |
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Store and read
Buffer._mem_attrs_initedvia std::atomic using acquire/release semantics to avoid races during lazy pointer memory- attribute initialization under free-threaded execution.At least strictly speaking,
_mem_attrs_initedis a flag that guards access to other memory attributes.My understanding is that on ARM processors with weak memory ordering this can be a problem even beyond compiler optimizations making use of the fact that order isn't strictly guaranteed (without the light use of atomics).
I.e.:
thread1 -> init _mem_attrs
thread1 -> sets _mem_attrs_inited
thread2 -> reads _mem_attrs_inited
thread2 -> reads _mem_attrs
Does not guarantee that thread2 doesn't read the uninitialized
_mem_attrsThis is probably not sufficiently covered by pytest-run-parallel, so if desired I/we should add an explicitly threaded test for it. (Although I have doubts we'll be able to fail the test even without the changes)