Fix: map A3 AICPU CPU IDs across dies - #2195
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds SoC and PHY-die queries to AICPU topology probing. It resolves CPU-ID bases by platform, limits scans to eight local cores, and adds unit tests for supported and invalid inputs. ChangesAICPU topology probing
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TopologyProbe
participant ACLRuntime
participant HAL
participant CPUBaseResolver
TopologyProbe->>ACLRuntime: Query SoC name with aclrtGetSocName
TopologyProbe->>HAL: Query system PHY die ID
TopologyProbe->>CPUBaseResolver: Resolve CPU-ID base
TopologyProbe->>TopologyProbe: Scan eight local AICPU cores
Merge Risk: ⚪ Minimal · up to The affinity mapping change is covered for A2 and both A3 dies, with no unresolved merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit reads each line, Comment |
a4f50e5 to
9f92a27
Compare
ChaoZheng109
left a comment
ChaoZheng109
left a comment
There was a problem hiding this comment.
审查意见（含 A3 真机验证）
先说结论：问题真实，修法的三个前提我都在一台 A3 机器上独立验证过，公式正确。 主要想讨论的是 SoC 名那条分支是否必要，另有三点健壮性/可维护性建议。
一、真机验证：缺陷与前提都成立
验证环境：Ascend910_9392，4 个 device = 2 芯 × 2 die。
前提验证（直接调 halGetDeviceInfo）：
| dev0 | dev1 | dev2 | dev3 | |
|---|---|---|---|---|
phy_chip_id |
0 | 0 | 1 | 1 |
phy_die_id |
0 | 1 | 0 | 1 |
AICPU/OCCUPY |
0xfc | 0xfc | 0xfc | 0xfc |
四个 die 全报 0xfc → OCCUPY 确实是 die 本地位图。这是整个修法的支点，成立。
另外核对了驱动头 ascend_hal_base.h：MODULE_TYPE_SYSTEM=0、INFO_TYPE_PHY_DIE_ID=19、INFO_TYPE_OCCUPY=8、MODULE_TYPE_AICPU=1 —— PR 里新增的常量全部正确。
缺陷复现（修复前的二进制跑 dummy_task，ASCEND_GLOBAL_LOG_LEVEL=1）：
die 0（device 2）——
allowed[0] = cpu_id 4 role=sched
allowed[1] = cpu_id 5 role=orch
thread idx=0 cpu=4 exec_idx=0 ACTIVE(sched)
thread idx=3 cpu=5 exec_idx=1 ACTIVE(orch)
die 1（device 3）——
allowed[0] = cpu_id 4 role=sched
allowed[1] = cpu_id 5 role=orch
AICPU filter gate: only matched 0/2 allowed cpus; filling missing exec slots by report order
thread idx=0 cpu=10 exec_idx=0 ACTIVE(sched)
thread idx=1 cpu=11 exec_idx=1 ACTIVE(orch)
die 1 的线程报的是 cpu 10-15（即 8 + {2..7}），与 allowed [4,5] 0/2 命中，退化为按到达顺序分配。而且三轮运行的角色落点分别是 10/11、11/13、14/13 —— 跨轮不确定；sched/orch 落在 cpu 10、11 即 die-1 的低 cluster，正是 #1045 要消除的那种放置。
代入 PR 的公式：die1 → base 8 → 池 {10..15} → cluster1 = {12,13,14,15}，与 PR 描述的 12-15 一致。
Root Cause 一节描述准确，回归测试 A3OffsetsTheSecondDieInTheSharedAicpuOs 是合格的屏障。
二、主要讨论点：SoC 名这条分支是必要的吗？
当前实现绕了一圈：先用 aclrtGetSocName() 判断是 A2 还是 A3，再决定要不要查 PHY_DIE_ID。但真正决定要不要加偏移的属性是「这个 device 的 AICPU OS 命名空间是否与兄弟 die 共享」，而 PHY_DIE_ID 直接回答了它 —— SoC 名只是它的代理。
如果 A2（910B）恒报 phy_die_id = 0，那么无条件 base = phy_die_id * kAicpuCoresPerDie 在两种硅片上都正确（A2 得 0，行为与今天完全一致），于是可以整体删掉：query_soc_name()、AclrtGetSocNameFn、has_soc_prefix()、kA2SocPrefix/kA3SocPrefix、#include <cstring>，以及 resolve_aicpu_cpu_id_base() 里的 soc 参数和两条前缀分支 —— 大约是 core 新增 68 行里的 30 行，连带消掉下面第 3 点的两条硬失败路径。
所以想请教一个可验证的问题：在一台 910B 上，halGetDeviceInfo(MODULE_TYPE_SYSTEM, INFO_TYPE_PHY_DIE_ID) 返回什么？
- 返回 0 → 建议收敛成无条件
die * 8，同时解决下面第 3 点； - 返回非 0 或 rc≠0 → SoC 分支就是必要的，请把这个实测结果写进代码注释 —— 它是这段代码存在的唯一理由，否则下一个人一定会重新问一遍。
我这边验证不了，手上只有 A3。
附带一个维护性观察：仓库里 SoC→arch 的映射已经有三份，且都是数据驱动的（从 CANN platform_config/<SoC>.ini 读 Short_SoC_version）——
docs/hardware/chip-architecture.md（规范表）tools/cann-examples/query/query.cpp:188.claude/skills/onboard-arch-precheck/check.sh:139
这个 PR 会成为第四份，也是唯一硬编码字符串前缀、且认不出就 fail-closed 的一份。另外 src/a2a3/ 和 src/common/ 的运行时 C++ 目前是 0 处 SoC 名分支，这会是第一处 —— 值得作为一个有意识的决定，而不是顺带引入。
三、建议修改
3.1 未知 SoC / SoC 名取不到 → 启动硬失败，是相对现状的行为回退
新增的两条 return false（query_soc_name() 返回 null、SoC 前缀不认识）会一路传到 device_runner.cpp:311 的 PTO_RUNTIME_ERR_INTERNAL。修复前任何 SoC 都能跑（A3 die1 只是慢），现在一个新 SKU 或 ACL 未初始化就是直接起不来。而且 aclrtGetSocName 只经 dlsym(nullptr, ...) 解析，不像 halGetDeviceInfo 带 dlopen 兜底。
建议降级为 LOG_WARN + base = 0 继续 —— base 0 恰好是修复前的行为，最坏退回现状，而不是从「慢」退化成「不能运行」。若立场是「宁可响亮地失败也不要静默错放」（这也站得住），请把理由写进描述和注释。
3.2 支点不变量没有写进代码
两条关键事实目前只在 PR 描述里：A3 的 OCCUPY 按 die 本地上报；AICPU OS 命名空间跨两 die 共享、步长 8。这是 local + base 能直接相加的唯一理由，建议落成注释。
同时三处文档需同步：AicpuLogicalCpu::cpu_id 语义变了（die 本地 id → AICPU OS 全局 id）却无注释；头文件里 probe_aicpu_topology 的注释没说这些 id 现在是 OS 全局的（而这正是设备侧门控依赖的契约）；resolve_aicpu_cpu_id_base 是该头文件里唯一没有注释的导出函数。
3.3 扫描上界 64 → 8 是静默截断，缺告警
若某个 SKU 的 OCCUPY 在 bit ≥ 8 有置位，新代码会得到空池 → probe 返回 false → 启动硬失败，而日志里没有任何线索指向 occupy 的形状。建议 (occupy >> kAicpuCoresPerDie) != 0 时打一条 WARN 把原始 occupy 打出来。一行代码，省掉一次几小时的排查。
（循环上界本身必须收窄 —— 一旦 base ≠ 0，扫到 bit 63 再加 base 会产出无意义的 cpu_id。这不是清理，是 base 化的必要配套。）
四、其它
cluster_id从(cpu_id % 8) / 4改为local / 4：因 base 是 8 的倍数，两者恒等，纯可读性改写，没问题。.h转#pragma once符合 codestyle 的「routine edits 时顺手转」，不算 drive-by。- a5 无需同步修改：a5 的一个 runtime device 本身横跨两 die，不存在这个错位。
- CI：
packaging-matrix (macos-latest, 3.10)的失败是 runner 基础设施问题（OSError: [Errno 5] Input/output error+/opt/homebrew/bin/ccache: No such file or directory），与本变更无关（只改 a2a3 onboard C++ 和一个 C++ UT，macOS 不构建 onboard 路径）。建议 rerun。
总体：问题真实且已复现，方法正确且前提已验证，回归测试到位。补上第二点的答复（A2 的 phy_die_id）和 3.1 / 3.2 之后就可以合入。感谢这个修复 —— die 1 上的角色放置确实是一直存在的 silent 退化。
9f92a27 to
4f458ec
Compare
|
已按建议更新 PR：
最新 rebase 后完整 C++ 非硬件测试 |
A3 reports AICPU occupancy with die-local bit positions while worker affinity uses the shared OS CPU namespace. Resolve the physical die and apply its eight-CPU base before selecting the affinity set. Keep A2 on its local numbering and reject unsupported SoC or invalid die metadata. Reject OCCUPY masks outside the die-local width instead of silently truncating them. Document the global CPU-ID contract and add unit coverage for both A3 dies and the A2 path.
2 participants
Summary
PHY_DIE_ID.Root Cause
On A3,
AICPU/OCCUPYreports positions within one die, while CANN workers use CPU IDs from the AICPU OS namespace shared by both dies. The second die therefore selected CPUs 4-7 while its workers were pinned to CPUs 10-15. The affinity gate could not match its four execution roles and assigned them by worker arrival order, which produced the observed latency split when scheduler roles landed on CPUs 10-11.The host topology probe now maps each local CPU as
phy_die_id * 8 + local_cpu_id. This keeps die 0 on CPUs 4-7 and selects CPUs 12-15 for die 1.Testing
pre-commit runon the staged files.Related to #1045.