Repository navigation
Add: document A5 AICPU core-selection strategy - #1622
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: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis design document defines A5 AICPU topology classification for FG, FG+SMT, PG1, and PG2. It proposes topology APIs and JSON output, specifies default placement and affinity rules, and describes capacity validation and the common selection flow. ChangesA5 AICPU core selection
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/a5-fg-pg-core-selection.en.md`:
- Line 1: Add the A5 AICPU core-selection documentation page to the appropriate
navigation entry in mkdocs.yml so mkdocs build --strict recognizes it;
alternatively, remove the page from docs/ if it should not be published.
- Line 156: Clarify the PG2 policy text for O and Scheduler placement by
explicitly stating whether PG2 overrides the global SMT-isolation and
dedicated-physical-CPU rules, and whether a Scheduler may share O’s physical
CPU; ensure the same precedence and sharing behavior is specified consistently
in the related PG2 sections.
- Around line 120-141: Complete the topology contract around AicpuTopology by
defining AicpuLogicalCpu with physical_cpu_id, cluster_id, die_id, SMT-sibling
state, and primary-thread information, and specify its JSON serialization. Align
os_schedulable_cpu_ids with the serialized logical-CPU representation or
explicitly reference the authoritative schema so tests and DFX can reproduce
placement decisions.
- Line 155: Update the PG1 policy description to replace normative hard-coded
IDs such as [T6, T7, T8, T9, T10] with topology-relative selection using
cluster_id, die_id, physical_cpu_id, and driver-provided logical-thread
metadata; if the T list remains, mark it as a non-normative example and define
behavior for both failure orientations.
- Around line 208-215: Update the CPU-affinity selection procedure around the
scenario policy chosen from kFg, kFgSmt, kPg1, or kPg2 to validate all
policy-specific capacity requirements before emitting any affinity array,
including eligible physical CPUs, a primary thread, an idle O sibling, dedicated
Scheduler CPUs, and required PG1 compensation threads. Return insufficient
capacity without producing a partial layout when any selected-policy constraint
is unmet.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0dfb38f1-c876-40ad-8d1b-2073ce4a86f8
📒 Files selected for processing (1)
docs/design/a5-fg-pg-core-selection.en.md
| @@ -0,0 +1,219 @@ | |||
| # A5 AICPU Core-Selection Strategy for All Scenarios | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
Fix the strict documentation build before merge.
mkdocs build --strict fails because this page is in docs/ but is absent from mkdocs.yml navigation. Add docs/design/a5-fg-pg-core-selection.en.md to the navigation or remove the file from the docs tree.
🧰 Tools
🪛 GitHub Actions: docs / 1_build.txt
[error] 1-1: MkDocs strict build failed because this page exists in the docs directory but is not included in the nav configuration. Add it to mkdocs.yml or remove the file.
🪛 GitHub Actions: docs / build
[error] 1-1: MkDocs strict build failed because this page exists in the docs directory but is not included in the nav configuration. Command: mkdocs build --strict --site-dir _site.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/a5-fg-pg-core-selection.en.md` at line 1, Add the A5 AICPU
core-selection documentation page to the appropriate navigation entry in
mkdocs.yml so mkdocs build --strict recognizes it; alternatively, remove the
page from docs/ if it should not be published.
Source: Pipeline failures
| struct AicpuTopology { | ||
| AicpuScenarioType scenario_type; | ||
| bool scheduler_smt_enabled; | ||
| uint32_t logical_cpu_count; | ||
| std::vector<int32_t> surviving_cluster_ids; | ||
| std::vector<AicpuLogicalCpu> os_schedulable_cpus; | ||
| }; | ||
|
|
||
| AicpuTopology QueryAicpuTopology(uint32_t device_id); | ||
| ``` | ||
|
|
||
| The tool should also provide JSON output for tests and DFX: | ||
|
|
||
| ```json | ||
| { | ||
| "architecture": "a5", | ||
| "scenario_type": "PG1", | ||
| "scheduler_smt_enabled": true, | ||
| "logical_cpu_count": 12, | ||
| "surviving_clusters": [0, 1, 2], | ||
| "os_schedulable_cpu_ids": [2, 3, 4, 5, 6, 7] | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Complete the topology API and JSON contract.
AicpuTopology uses std::vector<AicpuLogicalCpu>, but this document does not define AicpuLogicalCpu or its required fields. The JSON example exposes only logical CPU IDs, while selection requires physical_cpu_id, cluster_id, die_id, SMT-sibling state, and primary-thread information. Define the type and its JSON serialization, or reference the authoritative contract. Otherwise tests and DFX cannot reproduce placement decisions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/a5-fg-pg-core-selection.en.md` around lines 120 - 141, Complete
the topology contract around AicpuTopology by defining AicpuLogicalCpu with
physical_cpu_id, cluster_id, die_id, SMT-sibling state, and primary-thread
information, and specify its JSON serialization. Align os_schedulable_cpu_ids
with the serialized logical-CPU representation or explicitly reference the
authoritative schema so tests and DFX can reproduce placement decisions.
| | -------- | ---------------------- | | ||
| | FG | O owns the last suitable physical CPU; every Scheduler uses a dedicated physical CPU close to O; prefer one die | | ||
| | FG+SMT | Same as FG; O uses the first logical thread of the last suitable physical CPU and leaves its sibling idle; Schedulers do not use the second SMT thread by default | | ||
| | PG1 | Use the fixed SMT compensation threads returned by the driver without searching other layouts; O uses the first logical thread of the last suitable physical CPU and leaves its sibling idle; four Schedulers use the fixed compensation threads closest to O. For a failed right-side cluster, use `[T6, T7, T8, T9, T10]` with T10 as O; mirror the layout for a left-side failure | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Replace hard-coded PG1 CPU IDs with topology-relative selection.
The document states that runtime must not infer topology from contiguous CPU IDs, but the PG1 policy requires [T6, T7, T8, T9, T10] and only describes one failure orientation. Select compensation threads from cluster_id, die_id, physical_cpu_id, and driver-provided logical-thread metadata, or define an authoritative mapping for every possible failed cluster. Keep the T list only as a non-normative example if it is illustrative.
Also applies to: 184-184
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/a5-fg-pg-core-selection.en.md` at line 155, Update the PG1 policy
description to replace normative hard-coded IDs such as [T6, T7, T8, T9, T10]
with topology-relative selection using cluster_id, die_id, physical_cpu_id, and
driver-provided logical-thread metadata; if the T list remains, mark it as a
non-normative example and define behavior for both failure orientations.
| | FG | O owns the last suitable physical CPU; every Scheduler uses a dedicated physical CPU close to O; prefer one die | | ||
| | FG+SMT | Same as FG; O uses the first logical thread of the last suitable physical CPU and leaves its sibling idle; Schedulers do not use the second SMT thread by default | | ||
| | PG1 | Use the fixed SMT compensation threads returned by the driver without searching other layouts; O uses the first logical thread of the last suitable physical CPU and leaves its sibling idle; four Schedulers use the fixed compensation threads closest to O. For a failed right-side cluster, use `[T6, T7, T8, T9, T10]` with T10 as O; mirror the layout for a left-side failure | | ||
| | PG2 | Order CPUs by die, cluster, physical CPU, and logical CPU; O uses the last eligible logical CPU without isolation; place Schedulers in O's cluster first and the remaining Schedulers on the other die | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Define the PG2 override for O and Scheduler isolation.
The PG2 policy says O uses a CPU “without isolation” and permits shared physical CPUs. The global rules say O leaves its SMT sibling idle and prefer dedicated physical CPUs. The common flow applies both sections without stating which rule wins. State whether PG2 overrides the global rules and whether a Scheduler may share O’s physical CPU. Otherwise implementations can produce different affinity arrays and capacity results.
Also applies to: 179-190, 214-215
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/a5-fg-pg-core-selection.en.md` at line 156, Clarify the PG2
policy text for O and Scheduler placement by explicitly stating whether PG2
overrides the global SMT-isolation and dedicated-physical-CPU rules, and whether
a Scheduler may share O’s physical CPU; ensure the same precedence and sharing
behavior is specified consistently in the related PG2 sections.
| 2. Group logical CPUs by `physical_cpu_id`. | ||
| 3. Use only the O/S-schedulable CPU pool returned by the driver. The default | ||
| `1O+4S` requires at least five logical CPUs. If fewer are available, return | ||
| insufficient capacity and do not emit a partial affinity array. | ||
| 4. Select a scenario policy from the returned `kFg`, `kFgSmt`, `kPg1`, or | ||
| `kPg2`. | ||
| 5. Place O and S using the default `1O+4S` policy in Section 4 and the global | ||
| constraints in Section 5. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Validate capacity against the selected policy.
Five logical CPUs do not prove that the layout has enough eligible physical CPUs, an available primary thread, an idle O sibling, dedicated Scheduler CPUs, or the required PG1 compensation threads. Validate these constraints for the selected scenario before emitting the affinity array. Otherwise the topology can pass validation and still violate its placement policy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/a5-fg-pg-core-selection.en.md` around lines 208 - 215, Update the
CPU-affinity selection procedure around the scenario policy chosen from kFg,
kFgSmt, kPg1, or kPg2 to validate all policy-specific capacity requirements
before emitting any affinity array, including eligible physical CPUs, a primary
thread, an idle O sibling, dedicated Scheduler CPUs, and required PG1
compensation threads. Return insufficient capacity without producing a partial
layout when any selected-policy constraint is unmet.
Document the existing topology probe contract and define scenario-specific placement and capacity rules for FG, FG+SMT, PG1, and PG2. Add the design page to MkDocs navigation so strict documentation builds include it.
bba35cb to
047c5ce
Compare
2 participants
Summary
Testing
pre-commit run --files docs/design/a5-fg-pg-core-selection.en.md