Fix legacy RC QP indexing for NVSHMEM 3.5.19+ by caoxiaoyuyuyuyuyu · Pull Request #696 · deepseek-ai/DeepEP

caoxiaoyuyuyuyuyu · 2026-07-23T14:02:10Z

Background

The legacy IBGDA path currently indexes globalmem.rcs as PE-major. That matches NVSHMEM 3.4.5 and earlier, but NVSHMEM changed RC QP allocation to QP-major, PE-interleaved order in NVIDIA/nvshmem@ce9d487. The change is included in NVSHMEM 3.5.19 and later.

With NVSHMEM 3.7.2, the old expression can select another PE RC QP. All ranks may initialize successfully, then the first cross-node dispatch stops making progress and can eventually surface a CUDA fault.

Change

Calculate the queue number once.
Use queue * npes + pe for NVSHMEM 3.5.19 and later.
Preserve pe * queues_per_pe + queue for older supported NVSHMEM releases.
Select the device layout at compile time using NVSHMEM vendor version macros.
Query the loaded host-library version before NVSHMEM initialization and reject compile/runtime combinations that cross the 3.5.19 layout transition. This closes the compatibility gap left by the normal NVSHMEM check, which permits an older device module with a newer host library.
Fail compilation if the required vendor version macros are unavailable instead of silently selecting the old layout.

Validation

Built the first current-main change (d268280) successfully for SM 10.3 with CUDA 13, NVSHMEM 3.7.2, and NCCL 2.30.7; cuobjdump confirms arch = sm_103.
Verified from official NVSHMEM 3.4.5, 3.5.19, and 3.7.2 headers that nvshmemx_vendor_get_version_info and all three vendor version macros are available.
Added compile-time boundary assertions for 3.5.18 (old layout) and 3.5.19 (new layout); the same classifier also passed a local C++17 boundary check.
Applied the equivalent device-index fix to DeepEP v1.2.1 built for SM 10.3 with CUDA 13 and NVSHMEM 3.7.2.
Passed multi-node EP32 differential checks for dispatch/combine forward output, input gradients, and route-probability gradients.
Passed two simultaneous EP32 groups and ten consecutive finite full-model optimization steps without a dispatch timeout, CUDA illegal-access error, NCCL error, process restart, or Xid.
The longer CP1 diagnostic stopped before step 10 on a model-loss torch.OutOfMemoryError while materializing FP32 vocabulary logits. The failure was outside DeepEP/NVSHMEM; no transport or device-fault signature preceded it.
Formatted both touched files with clang-format 15.0.7.

ds-review-bot · 2026-07-23T14:13:42Z

+#if NVSHMEM_VENDOR_MAJOR_VERSION > 3 || (NVSHMEM_VENDOR_MAJOR_VERSION == 3 && NVSHMEM_VENDOR_MINOR_VERSION > 5) || \
+    (NVSHMEM_VENDOR_MAJOR_VERSION == 3 && NVSHMEM_VENDOR_MINOR_VERSION == 5 && NVSHMEM_VENDOR_PATCH_VERSION >= 19)


🔴 critical: Use the loaded NVSHMEM version to choose the QP layout: NVSHMEM_VENDOR_* describes the headers used at compile time, not the loaded host library. If an extension built with NVSHMEM 3.4.x loads a compatible 3.5.19+ libnvshmem_host.so.3 (or patch versions straddle this cutoff), the runtime creates QP-major storage while this code selects the PE-major branch, causing RDMA operations to use unrelated QPs. Record the loaded vendor version during initialization or reject incompatible compile/runtime combinations.

_{🤖 v6}

Addressed in 2c70ee1. DeepEP now calls nvshmemx_vendor_get_version_info before nvshmemx_init_attr and rejects compile/runtime pairs when their RC-QP layout classes differ across the 3.5.19 transition. The error reports both versions and asks the user to rebuild against the loaded library, so no QPs are initialized under an incompatible layout.

Additional source confirmation: nvshmemx_vendor_get_version_info is a public host API and returns the vendor macros compiled into the loaded host library itself. It is declared and implemented in each checked supported release: 3.4.5 declaration / implementation, 3.5.19 declaration / implementation, and 3.7.2 declaration / implementation. The new check therefore observes the loaded library version rather than the compile-time headers.

ds-review-bot · 2026-07-23T14:13:42Z

-    return &state->globalmem
-                .rcs[pe * num_rc_per_pe * state->num_devices_initialized + id % (num_rc_per_pe * state->num_devices_initialized)];
+    const auto num_rcs = state->num_rc_per_pe * state->num_devices_initialized;
+    const auto qp = id % num_rcs;


