UCX Notified Communication - #20
Open
joe-explr wants to merge 10 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? |
joe-explr
force-pushed
the
notified-rma-ucx
branch
from
July 24, 2026 20:00
93195db to
ad1271b
Compare
|
Hello! The Git Commit Checker CI bot found a few problems with this PR: ad1271b: Get and set notify changes
Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks! |
joe-explr
force-pushed
the
notified-rma-ucx
branch
from
July 24, 2026 20:14
ad1271b to
2ac49ff
Compare
joe-explr
force-pushed
the
notified-rma-ucx
branch
from
August 12, 2026 04:59
2ac49ff to
8081698
Compare
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>
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>
MPI_WIN_SET_NUM_NOTIFY is a blocking, synchronizing collective, but its num_notifications argument is local: MPI-5.1 section 12.6.1 states that the number of notification counters "can be different for different MPI processes". Both the C binding and osc/sm validated that argument and returned early, before the osc module's internal allgather. A single rank passing a bad value therefore returned an error while every other rank stayed blocked in that allgather forever, turning an erroneous argument into a hang. The binding rejected negative counts, and osc/sm additionally rejected counts above an mpi_assert_max_num_notify assertion -- a case the binding never covered, so osc/sm could hang even before this change. Drop the range check from the binding and carry each rank's verdict through the collective instead. osc/sm gathers ULONG_MAX as a sentinel that no legal count can collide with, since valid counts come from an int and never exceed INT_MAX; a single-process window has nobody to agree with and still answers immediately. All ranks then see the same gathered array and fail identically, so the window cannot end up half-reconfigured. osc/ucx already carried its verdict through the allgather and needed no change. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
The notification counters were reserved once at window creation and that reservation was a hard cap: MPI_WIN_SET_NUM_NOTIFY rejected any larger count, and MPI_WIN_NOTIFICATION_NUM_UB reported the reservation. MPI-5.1 section 12.2 defines the mpi_assert_max_num_notify info key as an assertion by the caller that it will not request more counters than the given value, and states that when the key is absent (zero) "the implementation does not assume any limit on the number of notification counters". Capping an unasserted window at the default reservation contradicts that, and left osc/ucx unable to satisfy programs that osc/sm -- which grows its counters -- accepts. Grow the counters instead. MPI_WIN_SET_NUM_NOTIFY already gathers every rank's requested count, so all ranks reach the same decision from the same array without extra communication, and grow to the largest request. Growing re-registers the region and re-exchanges base addresses, which is safe precisely because this procedure is a blocking, synchronizing collective: it resets every counter to zero, so a freshly allocated region is already the required contents, and it is erroneous to call it while an access epoch is open, so no remote atomic can be in flight against the region being replaced. The address allgather doubles as the barrier that lets the old region be released. The reservation is never shrunk, so a rank that lowers its count keeps its space and only genuine growth costs a re-registration. A window keeps a hard cap only when *every* rank asserted a bound; a rank that passed no key made no promise. NUM_UB now reports INT_MAX for an unasserted window and the asserted value otherwise, and NUM_SB follows the reservation, so both attributes stay consistent with what MPI_WIN_SET_NUM_NOTIFY will actually accept. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
The request-based notified operations built their request from the underlying rput/rget/accumulate and only then issued the counter update, so the request described the data movement alone. Two consequences. MPI-5.1 section 12.6.4 advises that "completion at the origin entails that the notification counter update has been sent to the target and thus notifications do not rely on progress of decoupled MPI activities at the origin". Because the counter update is a non-fetching atomic issued after the request-bearing flush, MPI_WAIT could return with that update still queued locally, leaving a polling target waiting until the origin happened to re-enter MPI. The failure paths were also malformed: once the underlying operation had succeeded, *request held a live request, yet a failing fence or atomic returned an error. A caller following the usual convention -- on error, do not touch the request -- would then never complete it. The request could not simply be released there either, since UCX already holds it with a completion callback. Issue the data movement and the counter update first, and build the request afterwards. The request is attached to ucp_worker_flush_nb, which covers every operation already issued on the worker regardless of which registration it used, so the flush now covers the counter update as well. Allocating the request last also means every failure point precedes it and those paths return with *request untouched. The accumulate variants get the same treatment. Their underlying accumulate is synchronous -- it ends with a blocking flush and completes its request immediately -- so passing a NULL request runs the accumulate to completion at the target and lets the notification that follows be ordered after it, with a fresh request covering both. Factor the request construction shared by all six operations into osc_ucx_request_over_flush(). rput and rget keep their previous behaviour. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
joe-explr
force-pushed
the
notified-rma-ucx
branch
from
September 4, 2026 20:15
c5c7b00 to
b140aa3
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
2 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