Skip to content

gin: make per-peer request ring size env-tunable for high-QP AWS EFA#1206

Closed
dmvevents wants to merge 1 commit into
aws:masterfrom
dmvevents:gin-tunable-ring-size
Closed

gin: make per-peer request ring size env-tunable for high-QP AWS EFA#1206
dmvevents wants to merge 1 commit into
aws:masterfrom
dmvevents:gin-tunable-ring-size

Conversation

@dmvevents

@dmvevents dmvevents commented Apr 24, 2026

Copy link
Copy Markdown

Summary

Makes the GIN plugin's per-peer request ring size runtime-configurable via a
new env var OFI_NCCL_GIN_MAX_REQUESTS, defaulting to the current compile-time
value of 128.

Motivation

The GIN plugin tracks outstanding putSignal requests in a per-peer ring whose
slot count is hard-coded to NCCL_OFI_MAX_REQUESTS = 128 (see
include/rdma/gin/nccl_ofi_gin.h:111 and the four % NCCL_OFI_MAX_REQUESTS
usages in src/rdma/gin/nccl_ofi_gin.cpp + include/rdma/gin/nccl_ofi_gin.h).
Applications that issue more than 128 concurrent putSignal operations to the
same peer before an ACK returns trip assert(false) at
src/rdma/gin/nccl_ofi_gin.cpp:427 with NCCL_OFI_WARN("Next sequence number is in use"). That error surfaces to callers as a plugin failure.

Concrete reproducer (external project)

DeepEP V2 (epv2-release branch) hybrid-mode dispatch on 2-node p5en.48xlarge
H200 EFA. V2's auto-QP formula num_sms * 16 + 1 yields 129 QPs for even small
num_sms values, overrunning the ring on the first dispatch.

With the cap bumped to 512 compile-time (this PR makes it env-tunable to 512
runtime), the DeepEP V2 hybrid-mode test at 128 tokens / 256 experts / topk=8
on 16 ranks (8 procs/rank x 2 pods) goes from crash to:

Metric Before (128 cap) After (512 cap)
Outcome CUDA_ERROR_LAUNCH_FAILED at first dispatch test passes 144/144 configs
D+C p50 mean crash 615us (52-60% faster than the V1 NVSHMEM baseline)
EFA TX per 100-iter run 0 6.13 GB across 32 NICs (verified via /sys/class/infiniband/rdmap*/ports/1/hw_counters/tx_bytes delta)
num_qps that now works <= 4 only up to 12 with headroom

This PR and a companion PR at deepseek-ai/DeepEP (makes V2 EFA-aware) are
needed together.

Change

  1. New parameter OFI_NCCL_GIN_MAX_REQUESTS (size_t, default 128) declared in
    include/nccl_ofi_param.h.
  2. nccl_ofi_gin_peer_rank_info::active_put_signal changes from
    bool[NCCL_OFI_MAX_REQUESTS] to std::vector<uint8_t>, sized once per comm
    at connect time.
  3. The four % NCCL_OFI_MAX_REQUESTS mod-ring expressions now use a new
    gin_max_requests_ member on nccl_ofi_rdma_gin_put_comm.
  4. Input validation: value is rounded down to the nearest power of two and
    clamped to GIN_IMM_SEQ_MASK + 1 - 2 * GIN_ACK_INTERVAL so that merged-ACK
    ranges always fit inside the seq-num window. Invalid inputs fall back to
    the default with a warning.
  5. The Next sequence number is in use warn now prints the effective
    gin_max_requests_ and tells the user which env var to raise.

NCCL_OFI_MAX_REQUESTS is unchanged — the sendrecv and rdma transports still
cap at 128, preserving backward compatibility on existing call paths.

Default preserved

Default is 128 — exactly the existing compile-time value. Users on CX7 or
small-scale jobs see zero behavior change.

Memory cost

Per-peer, per-comm: 1 byte per slot. At the default of 128, 128 bytes per peer
(same as before: bool + packing was 128 bytes). At the max (1024), 1 KiB per
peer. Negligible at the scales where this is needed.

Testing

  • Built locally on p5en.48xlarge against libfabric 2.3.1, CUDA 12.8, NCCL
    2.30.4 via ./autogen.sh && ./configure --with-libfabric=/opt/amazon/efa --with-cuda=/usr/local/cuda && make -j. Build clean.
  • libnccl-net-ofi.so still exports ncclGinPlugin_v13 (verified via
    nm -D).
  • Runtime validation on 2-node p5en.48xlarge with OFI_NCCL_GIN_MAX_REQUESTS=512:
    DeepEP V2 hybrid-mode test test_ep.py --num-processes 8 --num-tokens 128 --num-experts 256 --num-topk 8 --num-sms 3 --num-qps 12 --num-allocated-qps 12 EP_EFA_MAX_QPS=12 completes cleanly, D+C p50 615us, 6.13 GB real RDMA
    verified via EFA HW counter delta.

Why not increase the compile-time constant?

Because the CX7 fleet and most existing deployments stay well under 128
outstanding putSignals per peer, and larger fabrics want the knob only when
needed. A runtime param keeps small-scale jobs lean and surfaces the tuning
choice explicitly.

DCO

Signed off.

…QUESTS)

The GIN plugin tracks outstanding putSignal requests per peer in a
fixed-size ring whose slot count has been hard-coded to
NCCL_OFI_MAX_REQUESTS (128). Applications that issue more than 128
concurrent putSignal operations to the same peer before an ACK is
returned trip the 'Next sequence number is in use' assert and surface
to the caller (CUDA launch, NCCL collective, etc.) as a plugin error.

DeepEP V2 on AWS EFA with high QP counts (QP >= 5 on p5en.48xlarge
H200) reproduces this reliably: the hybrid-mode auto-QP formula
yields 129 QPs and the first dispatch crashes.

Add a runtime parameter OFI_NCCL_GIN_MAX_REQUESTS (default 128,
preserving existing behavior) that sizes the per-peer ring. The
previously-fixed 'bool active_put_signal[NCCL_OFI_MAX_REQUESTS]' array
becomes a std::vector<uint8_t> sized once during comm construction.
The value is rounded down to the nearest power of two and clamped to
the GIN_IMM_SEQ_MASK window minus 2 * GIN_ACK_INTERVAL of ACK
headroom.

Per-peer memory growth at the cap (1024) is 1 KiB, negligible for
the scales where this is needed.

Signed-off-by: Anton Alexander <dmvevents@users.noreply.github.com>
@dmvevents

Copy link
Copy Markdown
Author

Superseded by upstream commit 6e504db ("gin: Size active_put_signal to full sequence number space"), which landed on master 2026-04-24. That fix sizes the bitset to the full sequence-number space and eliminates the aliasing class entirely — strictly better than this PR's env-tunable ring approach (no knob, collision-free by construction). Closing.

Post-mortem on why we missed the better design and follow-up CUCo/LogLearner knowledge-base improvements: https://gist.github.com/dmvevents/9c6ce80affd9938332d149f8c32da0df

@dmvevents dmvevents closed this Apr 28, 2026
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.

1 participant