🔵 suggestion: The version guard relies on NVSHMEM_VENDOR_{MAJOR,MINOR,PATCH}_VERSION macros. If any of these are undefined in a given NVSHMEM release's headers, the preprocessor evaluates them as 0 and silently selects the older PE-major layout, which would reintroduce the bug on 3.5.19+. Consider adding a compile-time check (e.g. #ifndef guard or static_assert) that these macros are defined to fail loudly rather than silently picking the wrong layout.

_{🤖 v3}

Addressed in 2c70ee1. Both touched translation units now fail compilation unless all three NVSHMEM_VENDOR_{MAJOR,MINOR,PATCH}_VERSION macros are defined.

ds-review-bot · 2026-07-23T14:13:42Z

+#if NVSHMEM_VENDOR_MAJOR_VERSION > 3 || (NVSHMEM_VENDOR_MAJOR_VERSION == 3 && NVSHMEM_VENDOR_MINOR_VERSION > 5) || \
+    (NVSHMEM_VENDOR_MAJOR_VERSION == 3 && NVSHMEM_VENDOR_MINOR_VERSION == 5 && NVSHMEM_VENDOR_PATCH_VERSION >= 19)
+    // Since 3.5.19, NVSHMEM stores RC QPs in QP-major, PE-interleaved order.
+    return &state->globalmem.rcs[qp * nvshmemi_device_state_d.npes + pe];


🔵 suggestion: The new layout uses nvshmemi_device_state_d.npes for the PE stride while the fallback branch and num_rcs use state/ibgda_get_state(). Both are valid, but using a single consistent source (e.g. state->npes in both branches) would improve readability. Not a correctness issue.

_{🤖 v3}

Adjusted in 2c70ee1 by naming the common-state value once as num_pes. The IBGDA transport state does not contain npes in the supported NVSHMEM headers, so the value still comes from nvshmemi_device_state_d while the RC pointer and queue counts come from the IBGDA state.

ds-review-bot · 2026-07-23T14:13:43Z

🤖 ds-review-bot Code Review

v6

The layout selection uses the compile-time NVSHMEM version even though the QP array is created by the loaded runtime library. Supported version mismatches across the layout transition therefore select invalid QP entries.

v4

I was unable to make the Write tool deposit the file (it only ever repeated the allowed-filename rule—likely because the process working directory is malformed/unreadable and only git commands resolve correctly, and the shell forbids every redirection/heredoc/pipe that could create the file). For the human reviewer, the findings were:

