Skip to content

fix: identify GPU by PCI bus id when probing hw decompression - #176

Open
aminaramoon wants to merge 3 commits into
NVIDIA:mainfrom
aminaramoon:no_rmm_in_discovery
Open

fix: identify GPU by PCI bus id when probing hw decompression#176
aminaramoon wants to merge 3 commits into
NVIDIA:mainfrom
aminaramoon:no_rmm_in_discovery

Conversation

@aminaramoon

Copy link
Copy Markdown
Contributor

Problem

query_hw_decompression took a device ordinal and resolved it with rmm::cuda_set_device_raii:

rmm::cuda_set_device_raii set_device{rmm::cuda_device_id{static_cast<int>(cuda_ordinal)}};
return rmm::detail::hwdecompress::is_supported();

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. The value passed in was gpu.id — a position in this discovery's own CUDA_VISIBLE_DEVICES-filtered list — which need not name the same device to CUDA on a heterogeneous host (mixed SKUs, or a display GPU alongside compute GPUs).

The mismatch was latent because rmm::detail::hwdecompress::is_supported() only calls cudaDriverGetVersion. It answers "is the driver >= 12.8" and never asks a per-device question, so the ordinal selected nothing and the set_device was decoration. Any real per-device query would have made the wrong ordinal load-bearing.

Change

Query CU_DEVICE_ATTRIBUTE_MEM_DECOMPRESS_ALGORITHM_MASK against the device resolved by cuDeviceGetByPCIBusId:

bool query_hw_decompression(std::string const& pci_bus_id)
{
  auto const& api = load_cuda_driver_api();
  if (!api.available || pci_bus_id.empty()) { return false; }

  int device = 0;
  if (api.device_get_by_pci_bus_id(&device, pci_bus_id.c_str()) != cuda_driver_success) { return false; }

  int algorithm_mask = 0;
  if (api.device_get_attribute(&algorithm_mask,
                               cu_device_attribute_mem_decompress_algorithm_mask,
                               device) != cuda_driver_success) { return false; }
  return algorithm_mask != 0;
}

This is immune to both the FASTEST_FIRST reordering and CUDA_VISIBLE_DEVICES remapping, takes its device explicitly (no context created, calling thread's current device untouched), and reports actual silicon capability rather than a driver version. For MIG entries the bus id is the parent physical GPU's, which is the correct scope — the decompression engine is a property of the physical device.

Notes on the implementation:

  • The driver API is reached via dlopen("libcuda.so.1") + dlsym rather than a link dependency, so the library still loads on hosts without an NVIDIA driver, matching how NVML is already treated here.
  • CUdevice/CUresult are spelled as int to avoid pulling in <cuda.h>; both match the driver ABI.
  • Attribute 136 is written literally with a comment so the file still builds against pre-12.8 toolkit headers.
  • The runtime API is not an option: this CUDA version exposes no cudaDevAttrMemDecompress* equivalent, so the driver API is the only route.

This removes the only RMM use in topology_discovery.cpp (its sole source file), so rmm::rmm is dropped from the three topology targets and ${CMAKE_DL_LIBS} added. 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. The old code answered a different question. The field has no in-tree readers as of this branch, but downstream consumers relying on the permissive answer should be checked.

Verification

On a 2x RTX 6000 Ada host:

  • Full release build clean under -Wall -Wextra -Wpedantic -Wconversion with warnings-as-errors; CUCASCADE_TOPOLOGY_ONLY=ON builds clean too.
  • cucascade_topology_discovery_tests: 47 assertions in 6 test cases, all pass.
  • readelf -d on libcucascade_topology_discovery.so: libdl, libstdc++, libgcc_s, libc — no libcuda, no rmm.
  • pre-commit (clang-format, codespell, cmake-format, cmake-lint) passes.
  • Live probe including CUDA_VISIBLE_DEVICES=1 and UUID-form masking: id renumbers to 0 but pci stays 03:00.0 and resolves to the correct physical device.
  • Both GPUs now report hw_decomp=0. Confirmed a true negative rather than a swallowed error via a direct driver probe: attr136_rc=0 mask=0. Ada has no decompression engine; that is Blackwell hardware.

🤖 Generated with Claude Code

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>
@copy-pr-bot

copy-pr-bot Bot commented Jul 31, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Comment thread src/memory/topology_discovery.cpp Outdated
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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only necessary if you want to be able to compile without cuda.h. Is that required?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/memory/topology_discovery.cpp Outdated
aminaramoon and others added 2 commits July 31, 2026 08:33
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>
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.

2 participants