Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
RISC-V LL/SC selection needs A-extension gating, and unconstrained retry loops need a documented progress guarantee or fallback.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Adds 64-bit RISC-V LL/SC atomics and explicit acquire-load support for OPAL’s lock-free FIFO/LIFO structures.
Changes:
- Adds and registers RISC-V LR/SC implementations.
- Adds acquire-load primitives and pointer wrappers.
- Updates ARM64, PowerPC, FIFO, and LIFO ordering.
| File | Summary |
|---|---|
opal/include/opal/sys/riscv64/Makefile.am |
Registers the RISC-V atomic header. |
opal/include/opal/sys/riscv64/atomic_llsc.h |
Implements RISC-V LL/SC operations; requires A-extension gating and a fallback for unconstrained retry loops. |
opal/include/opal/sys/powerpc/atomic_llsc.h |
Adds acquire LL operations and ordering distinctions. |
opal/include/opal/sys/atomic.h |
Exposes acquire primitives and architecture selection. |
opal/include/opal/sys/atomic_impl_ptr_llsc.h |
Adds pointer-sized acquire wrappers. |
opal/include/opal/sys/arm64/atomic_llsc.h |
Separates relaxed and acquire LL/SC operations. |
opal/class/opal_lifo.h |
Uses acquire loads when popping. |
opal/class/opal_fifo.h |
Uses acquire loads and updates tail ordering. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Configure-time validation and forward-progress detection need correction, and the release-note citation must be updated.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (2)
Comment on lines
+553
to
+557
| AS_IF([test "$opal_check_riscv64" = "yes"], | ||
| [AS_IF([test "$enable_riscv_llsc_lifo" = "no"], | ||
| [riscv_llsc_result=0], | ||
| [test "$enable_riscv_llsc_lifo" = "yes"], | ||
| [riscv_llsc_result=1], |
devreal
force-pushed
the
lifo-weak-memory-redux3
branch
3 times, most recently
from
September 23, 2026 01:07
c447e09 to
28534dd
Compare
devreal
force-pushed
the
lifo-weak-memory-redux3
branch
2 times, most recently
from
October 5, 2026 20:16
d7f52f3 to
6b50af0
Compare
Previously, mca_pml_ob1_accelerator_init() unconditionally created two accelerator streams and 2*N events (default N=400) at MPI_Init time whenever a non-null accelerator component was selected, even if no device buffers were ever passed to MPI. Split the initialization into two phases. The existing mca_pml_ob1_accelerator_init() now only allocates the lightweight ring-buffer bookkeeping structures (pointer arrays and counters). A new static mca_pml_ob1_accelerator_lazy_init() creates the streams and events on first actual use, called through the public mca_pml_ob1_accelerator_ensure_init() wrapper. This also fixes a correctness issue: streams and events created at MPI_Init time capture whatever accelerator device happened to be selected at that point. Applications that set their device after MPI_Init would get streams and events bound to the wrong device. Deferring creation to the first MPI call that passes a device buffer ensures the resources are created in the correct device context. Lazy init is triggered from: - mca_pml_ob1_record_htod_event() before acquiring the htod lock - mca_pml_ob1_get_dtoh_stream() and mca_pml_ob1_get_htod_stream() (covering the COPY_ASYNC BTL path, e.g. btl/smcuda) - mca_pml_ob1_recv_request_progress_rget() when the receive buffer is a device buffer (covering the btl/sm + smsc/accelerator RGET path) - mca_pml_ob1_send_request_start_accelerator() for large device buffer sends A new MCA parameter pml_ob1_accelerator_lazy_init (default 1) allows the eager behavior to be restored for environments where streams and events should be pre-created before any GPU operation. Signed-off-by: Edgar Gabriel <edgar.gabriel@amd.com> Signed-off-by: Edgar Gabriel <Edgar.Gabriel@amd.com>
…or init Two correctness issues in the lazy accelerator initialization introduced in the previous commit: Add release/acquire memory barriers around the streams-initialized flag to ensure correct visibility on weakly-ordered architectures (aarch64, ppc64). A write barrier (opal_atomic_wmb) is inserted in mca_pml_ob1_accelerator_lazy_init() before setting the flag, so all stream and event stores are published before the flag becomes visible. The matching read barrier (opal_atomic_rmb) is inserted in mca_pml_ob1_accelerator_ensure_init() on the lock-free fast path, so a thread observing the flag is guaranteed to also see the fully-created streams and event arrays. Guard the stream pointer returned by mca_pml_ob1_get_dtoh_stream() and mca_pml_ob1_get_htod_stream() with a NULL check before installing the CONVERTOR_ACCELERATOR_ASYNC flag and the stream pointer in the convertor. If lazy init failed (e.g. due to stream or event creation errors) the getters return NULL, and setting ASYNC with a NULL stream would crash. Fall back silently to synchronous copies in that case. Co-authored-by: George Bosilca <gbosilca@nvidia.com> Signed-off-by: Edgar Gabriel <Edgar.Gabriel@amd.com>
The ob1 accelerator path pre-allocated two arrays of mca_pml_ob1_accelerator_events_max (400) events up front, one per direction, whether or not a device buffer was ever used. Replace that with a single event pool that starts empty and grows on demand, in batches of mca_pml_ob1_accelerator_events_batch events (new MCA parameter, default 32), up to the mca_pml_ob1_accelerator_events_max cap. Two counters describe the pool: the current number of allocated events and the cap it may grow to. When a batch cannot be fully created the pool keeps whatever events were made and shrinks to match; the "Out of event handles" error is raised only once the pool sits at the cap and not one event could be created. The growth batch is validated at registration to lie in [1, mca_pml_ob1_accelerator_events_max]. The completion events are not tied to a direction, so a single pool now backs all asynchronous copies and the event machinery is named accordingly (mca_pml_ob1_record_event, mca_pml_ob1_progress_one_event, and so on). The caller of record_event passes the stream the copy was queued on, so an event can be used with any stream. The separate device-to-host event pool, which was never actually fed any events, is removed; only the direction-specific streams (htod_stream, dtoh_stream) remain. Also remove the pml_ob1_accelerator_lazy_init MCA parameter. Deferring stream and event creation until the first device buffer is used is now unconditional; there is no reason to offer an eager mode that binds a process to a device during MPI_Init. Fold the two-function lazy init into a single out-of-line slow path, mca_pml_ob1_accelerator_create_streams(), reached through an inline mca_pml_ob1_accelerator_ensure_init(). The common, already-initialized case is now a lock-free flag read plus an acquire barrier with no function call on the critical path; only the first device-buffer use per process falls through to the slow path. Signed-off-by: George Bosilca <gbosilca@nvidia.com>
…celerator-init pml/ob1: defer accelerator stream and event creation to first use
devreal
force-pushed
the
lifo-weak-memory-redux3
branch
2 times, most recently
from
October 6, 2026 15:49
96ed958 to
f443ca6
Compare
- Introduces explicit acquire-ll, linked load with acquire semantics - Adds implementation of ll/sc for 64bit RISC-V platforms. Note: The RISC-V "A" extension spec (Volume I, §A.6 "Eventual Success of Store-Conditional Instructions") defines a constrained LR/SC loop that is guaranteed to eventually succeed: > A constrained LR/SC loop contains at most 16 instructions in a 64-byte aligned region, > has exactly one LR, one SC, and one backward branch to the LR, no > other memory accesses (besides the LR and SC themselves), and no other branches or jumps. The load of the next item pointer strictly makes our loops non-constrained. However, in practice current platforms will correctly handle this and allow the loop to succeed. A configure check attempts to check whether non-constrained loops are safe and disables LL/SC if not. Signed-off-by: Joseph Schuchart <joseph.schuchart@stonybrook.edu>
devreal
force-pushed
the
lifo-weak-memory-redux3
branch
from
October 7, 2026 18:19
f443ca6 to
1a29829
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: The RISC-V "A" extension spec (Volume I, §A.6 "Eventual Success of Store-Conditional Instructions") defines a constrained LR/SC loop that is guaranteed to eventually succeed:
The load of the next item pointer strictly makes our loops non-constrained. However, in practice current platforms will correctly handle this and allow the loop to succeed.