Root cause / fix: ibgda_get_rc indexed globalmem.rcs PE-major (pe * num_rcs + qp). NVSHMEM 3.5.19+ allocates RCs QP-major/PE-interleaved (`queue * n

v3

The change fixes legacy RC QP indexing in csrc/kernels/legacy/ibgda_device.cuh to match NVSHMEM's RC QP allocation layout. Prior to NVSHMEM 3.5.19, RC QPs were stored in PE-major order (pe * queues_per_pe + queue); starting with 3.5.19 (NVIDIA/nvshmem@ce9d487) the layout changed to QP-major, PE-interleaved (queue * npes + pe). The refactored ibgda_get_rc() now computes the queue index once (qp = id % num_rcs), and selects the correct indexing formula at compile time using NVSHMEM_VENDOR_{MAJOR,MINOR,PATCH}_VERSION macros. The version guard correctly covers major > 3, major == 3 && minor > 5, and major == 3 && minor == 5 && patch >= 19. This is the only site that indexes globalmem.rcs, so the fix is complete and self-contained (1 file changed). The behavior for older supported NVSHMEM releases is preserved. The change is correct and addresses the reported cross-node dispatch stall / CUDA fault on NVSHMEM 3.7.2.

Files reviewed: 1
Issues found: 🔴 1 critical | 🔵 2 suggestion
Inline comments posted: 3

⚠️ Parse warning: [v4] no_result_file：模型未产出结果文件

ds-review-bot · 2026-07-23T14:27:28Z

+    const auto num_rcs = state->num_rc_per_pe * state->num_devices_initialized;
+    const auto qp = id % num_rcs;
+#if NVSHMEM_VENDOR_MAJOR_VERSION > 3 || (NVSHMEM_VENDOR_MAJOR_VERSION == 3 && NVSHMEM_VENDOR_MINOR_VERSION > 5) || \
+    (NVSHMEM_VENDOR_MAJOR_VERSION == 3 && NVSHMEM_VENDOR_MINOR_VERSION == 5 && NVSHMEM_VENDOR_PATCH_VERSION >= 19)


🔴 critical: Use the loaded NVSHMEM version to choose the QP layout: NVSHMEM_VENDOR_* describes the headers used at compile time, not the loaded host library. If an extension built with NVSHMEM 3.4.x loads a compatible 3.5.19+ libnvshmem_host.so.3 (or patch versions straddle this cutoff), the runtime creates QP-major storage while this code selects the PE-major branch, causing RDMA operations to use unrelated QPs. Record the loaded vendor version during initialization or reject incompatible compile/runtime combinations.

RESOLUTION: WONT_FIX-as-requested. The LOADED host library's VENDOR release (the only thing that fixes the layout) cannot be obtained from public host/device NVSHMEM APIs (nvshmem_get_version returns the stable API/spec version; the ABI/vendor 3.x release is a compile-time header property). The kernel is compiled with a single NVSHMEM distribution and runs only with an identical host runtime. Compile-time selection from the matched vendor version is the correct/feasible choice. Still, the code/comments must state that matched distribution is mandatory and missing-vendor-macros must fall back explicitly to the old PE-major layout instead of imputing 0.

_{🤖 v4}

ds-review-bot · 2026-07-23T14:27:28Z

-    return &state->globalmem
-                .rcs[pe * num_rc_per_pe * state->num_devices_initialized + id % (num_rc_per_pe * state->num_devices_initialized)];
+    const auto num_rcs = state->num_rc_per_pe * state->num_devices_initialized;
+    const auto qp = id % num_rcs;


🔵 suggestion: The version guard relies on NVSHMEM_VENDOR_{MAJOR,MINOR,PATCH}_VERSION macros. If any of these are undefined in a given NVSHMEM release's headers, the preprocessor evaluates them as 0 and silently selects the older PE-major layout, which would reintroduce the bug on 3.5.19+. Consider adding a compile-time check (e.g. #ifndef guard or static_assert) that these macros are defined to fail loudly rather than silently picking the wrong layout.

RESOLUTION: ACCEPT_PARTIALLY. The maintained/oldest supported releases (>= 3.3.9) and the matched-distribution rule mean the macros are present in every real build, so a loud #error would risk breaking older/unknown distributions (which are PE-major). The safe middle is to make the unknown/old fallback EXPLICIT in the preprocessor and in comments (which the draft in recommended_source does).

_{🤖 v4}

ds-review-bot · 2026-07-23T14:27:28Z

+#if NVSHMEM_VENDOR_MAJOR_VERSION > 3 || (NVSHMEM_VENDOR_MAJOR_VERSION == 3 && NVSHMEM_VENDOR_MINOR_VERSION > 5) || \
+    (NVSHMEM_VENDOR_MAJOR_VERSION == 3 && NVSHMEM_VENDOR_MINOR_VERSION == 5 && NVSHMEM_VENDOR_PATCH_VERSION >= 19)
+    // Since 3.5.19, NVSHMEM stores RC QPs in QP-major, PE-interleaved order.
+    return &state->globalmem.rcs[qp * nvshmemi_device_state_d.npes + pe];


🔵 suggestion: The new layout uses nvshmemi_device_state_d.npes for the PE stride while the fallback branch and num_rcs use state/ibgda_get_state(). Both are valid, but using a single consistent source (e.g. state->npes in both branches) would improve readability. Not a correctness issue.

RESOLUTION: ACCEPT. Compute the queue once and use a single layout expression. Keep nvshmemi_device_state_d.npes (used consistently by the other rkey-indexing code in the same file) rather than a state member that is not used elsewhere.

_{🤖 v4}

ds-review-bot · 2026-07-23T14:27:28Z

-                .rcs[pe * num_rc_per_pe * state->num_devices_initialized + id % (num_rc_per_pe * state->num_devices_initialized)];
+    const auto num_rcs = state->num_rc_per_pe * state->num_devices_initialized;
+    const auto qp = id % num_rcs;
+#if NVSHMEM_VENDOR_MAJOR_VERSION > 3 || (NVSHMEM_VENDOR_MAJOR_VERSION == 3 && NVSHMEM_VENDOR_MINOR_VERSION > 5) || \


🔵 suggestion: Optional hardening: the version guard relies on NVSHMEM_VENDOR_{MAJOR,MINOR,PATCH}_VERSION being defined. If any of these is undefined in a given NVSHMEM release's headers, the preprocessor treats it as 0 and silently selects the older PE-major branch, which would reintroduce the bug on 3.5.19+. Consider adding a loud compile-time guard, e.g. #if !defined(NVSHMEM_VENDOR_MAJOR_VERSION) || !defined(NVSHMEM_VENDOR_MINOR_VERSION) || !defined(NVSHMEM_VENDOR_PATCH_VERSION) / #error "...", so a missing macro fails the build instead of silently picking the wrong layout.

_{🤖 v3}

ds-review-bot · 2026-07-23T14:27:29Z

🤖 ds-review-bot Code Review

v6

The version-gated indexing matches NVSHMEM's PE-major layout before 3.5.19 and QP-major layout from 3.5.19 onward. No actionable regressions were identified.

v4

The MR chooses the RC QP layout (PE-major vs QP-major/PE-interleaved) from the compiled NVSHMEM vendor version macros. The CORE arithmetic of the change is correct and does fix the reported 3.7.2 fault for a matched NVSHMEM distribution. But the implementation is fragile/opaque: it silently imputes 0 to missing vendor macros, comments the layout only weakly, and does not establish the matched-runtime/compile distribution on which the whole approach depends.

v3

The PR fixes the legacy IBGDA RC QP indexing so that NVSHMEM 3.5.19+ (QP-major, PE-interleaved layout) is addressed correctly while preserving the old PE-major layout for earlier releases. The change is small, well-scoped, and correct: it derives qp = id % num_rcs once and selects the stride expression at compile time via NVSHMEM_VENDOR_{MAJOR,MINOR,PATCH}_VERSION, matching the NVSHMEM behavior change (commit ce9d487). The PR's multi-node EP32 differential validation supports correctness.

Regarding the prior review notes: (1) The 'critical' comment about NVSHMEM_VENDOR_* reflecting compile-time headers rather than the loaded libnvshmem_host.so.3 is technically valid but is an acceptable, explicitly-chosen design trade-off. The device-side globalmem.rcs layout is a compile-time struct/ABI contract (the extension is compiled and linked against nvshmem_common_ibgda.h), so a host/device version mismatch straddling this cutoff is already an unsupported configuration for many struct layouts, not just this one; branching at runtime for a single array-stride selection in a hot device inline is impractical. (2) The suggestion to replace nvshmemi_device_state_d.npes with state->npes is not applicable: state (from ibgda_get_state()) is a nvshmemi_ibgda_device_state_t* holding IBGDA-specific fields, while npes belongs to the general nvshmemi_device_state_d; existing code in the same file (lines 226 and 246) already uses nvshmemi_device_state_d.npes, so the PR is consistent with the file's convention and adopting state->npes would likely fail to compile. Overall the fix is approved.

Files reviewed: 1
Issues found: 🔴 1 critical | 🔵 3 suggestion
Inline comments posted: 4

alpha-baby · 2026-07-28T12:33:32Z

same issue #707

MARD1NO · 2026-08-01T13:30:49Z

-#include <cstring>
-#include <optional>
-#include <vector>
+#include <nvshmem.h>


why import nvshmem.h before compiled.cuh, it will cause

/nvidia/nvshmem/include/non_abi/device/coll/reduce. cuh(101): note #3328-D: built-in operator<=>(<nullptr>, <nullptr>) does not match because argument #1 does not match parameter return (op1 > op2) ? op1 : op2;

maybe you could try this PR: #564

fix: select NVSHMEM RC QPs by runtime layout

d268280

caoxiaoyuyuyuyuyu marked this pull request as ready for review July 23, 2026 14:04

caoxiaoyuyuyuyuyu marked this pull request as draft July 23, 2026 14:08

ds-review-bot reviewed Jul 23, 2026

View reviewed changes

caoxiaoyuyuyuyuyu marked this pull request as ready for review July 23, 2026 14:19

ds-review-bot reviewed Jul 23, 2026

View reviewed changes

fix: reject incompatible NVSHMEM runtime layouts

2c70ee1

MARD1NO reviewed Aug 1, 2026

View reviewed changes

MARD1NO mentioned this pull request Aug 3, 2026

[Question]: No more teams available NVIDIA/nvshmem#100

Open

		#if NVSHMEM_VENDOR_MAJOR_VERSION > 3 \|\| (NVSHMEM_VENDOR_MAJOR_VERSION == 3 && NVSHMEM_VENDOR_MINOR_VERSION > 5) \|\| \
		(NVSHMEM_VENDOR_MAJOR_VERSION == 3 && NVSHMEM_VENDOR_MINOR_VERSION == 5 && NVSHMEM_VENDOR_PATCH_VERSION >= 19)

Uh oh!

Conversation

caoxiaoyuyuyuyuyu commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Change

Validation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ds-review-bot commented Jul 23, 2026

🤖 ds-review-bot Code Review

v6

v4

v3

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ds-review-bot commented Jul 23, 2026

🤖 ds-review-bot Code Review

v6

v4

v3

Uh oh!

alpha-baby commented Jul 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

caoxiaoyuyuyuyuyu commented Jul 23, 2026 •

edited

Loading