-
Notifications
You must be signed in to change notification settings - Fork 315
fix(cuda.bindings): Make param_packer.feed() free-threading-safe via import-time init #2417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
clin1234
wants to merge
18
commits into
NVIDIA:main
Choose a base branch
from
clin1234:threading-fixes
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+93
−11
Open
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
ef5249c
Make param_packer.feed() free-threading-safe via import-time init
clin1234 dfde2e3
Fix #1169: correct NumPy skip conditions for DLPack host writes (#2238)
ArsalanShakil 0f91cfa
Fix potential data race in bindings init functions (#2379)
mdboom 6fe47fe
Pathfinder: add support on cuQuantum library loading and header descr…
yangcal 3b3c547
fix(pathfinder): remove dead/misleading error handling in platform lo…
aryanputta 1300e0d
feat(pathfinder): add CTK-root canary fallback for NVIDIA binaries (#…
aryanputta 50b591a
ci: bump pixi to v0.73.0 to fix per-run Cython rebuild (#2138) (#2396)
rparolin c40724c
cuda.core: retain graph attachments with per-node user objects (#2357)
Andy-Jost c1299d6
New lowpp layer for nvrtc (#2398)
mdboom 9ec9217
Harden program-cache permissions, binary-path resolution, and coredum…
rparolin 6780959
toolshed: tolerate anonymous struct renames in check_cython_abi.py (#…
mdboom 2af276e
Fix latch test on devices without concurrent managed access (#2413)
Andy-Jost 79ce913
test(cuda.core): isolate pending-call saturation cleanup (#2406)
Andy-Jost 3548050
Add ObjectCode.get_module() for legacy driver API integration (#2339)
lijinf2 00edfbf
Revert SMResource splitting fix, the problem was the test (#2321)
seberg cb62ef7
Raise OverflowError on out-of-range c_int/c_byte kernel arguments (#2…
rparolin 60a48ed
Merge branch 'main' into threading-fixes
clin1234 c414a01
[pre-commit.ci] auto code formatting
pre-commit-ci[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1060,10 +1060,16 @@ DevicePtrHandle deviceptr_create_mapped_graphics( | |
| // MemoryResource-owned Device Pointer Handles | ||
| // ============================================================================ | ||
|
|
||
| static MRDeallocCallback mr_dealloc_cb = nullptr; | ||
| // Registered exactly once at import from _buffer.pyx (module-level statement, | ||
| // runs single-threaded before any MR-backed device pointer exists), so a plain | ||
| // pointer would already be safe. Made atomic as free-threading hardening: it is | ||
| // read/called from shared_ptr deleters on arbitrary threads, and the | ||
| // acquire/release pair gives a well-formed happens-before between the import | ||
| // registration and every later deleter read. | ||
| static std::atomic<MRDeallocCallback> mr_dealloc_cb{nullptr}; | ||
|
|
||
| void register_mr_dealloc_callback(MRDeallocCallback cb) { | ||
| mr_dealloc_cb = cb; | ||
| mr_dealloc_cb.store(cb, std::memory_order_release); | ||
| } | ||
|
|
||
| DevicePtrHandle deviceptr_create_with_mr(CUdeviceptr ptr, size_t size, PyObject* mr) { | ||
|
|
@@ -1081,8 +1087,9 @@ DevicePtrHandle deviceptr_create_with_mr(CUdeviceptr ptr, size_t size, PyObject* | |
| [mr, size](DevicePtrBox* b) { | ||
| GILAcquireGuard gil; | ||
| if (gil.acquired()) { | ||
| if (mr_dealloc_cb) { | ||
| mr_dealloc_cb(mr, b->resource, size, b->h_stream); | ||
| if (MRDeallocCallback cb = | ||
| mr_dealloc_cb.load(std::memory_order_acquire)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure I am following, this seems like unnecessary hardening? If this was a general pattern, I might not mind, but I think we have many of these globals that we assume are initialized once at module init time and if that is the only time it is set, it is fine? |
||
| cb(mr, b->resource, size, b->h_stream); | ||
| } | ||
| Py_DECREF(mr); | ||
| } | ||
|
|
||
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we re-wire everything here, can we just remove this global and make all type objects strong references instead? That seems much clearer.
(Unfortunately, I guess we are not vendoring pythoncapi-compat, so we don't have
PyDict_GetItemStringRef, here it's fine, so let's not worry, although I could see that we'll just need that in the future...)