Fix: honor ASCEND_RT_VISIBLE_DEVICES at direct-HAL device-id sites - #1600
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:
📝 WalkthroughWalkthroughThe host occupancy and AICore register paths now translate ACL logical device IDs through ChangesDevice ID translation
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 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. 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: 1
🤖 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/a2a3/platform/onboard/host/host_regs.cpp`:
- Around line 54-79: The hal_physical_device_id parsing in
src/a2a3/platform/onboard/host/host_regs.cpp:54-79 and the corresponding mapping
logic in src/a2a3/platform/onboard/host/aicpu_topology_probe.cpp:65-90 must
validate the entire ASCEND_RT_VISIBLE_DEVICES list before remapping. For each
strtol result, require a delimiter or end-of-string, reject ERANGE and negative
values, and continue validating tokens even after finding the selected logical
ID; return the original logical ID whenever any token is invalid.
🪄 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: 2479d69e-04c1-4ade-aaa9-6c0c5d22351b
📒 Files selected for processing (2)
src/a2a3/platform/onboard/host/aicpu_topology_probe.cppsrc/a2a3/platform/onboard/host/host_regs.cpp
bb8a0c2 to
cc6a7e8
Compare
|
Could you give some background on |
ChaoZheng109
left a comment
ChaoZheng109
left a comment
There was a problem hiding this comment.
The direction is right and the diagnosis is convincing: HAL sits below ACL and has
no notion of ASCEND_RT_VISIBLE_DEVICES, so a call site that hands an ACL logical
id to halMemCtl / halGetDeviceInfo* targets the wrong device under VIS
isolation. I checked every dlsym'd HAL symbol on the a2a3 side and the three
sites this PR converts are all of them — profiling_copy.cpp (SVM/rtMemcpy)
and the remaining init_* in device_runner.cpp go through ACL/rt and correctly
keep the logical id. Within a2a3, the coverage is complete.
Requesting changes on two blocking points plus a missing test; the rest is in the
inline comments.
1. Blocking — a5 has the same defect and this PR does not touch it
The title and description say "AI-core register path" without qualifying the arch,
but only a2a3 is changed. a5 has the identical class of bug:
src/a5/platform/onboard/host/host_regs.cpp:62
halFunc(static_cast<uint32_t>(device_id), &map_info, &map_addr, &len)—halResMapsrc/a5/platform/onboard/host/aicpu_topology_probe.cpp:103—halGetDeviceInfosrc/a5/platform/onboard/host/aicpu_topology_probe.cpp:116and:122—halGetDeviceInfoByBuff
and the dsmi variant, both insidequery_cpu_topo
Note that a5 has one more call site than a2a3: query_cpu_topo has no a2a3
counterpart, so porting the a2a3 change verbatim would silently leave it behind.
Could you either extend the fix to a5, or state explicitly in the description why
a5 is out of scope and qualify the title accordingly? As it stands a reader will
assume both arches are covered.
2. Blocking — the comm_hccl.cpp precedent in the description doesn't hold
comm_hccl.cppalready accounts for this remapping; these two paths did not.
I couldn't reproduce this claim. a2a3's comm_hccl.cpp contains no mention of any
VISIBLE_DEVICES variable at all. The only two mentions in the tree are a5's
comm_hccl.cpp:569 and :987, and they differ from this PR on both counts:
- they refer to
ASCEND_VISIBLE_DEVICES(container-level, Ascend Docker
Runtime), notASCEND_RT_VISIBLE_DEVICES(process-level, ACL) — different
variables with different scopes; - they describe an unresolved limitation, not an id translation:
aclrtDevicePeerAccessStatuscannot resolve a peer that a fork'd single-device
process neveraclrtSetDevice'd, so the code degrades to a warning plus a
file_barrier(see #1018). There is no logical→physical mapping there to
follow as precedent.
Since this sentence is the PR's main "there is prior art for this" argument, could
you reword it? Otherwise the next reader will go looking for a reference
implementation in comm_hccl.cpp that isn't there.
3. Missing test — and the harness for it already exists
All the new risk lives in hal_physical_device_id, a pure function whose body is
almost entirely defensive parsing (6x, ERANGE, negatives, out-of-range index,
empty). None of it is covered.
That's fixable at near-zero cost: tests/ut/cpp/CMakeLists.txt:745-761 already
defines test_a2a3_aicpu_affinity_select, which compiles
aicpu_topology_probe.cpp directly and is labelled no_hardware. No new target,
no device needed — the only blocker is that the helper is static, so it has
internal linkage and the test can't see it. Extracting it into a shared header
(see my inline comment on the duplication) solves both at once.
Suggested cases for tests/ut/cpp/a2a3/test_aicpu_affinity_select.cpp:
unset · "6" · "4,5,6,7" at logical 2 · "6x" → identity · "4,5" at logical 7
(out of range) → identity · empty string · leading/embedded spaces.
Per .claude/rules/discipline.md §3 a bugfix should leave a regression barrier
behind. The device-level repro genuinely needs VIS-isolated hardware, but the
parser does not, and the parser is where the new failure modes are.
4. Nit — worth a line in the troubleshooting docs
"ACL compute path works, but halMemCtl returns rc=42" is a failure signature
that is essentially impossible to derive from first principles. Two existing
places would take it:
docs/troubleshooting/device-error-codes.md— one row for the signaturedocs/dynamic-linking.md:127already lists
halMemCtl | Onboard host_regs.cpp | Register address mapping | a2a3 only—
a parenthetical that HAL call sites take driver-visible ids, not ACL ids, would
land naturally there
Per .claude/rules/doc-consistency.md §4 this should ride in the same commit.
| // back to the physical id the HAL expects; identity when the var is unset | ||
| // (legacy / no isolation). Without this, a chip forked under VIS remapping | ||
| // queries the wrong physical device's registers and halMemCtl fails (rc=42). | ||
| static int64_t hal_physical_device_id(int64_t logical_id) { |
There was a problem hiding this comment.
This 40-line function is duplicated verbatim into aicpu_topology_probe.cpp:66,
and both files compile into the same host_runtime target. If a5 gets the same
fix (see the review body), it becomes four copies of one parser.
Could this move to a single shared header both arches can include? That also
un-blocks the unit test — as a file-local static it has internal linkage, so
test_a2a3_aicpu_affinity_select (which already compiles this TU) cannot reach it.
While moving it, I'd ask for the interface itself to carry the rule, rather
than relying on each call site to remember. The bug this PR fixes is precisely
"one call site forgot", and the current shape — translate inline, at each site —
has the same failure mode for every HAL call site added in the future. Two cheap
things would go a long way:
- a name that states the conversion rather than the destination, e.g.
acl_to_hal_device_id(), sohal_fn(acl_to_hal_device_id(device_id))reads as
a layer crossing at the point of use; - a doc comment on the declaration stating the contract explicitly: every direct
hal*call takes a driver-visible id and must go through this; ACL/rt entry
points (aclrtSetDevice,rtMalloc,rtMemcpy, stream APIs) take the logical
id and must not.
That way the next person adding a dlsym'd HAL call has the rule in front of them
instead of having to rediscover it via an rc=42.
There was a problem hiding this comment.
Fixed — moved the parser into a shared common/acl_hal_device.h as acl_to_hal_device_id; both a2a3 files and a5 now route through it, and the declaration carries the contract (direct hal*/dsmi_* take the driver-visible id and must go through it; aclrtSetDevice/rtMalloc/rtMemcpy/stream APIs take the logical id and must not). Now reachable by a unit test: tests/ut/cpp/common/test_acl_hal_device.cpp.
| } | ||
| errno = 0; | ||
| char *end = nullptr; | ||
| long val = std::strtol(p, &end, 10); |
There was a problem hiding this comment.
.claude/rules/codestyle.md §8 asks host-side code (src/{arch}/**/host/ included)
to be modern C++ and reserve C idioms for the host↔device POD boundary, which this
isn't. The raw const char * walk plus std::strtol plus errno is squarely the
C shape the rule steers away from.
std::string_view + std::from_chars expresses the same validation — including the
"token must end at a delimiter" check, which from_chars gives you directly via the
returned ptr — in noticeably fewer lines, and drops the errno global entirely
(from_chars reports overflow through ec == std::errc::result_out_of_range).
There was a problem hiding this comment.
Fixed — rewritten with std::string_view + std::from_chars in the shared header. The "token must end at a delimiter" check comes from the returned ptr, overflow from ec == std::errc::result_out_of_range; errno is gone.
| struct AddrMapInPara in_map_para; | ||
| struct AddrMapOutPara out_map_para; | ||
| in_map_para.devid = device_id; | ||
| in_map_para.devid = phys_device_id; |
There was a problem hiding this comment.
in_map_para.devid is now phys_device_id, but the EACCES retry warning below at
line 169 still prints the logical id:
LOG_WARN(
"halMemCtl rc=13 (EACCES) on devid=%lld attempt %d/%d, retrying after %d ms",
(long long)device_id, ...
);Under VIS isolation — the exact scenario this PR targets — that line names a devid
that did not fail. It matters more than a usual logging nit here, because the
comment right above it carries physical-numbering field knowledge ("the failure
consistently lands on dev=11 (last die of last chip in the 8-11 range)"), which a
logical id would quietly invalidate.
Suggest printing phys_device_id, or both (devid=%lld (acl=%lld)).
There was a problem hiding this comment.
Fixed — the EACCES retry warning now names the driver-visible id that actually failed, with the acl id alongside: devid=%lld (acl=%lld).
| // Reject no-digits, overflow, negative, or a token not terminated by a | ||
| // list delimiter / end-of-string (e.g. "6x"). | ||
| if (end == p || errno == ERANGE || val < 0 || (*end != '\0' && *end != ' ' && *end != ',')) { | ||
| return logical_id; |
There was a problem hiding this comment.
The "validate the whole list, else don't remap at all" policy is a good call, and
the comment explains it well. My concern is that the fallback is completely silent.
If someone typos the variable, this returns the logical id and the process goes on
to hit the wrong device — reproducing the very rc=42 this PR exists to eliminate,
with nothing in the log pointing at the environment. A LOG_WARN naming the
offending value would turn a silent mis-target back into a two-second diagnosis.
Same for the out-of-range case at the end (physical_id still -1 because the list
is shorter than logical_id): it currently shares the identity return with
"variable unset", but semantically it's a definite misconfiguration, not a
no-isolation default. Worth distinguishing in the log.
And on the success path, a LOG_DEBUG("device id: acl %lld -> hal %lld") would make
the whole class of problem self-evident in any future bug report.
These are diagnostics only, no behavior gating, so .claude/rules/env-macro-gating.md
doesn't apply.
There was a problem hiding this comment.
Fixed — the shared helper now logs every non-remap path: LOG_WARN naming the offending value on a malformed list; a distinct LOG_WARN for the out-of-range (list-too-short) case, kept separate from the unset/no-isolation default; and LOG_DEBUG("device id: acl %lld -> hal %lld") on the success path.
| return cached_fn; | ||
| } | ||
|
|
||
| // ASCEND_RT_VISIBLE_DEVICES remaps ACL logical device ids to a 0..N-1 range, |
There was a problem hiding this comment.
Minor wording: the value returned here is the driver-visible device id, which is
not necessarily the absolute physical id. If container-level ASCEND_VISIBLE_DEVICES
is also in play — a5's comm_hccl.cpp:569 documents that combination — the driver's
own device numbering is already remapped, so ASCEND_RT_VISIBLE_DEVICES indexes into
that set, not into npu-smi's numbering.
Doesn't affect correctness (indexing the driver-visible set is exactly what's needed),
but "driver-visible device id" would save the next person from comparing this value
against npu-smi output inside a container and concluding the mapping is broken.
There was a problem hiding this comment.
Fixed — the header doc now calls it the driver-visible id and notes it need not equal the absolute physical (npu-smi) id when a container-level ASCEND_VISIBLE_DEVICES is also remapping the driver's numbering.
|
Follow-up on point 4 of my review — my wording there ("one row for the signature") 1. Not a row in that table — the table holds
A bare 2. Only the last cell needs extending, to note that HAL call sites take the Either one alone would do the job; (1) is the higher-value of the two since it's |
@ChaoZheng109 Good question — background below. Who sets it, and why. Nothing in this repo sets it, and that's expected. Like
On "the fork-per-chip path passes the real device id". Under VIS isolation the id we're handed is the logical one. VIS's whole job is to hide the physical numbering and renumber the visible cards to Why it still needs fixing here. ACL reads the variable and remaps for its own APIs ( Reproduced on a2a3 (single card,
With the translation, the same isolated launch produces output identical to the legacy (physical-id, no-VIS) path. The helper is identity when the variable is unset, so it is a no-op for any non-isolated deployment (your "passes the real device id" case) and only engages once a launcher has actually renumbered the devices. |
80cbb07 to
79fed80
Compare
|
a5 now included — plus what's verified vs assumed Following the "if a5 gets the same fix, four copies" point, I pulled the parser into the shared Verified on hardware (a2a3): reproduced the Applied but NOT verified (a5): we don't have an a5 node, so the a5 changes are by analogy to the a2a3 fix:
Happy to go whichever way you prefer: (a) split a5 into a follow-up and land a2a3 alone; (b) drop the DSMI translation and leave a TODO if the id-space assumption is too shaky; or (c) keep as-is if someone can validate on an a5 box. |
|
@ChaoZheng109 every suggestions has been addressed. Let me know if this is ok for you or if anything else is needed from my side. As I was saying in the previous post, unfortunately I do not have direct access to an A5 :( |
79fed80 to
d086ac1
Compare
That's not a blocker — CI does. The Why this is cheap: no new test logic is needed. All the translated sites
Following the existing CI step pattern, the key is variable gets the physical id, task-submit --timeout 900 --max-time 900 --device auto --device-num 1 \
--env ASCEND_RT_VISIBLE_DEVICES="$TASK_DEVICE" \
--run "python -m pytest tests/st/a2a3/tensormap_and_ringbuffer/dummy_task \
--platform a2a3 --device 0 -v --pto-session-timeout 600"and the a5 twin with What it buys:
On the unit test you added — it's a good test of the parser, and I'd keep it; One thing it still won't cover: the DSMI fallback in a5's Worth a quick local check that VIS doesn't upset anything else in the harness |
|
@lterrac Thanks for turning these around so quickly — the shared header, the a5 Three things left from my side:
Nothing else from me beyond these three. |
092c060 to
82e363e
Compare
lterrac
changed the title
|
Thanks — the "CI does it" framing is exactly right. All three done: 1. VIS-isolation ST on both arches ( 2. CI red — the affinity ST compiles 3. Title + description updated to the both-arches shape, dropped the inaccurate Also rebased onto |
068f542 to
bb5a156
Compare
Under `ASCEND_RT_VISIBLE_DEVICES` isolation a launcher renumbers the visible cards to logical `0..N-1` and hands simpler logical ids. ACL honors the variable for its own entry points, but the chip's direct driver calls bypass ACL and index the driver-visible space. Passing them the ACL-logical id targets the wrong device: the AI-core register map and the AICPU topology probe -- both on the chip-init path (`init_aicore_register_addresses`, `probe_aicpu_topology`) -- fail with `halMemCtl rc=42`, while the ACL compute path works. `common/acl_hal_device.h::acl_to_hal_device_id` is the single translation, and carries the contract on its declaration: direct `hal*` / `dsmi_*` calls take the driver-visible id and must go through it; ACL/rt entry points take the logical id and must not. It validates the whole visible list before trusting any entry -- one malformed token means the environment is misconfigured, so it falls back to the unremapped id and logs why -- and is the identity when the variable is unset. Every direct-driver device-id site now routes through it: `halMemCtl` plus `halGetDeviceInfo*` on a2a3, `halResMap` plus `halGetDeviceInfo*` / `dsmi_get_device_info` on a5. Two layers of coverage, because the parser and the call sites fail differently: - `tests/ut/cpp/common/test_acl_hal_device.cpp` pins the parser's remap and its malformed / out-of-range fallback policy. Hardware-free. - `tests/st/vis_isolation` runs `dummy_task` in a subprocess with the granted cards exposed under the variable, so both onboard jobs exercise the translation on real a2a3 and a5 silicon every run. Chip init is the only path needed: `init_aicore_register_addresses` and `probe_aicpu_topology` run on every bring-up, so one scene test covers every translated site. A unit test cannot reach this layer -- it passes on a branch where the helper exists and no call site was ever wired to it. The ST addresses a logical id whose card differs from its position, which is what makes the mapping observable; addressing one where they coincide would pass under the identity mapping. The visible list is ascending because `Runtime::GetVisibleDevices` rejects any other order with RT_ALL_ORDER_ERROR, voiding the whole list, after which even `rtSetDevice` fails with 107001 before a single HAL call is reached. The subprocess is load-bearing too: the variable must be set before ACL init, and mutating it in-process would leak into the session's pooled workers. Verified on hardware: reproduced `halMemCtl rc=42` on a2a3 under `ASCEND_RT_VISIBLE_DEVICES=6` with the worker handed logical `0`; the translation makes it disappear and the model produces output identical to the legacy physical-id path. One caveat. The a5 `dsmi_get_device_info` fallback in `query_cpu_topo` is only reached when `halGetDeviceInfoByBuff` fails first, so the ST does not cover it. It is translated for consistency but assumes DSMI shares the HAL's driver-visible id space -- flagged with an inline NOTE. If that turns out wrong it will surface on a real a5 node and can be revisited then. Co-authored-by: ChaoZheng109 <zhengchao47@huawei.com>
|
@lterrac I pushed to your branch directly (maintainer access) rather than sending 1. VIS verification moved from
|
|
Hi @ChaoZheng109, thanks for jumping in and reducing the review cycles, I really appreciate the help. Everything looks good to me, so if there is nothing left you can merge this PR :D |
2 participants
Under
ASCEND_RT_VISIBLE_DEVICESisolation a launcher renumbers the visible cards to logical0..N-1and hands simpler logical ids. ACL honors the variable for its own APIs (aclrtSetDevice,rtMalloc/rtMemcpy, streams, HCCL), but the chip's direct driver calls bypass ACL and index the driver-visible space. Passing them the ACL-logical id targets the wrong device: the AI-core register map and the AICPU topology probe — both on the chip-init path (init_aicore_register_addresses,probe_aicpu_topology) — fail withhalMemCtl rc=42, while the ACL compute path works.Fix
Add
common/acl_hal_device.h::acl_to_hal_device_id, a single translation that carries the contract on its declaration: directhal*/dsmi_*calls take the driver-visible id and must go through it; ACL/rt entry points take the logical id and must not. It validates the whole visible list (any malformed token → logged identity fallback) and is the identity when the variable is unset.Route every direct-driver device-id site through it, on both arches:
halMemCtl(register map) +halGetDeviceInfo*(AICPU topology).halResMap(register map) +halGetDeviceInfo*/dsmi_get_device_info(AICPU topology).Verification
st-onboardjobs runsdummy_taskunderASCEND_RT_VISIBLE_DEVICESwithpytest --device 0, so CI exercises the chip-init translation on a real a2a3 and a real a5 node every run (pre-fix it fails withrc=42; post-fix it passes). Permanent regression barrier for anyone who adds a direct-HAL call site and forgets to translate.VIS=6, logical0→ physical6):rc=42gone, output identical to the legacy physical-id path.tests/ut/cpp/common/test_acl_hal_device.cpp.Caveat
The a5
dsmi_get_device_infofallback inquery_cpu_topois only reached whenhalGetDeviceInfoByBufffails first, so the VIS ST does not cover it. It is translated for consistency but assumes DSMI shares the HAL's driver-visible id space — flagged with an inlineNOTE. If that turns out wrong it will surface on a real a5 node and can be revisited then.