UCX Notified Communication - #20
Open
joe-explr wants to merge 351 commits into
Open
Conversation
devreal
requested changes
Apr 1, 2026
devreal
left a comment
devreal
left a comment
Owner
There was a problem hiding this comment.
Looks good, just a few comments.
| + (uint64_t)notify * sizeof(uint64_t); | ||
| } | ||
|
|
||
| #define CHECK_NOTIFY_IDX(notify) \ |
Owner
There was a problem hiding this comment.
I would suggest passing in the module and checking against a value stored in there. OMPI_OSC_UCX_MAX_NOTIFY_COUNTERS is just a crutch until we have a better solution
|
|
||
| /* Counters are local memory — just read with a barrier to ensure | ||
| * any preceding remote writes to this counter are visible. */ | ||
| opal_atomic_rmb(); |
Owner
There was a problem hiding this comment.
Not sure this actually does anything. Shouldn't the read barrier come after the counter read to prevent subsequent reads from being reordered?
|
|
||
| volatile uint64_t *counter = | ||
| (volatile uint64_t *)(module->addrs[my_rank] + module->size) + notify; | ||
| *value = (OMPI_MPI_COUNT_TYPE)opal_atomic_swap_64((volatile int64_t *)counter, 0); |
Owner
There was a problem hiding this comment.
You will need to use UCX to swap the value, otherwise it is not guaranteed to be atomic wrt to other network atomic operations.
| /* create the segment */ | ||
|
|
||
| size_t total = 0; | ||
| size_t notify_size = OMPI_OSC_UCX_MAX_NOTIFY_COUNTERS * sizeof(uint64_t); |
Owner
There was a problem hiding this comment.
Replace the use of OMPI_OSC_UCX_MAX_NOTIFY_COUNTERS with a local variable that we can set to something that comes out of the info object. That makes it easier going forward. Also, store the value of that variable in the module (see my earlier comment).
Owner
|
Oh, I guess we'll need to get #9 in first? |
Owner
|
Is this ready to go in? |
Bump actions/checkout to v6
Signed-off-by: BRELLE, EMMANUEL <emmanuel.brelle@bull.com>
Signed-off-by: BRELLE, EMMANUEL <emmanuel.brelle@bull.com>
Signed-off-by: BRELLE, EMMANUEL <emmanuel.brelle@bull.com>
Signed-off-by: BRELLE, EMMANUEL <emmanuel.brelle@bull.com>
Signed-off-by: BRELLE, EMMANUEL <emmanuel.brelle@bull.com>
…on osc/ubcl Signed-off-by: BRELLE, EMMANUEL <emmanuel.brelle@bull.com>
Signed-off-by: BRELLE, EMMANUEL <emmanuel.brelle@bull.com>
Signed-off-by: BRELLE, EMMANUEL <emmanuel.brelle@bull.com>
Signed-off-by: Brelle Emmanuel <emmanuel.brelle@bull.com>
…main Update 05/2026: UBCL components with latest Bull cleaning
lsf: add note to docs about problems with
coll/accelerator: stage full input for MPI_IN_PLACE in reduce_scatter and rs_block
…ulate" This reverts commit 43c29a01dbbe44c35dfba315f5bbc56b8d34d63d. Type has just been introduced by recent versions Signed-off-by: Brelle Emmanuel <emmanuel.brelle@bull.com>
…efines Signed-off-by: Brelle Emmanuel <emmanuel.brelle@bull.com>
Signed-off-by: Brelle Emmanuel <emmanuel.brelle@bull.com>
…inking flags become optional Signed-off-by: Brelle Emmanuel <emmanuel.brelle@bull.com>
Signed-off-by: Brelle Emmanuel <emmanuel.brelle@bull.com>
…FORTRAN_LOGICAL16 Signed-off-by: Brelle Emmanuel <emmanuel.brelle@bull.com>
Provide a concise entry point for AI coding agents (and the humans driving them) working in the Open MPI source tree. It captures the mental model -- the OPAL/OMPI/OSHMEM projects and their linker boundaries, the MCA architecture -- and the handful of conventions agents most often get wrong (prefix rule, config.h-first, no back-end MPI_*() calls, copyright headers, warning-free builds). It also covers build/smoke-test flow, generated/do-not-edit trees, performance discipline, and the sign-off / commit / branch process. Rather than duplicate the authoritative developer docs, it links into docs/developers/ and docs/contributing.rst so it stays a thin, low-drift orientation map. Signed-off-by: Jeff Squyres <jeff@squyres.com>
Add AGENTS.md to orient AI coding agents
Correct clear spelling mistakes, repeated words, and grammatical issues across the RST documentation tree. This includes developer and user guides, tuning and installation docs, release notes, and MPI/OpenSHMEM man-page sources. Also fix misspelled RST labels and update their references so the documentation links continue to resolve correctly. Validation performed: - aspell scan over docs/**/*.rst with false positives manually filtered - targeted rg sweeps for common misspellings, repeated words, and grammar patterns - git diff --cached --check Signed-off-by: Jeff Squyres <jeff@squyres.com>
Add guidance for agents to wrap commit-message lines at around 75 characters. Also clarify that release-branch cherry-picks must leave the cherry-pick tagline at the end of the commit message. Point agents at git cherry-pick -x as the preferred way to add it. Signed-off-by: Jeff Squyres <jeff@squyres.com>
may need to use a different email address Signed-off-by: Howard Pritchard <howardp@lanl.gov>
AGENTS: document commit message conventions
docs: add code of conduct file
MPI_Dist_graph_create exchanges distributed-graph edge metadata with PML sends and receives on the parent communicator before the new communicator is fully created. The topo base code used hard-coded internal tags -50 and -51 for this exchange. Those tags are reachable by the dynamic nonblocking collective tag allocator. In repeated communicator creation, comm->c_nbc_tag can count down into -50/-51. This creates a race where some ranks may still be matching distributed-graph edge traffic while other ranks have already entered ompi_comm_nextcid and started its nonblocking allreduce on the same parent communicator. The resulting tag collision can leave the CID allocation request waiting forever. Reserve the distributed-graph edge tags in coll_tags.h as static internal tags, before MCA_COLL_BASE_TAG_STATIC_END, so the libnbc nonblocking collective range starts below them. Use those central tag definitions in topo_base_dist_graph_create instead of local hard-coded values. This keeps distributed-graph construction traffic disjoint from nonblocking collective traffic on the parent communicator and avoids the loop-triggered deadlock in MPI_Dist_graph_create. Fixes open-mpi#13918 Signed-off-by: George Bosilca <gbosilca@nvidia.com>
Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
put_with_notify get_with_notify Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
put_with_notify
get_with_notify
Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
…for a single and multi rank window. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
…eph.antony18@gmail.com>
… <jajoseph.antony18@gmail.com>
MPI-5.1 names the error class for an invalid notification index MPI_ERR_RMA_NOTIFICATION. Rename the placeholder used by the notified RMA work to match the standard. The class was never registered with the error code subsystem, so MPI_Error_string() and MPI_Error_class() did not know about it; add the missing CONSTRUCT_ERRCODE()/OBJ_DESTRUCT() pair in errcode.c. Also fix the binding generator's ERROR_CLASSES list, where the entry was inserted without a trailing comma and so was silently concatenated with the following 'MPI_ERR_TYPE' element rather than added as a class of its own. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Add the two remaining notification-management procedures from MPI-5.1 section 12.6.1: MPI_WIN_SET_NUM_NOTIFY is a blocking, synchronizing collective that sets the number of notification counters attached at the calling MPI process to exactly num_notifications and resets all of them to zero. MPI_WIN_GET_NUM_NOTIFY is local and returns the number of counters attached at target_rank. Both are wired through the osc framework as new module entry points, so components that do not implement them return MPI_ERR_UNSUPPORTED_OPERATION rather than crashing. The osc/sm implementation carves a fixed per-rank counter region out of the shared segment at window creation, which is therefore the effective MPI_WIN_NOTIFICATION_NUM_UB; a request beyond that capacity is rejected with MPI_ERR_ARG. Each rank publishes its own attached count into the shared segment, so the collective needs only a barrier -- no counts have to be exchanged -- and MPI_WIN_GET_NUM_NOTIFY is a plain shared-memory read. Also add the missing put_notify/get_notify entries to interface_profile_sources, which were omitted when those two procedures were introduced. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Three correctness problems in the osc/sm notified communication path: 1. The notification counters were plain uint64_t but are incremented concurrently by remote origins with opal_atomic_add() and polled by the local rank. Type them opal_atomic_int64_t so that the reads in MPI_WIN_GET_NOTIFY_VALUE are atomic and cannot be hoisted out of a caller's polling loop. 2. The notification index was validated *after* the data movement, so an erroneous call had already overwritten the target window (or, for get, the origin buffer) by the time the error was returned. MPI-5.1 section 12.6.1 makes referencing an out-of-range counter erroneous at initiation, so hoist the check above ompi_datatype_sndrcv() in all four notified operations. The check is factored into a helper, which also fixes MPI_GET_NOTIFY returning OMPI_ERR_BAD_PARAM instead of MPI_ERR_RMA_NOTIFICATION. 3. The get paths used opal_atomic_rmb() before incrementing the target's counter. The notification tells the target that the get has read the window, so the constraint is load-before-store, which a load-load fence does not express; opal_atomic_add() is relaxed and adds no ordering of its own. Use a full opal_atomic_mb(). In MPI_WIN_GET_NOTIFY_VALUE the barrier was likewise placed before the counter load, where it ordered nothing; move it after so that it gives the acquire semantics the caller needs. MPI_WIN_RESET_NOTIFY_VALUE also gains the trailing barrier for the same reason. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Adds the six remaining notified operations from the MPI-5.1 draft -- accumulate, get_accumulate, and the four request-based forms -- so that osc/sm covers all eight defined in section 12.3, and sizes the notification counters from window info rather than a compile-time constant. The counter region was a fixed 16 entries per MPI process carved out of the shared segment, and MPI_WIN_SET_NUM_NOTIFY rejected anything larger. That conflicts with the mpi_assert_max_num_notify info key, whose default of 0 the standard defines as "the implementation does not assume any limit on the number of notification counters". The reservation now comes from that key when one is given, and otherwise from a new osc_sm_num_notify_counters MCA parameter. A request beyond the reservation relocates the counters to a dedicated shared segment instead of failing; when the key was given it is a hard bound, since the window was sized on the strength of that assertion. Growth is collective and runs inside MPI_WIN_SET_NUM_NOTIFY, which the standard already defines as a blocking synchronizing collective. Every process agrees on the new layout through an allgather of the requested counts, and on whether the attach succeeded through an allreduce, so a failure at one process cannot leave others incrementing counters that nobody reads. The published count stays clamped to the current allocation until the larger one exists, so a failed growth cannot leave behind a count that would admit writes past the end of the region. A barrier separates the attach from the unlink, because attach opens the backing file by name and the broadcast does not tell rank 0 that the other processes are finished with it. Each process now caches a per-target pointer to the counters, making the lookup on the path of every notified operation a single indexed load -- cheaper than the previous base-plus-offset arithmetic -- so the ability to relocate the region costs the hot path nothing. Also corrects the reset at the end of component_select(), which zeroed the whole node state and so wiped the notification fields it had just written, and removes a stray double semicolon in ompi_osc_sm_fetch_and_op(). Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
MPI-5.1 section 12.2.6, Table 12.1 caches three attributes on every window: MPI_WIN_NOTIFICATION_NUM_SB, the number of notification counters the implementation supports efficiently; MPI_WIN_NOTIFICATION_NUM_UB, the upper bound on that number; and MPI_WIN_NOTIFICATION_VALUE_UB, the upper bound on a counter value. Without them a program has no portable way to ask how many counters it may request, since MPI_WIN_GET_NUM_NOTIFY reports how many are attached rather than how many are available. The values come from a new osc_win_get_notify_bounds entry point on the osc module, queried once per window while it is configured. A component that does not implement notified communication leaves the entry point NULL and all three attributes read zero, which is the honest answer for such a window and is consistent with its notified operations returning MPI_ERR_UNSUPPORTED_OPERATION. For osc/sm the bounds follow the reservation: with an mpi_assert_max_num_notify assertion both NUM_SB and NUM_UB are that value, since the window was sized for exactly it; without one, NUM_SB is what was reserved and nothing bounds NUM_UB short of the notification index type, because the counters grow on demand. The keyvals are appended after MPI_FT so that the existing predefined values stay put, with matching entries in mpif-values.py to keep the C and Fortran numbering identical. The predefined-keyval bitmap is already bounded by MPI_ATTR_PREDEFINED_KEY_MAX and needed no change. Table 12.1 types VALUE_UB as MPI_Count *, and the attribute machinery has no MPI_Count slot -- every other predefined attribute is integer- or address-valued. It is stored as an MPI_Aint, which is the same width wherever Open MPI runs now that 32-bit environments are unsupported; the reasoning is recorded at the call site so the choice does not later read as a type error. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Covers counter management, all eight notified operations in their blocking and request-based forms, notification index errors, growth past the reserved capacity, the mpi_assert_max_num_notify bound, and the three notification window attributes. A second test forces osc/rdma and checks that every notified entry point reports MPI_ERR_UNSUPPORTED_OPERATION without moving any data, and that the attributes read zero. That is the contract which lets components that do not implement the chapter remain untouched, so it is worth testing directly rather than assuming. Both are single-process tests wired into make check, so the shared segment growth path is exercised only in its single-rank form, where the counters are a plain heap allocation. The collective path -- segment creation, broadcast, attach, the status allreduce and the barrier before unlink -- needs a multi-rank test that this harness cannot host. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
The notified communication code carried long block comments that restated the standard at length and recorded design deliberation. Reduce them to short notes that say what the code does and cite the relevant MPI-5.1 section, so the comment density matches the surrounding osc/sm sources. No functional change. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
MPI_WIN_GET_NUM_NOTIFY takes target_rank as a nonnegative integer in the group of the window, but the frontend did not validate it. The osc/sm and osc/ucx backends both range-check it and return MPI_ERR_RANK, so this was not a crash, but argument validation belongs in the frontend under MPI_PARAM_CHECK, consistent with the notified communication operations and with MPI_Win_shared_query. MPI_PROC_NULL is deliberately not accepted here: unlike the notified communication operations, the spec specifies target_rank as nonnegative. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
The base branch renamed the notified-communication error class from MPI_ERR_NOTIFY_IDX to MPI_ERR_RMA_NOTIFICATION; osc/ucx still referenced the old, now-undefined name. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
joe-explr
force-pushed
the
notified-rma-ucx
branch
from
August 12, 2026 04:59
2ac49ff to
8081698
Compare
Adds the four accumulate-flavored notified operations, the notification window attributes, and a configurable counter reservation, and corrects MPI_Win_set_num_notify. MPI_Accumulate_notify, MPI_Get_accumulate_notify, MPI_Raccumulate_notify and MPI_Rget_accumulate_notify follow the pattern already used by the put/get variants: issue the base operation, order it ahead of the counter with a fence (or a flush where a result must also be locally valid), then post the atomic increment. The five copies of that increment are now one helper. osc_win_get_notify_bounds was left NULL, so ompi_win_init cached zero for MPI_WIN_NOTIFICATION_NUM_SB, MPI_WIN_NOTIFICATION_NUM_UB and MPI_WIN_NOTIFICATION_VALUE_UB on every UCX window. Zero is how a component says it supports no counters at all, so a program that checked the attributes before calling MPI_Win_set_num_notify would skip notified communication even though UCX implements it. MPI_Win_set_num_notify only ever raised the attached count, but §12.6.1 says MPI_WIN_GET_NUM_NOTIFY returns the value given to MPI_WIN_SET_NUM_NOTIFY, so lowering it has to take effect. Gathering the requested value directly gives that, and also stops an origin from addressing counters the target has since detached. A rank whose argument is out of range no longer returns before the allgather either; that left the rest of the group blocked in a synchronizing collective. The reservation was a hard-coded 16. It is now the osc_ucx_num_notify_counters MCA parameter, overridable per window with the mpi_assert_max_num_notify info key, matching osc/sm. Because the counters share the window's memory registration, which cannot grow, the reservation is a real upper bound and is reported as such in MPI_WIN_NOTIFICATION_NUM_UB; osc/sm can reallocate its segment and so advertises INT_MAX. Ranks agree on one value so the shared-memory layout stays uniform. MPI_Win_notify_threshold (§12.6.3) is still unimplemented. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The counters were appended to the window data and registered as part of the same region. For MPI_WIN_FLAVOR_CREATE the data region is the caller's buffer, sized for the window data alone, so this registered and then wrote past the end of memory the MPI library does not own -- at window creation, and again on every remote notification. Making the reservation configurable turned that from a fixed 128-byte overrun into one the user can scale. It went unnoticed because the notified communication tests only ever call MPI_Win_allocate. The counters now get their own registered region, alongside the window state rather than inside the window data. That removes the overrun, and since the region no longer depends on there being a data region at all, dynamic windows can support notified communication instead of being refused with MPI_ERR_RMA_FLAVOR. The shared-memory segment layout goes back to what it was before notified communication was added. Also from review of the previous commit: MPI_Win_set_num_notify validated its argument on each rank before the allgather, so a rank whose count was out of range returned while the rest of the group stayed blocked in the collective. The check now rides the collective. A refused call also no longer republishes the attached counts, so the group is not left half-reconfigured; the counters are still reset first, which is what makes the standard's "will not return until all processes have adjusted" hold. Window creation had the same defect: a malformed mpi_assert_max_num_notify value returned before the reservation allreduce. The failure is carried through that collective now. The reservation is also range-checked before being narrowed to int for the exchange. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Notification counters are incremented by remote origins with a UCX atomic. Depending on the transport and the atomic mode in use, UCX may emulate that atomic in software on the target's worker rather than offloading it to the NIC. When it does, the counter only advances while the local worker is progressed. MPI_WIN_GET_NOTIFY_VALUE read the counter through a volatile pointer without progressing the worker, so a consumer polling it in a loop -- the natural way to wait for a notification, and the pattern the notified-communication interface exists to support -- could spin forever on a value that can never change. The operation completed only if the application happened to call some other MPI routine that progressed the worker as a side effect. Progress the worker before reading the counter, as every other spin-wait in this component already does. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
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
14 participants
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.
Signed-off-by: Joseph Antony jajoseph.antony18@gmail.com