Refactor: localize A5 HBG scheduler state and configuration - #2398
Conversation
|
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 (14)
💤 Files with no reviewable changes (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe scheduler now stores mutable execution state in ChangesResident scheduler local-state migration
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant AICoreExecutor
participant SchedulerLocalState
participant ReadyInbox
participant DispatchHelpers
participant CompletionHelpers
AICoreExecutor->>SchedulerLocalState: Initialize and validate local configuration
AICoreExecutor->>ReadyInbox: Bootstrap ready tasks
AICoreExecutor->>DispatchHelpers: Fill and publish dispatch slots
DispatchHelpers->>CompletionHelpers: Service completions and refills
CompletionHelpers->>ReadyInbox: Refresh and claim ready work
Merge Risk: ⚪ Minimal · up to The reviewed concerns do not identify a remaining correctness or documentation issue; the change is ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 3.30% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 182 functions across 8 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 checks local state, Comment |
678417b to
4317d19
Compare
poursoul
left a comment
poursoul
left a comment
There was a problem hiding this comment.
结论
机制正确，CI 全绿，没有发现会导致错误结果的缺陷。owner pending 队列 / advertised 标志 / profiling loop 计数本来就是 owner 私有状态（thief 偷任务只读 inbox->head 和 GM 链表，从不碰 owner queue），放在 GM 里纯属浪费 —— 搬到 core-local 的方向是对的，alignas(128) + 已有的 offsetof 断言也让布局改动在编译期自校验。
主要问题是 PR 描述与实际改动不一致，另有两处硬编码 / 边界隐患建议修掉。verdict: needs discussion（补说明 + 修 2 项后可合）。
一、声明与实现不一致
1. 「热路径尚未消费 route 快照」与代码不符 ❌
正文写「does not switch the dispatch/completion hot path to those snapshots yet」，并把 SchedulerWorkerRoute::{active, core_type, dispatch_payload_offset} 的消费归给后续 commit 293a8001。实际上本 PR 里已经在消费了：
worker_routes[].worker_id全线替代cluster_worker_ids[]：scheduler_dispatch.h:65/127/248/270、scheduler_completion.h:48/238、aicore_executor.cpp:233worker_routes[self_lane].dispatch_payload_offset替代了 executor ready 派发路径上的context->dispatch_payload_offset：aicore_executor.cpp:423
确实纯预留的只有 .active（仅在 config 校验里读一次）、.core_type 和 SchedulerLocalSlotState::timing_slot（完全未被读写）。
这句话需要改写 —— 否则 reviewer 会误以为可以不审 route 快照的正确性。
2. 两处 scheduler_observe_cache_line 移除未披露 ➕
scheduler_ready.h 里去掉了 scheduler_lookup_callable_address 读 callable table 前、以及 scheduler_resolve_completion 读 task metadata 前的缓存行失效。
这两处删除是安全的：aicore_execute 在 bootstrap 之前对每个 active core 调了 scheduler_observe_data_cache(...)，它是 dcci(addr, ENTIRE_DATA_CACHE) 整个数据缓存失效，而 callable table 和 task metadata 整轮不变。但这是热路径上的内存序改动，正文完全没提，建议补上这条兜底理由。
3. 「单测共用一个 fixture」只做了 2/3 ⚠️
test_hbg_scheduler_contracts.cpp 仍自带一份 SchedulerStateBuffer（:84）和 GraphBuffer（:98），与新 header 里的重复；新 header 的注释还指回它（"see the same note in test_hbg_scheduler_contracts.cpp"）。要么一起收口，要么说明为什么 contracts 留在外面。
4. 搭车改动 ➕
RUNTIME_LOGIC.md那段调度描述的重写，改的是本 PR 之前就存在的文档陈旧问题。修得对，但顺手删掉的「the highest-index thread first attaches the prebuilt runtime and publishes the boot barrier」这个事实在新表述里没有落点。SchedulerCompletionStats字段重排：确认是逐字段赋值（aicore_executor.cpp:94-97）而非 memcpy，所以无副作用 —— 但也就是纯 churn。
另外，删掉的 3 个测试核过了：2 个是 SchedulerReadyOwnerState 删除后的强制清理；LocalCompletionKeepsSharedSlotMetadataPublished 的断言（local slot 转 FREE / 共享 slot publication 仍为 READY / kernel 时戳落入 trace）确实由 SpscGenerationCompletesNormalTask（test_hbg_scheduler_dispatch.cpp:188-202）覆盖 —— 「重复覆盖」的说法成立。
二、建议修改
Should fix
1. owner_pending_endpoints 的初始化硬编码了 2 个元素
scheduler_ready.h:164-167：
uint64_t owner_pending_endpoints[SCHEDULER_CORE_TYPE_COUNT]{
SCHEDULER_READY_PENDING_EMPTY,
SCHEDULER_READY_PENDING_EMPTY,
};旧的 GM 版本是 for (type = 0; type < SCHEDULER_CORE_TYPE_COUNT; ++type) 循环写 sentinel，与规模无关。
失败场景：SCHEDULER_CORE_TYPE_COUNT 从 2 改成 3 后，owner_pending_endpoints[2] 被零初始化成 0，而 EMPTY 是 UINT64_MAX。于是 scheduler_ready_pending_head(0) == 0 != EMPTY，scheduler_refresh_ready_inbox_type 会走 pending 提升分支，把不存在的 task 0 当成 pending head 发布到 inbox —— 静默重复派发 task 0，没有任何编译期或运行期告警。
建议加 static_assert(SCHEDULER_CORE_TYPE_COUNT == 2)，或在构造函数里循环填。
2. worker_id() 越界索引无保护，且默认值就是越界值
scheduler_ready.h:156-157：
inline __aicore__ bool is_scheduler() const {
return config.self_lane < PLATFORM_CORES_PER_BLOCKDIM && config.self_lane == config.scheduler_lane;
}
inline __aicore__ uint64_t worker_id() const { return worker_routes[config.self_lane].worker_id; }self_lane 默认 UINT32_MAX，兄弟访问器 is_scheduler() 明确做了上界检查，worker_id() 没有。
当前不可达：aicore_execute 里 config_valid = (context->active == 0) || initialize(...)，两条使用路径都额外 && context->active != 0，所以 active == 0（local state 全默认）时不会调到。但这靠调用点的两个条件耦合维持，属于隐式不变式；而且 scheduler_initialize_local_config 失败返回时还会留下部分填充的 local state。建议给 worker_id() 加同样的上界保护，或至少把「self_lane 已校验」写成注释 / 断言。
Consider
scheduler_ssbuf_reserved1和 topology 组尾部那 8 字节隐式 padding，都没遵循同结构体内graph_reserved_address立下的规矩（注释要求「写 0 并拒绝非 0 读」）。实际值是 0（host 侧std::vector<uint8_t> storage(allocation_size)值初始化），但 padding 建议显式命名成topology_reserved[3]。scheduler_ready_batch_push的inbox_index形参现在只有一个调用点、传的就是context->config.scheduler_index，函数内inbox_index != context->config.scheduler_index守卫因此恒真 —— 参数和守卫都可以去掉。- 被删的
PackedOwnerEndpointsRoundTripAsOneWord覆盖的是 pack/unpack 的边界值（INT32_MAX、pack(EMPTY,EMPTY) == UINT64_MAX）。现在只有 FIFO 测试里的小 task id 间接覆盖，边界丢了。这个测试改一行（换成读 local 数组）就能留，比删掉划算。 scheduler->is_scheduler() == 0/!= 0（scheduler_completion.h:151、scheduler_dispatch.h:53、scheduler_ready.h:314）—— 返回值已是bool，这几处是从整数字段迁移时留下的写法。SchedulerLocalState大约从 ~290B 涨到 ~510B，而它是aicore_execute里的栈对象。onboard CI 通过说明没爆，但后续 commit 还要往里加东西，建议确认一下 AICore 标量栈的余量。- 给
scheduler_initialize_local_config补一句注释：失败返回时local处于部分初始化状态，调用方不得使用（新测试RejectsInvalidTopologyBeforeMailboxAccess正是复用同一个local连续调用的）。
三、其他
改动规模：Core 769 行 / Test 1593 行 / Docs 8 行，总 churn 2370。超标部分几乎全在测试，且其中绝大多数是签名穿线（去掉 owner_state / scheduler_local_state 两个参数）和 fixture 抽取的机械改动，不是新逻辑 —— 单次可审，不要求拆分。
pto_isa.pin 仍固定在 c0d7148e，本 PR 未改动任何 pto-isa 头文件引用路径也没新增 include，确认当前 pin 够用即可。
最后：性能数据那块处理得很规范 —— 明确标注了「这是完整实现 vs 老 main 的测量，不是本 PR 单独的贡献」，还主动披露了 AIC Host 时间 +42% 需要单独查。这个自我约束值得保留。
4317d19 to
068c1b1
Compare
|
@poursoul 按原 review 的顺序逐项回复如下。 一、声明与实现不一致1.「热路径尚未消费 route 快照」与代码不符已修正 PR 描述：worker ID 已用于派发／完成路径，Executor 已消费本地 payload 地址信息。修复后使用一个公共 payload 起点和三个 16-bit worker ID，移除独立 route 结构；不再把这些消费归给后续提交。原来的 active/core_type 本地副本移除，但 GM 的参与控制保留。 2. 两处
|
068c1b1 to
e953777
Compare
Keep ready-owner queues and a single profiling loop counter in core-local state, consolidate local slot and completion bookkeeping, and validate immutable configuration after READY. Use compact worker IDs and a shared payload base; reconstruct self-ready tokens from occupied local slots. Retain GM mailboxes, worker participation, and per-task accounting. Share parameterized scheduler fixtures without changing their capacities. Preserve completion-stat ordering and endpoint boundary coverage, validate narrowed offsets and reserved fields, and guard invalid local worker lookup. Document immutable-cache assumptions and the 336-byte local state layout.
2 participants
A5 HBG repeatedly accesses GM for scheduler-owned bookkeeping and immutable configuration. This refactor moves ready-owner pending queues and the profiling loop counter into per-core local state, consolidates local slot/completion bookkeeping under
SchedulerLocalState, and captures validated configuration after READY for each invocation.Dispatch and completion already consume cached worker IDs. The Executor derives payload addresses from one shared payload-region offset and three 16-bit worker IDs. Fixed-prefix offsets use 32 bits after range checks; task-dependent offsets retain 64 bits. Per-worker core type and active copies are omitted, while shared GM participation control remains unchanged.
Self-execution readiness uses a pending-slot mask and reconstructs the publication from the local slot generation/state. A pending READY slot cannot be reused before the same-core Executor claims and completes it; generation, state, and duplicate-publication checks remain. Worker trace validity uses one mask, and the loop counter has one source in local state.
SchedulerLocalStateis 336 B under the 64-bit ABI, versus 264 B before this PR and 536 B in its earlier form. These are structure sizes, not the complete AICore stack high-water mark. Profiling fields remain allocated when profiling is disabled.Shared ready inboxes, dependencies, dispatch/completion mailboxes, and per-task completion accounting remain in GM. Dispatch-slot and completion-inbox offsets are read from the shared context rather than cached locally. Before bootstrap, each participating core invalidates its entire data cache; the callable table and task metadata are immutable for that invocation, allowing the two repeated immutable-line invalidations to be omitted.
The three scheduler test suites share a parameterized fixture, preserving capacities 16 and 8192 and the existing assertions. Owner-endpoint sentinel/INT32_MAX boundaries, narrowed-offset rejection, route consistency, and pending-notification stability are covered.
SchedulerCompletionStatskeeps its original field order. Named GM reserved bytes retain the wire layout and reject nonzero values.Follow-up: packed SSBUF mailboxes
This PR is the first commit of the A5 HBG SSBUF work. The earlier full stack remains available on
feat/a5-hbg-ssbuf-split; it has not yet absorbed these review changes.293a8001replaces GM dispatch/completion generations and Executor trace handoffs with packed SSBUF mailboxes. Localtiming_slotfields belong there, and Executor trace storage should be limited to the two self-execution slots.cfa87998batches completion accounting and reduces ready-directory/profiling overhead.subtask_slotis retained.The follow-up branch starts from the earlier form of this refactor and will need rebasing/adaptation before submission.
Expected overall benefit of the complete SSBUF optimization
A direct hardware comparison of the earlier complete implementation (
e9bbcf4a) against old main (72a4cc03) provides a performance reference for this PR plus its follow-ups. These are measured results for the complete implementation, used here to indicate the expected overall benefit; they are not measurements of this preparatory PR alone or of the newly split/rebased stack.kernel_repeats=8)Device time is the full on-NPU AICPU RunWall, not kernel-only time. The larger AIC chain repeats the scalar dependency-check/writeback body eight times; a separate profiling run measured about 5.3× the original per-task kernel duration. It preserves the same 64-task chain.
Measurement setup:
c0d7148e95ef73bd12a73165fdce4b723a3b7e72) on both sides, with separate build environments. The baseline receives only the matching chain workload and PTO pin, not the runtime optimizations.The complete implementation had been rebased onto
6e383fc5, so this old-main comparison also includes intervening mainline changes; it does not isolate the contribution of the three optimization commits. The split/rebased follow-ups still need their own performance validation.Host wall time was variable and does not establish a stable end-to-end gain. Observed trimmed Host means were 2513.7→3570.0 μs for AIC chain (+42.0%), 4022.0→2460.9 μs for AIV chain (−38.8%), 4016.9→3462.9 μs for the larger AIC chain (−13.8%), and 33265.2→32787.2 μs for paged attention (−1.4%). The AIC Host increase needs separate investigation; the Device reductions above should not be presented as end-to-end improvements.
Validation
test_result_reads_while_successor_runs_hbgfailed to observe the successor remaining Pending across the predecessor-result read; all predecessor results were valid. The same failure reproduced in an isolated run and with independently built, unmodified PR binaries. This remains a known validation limitation, not a claimed full onboard pass.e953777a. The final commit passed 26 HBG simulation tests and the phase-Profiling simulation test (one other test deselected). CI independently validates the final pushed commit.