bug fix: probe RDMA link rate via sysfs, survive probe failure - #734
bug fix: probe RDMA link rate via sysfs, survive probe failure#734Xuan-1998 wants to merge 1 commit into
Conversation
|
|
||
| # The un-overridden default may simply not exist on this fabric; an explicitly named NIC | ||
| # must not fall back silently | ||
| if 'EP_NIC_NAME' not in os.environ: |
There was a problem hiding this comment.
🟡 warning: 不要把显式 nic_name 回退到其他设备: 当调用方显式传入 get_rdma_gbs("missing-or-down-nic") 且未设置 EP_NIC_NAME 时，此条件仍会扫描全部设备并返回另一块 NIC 的速率。这违背了显式指定 NIC 不应猜测回退的约定，并可能使用错误带宽进行调优；应依据参数是否被省略来决定是否自动发现设备。
🤖 v6
There was a problem hiding this comment.
done, now we distinguished whether the EP_NIC_NAME is being set explicitly and then deal with them differently
| ports_dir = os.path.join('/sys/class/infiniband', nic_name, 'ports') | ||
| try: | ||
| for port in os.listdir(ports_dir): | ||
| with open(os.path.join(ports_dir, port, 'rate')) as f: |
There was a problem hiding this comment.
🔵 suggestion: 正则 r'\s*(\d+)\sGb/sec' 无法匹配带小数的速率（如老式 SDR 1X 的 "2.5 Gb/sec"），此类设备会被解析为 0。可考虑改用 r'\s(\d+(?:.\d+)?)\s*Gb/sec' 并用 float 解析。对现代 HPC 网卡影响可忽略，仅作健壮性建议。
🤖 v5
| gbs: the device's link rate in GB/s (0 if the device or its rate is unavailable). | ||
| """ | ||
| rate = 0 | ||
| ports_dir = os.path.join('/sys/class/infiniband', nic_name, 'ports') |
There was a problem hiding this comment.
🔵 suggestion: sysfs 的 ports//rate 在端口 state 为 DOWN 时也可能报告一个速率。在 EP_NIC_NAME 未设置的『选最快设备』分支里，可能选中一个链路未激活的设备。可选改进：同时读取 ports//state，仅统计 ACTIVE（"4: ACTIVE"）端口。
🤖 v5
| if gbs > 0: | ||
| return gbs | ||
|
|
||
| # The un-overridden default may simply not exist on this fabric; an explicitly named NIC |
There was a problem hiding this comment.
🔵 suggestion: 当所有探测（sysfs、最快设备扫描、ibstat）都失败时函数仍返回 0，调用方 get_theoretical_num_sms()（deep_ep/buffers/elastic.py:761 附近）在 num_rdma_ranks > 1 时仍可能触发 ZeroDivisionError。本 MR 已消除 EFA 场景的主要触发路径，但如果想彻底『survive probe failure』，建议在调用方对 rdma_gbs == 0 给出明确报错（提示设置 EP_NIC_NAME 或显式传入 --num-sms）。
🤖 v5
| devices = sorted(os.listdir('/sys/class/infiniband')) | ||
| except OSError: | ||
| devices = [] | ||
| gbs = max((_get_sysfs_rdma_gbs(device) for device in devices), default=0) |
There was a problem hiding this comment.
🟡 warning: 当 /sys/class/infiniband 下没有任何设备（或所有设备的 rate 都读不到）且后续 ibstat 也失败时，get_rdma_gbs() 仍会返回 0；而 deep_ep/buffers/elastic.py 的 get_theoretical_num_sms() 在 num_rdma_ranks > 1 时会执行 rdma_traffic / rdma_gbs，仍会触发 ZeroDivisionError。既然目标是 survive probe failure，建议在调用处对探测结果为 0 的情况做兜底（例如回退到保守默认带宽或给出明确报错），而不是继续除以 0。
🤖 v4p
There was a problem hiding this comment.
now it will not exit with number 0, it will either give a positive number or report errors.
|
|
||
| # The un-overridden default may simply not exist on this fabric; an explicitly named NIC | ||
| # must not fall back silently | ||
| if 'EP_NIC_NAME' not in os.environ: |
There was a problem hiding this comment.
🔵 suggestion: 自动猜测逻辑用 'EP_NIC_NAME' not in os.environ 判断“是否显式指定了网卡”，但函数参数 nic_name 也可以由调用方显式传入。例如未设置 EP_NIC_NAME 时调用 get_rdma_gbs('mlx5_0')，若 mlx5_0 不存在，代码仍会静默选择其他最快设备，与“显式指定网卡应 loudly fail”的说明不一致。建议用 sentinel 默认值（如 nic_name=None 时再取环境变量默认值）来区分默认值与显式传参。
🤖 v4p
🤖 ds-review-bot Code Reviewv6自动发现条件无法区分默认调用与显式函数参数，导致显式指定失败时静默使用无关设备的速率。 v5该 MR 修复了 EFA 主机上 get_rdma_gbs() 探测失败导致 get_theoretical_num_sms() 除零崩溃的问题。改动仅涉及 deep_ep/utils/envs.py（与描述一致，变更文件数为 1）：新增 _get_sysfs_rdma_gbs() 优先从 /sys/class/infiniband/<nic>/ports//rate 读取链路速率（对所有 verbs provider 通用，包括 EFA 的 rdmap 设备）；保留 ibstat 作为最终回退；当 EP_NIC_NAME 未显式设置且默认设备（mlx5_0）不存在时，扫描 /sys/class/infiniband 选取最快设备，而显式指定的 NIC 不会静默回退、仍会明确失败。已核对的行为要点：多端口 CA 取各端口速率的 max，与旧 ibstat 路径（只取第一个 Port 的 Rate）语义一致；『不静默回退』通过 'EP_NIC_NAME' in os.environ 判断实现，显式传参 nic_name 时同样不回退，符合描述；lru_cache 以 nic_name 为键、行为依赖进程内稳定的环境变量，无问题；OSError 均被捕获，防御性良好；文件头新增的 MIT/Amazon 版权声明为合规性改动，与功能无关。整体实现正确、与提交说明完全吻合，建议合并。以下评论均为非阻塞的改进建议。 v4p该 MR 旨在修复 RDMA 带宽探测在 EFA 等无 umad CA 设备上返回 0、进而导致 get_theoretical_num_sms 自动计算 SM 数时除零崩溃的问题：新增从 /sys/class/infiniband/<nic>/ports/*/rate 读取速率，保留 ibstat 作为回退，并在未设置 EP_NIC_NAME 且默认网卡缺失时自动选择 sysfs 下最快的 RDMA 设备。整体方向正确、实现简洁，但探测完全失败时仍会返回 0，且显式传入 nic_name 参数时仍可能被自动猜测，建议补齐这两个边界。 Files reviewed: 1 |
get_rdma_gbs() relied on ibstat, which cannot see verbs providers without a umad interface: on EFA hosts (rdmap* devices) it always failed, returned 0, and get_theoretical_num_sms() divided by zero. Probe sysfs (/sys/class/infiniband/<nic>/ports/*/rate) first, which works for any verbs provider without external tools; keep ibstat as a fallback. When no NIC is named at all -- neither via the nic_name argument nor via EP_NIC_NAME -- and the default device does not exist (EFA hosts have no mlx5_0), fall back to the fastest device under /sys/class/infiniband. Automatic discovery is gated on the NIC being truly unnamed: nic_name now defaults to None (sentinel) so an explicitly passed device is distinguishable from the default, and an explicitly named NIC never falls back to another device's rate. When every probe fails, raise a RuntimeError naming the device and the two ways out (set EP_NIC_NAME, or bypass detection by passing the SM count explicitly) instead of returning 0 for the caller to divide by.
e9ef8b3 to
dacd416
Compare
2 participants
get_rdma_gbs() only knew how to ask ibstat for a CA named EP_NIC_NAME (default mlx5_0). EFA devices expose no umad CA, so on EFA hosts the probe returned 0 and get_theoretical_num_sms() divided by it -- any multi-node run without an explicit --num-sms crashed with ZeroDivisionError.
Read /sys/class/infiniband//ports/*/rate first, which works for every verbs provider, and fall back to ibstat for setups whose rate only shows there. When EP_NIC_NAME is unset and the default device is absent, pick the fastest device under /sys/class/infiniband instead of failing; an explicitly named NIC still fails loudly rather than guessing.