Repository navigation
[libcu++] Make resolving a default memory pool legal under stream capture - #11360
Conversation
…ture `__get_default_memory_pool` applies the library's retention policy to the driver's default pool by reading (and, the first time, writing) the release-threshold attribute. `cuMemPoolGetAttribute`/`SetAttribute` are "potentially unsafe" driver calls: while the calling thread is in a global or thread-local stream capture they fail with CUDA_ERROR_STREAM_CAPTURE_UNSUPPORTED and invalidate the capture. The default-pool lookup itself is a pure query and is legal under capture; only the policy step is not. Any lazy first touch of a default pool under an active capture therefore threw: the first `device_default_memory_pool()` of a process, a pool ref constructed while capturing, and every consumer that resolves the pool on its allocation path (cudax's locality-domain data places since NVIDIA#11202, which broke the sharded graph-capture tests). Fix: run the policy step inside a `__relaxed_capture_scope`, an RAII scope around `cuThreadExchangeStreamCaptureMode` that puts the calling thread in relaxed capture mode and restores the previous mode on exit. Capture mode is a per-thread property, so this is independent of any stream, has no observable effect when the thread is not capturing, and costs two cheap driver calls (~50 ns measured). The attribute write executes immediately rather than being recorded, which is the intent for a process-global setting. Nothing that should be captured is issued inside the scope. Adds `__driver::__threadExchangeStreamCaptureMode`. Test (libcudacxx/test/.../resources/default_pool_under_capture.cu): begins a capture in global and in thread-local mode, resolves the device default pool through the public once-only accessor (first touch of the process, on purpose) and through the helper, allocates from the pool inside the capture, and checks the capture ends successfully with at least one node and that the policy was applied. A final section proves the thread's mode is restored: an unsafe pool query right after the accessor is refused again. Driver entry points only, as the ccclrt fixture requires. Fails before the fix with "operation not permitted when stream is capturing(900): Failed to get attribute for a memory pool"; passes after. memory_pools, locality_domain_memory_pool and device_memory_resource tests unchanged. Co-Authored-By: Claude Fable 5.1 <[email protected]>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 SummarySummary by CodeRabbit
WalkthroughThe change adds a CUDA driver wrapper and shared RAII scope for temporary relaxed stream-capture mode during default memory-pool attribute operations. A driver-only test covers global and thread-local capture. ChangesDefault pool capture support
Priority: ⬇️ Low Change: Bug fix Merge Risk: 🔵 Low · up to Functionality is covered for both capture modes, but thread-local lazy initialization remains untested and should be covered before merge. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libcudacxx/test/libcudacxx/cuda/memory_resource/resources/default_pool_under_capture.cu (1)
120-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Assert the exact restored capture mode.
For each
capture_modescase, use::cuda::__driver::__threadExchangeStreamCaptureModeaftercuda::__get_default_memory_pool, assert that the exchanged value equalsc.mode, then exchange it back. The current check covers only global mode; restoring thread-local mode as global could still pass. Use the driver wrapper because this test must not call cudart.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3c940ea1-d91e-4a8e-94e9-5f4fe06ab723
📒 Files selected for processing (3)
libcudacxx/include/cuda/__driver/driver_api.hlibcudacxx/include/cuda/__memory_pool/memory_pool_base.hlibcudacxx/test/libcudacxx/cuda/memory_resource/resources/default_pool_under_capture.cu
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
…lt-pool accessor Co-Authored-By: Claude Fable 5.1 <[email protected]>
Review feedback on NVIDIA#11360. Co-Authored-By: Claude Fable 5.1 <[email protected]>
…capture_scope.h Review feedback on NVIDIA#11360. Co-Authored-By: Claude Fable 5.1 <[email protected]>
|
/ok to test 749cf07 |
… host compiler
Since CUDA 13.x `CUmemLocation::id` lives inside an anonymous union (next to
the `localized` struct), so `CUmemLocation{type, id}` brace-elides a
subobject and clang 20 promotes -Wmissing-braces to an error under -Werror
in the CUDA 13.3 + llvm20 "Build libcudacxx" job. Default-initialize and
assign the fields instead, matching the style used in memory_pool_base.h.
Co-Authored-By: Claude Fable 5.1 <[email protected]>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
libcudacxx/test/libcudacxx/cuda/memory_resource/resources/default_pool_under_capture.cu (1)
92-116: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winimportant:
capture_modesruns"global"before"thread-local".device_default_memory_pool(dev)initializes each__default_device_memory_poolthrough__call_once, so the thread-local iteration uses the cached pool and does not exercise first-use initialization. A regression in that path can pass. Put"thread-local"before"global"incapture_modesto give the thread-local case fresh per-device initialization coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 243721ae-1980-4416-9d69-cfeaf5923339
📒 Files selected for processing (1)
libcudacxx/test/libcudacxx/cuda/memory_resource/resources/default_pool_under_capture.cu
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
|
/ok to test 9659c04 |
Co-authored-by: Jacob Faibussowitsch <[email protected]>
…nd use it in the destructor instead of try/catch (review: Jacobfaib); throwing wrapper now goes through _CCCL_TRY_DRIVER_API like the other NoThrow pairs Co-Authored-By: Claude Fable 5.1 <[email protected]>
…cobfaib) — no docstrings on driver wrappers, one-line notes elsewhere Co-Authored-By: Claude Fable 5.1 <[email protected]>
Co-Authored-By: Claude Fable 5.1 <[email protected]>
|
/ok to test 0394e24 |
|
/ok to test 6eb6960 |
This comment has been minimized.
This comment has been minimized.
clang-tidy's modernize-use-default-member-init (warnings-as-errors in the tidy CI job) rejects initializing __previous_ from a constant in the constructor's mem-initializer list. Move the initializer onto the member declaration; behavior is unchanged. Co-Authored-By: Claude Fable 5.1 <[email protected]>
|
/ok to test 17912ac |
This comment has been minimized.
This comment has been minimized.
…ectness)
clang-tidy (misc-const-correctness, WarningsAsErrors) flagged the two
`cuda::stream stream{dev};` locals in default_pool_under_capture.cu: the
streams are only ever read through the const `get()` accessor and passed
to `allocate`/`deallocate` by value as a `stream_ref`, so they can be
`const`, matching how the sibling memory_resource tests declare them.
Co-Authored-By: Claude Fable 5.1 <[email protected]>
|
/ok to test 67b98f8 |
This comment has been minimized.
This comment has been minimized.
🥳 CI Workflow Results🟩 Finished in 6h 13m: Pass: 100%/221 | Total: 1d 17h | Max: 52m 44s | Hits: 96%/558220See results here. AI failure analysis1. cuVS IVF-RaBitQ cannot resolve rmm::cuda_stream_view · 2 jobsExplanation: Both CUDA 13.3 jobs first fail in the same cuVS IVF-RaBitQ headers, while the preceding RMM build confirms that `cuda_stream_view` still exists but is deprecated. This indicates missing or conditional header visibility in those cuVS translation units; the exact trigger is uncertain because the logs contain no preprocessor/include trace, and the PR diff does not modify RMM or cuVS sources. Evidence: Copy this prompt into a coding agentJobs: |
…pool capture fix) After merging upstream main: * MGMN concepts and algorithms now live under `cuda::experimental::mgmn` (NVIDIA#11276): requalify the bridge and its tests accordingly. * Drop the local locality-domain workaround for capture-time allocation: `cuda::__get_default_memory_pool` is now legal under stream capture (NVIDIA#11360), so the data place's allocate() is back to the upstream form. Co-Authored-By: Claude Fable 5.1 <[email protected]>
Description
cuda::__get_default_memory_poolapplies the library's retention policy to the driver's default pool by reading (and, the first time, writing) the release-threshold attribute.cuMemPoolGetAttribute/cuMemPoolSetAttributeare "potentially unsafe" driver calls: while the calling thread is inside a global or thread-local stream capture they fail withCUDA_ERROR_STREAM_CAPTURE_UNSUPPORTEDand invalidate the capture. The default-pool lookup itself (cuMemGetDefaultMemPool, device attribute queries) is a pure query and is legal under capture; only the policy step is not.So any lazy first touch of a default pool under an active capture threw:
cuda::device_default_memory_pool()of a process,operation not permitted when stream is capturing(900): Failed to get attribute for a memory pool).Fix
Run the policy step inside a new
__relaxed_capture_scope: an RAII scope aroundcuThreadExchangeStreamCaptureModethat puts the calling thread in relaxed capture mode and restores the previous mode on exit.Adds
__driver::__threadExchangeStreamCaptureMode.Test
libcudacxx/test/libcudacxx/cuda/memory_resource/resources/default_pool_under_capture.cubegins a capture in global and in thread-local mode, resolves the device default pool through the public once-only accessor (deliberately the first touch of the process) and through the helper, allocates from the pool inside the capture, and checks the capture ends successfully with at least one node and that the policy was applied. A final section proves the thread's mode is restored: an unsafe pool query right after the accessor is refused again. Driver entry points only, as the ccclrt fixture requires.Fails before the fix (unexpected
cuda_errorexception), passes after.memory_pools,locality_domain_memory_poolanddevice_memory_resourcetests unchanged. Verified on GB300, CTK 13.4.Not in this PR
cudax's
locality_domain_data_place_impl::allocatestill calls the accessor per allocation (~+80 ns over a cached handle); caching the handle is a separate cudax follow-up. This PR makes the lazy path correct wherever it happens.Checklist
🤖 Generated with Claude Code