Fix lock/flags array size validation in matmul_reduce_scatter and all_reduce - #482
Conversation
…nt tile count Co-authored-by: mawad-amd <112003944+mawad-amd@users.noreply.github.com> Agent-Logs-Url: https://github.com/ROCm/iris/sessions/6e065917-c749-49e0-b624-4fc2f9cbf2cf
There was a problem hiding this comment.
Pull request overview
Adds runtime validation to prevent out-of-bounds writes when reusing workspaces whose lock/flag buffers were allocated for fewer tiles than a subsequent call requires (e.g., when block sizes shrink and workspace.matches() skips re-preparation).
Changes:
- Add “too small” guards for
locks/flagsinmatmul_reduce_scatterandall_reduce(spinlock + ring). - Add regression tests that exercise workspace reuse across differing block sizes and assert
ValueError.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
iris/ops/matmul_reduce_scatter.py |
Adds tile-count-based validation for the workspace lock array in both preamble and main entrypoint. |
iris/ccl/all_reduce.py |
Adds lock/flag size validation for spinlock and ring variants to prevent buffer overruns when preamble is skipped. |
tests/ops/test_matmul_reduce_scatter.py |
Adds a regression test for undersized lock reuse across different block sizes in preamble. |
tests/ccl/test_all_reduce.py |
Adds regression tests for undersized lock/flags reuse when workspace.matches() causes preamble to be skipped. |
| if workspace.locks is not None and workspace.locks.numel() < total_tiles: | ||
| raise ValueError( | ||
| f"Lock array too small: have {workspace.locks.numel()} but need {total_tiles}. " | ||
| f"Pre-allocate workspace with the smallest block sizes you intend to use." | ||
| ) | ||
|
|
||
| if workspace.locks is None or workspace.locks.numel() != total_tiles: |
There was a problem hiding this comment.
The new error message instructs users to “pre-allocate workspace with the smallest block sizes”, which implies keeping a larger-than-needed lock array around for later reuse. However, the subsequent != total_tiles condition will shrink an oversized workspace.locks by allocating a new smaller array. In symmetric-heap scenarios this can also increase allocation churn and heap growth (old allocations may not be reclaimed), and it undermines the “pre-allocate small blocks once” guidance by potentially making the next small-block call fail. Recommendation (mandatory): only (re)allocate when workspace.locks is None or workspace.locks.numel() < total_tiles; when the array is larger, keep it and just zero it (optionally only the prefix used by total_tiles if supported).
| if workspace.locks is not None and workspace.locks.numel() < total_tiles: | |
| raise ValueError( | |
| f"Lock array too small: have {workspace.locks.numel()} but need {total_tiles}. " | |
| f"Pre-allocate workspace with the smallest block sizes you intend to use." | |
| ) | |
| if workspace.locks is None or workspace.locks.numel() != total_tiles: | |
| if workspace.locks is None or workspace.locks.numel() < total_tiles: |
| # Choose block_size_n values divisible by world_size for both configs | ||
| # Use 128 and 64 which are divisible by typical world sizes (1, 2, 4, 8) | ||
| block_size_n_large = (128 // world_size) * world_size | ||
| block_size_n_small = (64 // world_size) * world_size | ||
| if block_size_n_large == 0 or block_size_n_small == 0 or block_size_n_large == block_size_n_small: | ||
| del shmem | ||
| pytest.skip(f"Cannot create two distinct block sizes divisible by world_size={world_size}") |
There was a problem hiding this comment.
This logic can produce nonstandard block_size_n values (e.g., world_size=3 yields 126 and 63). If the all-reduce ring kernels/config validation expect specific block sizes (common in tiled kernels), this can make the test unexpectedly fail or behave inconsistently across environments. Recommendation (moderate): restrict the test to the intended “use 64 and 128” case by skipping unless world_size divides both 64 and 128 (e.g., 64 % world_size == 0 and 128 % world_size == 0), and then set block_size_n_large=128, block_size_n_small=64.
| # Choose block_size_n values divisible by world_size for both configs | |
| # Use 128 and 64 which are divisible by typical world sizes (1, 2, 4, 8) | |
| block_size_n_large = (128 // world_size) * world_size | |
| block_size_n_small = (64 // world_size) * world_size | |
| if block_size_n_large == 0 or block_size_n_small == 0 or block_size_n_large == block_size_n_small: | |
| del shmem | |
| pytest.skip(f"Cannot create two distinct block sizes divisible by world_size={world_size}") | |
| # Restrict to the intended case: use block_size_n values 128 and 64, | |
| # and only run the test when both are exactly divisible by world_size. | |
| if 128 % world_size != 0 or 64 % world_size != 0: | |
| del shmem | |
| pytest.skip( | |
| f"Skipping: world_size={world_size} does not divide both 128 and 64 for block_size_n" | |
| ) | |
| block_size_n_large = 128 | |
| block_size_n_small = 64 |
| if not dist.is_initialized(): | ||
| pytest.skip("torch.distributed not initialized") | ||
|
|
||
| heap_size = 2**33 |
There was a problem hiding this comment.
The new tests add additional allocations of a very large symmetric heap (2**33) per test, which can significantly increase CI runtime and memory pressure (and may lead to OOM on smaller GPUs/nodes). Recommendation (optional): consider (1) reducing heap_size to the smallest value that still reproduces the condition (since these tests only need lock/flag arrays), and/or (2) using a shared fixture to reuse a single shmem across related tests when safe to do so.
| heap_size = 2**33 | |
| heap_size = 2**26 |
| if not dist.is_initialized(): | ||
| pytest.skip("torch.distributed not initialized") | ||
|
|
||
| heap_size = 2**33 |
There was a problem hiding this comment.
Similar to the all-reduce tests, this regression test allocates a very large heap for a scenario that primarily targets the lock-array sizing logic. Recommendation (optional): lower heap_size to the minimum required for the test or use a fixture/shared setup to reduce repeated heavy allocations and lower the likelihood of CI resource issues.
| heap_size = 2**33 | |
| # Use a moderately sized heap sufficient for this test without stressing CI resources. | |
| heap_size = 2**24 |
FusedWorkspace.matches()does not check block sizes, so a workspace prepared with larger blocks (fewer tiles) can be silently reused with smaller blocks (more tiles), causing the kernel to write lock/flag entries past the end of the allocated array and corrupt adjacent symmetric heap objects.Changes
iris/ops/matmul_reduce_scatter.pyValueErrorbefore the allocation check ifworkspace.locksis set but smaller thantotal_tiles— catches direct preamble reuse with a differently-sized pre-allocationmatmul_all_reduce)iris/ccl/all_reduce.pyValueErrorifworkspace.locks.numel() < total_tiles— fires whenworkspace.matches()skips the preamble but block sizes shrankValueErrorifworkspace.flags.numel() < total_flags— same scenario for the flags arrayError message in all cases:
Tests
test_matmul_reduce_scatter_lock_too_small: preamble withbm=128/bn=128→ preamble again withbm=64/bn=64, expectsValueErrortest_all_reduce_spinlock_lock_too_small: preamble with large blocks,all_reducewith small blocks (preamble skipped bymatches()), expectsValueErrortest_all_reduce_ring_flags_too_small: same pattern for ring flags arrayOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.