Skip to content

[TENT] Prefer the LAG-effective port speed from ibv_query_port_speed - #3777

Merged
alogfans merged 3 commits into
kvcache-ai:mainfrom
SongOf:fix/tent-lag-effective-speed
Sep 1, 2026
Merged

alogfans merged 3 commits into
kvcache-ai:mainfrom
SongOf:fix/tent-lag-effective-speed

Conversation

@SongOf

@SongOf SongOf commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Description

RdmaContext::linkSpeedGbps() decodes active_speed × active_width, the physical link's encoded rate. For a VF over VF LAG / multiport LAG, a PF dropping out of the bond halves the VF's bandwidth while its port stays ACTIVE at the same encoding, so the re-read on IBV_EVENT_DEVICE_SPEED_CHANGE (#3644) saw no change and never re-seeded the selector. rdma-core v62 added ibv_query_port_speed() alongside that event to report the effective speed (100 Mb/s units, LAG-aware).

  • ibPortSpeedGbps() in ib_link_speed.h: effective speed wins when > 0, the encodings decide otherwise.
  • IbvLoader resolves ibv_query_port_speed as an optional symbol (nullptr on older libraries; never disables RDMA).
  • RdmaContext queries it after every ibv_query_port (open and refresh), caches it, and linkSpeedGbps() prefers it. A present-but-failing verb records 0 so the fallback applies.
  • RdmaContext::verbs_ becomes a per-context copy of the loader's table so tests can substitute entries (RdmaContextTestPeer), covering the effective-speed and refresh paths without an RNIC.

No public API or config changes. Refs #3750 (follow-up section).

Module

  • Transfer Engine (mooncake-transfer-engine)
  • Common (mooncake-common)
  • Docs

Type of Change

  • Bug fix

How Has This Been Tested?

Test commands:

cd build
cmake --build . --target ib_link_speed_test tent_rdma_transport_test tent_device_selector_test tent_qos_contract_test bw_arbitration_test
ctest -R 'ib_link_speed_test|tent_rdma_transport_test|tent_device_selector_test|tent_qos_contract_test|bw_arbitration_test' --output-on-failure

Test results:
- [x] Unit tests pass — 100% tests passed, 0 tests failed out of 5 (Ubuntu 22.04, rdma-core 39, no RNIC)
- [ ] Integration tests pass (if applicable)
- [x] Manual testing done (describe below)

New tests, each observed failing before the change:
- ib_link_speed_test: effective speed beats the encoded rate (2000 → 200 Gbps over an NDR 4x link); falls back to the encodings when 0.
- tent_rdma_transport_test: the optional symbol resolves iff this host's libibverbs exports it, with the mandatory verbs still resolved; with fake verbs injected through RdmaContextTestPeer: effective speed preferred, cleared when the verb fails, skipped when absent, refresh sees a renegotiated link, failed ibv_query_port keeps cached values; end-to-end through Workers::applyContextEvent: PORT_ACTIVE at the same speed keeps the learned EWMA, a LAG-halved effective speed re-seeds it to the new rate and the device stays available.

Not covered without hardware: a real driver reporting a LAG-reduced speed (rdma-core ≥ 62 + Linux ≥ 7.0 + mlx5 + LAG).

Checklist

- [x] I have performed a self-review of my own code
- [x] I have formatted my code using ./scripts/code_format.sh
- [x] I have run pre-commit on the files changed in this PR and all hooks pass
- [x] I have updated the documentation (if applicable)
- [x] I have added tests to prove my changes are effective
- [ ] For changes >500 LOC: I have filed an RFC issue (N/A, ~290 LOC incl. tests)

AI Assistance Disclosure

- [ ] No AI tools were used
- [x] AI tools were used (specify below)

@catyans catyans left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The implementation matches the upstream verbs contract: ibv_query_port_speed() uses the expected (context, uint32_t port, uint64_t*) ABI and reports 100 Mb/s units, so dividing by 10 produces Gbps. Resolving it with dlsym keeps older libibverbs versions compatible, while a missing, failed, or zero result falls back to the existing encoded speed path.

The runtime integration is also covered well: initial device open and port refresh update the effective value, stale effective data is cleared on query failure, and the event-chain test verifies that a real speed change reseeds the selector while an unchanged rate preserves learned EWMA state. CI is green. Approved.

Non-blocking follow-up suggestion: expose a counter or rate-limited warning for effective-speed query failures/fallbacks, since a transient failure on a degraded LAG can temporarily restore the higher encoded rate and would otherwise be difficult to diagnose.

Comment on lines +178 to +179
// From ibv_query_port_speed(), 100 Mb/s units; 0 = unavailable.
std::atomic<uint64_t> effective_speed_100mbps_{0};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What's the difference between this one and active_speed_? Additionally we'd prefer to store values as mbps.

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.

Done in 5fb0da4: the context now stores plain Mb/s (effective_speed_mbps_, x100 at the store since the verb reports 100 Mb/s granularity), and ibPortSpeedGbps() takes effective_mbps (/1000). The fake verb in the tests still returns 100 Mb/s units like the real one, so EffectiveSpeedPreferredWhenVerbReportsIt (2000 -> 200.0 Gbps) covers the conversion end to end.

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.

To expand on the difference, with the upstream references:

active_speed_ vs effective_speed_mbps_. active_speed_ caches ibv_port_attr::active_speed from ibv_query_port() - an encoding of the per-lane signalling class (1/2/4/.../256 = SDR...XDR), which only becomes a rate after multiplying by the lane count from active_width (ibLinkSpeedGbps()). effective_speed_mbps_ caches what ibv_query_port_speed() returns: the port's actual effective bandwidth as a number (the verb reports 100 Mb/s granularity; we store Mb/s since 5fb0da4).

Why a second verb at all. From the rdma-core commit that introduced it (linux-rdma/rdma-core#1674, commit 8504597, first released in v62.0): "This is useful for VF LAG and multiport LAG configurations, since ibv_query_port does not report the combined bandwidth of all aggregated ports." Three concrete gaps in ibv_query_port that the new verb closes:

  1. For a VF/port over LAG, ibv_query_port reports a single parent port's speed x width; ibv_query_port_speed reports the aggregate of the active bond members.
  2. When a bond member fails, the VF's bandwidth drops (e.g. halves) while its port stays ACTIVE with the same encoding/width - invisible to ibv_query_port; the effective speed changes, and the paired IBV_EVENT_DEVICE_SPEED_CHANGE async event fires (we already consume that event to re-seed the selector).
  3. Aggregated rates (e.g. 2x200G) have no speed-x-width encoding; the kernel series notes it supports "speeds not expressible as IB speed * width", which is why the verb returns a plain number instead of an encoding.

Kernel side: "[PATCH rdma-next 00/10] Support effective VF bandwidth query in LAG mode" adds ib_query_port_speed() and the speed-change event, with mlx5 propagating the aggregated LAG speed to attached vports and updating it on member link changes.

That is also why linkSpeedGbps() prefers the effective value and falls back to active_speed_ x active_width_ only when the verb is absent (< rdma-core 62, resolved as an optional symbol) or reports 0.

@SongOf
SongOf force-pushed the fix/tent-lag-effective-speed branch from e9ea1bd to 7f56809 Compare August 31, 2026 14:06
@SongOf

SongOf commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the non-blocking follow-up in cc09885, in this PR:

  1. A verb error now keeps the last known effective speed instead of reverting to the encoded rate (on a degraded LAG the fallback would briefly restore the higher rate and re-seed the selector at overstated capacity until the next port event). A verb that is absent or reports 0 still falls back as before.
  2. Failure episodes are logged once per transition (WARNING with the errno-style rc and the held value; INFO on recovery), and the "link speed X -> Y, re-seeding" warning now names its source (effective speed vs encoded rate) so a fallback is no longer indistinguishable from a genuine renegotiation.
  3. Per-device counter: RdmaContext::effectiveSpeedQueryFailures().

Covered hardware-free via the fake-verbs fixture: EffectiveSpeedHeldWhenVerbFails (EIO holds 200G over two refreshes, counter 2, recovery at 400G picked up) replaces the old EncodedRateWhenVerbFails expectation, and EncodedRateWhenVerbReportsZero pins that a zero report is not an error.

@catyans catyans left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. The update safely preserves the last known effective speed on transient query failures and adds useful observability and regression coverage. All checks are green.

@alogfans
alogfans merged commit 40aefcc into kvcache-ai:main Sep 1, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Common documentation Improvements or additions to documentation run-ci Transfer Engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants