Skip to content

feat(cco): honor reqs.sdmaQueueCount, clamp to HW max, guard out-of-range queueId#501

Open
jhchouuu wants to merge 1 commit into
ROCm:mainfrom
jhchouuu:jhchouuu/cco-sdma-queue-count
Open

feat(cco): honor reqs.sdmaQueueCount, clamp to HW max, guard out-of-range queueId#501
jhchouuu wants to merge 1 commit into
ROCm:mainfrom
jhchouuu:jhchouuu/cco-sdma-queue-count

Conversation

@jhchouuu

Copy link
Copy Markdown
Collaborator

Motivation

ccoDevCommRequirements.sdmaQueueCount was declared but never consumed. The
SDMA channel count could only be set via MORI_SDMA_NUM_CHANNELS, because the
queues were materialized in ccoCommCreate — before reqs is available — and
ccoDevCommCreate merely copied comm->sdmaNumQueue. Setting
reqs.sdmaQueueCount had no effect.

Additionally, thread-scope put/commit/quietQueue did no bounds check on
queueId, so a queueId >= sdmaNumQueue was undefined behavior (wrong-queue
use or an out-of-bounds handle/signal access).

Changes

  • Wire reqs.sdmaQueueCount. SDMA queue setup is deferred to
    ccoDevCommCreate, where reqs is known. ccoSdmaSetupCommQueues resolves
    the count from reqs.sdmaQueueCount (0 = env default) and is idempotent per
    comm (the first DevComm fixes the count; later ones reuse the handles).
  • EnsureSdmaTransport(int requestedChannels = 0) connects with the
    requested count (0 = MORI_SDMA_NUM_CHANNELS) and exposes the count actually
    connected via SdmaChannels(); CCO adopts it so device-side indexing stays in
    bounds. SHMEM's existing no-arg call is unchanged (defaults to env).
  • Clamp instead of abort. Requests above the CDNA per-pair hardware max
    (kMaxSdmaChannelsPerPair = 8, i.e. 4 queues/engine × 2 engines) are clamped
    with a warning rather than failing in hsaKmtCreateQueueExt.
  • Guard out-of-range queueId in thread-scope put/commit/quietQueue:
    now a safe no-op. warp/block scope already clamp via the lane/thread → queue
    mapping.

Test Plan

Validated on MI355X (gfx950, 2 GPU, own build) with a 2-rank harness that sets
reqs.sdmaQueueCount directly and probes devComm.sdma.sdmaNumQueue:

  • reqs.sdmaQueueCount = 8 → 8 distinct queues; put+quiet PASS.
  • = 0, env unset → default 2; PASS.
  • = 0, MORI_SDMA_NUM_CHANNELS=4 → 4; PASS.
  • = 16 → clamped to 8 (warn, no crash); PASS.
  • out-of-range queueId (n+3) → recv untouched, kernel returns (no hang/fault).
  • Default path (no sdmaQueueCount) unchanged.
  • mori_cco and mori_shmem both build with the new EnsureSdmaTransport
    signature.

🤖 Generated with Claude Code

…ange queueId

reqs.sdmaQueueCount was declared but never consumed: the SDMA channel count
came only from MORI_SDMA_NUM_CHANNELS because queues were materialized in
ccoCommCreate, before reqs is available.

- Defer cco SDMA queue setup to ccoDevCommCreate, where reqs is known.
  ccoSdmaSetupCommQueues now resolves the count from reqs.sdmaQueueCount
  (0 = env default) and is idempotent per comm (first DevComm fixes the count).
- EnsureSdmaTransport takes the requested channel count and exposes the count
  actually connected via SdmaChannels(); cco adopts it so device-side indexing
  stays in bounds. shmem's no-arg call is unchanged (defaults to env).
- Clamp requests above the CDNA per-pair hardware max (kMaxSdmaChannelsPerPair
  = 8) with a warning instead of aborting in hsaKmtCreateQueueExt.
- Guard thread-scope put/commit/quietQueue against queueId >= sdmaNumQueue
  (out-of-range is now a safe no-op, not an OOB access).

Co-Authored-By: Claude Opus 4.6 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant