[TransferEngine] Clamp RD-atomic QP depths to the device capability

[Dashener2]

Description

Fixes #4309. The RTR transition hard-coded max_dest_rd_atomic = 16 and the RTS transition hard-coded max_rd_atomic = 16. Some RNICs advertise fewer, so ibv_modify_qp() returns an error and the endpoint never reaches RTS.

Reuse the device-capability clamping already performed by updateGlobalConfig(): cache max_qp_init_rd_atom / max_qp_rd_atom in GlobalConfig, lower them to the values reported by ibv_query_device(), and apply them when building the QP attributes. This is the same pattern already used for max_wr, max_sge, max_cqe, etc.

Module

• Transfer Engine (mooncake-transfer-engine)

Type of Change

• Bug fix

How Has This Been Tested?

Test commands:

cmake -S . -B build -DCMAKE_BUILD_TYPE=Release -DWITH_STORE=OFF -DBUILD_BENCHMARK=OFF
cmake --build build --target rdma_atomic_cap_test -j16
./build/mooncake-transfer-engine/tests/rdma_atomic_cap_test
export MC_METADATA_SERVER=P2PHANDSHAKE
for t in transport_uint_test multi_transport_batch_test multi_transport_locality_test dmabuf_export_test endpoint_store_test shared_segment_test; do
  ./build/mooncake-transfer-engine/tests/$t
done

Test results:

• Unit tests pass
• Integration tests pass (not applicable)
• Manual testing done: rdma_atomic_cap_test is hardware-free and drives updateGlobalConfig() with a synthetic ibv_device_attr (device cap 4/8 clamps the config; a more capable device keeps the configured 8/12). On the test host all mlx5 HCAs advertise max_qp_rd_atom = 16, so runtime behavior is unchanged there and the sub-16 path is covered by the unit test instead. The regression sweep stays green.

Checklist

• I have performed a self-review of my own code
• I have formatted my code using ./scripts/code_format.sh (git-clang-format on changed lines is clean)
• I have run pre-commit on the files changed in this PR and all hooks pass
• I have updated the documentation (not applicable)
• I have added tests to prove my changes are effective
• For changes >500 LOC: I have filed an RFC issue (not applicable)

AI Assistance Disclosure

• AI tools were used: opencode drafted the change and the tests; the submitter reviewed every changed line and reran the build/tests.

[sususama]

I was looking at this exact code path after reading #4309, and found this PR already up — so switching to review instead of a competing patch.

Clamping to the device capability is the right fix, thanks — reusing updateGlobalConfig() for this is what I'd have done too.

One design question. GlobalConfig is process-wide and updateGlobalConfig() only ever lowers values, so on a heterogeneous host the weakest NIC caps every QP in the process: three mlx5 advertising 16 plus one RNIC advertising 4 would put all of them at 4, i.e. the healthy cards lose 4x outstanding-READ depth through no fault of their own. RdmaContext already carries per-NIC properties that RdmaEndPoint consumes (numLagPorts() at rdma_endpoint.cpp:1612, activeMTU() at :1528), so caching the two caps there next to the existing updateGlobalConfig(device_attr) call would avoid that coupling. If you'd rather keep it in GlobalConfig for symmetry with max_wr, maybe at least LOG(WARNING) when 16 gets lowered, so operators can see where the reduced depth came from.

Smaller second point: a device that doesn't support single-sided READ reports max_qp_rd_atom = 0. After clamping that turns into attr.max_dest_rd_atomic = 0 — the QP then reaches RTS successfully and every later READ fails at runtime, quieter and harder to debug than today's hard ibv_modify_qp() failure. A floor of max(1, ...) would keep that case loud.

Not blocking this PR either way — just something worth deciding before it becomes hard to change.

[Dashener2]

I was looking at this exact code path after reading #4309, and found this PR already up — so switching to review instead of a competing patch.

Clamping to the device capability is the right fix, thanks — reusing updateGlobalConfig() for this is what I'd have done too.

One design question. GlobalConfig is process-wide and updateGlobalConfig() only ever lowers values, so on a heterogeneous host the weakest NIC caps every QP in the process: three mlx5 advertising 16 plus one RNIC advertising 4 would put all of them at 4, i.e. the healthy cards lose 4x outstanding-READ depth through no fault of their own. RdmaContext already carries per-NIC properties that RdmaEndPoint consumes (numLagPorts() at rdma_endpoint.cpp:1612, activeMTU() at :1528), so caching the two caps there next to the existing updateGlobalConfig(device_attr) call would avoid that coupling. If you'd rather keep it in GlobalConfig for symmetry with max_wr, maybe at least LOG(WARNING) when 16 gets lowered, so operators can see where the reduced depth came from.

Smaller second point: a device that doesn't support single-sided READ reports max_qp_rd_atom = 0. After clamping that turns into attr.max_dest_rd_atomic = 0 — the QP then reaches RTS successfully and every later READ fails at runtime, quieter and harder to debug than today's hard ibv_modify_qp() failure. A floor of max(1, ...) would keep that case loud.

Not blocking this PR either way — just something worth deciding before it becomes hard to change.

@sususama Thanks for the detailed review — really helpful.

On (1): You're right that a process-wide cap means the weakest NIC
drags down every QP. I'll move the two caps into RdmaContext (next to
numLagPorts() / activeMTU()), so per-NIC isolation is preserved.

On (2): Good catch. I'll use max(1, ...) so a device reporting
max_qp_rd_atom = 0 fails loudly at ibv_modify_qp() instead of silently
failing every READ later.

I'll push an update shortly. Feedback welcome.

[he-yufeng]

Verified in the repo container (mc-dev:local): built from this head and ran rdma_atomic_cap_test, 3/3 pass.

Read the full diff line by line. The shape is right where it matters: both atomic depths are clamped (the RTS initiator depth at the handshake site and the RTR responder depth in doSetupConnection), the values come from the device attr already queried at context construction so there is no extra verbs call, per-NIC values keep a slow card from capping a fast one on heterogeneous hosts, and the floor-at-1 choice makes a device advertising 0 fail in modify_qp rather than mysteriously on every later READ. The clamp is capped at the historical 16, so capable NICs see no behavior change. The hardware-free test covers exactly the boundary that matters.

One note for reviewers, not a blocker: the fail-loud-on-zero case still cannot be exercised without an affected NIC, so the on-device confirmation from #4309's reporter remains the missing runtime datapoint. Everything else about the change is compile-level and covered by the new test.

Looks right to me.

[Dashener2]

Verified in the repo container (mc-dev:local): built from this head and ran rdma_atomic_cap_test, 3/3 pass.

Read the full diff line by line. The shape is right where it matters: both atomic depths are clamped (the RTS initiator depth at the handshake site and the RTR responder depth in doSetupConnection), the values come from the device attr already queried at context construction so there is no extra verbs call, per-NIC values keep a slow card from capping a fast one on heterogeneous hosts, and the floor-at-1 choice makes a device advertising 0 fail in modify_qp rather than mysteriously on every later READ. The clamp is capped at the historical 16, so capable NICs see no behavior change. The hardware-free test covers exactly the boundary that matters.

One note for reviewers, not a blocker: the fail-loud-on-zero case still cannot be exercised without an affected NIC, so the on-device confirmation from #4309's reporter remains the missing runtime datapoint. Everything else about the change is compile-level and covered by the new test.

Looks right to me.

Thanks for the thorough review and the container validation.

Agreed on the note — the fail-loud-on-zero path needs an affected NIC to
exercise; happy to coordinate with the #4309 reporter if they can run it
on real hardware.

Thanks again!

[alogfans]

LGTM