fix: correctly detect RDMA bandwidth for bonded NICs - #705
Conversation
For RDMA bond devices (e.g., mlx5_bond_*), ibstat only reports a single logical port with per-port rate, missing the aggregated bandwidth of multiple physical ports. This causes get_rdma_gbs() to return 50 GB/s instead of the correct 100 GB/s on dual-port CX7/CX8 NICs in bond mode, leading to suboptimal SM/QP allocation in get_theoretical_num_sms(). Fix by adding sysfs-based speed detection for bond devices: - Map IB device to netdev via /sys/class/infiniband/<dev>/ports/<port>/gid_attrs/ndevs/0 - Read the kernel-reported aggregated speed from /sys/class/net/<iface>/speed - Fall back to ibstat parsing if sysfs is unavailable Also add EP_OVERRIDE_RDMA_GBS environment variable for manual override, consistent with existing EP_OVERRIDE_RDMA_SL convention.
| nic_name: the NIC device name. | ||
|
|
||
| Returns: | ||
| gbs: the RDMA bandwidth in GB/s (0 if detection fails). |
There was a problem hiding this comment.
🟡 warning: The new EP_OVERRIDE_RDMA_GBS environment variable is not documented in the README 'Networking' env var section, unlike its sibling EP_OVERRIDE_RDMA_SL (README.md:342). Since the PR explicitly frames this override as following the EP_OVERRIDE_RDMA_SL convention, please add a matching entry (e.g. 'EP_OVERRIDE_RDMA_GBS: float, override the detected RDMA bandwidth in GB/s') so operators can discover it.
🤖 v3
|
|
||
| speed_path = f'/sys/class/net/{net_iface}/speed' | ||
| with open(speed_path) as f: | ||
| speed_mbps = int(f.read().strip()) |
There was a problem hiding this comment.
🔵 suggestion: The broad except Exception: return 0 swallows all errors silently (unlike the ibstat branch which prints a diagnostic on failure). For a bond device where sysfs is expected to work, a silent fallback can mask misconfiguration and quietly re-introduce the under-reported bandwidth this PR is fixing. Consider a debug/warn log on the sysfs failure path to aid troubleshooting, while still falling back to ibstat.
🤖 v3
|
|
||
| return speed_mbps / 8000 if speed_mbps > 0 else 0 | ||
| except Exception: | ||
| return 0 |
There was a problem hiding this comment.
🔵 suggestion: Consider a brief inline comment on the speed_mbps / 8000 conversion (e.g. 'Mbps -> GB/s: /8 for bits->bytes, /1000 for M->G'). The 8000 magic number is non-obvious and easy to misread as a bug; it is in fact correct and consistent with the ibstat branch's rate/8.
🤖 v3
| # Environment variable override (highest priority) | ||
| if 'EP_OVERRIDE_RDMA_GBS' in os.environ: | ||
| return float(os.environ['EP_OVERRIDE_RDMA_GBS']) | ||
|
|
There was a problem hiding this comment.
🔵 suggestion: The bond-detection heuristic uses substring match 'bond' in nic_name. This matches mlx5_bond_* naming and, combined with the safe 0/fallback behavior of get_net_device_speed_gbs(), preserves non-bond behavior as intended. Consider either attempting the sysfs path unconditionally (with ibstat as fallback) so other multi-port topologies also benefit, or adding a short comment explaining that the bond-only gating is an intentional compatibility guarantee.
🤖 v3
🤖 ds-review-bot Code Reviewv6The bonded-NIC sysfs lookup, unit conversion, fallback behavior, and explicit override are consistent with existing bandwidth detection semantics. No actionable regressions were identified. v4v3The change adds a sysfs-based RDMA bandwidth detection path to correctly report the aggregated link speed for bonded NICs (e.g. mlx5_bond_*), where ibstat only reports a single logical port's rate. A new helper get_net_device_speed_gbs() maps the IB device to its network interface via /sys/class/infiniband/<dev>/ports/<port>/gid_attrs/ndevs/0 and reads /sys/class/net/<iface>/speed (Mbps), and get_rdma_gbs() is updated with a three-level priority: EP_OVERRIDE_RDMA_GBS env override, sysfs aggregated speed for bond devices, then the original ibstat fallback. The implementation is well-scoped, defensive (returns 0 / falls back on any failure), and preserves behavior for non-bond devices. The unit conversion is correct and consistent with the ibstat branch (speed_mbps/8000 == rate/8 semantics; both decimal GB/s). Reading only the first port's aggregated bond netdev speed is correct and does not double-count. get_rdma_gbs remains lru_cache'd, so the env override / sysfs read happens once per nic_name per process, consistent with pre-existing behavior. The empirical validation table is convincing. Overall a solid, low-risk fix; the notable follow-ups are documenting the new env var and improving observability of the sysfs failure path. Files reviewed: 1 |
2 participants
Summary
Fix incorrect RDMA bandwidth detection for bonded NIC devices (e.g.,
mlx5_bond_*), which causes suboptimal SM allocation and reduced dispatch/combine throughput.Problem
In RDMA LAG (bond) mode,
ibstatreports only a single logical port with per-port rate:The existing
get_rdma_gbs()parses this single port rate and computes400 / 8 = 50 GB/s. However, dual-port CX7/CX8 NICs in bond mode aggregate two 400 Gb/s physical ports, so the actual available bandwidth is800 / 8 = 100 GB/s.This under-reported bandwidth causes
get_theoretical_num_sms()to recommend fewer SMs than optimal, directly reducing dispatch/combine kernel performance.Changes
get_net_device_speed_gbs(): maps IB device to its network interface via sysfs (/sys/class/infiniband/<dev>/ports/<port>/gid_attrs/ndevs/0), then reads the kernel-reported aggregated link speed from/sys/class/net/<iface>/speed(in Mbps).get_rdma_gbs()with a three-level detection priority:EP_OVERRIDE_RDMA_GBSenvironment variable — manual override, consistent withEP_OVERRIDE_RDMA_SLconventionbond)ibstatRate parsing — original fallback for non-bond devices or when sysfs is unavailableValidation
Verified on a multi-node cluster with CX8 dual-port NICs in RDMA LAG mode (
EP_NIC_NAME=mlx5_bond_0):Compatibility
bondibstatparsing (returns 0 on any failure)EP_OVERRIDE_RDMA_GBS=<value_in_GBs>takes highest priority for any device type