Repository navigation
Fix: honor ASCEND_RT_VISIBLE_DEVICES at comm_hccl MemSetAccess sites - #1826
ChaoZheng109 merged 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughVMM access descriptors now convert ACL-logical device IDs to driver-visible IDs before memory access setup. Documentation records the field-specific ID rules. Visibility-isolation tests validate communication-domain mappings across remapped logical ranks. ChangesVMM device ID translation
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This change translates device identifiers for isolated-device memory access while preserving existing behavior for identity mappings; no actionable merge-blocking risk remains beyond normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Under `ASCEND_RT_VISIBLE_DEVICES` isolation a launcher renumbers the visible cards
to logical `0..N-1`. `comm_alloc_domain_windows` then fails on every card pair
except an identity mapping, aborting the domain with
domain_alloc_via_fabric: create local Fabric window -> 507899
507899 is `ACL_ERROR_RT_DRV_INTERNAL_ERROR`: the driver was handed a device id in
a numbering it does not use.
`aclrtMemAccessDesc::location.id` is consumed in the driver-visible space and must
translate, even though `aclrtMemSetAccess` is an ACL entry point. Its sibling
`aclrtPhysicalMemProp::location.id` must NOT — `aclrtMallocPhysical` rejects a
driver-visible id outright. Walking the window sequence one call at a time under
`ASCEND_RT_VISIBLE_DEVICES=1,2`, bound to logical 0 (card 1):
aclrtMemGetAllocationGranularity prop.location.id=0 (logical) -> 0
aclrtMallocPhysical prop.location.id=0 (logical) -> 0
aclrtMallocPhysical prop.location.id=2 (driver-visible) -> 107001
aclrtReserveMemAddress -> 0
aclrtMapMem -> 0
aclrtMemSetAccess desc.location.id=0 (logical) -> 507899
aclrtMemSetAccess desc.location.id=1 (driver-visible) -> 0
With the variable unset and card 1 bound directly the whole sequence completes.
Identical for `ACL_HBM_MEM_HUGE` (fabric) and `ACL_HBM_MEM_NORMAL` (ipc). The
asymmetry is per-field, not per-API, so `acl_hal_device.h` records both halves —
translating the memory property too would break allocation outright.
Three sites carry the id into `aclrtMemSetAccess`: a2a3's
`reserve_and_map_vmm_window`, the funnel for every local and imported window, and
a5's `alloc_windows_via_ipc` / `domain_alloc_via_ipc`, whose descriptors are
reused for their peer mappings.
The ST gains a comm-domain case beside the chip-init one, so both device-id layers
are covered: chip init never maps device memory, so it cannot catch this. It runs
on all granted cards rather than a single logical id, and keeps the existing
skip-on-identity guard — an identity grant makes the two id spaces the same number
and would pass without proving anything.
a2a3 verified on 910B2: with the stock runtime both ranks abort with 507899 on
cards 1,2; with this change they complete byte-exact, and an identity grant (cards
0,1) still passes. a5 has no hardware here and rides on CI.
Related: the peer-access-status confusion recorded at a5 comm_hccl.cpp:553-561 and
:972-977 (hw-native-sys#1018) is the same id-space split at a different call, left untouched.
1c2b5ba to
a3a3b35
Compare
| * aclrtMallocPhysical prop.location.id=0 -> 0 prop.location.id=2 -> 107001 | ||
| * aclrtMemSetAccess desc.location.id=0 -> 507899 desc.location.id=1 -> 0 |
There was a problem hiding this comment.
The two rows in this table use different driver-visible ids for the same logical id, and I can't tell which one is the typo:
Under isolation, binding logical 0 of ASCEND_RT_VISIBLE_DEVICES=1,2:
aclrtMallocPhysical prop.location.id=0 -> 0 prop.location.id=2 -> 107001
aclrtMemSetAccess desc.location.id=0 -> 507899 desc.location.id=1 -> 0
For logical 0 under 1,2 the driver-visible id is 1, which is what the MemSetAccess row uses. The MallocPhysical row uses 2 — that is the driver-visible id of logical 1, and it is also out of range of the logical space [0,1]. So 107001 there is equally consistent with "invalid device id", which means that half of the row does not distinguish the two id spaces at all.
The left half does: prop.location.id=0 -> 0 succeeds even though card 0 is not in the granted set, which is real evidence that the field is logical. The conclusion looks right to me — it's specifically the second column I can't reconcile.
This matters more than a normal doc nit because this table is the only stated evidence for the per-field-not-per-API asymmetry, and it's what will stop the next reader from "fixing" prop.location.id symmetrically and silently allocating on the wrong card. Could you confirm whether 2 is what was actually measured, and if so which logical id it was bound to?
The same figure appears in the PR description.
There was a problem hiding this comment.
You're right, and to answer the question directly: yes, 2 is what was measured, bound to logical 0. So the row does not distinguish the two spaces — and it is a bit worse than ambiguous, because 107001 is literally ACL_ERROR_RT_INVALID_DEVICEID, which makes "invalid id" the natural reading rather than a competing one.
Under a 1,2 grant nothing can discriminate: the translated id of logical 0 is 1, and 1 is itself a valid logical id there, so every value is legal in both spaces.
Re-measured on ASCEND_RT_VISIBLE_DEVICES=2,3 bound to logical 0 (card 2), where the translated id of the bound device falls outside the logical range [0,1]:
aclrtMallocPhysical prop.location.id=0 (bound logical) -> 0 2 GiB appears on card 2
prop.location.id=2 (translated) -> 107001
prop.location.id=1 (other logical) -> 507899
aclrtMemSetAccess desc.location.id=0 (logical) -> 507899
desc.location.id=2 (translated) -> 0
Now the 107001 row carries the argument: under a driver-visible reading, 2 is the bound card, so the call would have succeeded. aclrtMemGetAllocationGranularity rejects the same value identically, so the check is not specific to the allocation call, and the card the memory landed on was read off npu-smi's HBM column during a 2 GiB hold rather than inferred.
One thing your question made me test, and I had it wrong: I would have said a symmetric translation of the memory property allocates on the wrong card silently. It does not — 507899 for any other logical id — so it fails loudly. The header now says that instead, since "silent mis-allocation" would have been exactly the wrong warning to leave for the next reader.
Fixed in f6b511a, PR description included.
| } | ||
|
|
||
| // aclrtMemAccessDesc::location.id is consumed in the driver-visible space, unlike | ||
| // aclrtPhysicalMemProp::location.id above, which takes the ACL-logical id. Under |
There was a problem hiding this comment.
"unlike aclrtPhysicalMemProp::location.id above" — in this file the prop is not above: it lives in create_local_vmm_window at line 301, which is this function's caller and is defined below it.
Looks like this block was copied from the a5 file, where the wording is accurate (prop at 460, desc at 492).
Suggest: "unlike aclrtPhysicalMemProp::location.id in its caller create_local_vmm_window, which takes the ACL-logical id".
There was a problem hiding this comment.
Correct — copied from the a5 file, where prop (460) does precede desc (492). In this file it is in the caller create_local_vmm_window, defined below. Used your wording in f6b511a.
Review caught that the aclrtMallocPhysical row did not support the claim it was
cited for. It was taken under ASCEND_RT_VISIBLE_DEVICES=1,2 bound to logical 0,
and probed prop.location.id=2 — which is neither the translated id of the bound
device (that is 1) nor inside the logical range [0,1]. Since 107001 is literally
ACL_ERROR_RT_INVALID_DEVICEID, that rejection is equally explained by "invalid
id", so the row could not tell the two id spaces apart. Under that grant nothing
can: translated(logical 0) = 1 is itself a valid logical id, so every value is
legal in both spaces.
Re-measured under ASCEND_RT_VISIBLE_DEVICES=2,3 bound to logical 0 (card 2),
where the translated id of the bound device falls outside the logical range:
aclrtMallocPhysical prop.location.id=0 (bound logical) -> 0 2 GiB appears on card 2
prop.location.id=2 (translated) -> 107001
prop.location.id=1 (other logical) -> 507899
aclrtMemSetAccess desc.location.id=0 (logical) -> 507899
desc.location.id=2 (translated) -> 0
The 107001 row now settles the question: under a driver-visible reading, 2 *is*
the bound card, so the call would have succeeded. It does not, and the same value
is rejected identically by aclrtMemGetAllocationGranularity, so the check is not
specific to the allocation call. The card the memory lands on was read off
npu-smi's HBM column during a 2 GiB hold, not inferred.
One prediction is withdrawn by the same measurement: a symmetric translation of
the memory property does not silently allocate on the wrong card. It fails
loudly — 107001 for the translated id, 507899 for any other logical id — because
the field must name the bound logical device, not merely a logical one.
Also corrects the a2a3 comment: aclrtPhysicalMemProp is not "above" in that file,
it is in the caller create_local_vmm_window, which is defined below. The wording
was copied from the a5 file, where prop does precede desc.
No functional change.
Problem
Under
ASCEND_RT_VISIBLE_DEVICESisolation,comm_alloc_domain_windowsfails on every card pair except an identity mapping:507899isACL_ERROR_RT_DRV_INTERNAL_ERROR— the driver was handed a device id in a numbering it does not use. Cards0,1hide it because there logical == physical, which is also why serving has never hit it: DeepSeek V4 takes all 8 cards.This is the class #1600 fixed, at a site it did not cover — neither arch's
comm_hccl.cppincludedcommon/acl_hal_device.h.Root cause — the two
location.idfields want different id spacesaclrtMemAccessDesc::location.idis consumed in the driver-visible space and must translate, even thoughaclrtMemSetAccessis an ACL entry point. Its siblingaclrtPhysicalMemProp::location.idmust not —aclrtMallocPhysicalrejects a driver-visible id outright.Measured by walking the window sequence one call at a time, in one process. The grant matters: the translated id of the bound device has to fall outside the logical range, or the same value is legal in both spaces and no outcome can tell them apart — an earlier revision of this section probed
1,2, where it cannot. UnderASCEND_RT_VISIBLE_DEVICES=2,3bound to logical0(= card 2), logical space[0,1]:107001isACL_ERROR_RT_INVALID_DEVICEID, and that row is what fixes the field's id space: under a driver-visible reading,2is the bound card and the call would have succeeded. The card the memory lands on was read off npu-smi's HBM column during a 2 GiB hold, not inferred.With the variable unset and the card bound directly, the whole sequence completes — measured on card 1 when this was first traced, and identical for
ACL_HBM_MEM_HUGE(fabric) andACL_HBM_MEM_NORMAL(ipc). Today's re-measurement above coversACL_HBM_MEM_HUGEonly; the id spaces are a property of the fields, not ofmemAttr.The asymmetry is per-field, not per-API, so
acl_hal_device.hnow records both halves. A symmetric "fix" of the memory property fails loudly rather than silently allocating elsewhere —107001for the translated id,507899for any other logical id — because the field must name the bound logical device, not merely a logical one.Change
Three sites carry the id into
aclrtMemSetAccess:reserve_and_map_vmm_window— the funnel for every local and imported window, fabric and IPCalloc_windows_via_ipcanddomain_alloc_via_ipc— their descriptors are reused for the peer mappings, so the translation rides alongNo CMake change: both onboard host targets already carry
src/common/platform/include.Test
tests/st/vis_isolationgains a comm-domain case next to the chip-init one, so both device-id layers live in one module. Chip init never maps device memory, so it cannot catch this. The new case runs on all granted cards rather than a single logical id, and reuses the existing ascending-list rule and skip-on-identity guard — under an identity grant the two id spaces hold the same number and the test would pass without proving anything.Verification
a2a3, on 910B2 — A/B on the same granted cards in the same job, only the runtime differing:
1,2, logical 0,1create local Fabric window -> 5078990,1, logical 0,1The identity case matters: the broker hands out the lowest free cards, so a two-card request usually lands on exactly the mapping that proves nothing.
a5 — no a5 hardware available to me; it rides on
_st-npu-a5.yml. The change there is the same pattern as a2a3, applied to the two inlined copies.Related, not fixed here
The peer-access-status confusion already recorded in comments at a5
comm_hccl.cpp:553-561and:972-977(#1018) is the same id-space split at a different call (aclrtDevicePeerAccessStatus/aclrtDeviceEnablePeerAccesstake a peer id). Left untouched rather than widening this diff.