arch-riscv: Limit PTW level parallelism#938
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds PTW level-limit configuration and wiring, expands walker state and statistics for PTW blocking and miss-queue tracking, and updates RISC-V TLB timing translation to gate PTW starts, enqueue deferred misses, and notify refill hints. ChangesPTW Level Limit Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (2)
configs/example/idealkmhv3.py (1)
24-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse lower_snake_case for the new helper.
Rename
setPtwLevelLimitParamstoset_ptw_level_limit_paramsand update the two call sites. As per coding guidelines, “Functions and methods should use lower_snake_case naming convention”.Also applies to: 39-40
🤖 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 `@configs/example/idealkmhv3.py` around lines 24 - 30, Rename the helper setPtwLevelLimitParams to set_ptw_level_limit_params to match the lower_snake_case naming convention, and update both call sites that invoke it in the same config module so they reference the new function name consistently.Source: Coding guidelines
configs/example/kmhv3.py (1)
23-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse lower_snake_case for the new helper.
Rename
setPtwLevelLimitParamstoset_ptw_level_limit_paramsand update the two call sites. As per coding guidelines, “Functions and methods should use lower_snake_case naming convention”.Also applies to: 38-39
🤖 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 `@configs/example/kmhv3.py` around lines 23 - 29, The new helper name in this config uses mixed case instead of the required lower_snake_case convention. Rename setPtwLevelLimitParams to set_ptw_level_limit_params, and update both call sites that invoke it so they use the new function name consistently.Source: Coding guidelines
🤖 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 `@configs/common/xiangshan.py`:
- Around line 898-927: Reject non-positive values at argument parsing for the
PTW settings in the xiangshan parser. Update the `parser.add_argument` calls for
`--ptw-level0-limit`, `--ptw-level1-limit`, `--ptw-level2-limit`,
`--ptw-level3-limit`, and `--ptw-miss-queue-size` to use the existing
`positive_int` validator instead of plain `int`, so `main`/CLI setup only
accepts positive limits and queue sizes.
In `@src/arch/riscv/pagetable_walker.cc`:
- Around line 209-214: The PTW level limiting logic in
Walker::usePtwLevelLimitForStart currently only exempts forward and back
pre-requests, so Request::isPrefetch() traffic can still be gated and
backpressured. Update the gating path in usePtwLevelLimitForStart and the
related canStartPtwLevel/pre-start TLB checks to accept the request’s prefetch
status and skip PTW level limiting for prefetch requests while keeping the
existing non-prefetch restrictions intact.
- Around line 259-263: The PTW MissQueue admission logic in pagetable_walker.cc
can deadlock when ptwMissQueueSize is zero because non-front entries are
diverted to ptwMissQueueWaiters and can never be retried successfully. Add a
constructor or configuration validation guard in the PagetableWalker setup path
before enqueue/admission logic runs, and reject or clamp zero-sized
ptwMissQueueSize so the enqueue paths around the MissQueue handling and retry
admission checks cannot operate with an impossible queue size.
In `@src/arch/riscv/tlb.cc`:
- Around line 1780-1790: The PTW miss blocking logic in the RISC-V TLB timing
translation path is currently applied to all translations, including prefetches,
so prefetch requests can be queued behind demand misses and resource limits
unexpectedly. Update the checks around the walker calls in the translation
handling code to exclude prefetches by gating the FIFO/resource-blocking paths
with !req->isPrefetch() once the walker accepts that bit, and apply the same fix
in both affected translation sites so prefetches bypass the new PTW miss limit
as intended.
---
Nitpick comments:
In `@configs/example/idealkmhv3.py`:
- Around line 24-30: Rename the helper setPtwLevelLimitParams to
set_ptw_level_limit_params to match the lower_snake_case naming convention, and
update both call sites that invoke it in the same config module so they
reference the new function name consistently.
In `@configs/example/kmhv3.py`:
- Around line 23-29: The new helper name in this config uses mixed case instead
of the required lower_snake_case convention. Rename setPtwLevelLimitParams to
set_ptw_level_limit_params, and update both call sites that invoke it so they
use the new function name consistently.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fdf0aa3-1019-4664-b463-fac5759d9e67
📒 Files selected for processing (8)
configs/common/xiangshan.pyconfigs/example/idealkmhv3.pyconfigs/example/kmhv3.pysrc/arch/riscv/RiscvTLB.pysrc/arch/riscv/pagetable_walker.ccsrc/arch/riscv/pagetable_walker.hhsrc/arch/riscv/tlb.ccsrc/arch/riscv/tlb.hh
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/arch/riscv/pagetable_walker.cc (1)
1909-1913: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep PTW level limiting out of two-stage walks.
usePtwLevelLimit()includestwoStageMode, so the new two-stagedeferPtwLevelRead()paths can reserve/block PTW levels despite the PR scope being one-stage direct only.Proposed fix
bool Walker::WalkerState::usePtwLevelLimit() const { - return timing && (translateMode == defaultmode || - translateMode == twoStageMode) && + return timing && translateMode == defaultmode && !fromPre && !fromBackPre; }Also applies to: 1198-1202, 2046-2048, 2066-2068, 2101-2103
🤖 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 `@src/arch/riscv/pagetable_walker.cc` around lines 1909 - 1913, Keep PTW level limiting out of two-stage walks: Walker::WalkerState::usePtwLevelLimit() currently allows twoStageMode, which lets the new deferPtwLevelRead() flow reserve or block PTW levels during two-stage translation. Update the PTW-level limit checks in usePtwLevelLimit() and the related defer/level-read call sites (including the other referenced spots in pagetable_walker.cc) so the limit applies only to one-stage direct walks, not two-stage mode.src/arch/riscv/tlb.cc (1)
2320-2335: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not enqueue two-stage translations into the one-stage PTW MissQueue.
doTwoStageTranslate()now blocks onwalker->hasPendingPtwMiss()andcanStartPtwLevel(), which applies the direct PTW MissQueue/level limit to two-stage walks. The PR scope says this limit is only for timing, one-stage direct, non-prefetch PTW requests; leave two-stage PTW starts outside this queue path.🤖 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 `@src/arch/riscv/tlb.cc` around lines 2320 - 2335, The PTW MissQueue gating in doTwoStageTranslate is being applied to two-stage walks, but this queue/level limit should only affect timing, one-stage direct, non-prefetch PTW requests. Update the logic around walker->hasPendingPtwMiss(), canStartPtwLevel(), and enqueuePtwMiss() so two-stage translations bypass the MissQueue path entirely, while keeping the existing queueing behavior only for the intended one-stage case.
🧹 Nitpick comments (2)
src/arch/riscv/pagetable_walker.cc (1)
256-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename new helper methods to lower_snake_case.
The newly added helper names use lowerCamelCase; please rename the changed API surface and matching declarations/call sites, e.g.
ptw_miss_queue_hint_matchandnotify_tlb_refill_hint. As per coding guidelines,**/*.{cpp,cc,cxx,h,hpp,py}functions and methods should use lower_snake_case naming convention.🤖 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 `@src/arch/riscv/pagetable_walker.cc` around lines 256 - 268, Rename the newly added helper methods in Walker to lower_snake_case to match the project naming convention. Update ptwMissQueueHintMatch to ptw_miss_queue_hint_match and notifyTlbRefillHint to notify_tlb_refill_hint, and make sure any corresponding declarations and call sites use the same names consistently.Source: Coding guidelines
src/arch/riscv/tlb.hh (1)
246-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse lower_snake_case for the new helper methods.
refillHintMaySatisfyandretryTimingPtwMissare newly introduced APIs; rename them and their definitions/callers to lower_snake_case before the names spread. As per coding guidelines, “Functions and methods should use lower_snake_case naming convention.”<coding_guidelines>Also applies to: 280-282
🤖 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 `@src/arch/riscv/tlb.hh` around lines 246 - 248, The new helper APIs currently use camelCase, so rename refillHintMaySatisfy and retryTimingPtwMiss to lower_snake_case throughout their declarations, definitions, and all call sites. Update the corresponding method names in TLB-related code and any references in the surrounding TLB/MMU flow so the new helpers follow the project’s function naming convention before they are further propagated.Source: Coding guidelines
🤖 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 `@src/arch/riscv/pagetable_walker.cc`:
- Around line 293-309: The PTW miss queue hint retry path in
`retryPtwMissQueueHint()` can free capacity in `ptwMissQueue` but still leave
`ptwMissQueueWaiters` blocked until another unrelated event runs
`retryPtwMissQueue()`. After processing hint-resolved entries and updating the
PTW miss queue stats, explicitly kick the waiter-admission path from this
function (or otherwise reuse the existing `retryPtwMissQueue()` logic) whenever
queue space may have opened, so freed capacity immediately admits waiting
requests. Use the existing `processingPtwMissQueueHint`, `ptwMissQueue`, and
`ptwMissQueueWaiters` symbols to locate the hook point.
---
Outside diff comments:
In `@src/arch/riscv/pagetable_walker.cc`:
- Around line 1909-1913: Keep PTW level limiting out of two-stage walks:
Walker::WalkerState::usePtwLevelLimit() currently allows twoStageMode, which
lets the new deferPtwLevelRead() flow reserve or block PTW levels during
two-stage translation. Update the PTW-level limit checks in usePtwLevelLimit()
and the related defer/level-read call sites (including the other referenced
spots in pagetable_walker.cc) so the limit applies only to one-stage direct
walks, not two-stage mode.
In `@src/arch/riscv/tlb.cc`:
- Around line 2320-2335: The PTW MissQueue gating in doTwoStageTranslate is
being applied to two-stage walks, but this queue/level limit should only affect
timing, one-stage direct, non-prefetch PTW requests. Update the logic around
walker->hasPendingPtwMiss(), canStartPtwLevel(), and enqueuePtwMiss() so
two-stage translations bypass the MissQueue path entirely, while keeping the
existing queueing behavior only for the intended one-stage case.
---
Nitpick comments:
In `@src/arch/riscv/pagetable_walker.cc`:
- Around line 256-268: Rename the newly added helper methods in Walker to
lower_snake_case to match the project naming convention. Update
ptwMissQueueHintMatch to ptw_miss_queue_hint_match and notifyTlbRefillHint to
notify_tlb_refill_hint, and make sure any corresponding declarations and call
sites use the same names consistently.
In `@src/arch/riscv/tlb.hh`:
- Around line 246-248: The new helper APIs currently use camelCase, so rename
refillHintMaySatisfy and retryTimingPtwMiss to lower_snake_case throughout their
declarations, definitions, and all call sites. Update the corresponding method
names in TLB-related code and any references in the surrounding TLB/MMU flow so
the new helpers follow the project’s function naming convention before they are
further propagated.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 703ba7b4-e581-433b-af90-9aa7af826e9e
📒 Files selected for processing (4)
src/arch/riscv/pagetable_walker.ccsrc/arch/riscv/pagetable_walker.hhsrc/arch/riscv/tlb.ccsrc/arch/riscv/tlb.hh
🚧 Files skipped from review as they are similar to previous changes (1)
- src/arch/riscv/pagetable_walker.hh
There was a problem hiding this comment.
🧹 Nitpick comments (2)
configs/example/idealkmhv3.py (2)
39-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew function name doesn't follow lower_snake_case.
setPtwLevelLimitParamsuses camelCase-style naming; the repository guideline requires lower_snake_case for functions. While sibling functions in this file (setKmhV3IdealParams) already deviate from this, this is a newly introduced function per this PR and is a good opportunity to align with the convention (e.g.,set_ptw_level_limit_params).As per coding guidelines, "Functions and methods should use lower_snake_case naming convention."
🤖 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 `@configs/example/idealkmhv3.py` around lines 39 - 40, The new helper name does not follow the repository’s lower_snake_case convention. Rename setPtwLevelLimitParams to a lower_snake_case identifier such as set_ptw_level_limit_params, and update both call sites in the idealkmhv3 config to use the new name consistently.Source: Coding guidelines
32-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the PTW level-limit helper.
This is duplicated inconfigs/example/idealkmhv3.pyandconfigs/example/kmhv3.py; moving it to a shared helper will keep the two config paths from drifting.🤖 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 `@configs/example/idealkmhv3.py` around lines 32 - 40, The PTW level-limit setup in setKmhV3IdealParams is duplicated across the kmhv3 config paths, so extract it into a shared helper and call that helper from both config flows. Locate the repeated setPtwLevelLimitParams usage around setKmhV3IdealParams and the corresponding kmhv3 setup, move the common logic to a shared function/module, and update both call sites to use it so the behavior stays aligned.
🤖 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.
Nitpick comments:
In `@configs/example/idealkmhv3.py`:
- Around line 39-40: The new helper name does not follow the repository’s
lower_snake_case convention. Rename setPtwLevelLimitParams to a lower_snake_case
identifier such as set_ptw_level_limit_params, and update both call sites in the
idealkmhv3 config to use the new name consistently.
- Around line 32-40: The PTW level-limit setup in setKmhV3IdealParams is
duplicated across the kmhv3 config paths, so extract it into a shared helper and
call that helper from both config flows. Locate the repeated
setPtwLevelLimitParams usage around setKmhV3IdealParams and the corresponding
kmhv3 setup, move the common logic to a shared function/module, and update both
call sites to use it so the behavior stays aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cbb3af42-8430-4e28-a55b-6b229a9816af
📒 Files selected for processing (5)
configs/common/xiangshan.pyconfigs/example/idealkmhv3.pyconfigs/example/kmhv3.pysrc/arch/riscv/pagetable_walker.ccsrc/arch/riscv/tlb.cc
🚧 Files skipped from review as they are similar to previous changes (4)
- configs/common/xiangshan.py
- configs/example/kmhv3.py
- src/arch/riscv/pagetable_walker.cc
- src/arch/riscv/tlb.cc
Gekyume777
force-pushed
the
ptw-level-parallelism-limit
branch
from
76dce1b to
d40dd6b
Compare
Enable the existing PTW level resource model for two-stage walks and route MissQueue retries through the top-level translation path so H-mode requests preserve their translation mode when replayed.
Notify the PTW MissQueue when an H-mode all-stage leaf refill completes, then retry queued translations that can now be satisfied by the refreshed TLB state. Keep the hint path conservative: direct one-stage hints are unchanged, VS-stage and G-stage refill hints remain disabled, and H-mode hints only use all-stage leaf refill events. This commit is applied after merging the latest upstream xs-dev conflict resolution.
Gekyume777
force-pushed
the
ptw-level-parallelism-limit
branch
from
76dce1b to
4e4e6da
Compare
Skip PTW level limiting for prefetch requests. Keep the branch conflict resolution on the current xs-dev base. Remove temporary PTW level and MissQueue profiling stats.
Gekyume777
force-pushed
the
ptw-level-parallelism-limit
branch
from
c717a39 to
7c559ea
Compare
|
We can confirm the MissQueue size later. For now, let’s merge it into the main branch and adjust the configuration afterward if needed. |
2 participants
This PR adds a parameterized PTW level parallelism limit model for RISC-V one-stage direct translation.
Changes:
Summary by CodeRabbit