fix: identify GPU by PCI bus id when probing hw decompression - #176
Merged
Merged
Conversation
wence-
reviewed
Jul 31, 2026
Comment on lines
+67
to
+69
| * `CUdevice` and `CUresult` are spelled as `int` to avoid pulling in `<cuda.h>`: | ||
| * `CUdevice` is a typedef for `int` and `CUresult` is an int-sized enum, so both | ||
| * match the driver ABI. |
There was a problem hiding this comment.
This is only necessary if you want to be able to compile without cuda.h. Is that required?
Contributor
Author
There was a problem hiding this comment.
I guess topology discovery could, but you do have a point as cucascade has cuda as a dependency. so technicallyt it will never happen. I can change that.
aminaramoon
added a commit
to aminaramoon/cucs
that referenced
this pull request
Jul 31, 2026
Address review feedback on NVIDIA#176. Spell the driver entry points with CUdevice/CUresult/CUdevice_attribute and use CU_DEVICE_ATTRIBUTE_MEM_DECOMPRESS_ALGORITHM_MASK directly rather than hand-rolled int signatures and a literal 136. Including <cuda.h> costs nothing here: the toolkit include path already comes in via CUDA::nvml_static, the project requires CUDA 12.9+ so the 12.8 enumerator is always present, and the header adds no link dependency -- the .so still has no DT_NEEDED on libcuda.so.1. The dlopen indirection stays, since that is what keeps the library loadable on driverless hosts; only the type spelling changes. Resolve symbols by clearing dlerror() and inspecting it afterwards. A null return from dlsym is not by itself an error, so the previous null-check was the wrong test. This also drops the memcpy: a plain reinterpret_cast compiles clean under the project's full warning set including -Wpedantic -Werror, so the workaround was unnecessary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aminaramoon
requested review from
felipeblazing,
pentschev and
wence-
and removed request for
wence-
September 14, 2026 16:49
wence-
approved these changes
Sep 15, 2026
| @@ -0,0 +1 @@ | |||
| clangd/ No newline at end of file | |||
query_hw_decompression took a device ordinal and resolved it with
rmm::cuda_set_device_raii. Ordinals are not a stable identity across
APIs: NVML enumerates in PCI-bus order while the CUDA runtime defaults
to CUDA_DEVICE_ORDER=FASTEST_FIRST, so gpu.id -- a position in this
discovery's own CUDA_VISIBLE_DEVICES-filtered list -- need not name the
same device to CUDA on a heterogeneous host.
The mismatch was latent because rmm::detail::hwdecompress::is_supported()
only calls cudaDriverGetVersion; it answers "is the driver >= 12.8",
never a per-device question, so the ordinal selected nothing.
Query CU_DEVICE_ATTRIBUTE_MEM_DECOMPRESS_ALGORITHM_MASK against the
device resolved by cuDeviceGetByPCIBusId instead. That is immune to both
the FASTEST_FIRST reordering and CUDA_VISIBLE_DEVICES remapping, takes
its device explicitly (no context created, current device untouched),
and reports actual silicon capability rather than a driver version.
The driver API is reached via dlopen("libcuda.so.1") + dlsym rather than
a link dependency, so the library still loads on driverless hosts --
matching the treatment of NVML -- and CUdevice/CUresult are spelled as
int to avoid pulling in <cuda.h>. The runtime API is not an option here:
this CUDA version exposes no cudaDevAttrMemDecompress* equivalent.
This removes the only RMM use in topology_discovery.cpp, so drop
rmm::rmm from the three topology targets. Side effect:
CUCASCADE_TOPOLOGY_ONLY=ON now configures and builds -- it never calls
find_package(rmm), so linking rmm::rmm had it failing at generate time.
Behavior change: hw_decompression_available now reports false on
pre-Blackwell GPUs that previously reported true on any >= 12.8 driver.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Address review feedback on NVIDIA#176. Spell the driver entry points with CUdevice/CUresult/CUdevice_attribute and use CU_DEVICE_ATTRIBUTE_MEM_DECOMPRESS_ALGORITHM_MASK directly rather than hand-rolled int signatures and a literal 136. Including <cuda.h> costs nothing here: the toolkit include path already comes in via CUDA::nvml_static, the project requires CUDA 12.9+ so the 12.8 enumerator is always present, and the header adds no link dependency -- the .so still has no DT_NEEDED on libcuda.so.1. The dlopen indirection stays, since that is what keeps the library loadable on driverless hosts; only the type spelling changes. Resolve symbols by clearing dlerror() and inspecting it afterwards. A null return from dlsym is not by itself an error, so the previous null-check was the wrong test. This also drops the memcpy: a plain reinterpret_cast compiles clean under the project's full warning set including -Wpedantic -Werror, so the workaround was unnecessary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Remove 19 comments in topology_discovery.cpp that narrate the line
below them without adding context ("// Get GPU count" above a
GetCount call, "// Convert to lowercase" above a tolower loop, and
similar).
Comments carrying information the code cannot express are kept: the
NVML re-init SEGV explanation, the MIG parent/instance rationale, the
NVML-vs-sysfs PCI bus id format mismatch, the path-type proximity
heuristic, and the /sys state file format.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drop the internal cuInit(0) call from discover_runtime_attributes and document the precondition instead. Callers of discover_runtime_attributes (directly or via discover(with_runtime_attributes=true)) are expected to have already initialized the CUDA driver API, either explicitly via cuInit(0) or transitively via a prior CUDA runtime call. This function still does not create a CUDA context. Per-GPU queries that fail because the driver is uninitialized silently yield hw_decomp=false, consistent with the existing best-effort behavior of query_hw_decompression.
aminaramoon
force-pushed
the
no_rmm_in_discovery
branch
from
September 15, 2026 17:32
6139dee to
988048f
Compare
Member
|
/ok to test 988048f |
pentschev
approved these changes
Sep 15, 2026
pentschev
left a comment
pentschev
left a comment
Contributor
There was a problem hiding this comment.
Approving based on verification of CUDA initialization, I have been able to confirm that no distortion of CUDA_VISIBLE_DEVICES occur due to the early CUDA context initialization, which fixes the issue we had with RapidsMPF's use of topology discovery and allows us to resume using cuCascade's main branch during development.
Thanks @aminaramoon .
Co-authored-by: Peter Andreas Entschev <peter@entschev.com>
Contributor
Author
|
/merge |
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
4 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.
Description