Notified-COMM: [BUG FIX]: Fixing the failure path check and error handling - #31
Closed
joe-explr wants to merge 38 commits into
Closed
Notified-COMM: [BUG FIX]: Fixing the failure path check and error handling #31joe-explr wants to merge 38 commits into
joe-explr wants to merge 38 commits into
Conversation
Signed-off-by: Joseph Schuchart <joseph.schuchart@stonybrook.edu>
This commit adds notification support to the OSC SM component by implementing the put_with_notify, get_with_notify, rput_with_notify, and rget_with_notify functions. These functions perform the same operations as their non-notify counterparts but also increment notification counters after the data transfer completes. The changes include: - Added function pointer types for notify variants in osc.h - Added function prototypes in osc_sm.h - Implemented the notify functions in osc_sm_comm.c - Updated the module template to register the new functions - Removed TODO comments that have been addressed Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.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 Schuchart <joseph.schuchart@stonybrook.edu>
Each MPI process kept a per-rank byte offset in the shared node state and turned it into a pointer whenever the counters moved. That offset was redundant. The counters sit back to back in rank order, so where a rank's counters start follows from the capacities the node state already carries. Drop notify_counter_offset and let notify_bases be the only record of where the counters live. ompi_osc_sm_refresh_notify_bases() now takes the start of the counter region and walks it, and its two callers hand it the region they just placed: the area reserved inline in the main segment at window creation, and the new segment after a growth. The grow path republishes only the capacities. Nothing about counter placement is published in shared memory any more. Every MPI process derives the same layout from the same capacities, so the pointers still agree across the window, and a rank that is not growing no longer has its offset rewritten underneath it. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
MPI_Win_get_info never consults the osc module. It copies the info the window itself carries, which the infosubscribe machinery maintains, so ompi_osc_sm_get_info() has been dead code since the conversion to .super infosubscriber. Setting mpi_assert_max_num_notify there never reached the user. Subscribe to the key on the window instead, and report the assertion osc/sm actually enforces. The counter reservation is fixed when the window is created, so the callback returns the module's own value and an assertion handed to MPI_Win_set_info afterwards cannot appear to take effect. Such a key was previously neither honored nor reported: an unsubscribed key is stored internal and filtered out of the info handed back to the user. The key now always appears in MPI_Win_get_info, reading 0 when no assertion was made, matching how no_locks reports in osc/rdma. The value the callback returns is stored on the module rather than on its stack, since the subscriber copies it after the callback returns. Extend the win_notify test with the three cases this covers: the default window, an asserted window, and MPI_Win_set_info on each. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
ompi/test/general/win_notify.c creates every window on MPI_COMM_SELF, so make check never reaches the comm_size > 1 paths in osc/sm: the notification counters carved out of the shared segment at window creation, the allgather that sizes them, and the collective reallocation MPI_Win_set_num_notify performs when a rank asks for more counters than were reserved. MPI_Win_set_num_notify short-circuits at comm_size == 1 and returns before any of it. Add a program under test/simple, where the hand-launched MPI tests live, covering a notification passed around a ring, growth past the reservation followed by a notification from every peer, per-rank counter counts, and the info and attribute reporting. Collapsing every rank's counter base onto one address makes it fail on four ranks while win_notify still passes all of its checks, which is the coverage it exists to add. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
win_notify_multi has every rank write to the slot named by its own rank, but the window was a fixed eight ints, so a job of more than eight ranks wrote past the end of it. The runs that found the test useful were all at eight ranks or fewer, which is exactly why it went unnoticed. Size the window for the job instead. Window creation reports its errors through the communicator rather than through the window, which does not exist yet. The skips for a window that cannot be created were therefore unreachable: MPI_COMM_ WORLD's default handler aborted the job before MPI_Win_allocate_ shared ever returned. Ask the communicator to return errors first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
get_notify, rget_notify, and the accumulate_notify family (via osc_sm_notify_accumulate_done) each read and/or write the target's window, then bump its notification counter. That ordering only needs a load/store-then-store barrier: nothing in these functions reads or writes shared memory after the counter increment, so there is nothing for a full barrier's extra store-then-load ordering to protect. opal_atomic_wmb() built on GCC's or C11's __atomic_thread_fence with release ordering -- the backend this tree builds with -- gives exactly that: the standard defines a release fence as ordering any preceding read or write against any following write, not merely a preceding write against a following write. opal_atomic_mb() was paying for an ordering guarantee these call sites never used. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Every caller of this helper -- accumulate_notify, raccumulate_notify, get_accumulate_notify, and rget_accumulate_notify -- reaches it immediately after releasing the target's accumulate_lock, past nothing but an error check that returns before calling this at all on failure. opal_atomic_unlock() already issues a release fence there, and release ordering applies to everything sequenced after it in program order, not only the instruction immediately following it, so it already orders the accumulate's read/write against the counter store this helper performs. opal_atomic_add() is relaxed and adds no ordering of its own, so nothing was otherwise depending on the fence removed here. get_notify and rget_notify keep their own release fence: they take no lock, so there is no unlock to lean on. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Merging the old 'notified-rma' branch (bc02964, "First draft and TODOs for notified comm") brought in a struct field and three TODO comments that never became part of the real implementation. notify_counters (uint64_t **) was only ever used by that commit's ompi_osc_sm_get_with_notify(), a sketch that was never registered in the module's function table, had no prototype, no notification-index validation, and ordered its counter increment with opal_atomic_rmb() -- the exact ordering bug commit 773703c later documents fixing in the real ompi_osc_sm_get_notify(). That sketch itself did not survive the merge; only its now-orphaned field declaration did. The three TODO comments ask for exactly what already exists: put/get and rput/rget notify are implemented, registered, and tested. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
MPI-5.1 section 12.6.1 defines MPI_WIN_NOTIFICATION_NUM_UB as the number of notification counters the implementation supports, and MPI_WIN_NOTIFICATION_NUM_SB as the number it supports efficiently. The mpi_assert_max_num_notify info key is neither of those: it is the user asserting what will be requested, which lets us size the counter reservation. It says nothing about what osc/sm can do, so it must not lower the upper bound. Report NUM_UB as INT_MAX on every window -- osc/sm grows into a new shared segment on demand, so nothing short of memory bounds what may be attached -- and keep NUM_SB at the counters reserved in the window's segment at creation, which is the number that the assertion legitimately moves. For that upper bound to be true rather than advertising room we then refuse, MPI_Win_set_num_notify no longer rejects a request above the assertion. Such a request now grows the counters exactly as one above the default reservation does, which is what the num_notify_counters MCA parameter already documented. The tests that encoded the old cap check the growth instead: end to end on a counter that only the growth provided, and against a peer's count for the collective case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> 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> (cherry picked from commit fe59a33)
The previous fix computed "bad" but never sent the ULONG_MAX sentinel through the allgather, leaving the agree: label unused. Only -1 hit the sentinel, by accident of the cast; -5 became a huge request and crashed in the grow path, and a request above mpi_assert_max_num_notify grew instead of failing. Also correct the comment that called the assertion not a limit. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
Test the broadcast descriptor with the shmem framework's validity flag instead of peeking at seg_name. segment_create sets the flag only on success, and the zeroed descriptor rank 0 sends when it cannot create the segment has it clear, so every rank still fails together. Signed-off-by: Joseph Antony <jajoseph.antony18@gmail.com>
|
Hello! The Git Commit Checker CI bot found a few problems with this PR: d477d4f: Fix MPI_WIN_SET_NUM_NOTIFY hang on an invalid argu...
Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks! |
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.
No description provided.