Repository navigation
[TENT] Derive per-NIC bandwidth from the negotiated port speed - #3644
Conversation
github-actions
Bot
added
documentation
he-yufeng
left a comment
he-yufeng
left a comment
There was a problem hiding this comment.
Read the full diff and checked the encodings against the kernel enums (ib_port_speed / ib_port_width in include/rdma/ib_verbs.h): both tables match, including the easy-to-miss IB_WIDTH_2X = 16, and handling the XDR overflow of the u8 active_speed via active_speed_ex is the right call. The premise also checks out on current main: bw_gbps is stamped kDefaultBwGbps in loadTopology and nothing ever overwrites it, so the EWMA clamp really is [5, 500] GB/s for every NIC today.
Two things I verified beyond the tables: setDeviceBandwidth re-seeds the EWMA, not just the stored rate, and the inert-slot path keeps the configured default without double-warning. Consolidating the show_links table also removes its guessy fallbacks (max(0, active_speed), lanes = 1).
I did not build or run the new test locally (no RDMA stack on this machine), so this stays a comment, but from a read-through this is a good fix.
There was a problem hiding this comment.
active_speed_ and active_width_ are sampled only once in openDevice(). The existing recovery path resumes the context on IBV_EVENT_PORT_ACTIVE, but it does not re-query the port or update DeviceSelector; IBV_EVENT_DEVICE_SPEED_CHANGE is not handled either.
If a link comes back at a different negotiated speed—for example, 400G falling back to 100G—the EWMA seed and clamp will remain based on the old speed. In particular, a large stale EWMA floor can reproduce the exact slow-link selection problem this PR is intended to fix.
Could we refresh the port attributes and call setDeviceBandwidth() on PORT_ACTIVE and DEVICE_SPEED_CHANGE?
There was a problem hiding this comment.
@alogfans Agreed, and the second half of the suggestion is stronger than I first read it: IBV_EVENT_DEVICE_SPEED_CHANGE (rdma-core ≥ 62 / Linux 7.0, currently raised by mlx5) is a device-level event that fires when a port's effective speed changes without a link flap — e.g. a VF over LAG losing a PF — which PORT_ACTIVE cannot observe.
Fixed in 0f271d5: RdmaContext::refreshPortAttributes() re-issues ibv_query_port on the open device (the speed/width decode is shared with openDevice() via recordPortSpeed()). Workers::refreshLinkSpeed() calls it and re-seeds through setDeviceBandwidth() only when linkSpeedGbps() actually changed, so a link returning at the same speed keeps what it learned. handleContextEvents calls it on PORT_ACTIVE — before the device is marked available again, so no worker scores it on the old rate — and on DEVICE_SPEED_CHANGE behind a CMake probe (HAVE_IBV_EVENT_DEVICE_SPEED_CHANGE, same pattern as HAVE_IBV_ACTIVE_SPEED_EX); it is an enumerator, so older rdma-core compiles without that branch. active_speed_/active_width_ and bw_gbps become atomic since the monitor thread now writes them under worker reads.
The selector re-seed is unit-tested (EWMA learned at 400G, then setDeviceBandwidth(25) resets it and a 3.1 GB/s link converges instead of sitting on the stale 5 GB/s floor), and refreshPortAttributes() on an inert context is rejected. The speed-change branch was syntax-checked with the enumerator injected (CI's rdma-core 39 predates it); exercising the events themselves needs an RNIC. ibv_query_port_speed() for LAG-effective speed can follow separately, resolved as an optional symbol through IbvLoader.
| if (!context || context->status() == RdmaContext::DEVICE_UNINIT) | ||
| continue; |
There was a problem hiding this comment.
An inert context is different from an enabled NIC whose speed is unknown. loadTopology() has already inserted every RDMA NIC into devices_, and getAggregateEwmaBandwidth() sums every entry. Skipping an uninitialized context here therefore leaves that NIC's existing bandwidth seed in the aggregate even though it cannot carry traffic.
This can overestimate the capacity used by the admission/deadline predictor; depending on the device mask, the NIC may also remain a scheduling candidate. Please explicitly mark failed or skipped contexts unavailable and exclude them from candidate construction and aggregate bandwidth calculations. The configured default should apply only to usable NICs whose speed cannot be determined.
There was a problem hiding this comment.
@alogfans Thanks for your review. Confirmed and fixed. loadTopology() filters on NIC_RDMA only, so a NIC whose construct() failed stayed in devices_ at the 400G default; the seed loop's "skip inert" left it there, and buildCandidates had no status check — a dead NIC with zero inflight and a 50 GB/s seed actually scored best and got picked until max_retry_count. The aggregate over-counted by the same amount, and a port down at open (DEVICE_PAUSED) had the same shape.
Fixed in 89cfb6a: DeviceSelector gains a per-device available flag (atomic, runtime-settable). Unavailable devices are excluded from candidate construction (both passes — the priority fallback never relaxes availability — and the round-robin path) and from the aggregate; all-unavailable yields -1, which switches the admission drop off. Workers seeds per context status: DEVICE_UNINIT/DISABLED → unavailable with no bandwidth at all, DEVICE_PAUSED → seeded but unavailable until PORT_ACTIVE, ENABLED → seeded and available; handleContextEvents now receives the NicID and flips the flag on PORT_ERR/DEVICE_FATAL/PORT_ACTIVE. The configured default is thereby only ever applied to a usable NIC whose speed could not be read. getNicLoadStats keeps listing unavailable NICs so operators can see them.
Tests (each observed failing first): exclusion from allocation in smart and round-robin mode, from the aggregate, all-unavailable, the priority-filter fallback, and — hardware-free via RdmaTransportTestPeer — a failed context being unavailable to the selector after Workers construction with a negative aggregate.
4fe679c to
480b74a
Compare
480b74a to
0f271d5
Compare
| return Status::OK(); | ||
| } | ||
|
|
||
| Status DeviceSelector::getNicLoadStats(std::vector<NicLoadStats>& stats) const { |
There was a problem hiding this comment.
getNicLoadStats() still lists unavailable devices with their meaningless 400G seed, inconsistent with the aggregate the admission queue reads.
There was a problem hiding this comment.
Agreed, and my "so operators can see them" rationale did not hold: the listed numbers (zero inflight, the never-corrected 400G seed) describe nothing, and since this snapshot is the load signal for #2516's replica scoring (#2996), a dead NIC would have looked like the emptiest, fastest one — the mis-selection 89cfb6a removed from slice placement, one layer up. Fixed in 2a80158: getNicLoadStats() skips unavailable devices, the same rule as the aggregate; the contract is documented on NicLoadStats and the C struct. I did not add an availability field because tent_nic_load_stat/nic_load_stat are C ABI and consumed through FFI by the Rust binding. Test: two NICs → two entries, one marked unavailable → the other only, restored → both.
|
|
||
| // Truncated to whole Gbps so the diagnostic output keeps its integer shape. |
There was a problem hiding this comment.
the legacy (non-TENT) path stores uint8_t active_speed, so the XDR encoding (256) wraps to 0. Will show_links report 0 Gbps on XDR hardware even though the new shared table supports it?
There was a problem hiding this comment.
Yes — it did before this PR too (the old table's case 256 was equally unreachable through the uint8_t field), but lifting the table to a shared header while only wiring active_speed_ex into TENT left the classic half claiming XDR support it could not deliver. Fixed in e50c92e: the classic RdmaContext prefers active_speed_ex under the same HAVE_IBV_ACTIVE_SPEED_EX probe (added to src/CMakeLists.txt for the classic tree; TENT keeps its own, the cache variable is shared), so show_links reports 800 Gbps for XDR 4x. Tested through the classic interposed ibv_query_port in rdma_context_reprobe_test: HDR 4x → 200 Gbps on any host; the XDR case asserts activeSpeed() == 256 and 800 Gbps, is skipped where the header lacks the field (CI's rdma-core 39), and was run RED→GREEN here against a verbs.h copy with the field added.
| } else if (event.event_type == IBV_EVENT_DEVICE_FATAL || | ||
| event.event_type == IBV_EVENT_PORT_ERR) { | ||
| context->pause(); | ||
| // Out of selection and out of the aggregate until the port returns; | ||
| // its EWMA cannot learn anything while no traffic flows. | ||
| device_selector_->setDeviceAvailable(dev_id, false); | ||
| LOG(WARNING) << "Action: " << context->name() << " down"; | ||
| } else if (event.event_type == IBV_EVENT_PORT_ACTIVE) { | ||
| context->resume(); | ||
| // The link may have renegotiated while down: re-seed before the | ||
| // device becomes selectable so no worker scores it on the old rate. | ||
| refreshLinkSpeed(dev_id, *context); | ||
| device_selector_->setDeviceAvailable(dev_id, true); |
There was a problem hiding this comment.
PORT_ERR/PORT_ACTIVE handlers ignore event.element.port. Async events are device-scoped, so on a dual-port HCA used as two rails, a link flap on one port wrongly marks the other rail's device unavailable and drops it from the aggregate bandwidth. The pause()/resume() half is pre-existing, but the PR extends it to scheduling — cheap fix is to compare event.element.port against context->portNum().
There was a problem hiding this comment.
Confirmed — the event's element.port_num was never compared, so on a single-device multi-port HCA a flap on a port this process does not use paused the context (pre-existing) and, since 89cfb6a, also dropped it from selection and the aggregate. Fixed in 8d3bae6: PORT_ERR/PORT_ACTIVE are matched against portNum() and ignored (INFO log) for any other port; DEVICE_FATAL and DEVICE_SPEED_CHANGE stay device-scoped. On mlx5 with one device per port this is a no-op. The decision is split out as applyContextEvent() so synthesized events drive it hardware-free: flips for the opened port, ignored for another port in both directions, DEVICE_FATAL regardless of port, CQ_ERR untouched — each observed failing first.
7bcc9db to
e50c92e
Compare
Description
DeviceSelector stamped every RDMA device with a constant 400 Gbps and nothing ever overwrote it, so the "theoretical bandwidth" was 50 GB/s for every NIC. That value seeds the EWMA the smart selector scores with and bounds it to [0.1x, 10x] = [5, 500] GB/s regardless of the real link. A 25G NIC (~3.1 GB/s measured) sits below that floor, so the clamp pins its EWMA at 5 GB/s and it never learns its real rate; on hosts mixing link speeds the selector keeps steering slices at the slow NIC. The same number feeds getAggregateEwmaBandwidth(), used by the admission queue's deadline-drop predictor. The default/min/max_bandwidth_gbps config knobs were read into SchedulingParams but never consulted.
DEVICE_UNINIT/DISABLED) is marked unavailable and gets no bandwidth at all; a port down at open (DEVICE_PAUSED) is seeded but unavailable untilPORT_ACTIVE;DEVICE_ENABLEDis seeded from the negotiated speed.DeviceSelector::setDeviceAvailable()excludes a device from candidate construction (both passes, both modes) and from the aggregate bandwidth the admission queue's deadline predictor reads; all-unavailable yields -1, which switches the drop predictor off.handleContextEventsflips availability onPORT_ERR/DEVICE_FATAL/PORT_ACTIVE. The configured default is applied only to a usable NIC whose speed could not be read. (review: failed contexts were left at 400G in the aggregate and stayed selection candidates)IBV_EVENT_PORT_ACTIVEand, behind a CMake probe (HAVE_IBV_EVENT_DEVICE_SPEED_CHANGE, rdma-core >= 62), onIBV_EVENT_DEVICE_SPEED_CHANGE;RdmaContext::refreshPortAttributes()re-issuesibv_query_port, andWorkers::refreshLinkSpeed()re-seeds throughsetDeviceBandwidth()only when the speed actually changed, before the device becomes selectable again. (review: a link returning at a lower speed kept the old seed and clamp)ibv_port_attr::active_speed_ex(sameHAVE_IBV_ACTIVE_SPEED_EXprobe, insrc/CMakeLists.txtfor the classic tree), so show_links reports 800 Gbps for XDR 4x instead of 0. (review)PORT_ERR/PORT_ACTIVEfor every port of that device, so on a multi-port HCA a flap on an unused port no longer pauses the context or drops it from selection. The decision half ofhandleContextEventsis nowapplyContextEvent(), so a synthesized event can drive it in tests. (review)getNicLoadStats()reports only NICs able to carry traffic, the same rule as the aggregate; the C struct is unchanged (C ABI, consumed by the Rust binding via FFI). (review)No public API or wire format changes. Not related to #1974 (classic TE GPU↔NIC PCI ranking).
Module
Type of Change
How Has This Been Tested?
Test commands:
cd build
cmake --build . --target ib_link_speed_test tent_device_selector_test tent_rdma_transport_test tent_qos_contract_test bw_arbitration_test rdma_context_reprobe_test
ctest -R 'ib_link_speed_test|tent_device_selector_test|tent_rdma_transport_test|tent_qos_contract_test|bw_arbitration_test|rdma_context_reprobe_test' --output-on-failure
Test results:
New tests, both written first and observed failing on the unpatched tree:
PORT_ERR/PORT_ACTIVEfor the opened port flip availability and the aggregate, events for another port are ignored in both directions,DEVICE_FATALapplies regardless of port,CQ_ERRleaves availability alone.active_speed_ex= 256 andactive_speed= 0, activeSpeed() is 256 and the shared table yields 800 Gbps. The XDR case is skipped where the header lacks the field (CI's rdma-core 39); it was run RED→GREEN against a verbs.h copy with the field added.Manual: the HAVE_IBV_ACTIVE_SPEED_EX branch was compile-checked against a copy of verbs.h with the field added, and the CMake probe confirmed to report yes/no against headers with/without it; the HAVE_IBV_EVENT_DEVICE_SPEED_CHANGE branch was syntax-checked with the enumerator injected (CI's rdma-core 39 predates it). Not covered without hardware: openDevice populating the fields from a real port, and the PORT_ERR/PORT_ACTIVE/DEVICE_SPEED_CHANGE events reaching handleContextEvents (the last also needs rdma-core >= 62 + Linux 7.0 + mlx5).
Checklist
AI Assistance Disclosure