Notified-COMM: [BUG FIX]: Fixing the failure path check and error handling

ompi/mca/osc/sm/osc_sm.h

@@ -100,7 +100,8 @@ struct ompi_osc_sm_module_t {
     void *notify_segment_base;
     /* mpi_assert_max_num_notify as given at window creation, or 0 for none.
      * It sized the counter reservation and is what MPI_WIN_NOTIFICATION_NUM_SB
-     * and the window's info report; it does not cap what may be attached. */
+     * and the window's info report; MPI_Win_set_num_notify rejects a request
+     * above it. */
     unsigned int notify_max_assert;
     /* notify_max_assert rendered for the window's info; the info subscriber
      * hands this back, so it must outlive the callback that returns it. */

ompi/mca/osc/sm/osc_sm_comm.c

@@ -146,7 +146,7 @@ osc_sm_grow_notify_counters(ompi_osc_sm_module_t *module, const unsigned long *n
         return ret;
     }
 
-    if ('\0' == new_seg_ds.seg_name[0]) {
+    if (!OPAL_SHMEM_DS_IS_VALID(&new_seg_ds)) {
         return MPI_ERR_NO_MEM;
     }
 
@@ -223,7 +223,7 @@ ompi_osc_sm_win_set_num_notify(struct ompi_win_t *win,
     int rank = ompi_comm_rank(module->comm);
     unsigned long requested = (unsigned long) num_notifications;
     unsigned long *new_caps;
-    bool grow = false;
+    bool grow = false, bad;
     int ret, i;
 
     /* "mpi_assert_same_num_notifications" would let us skip the allgather below
@@ -232,15 +232,30 @@ ompi_osc_sm_win_set_num_notify(struct ompi_win_t *win,
      * synchronizing and collective. */
     (void) info;
 
-    if (num_notifications < 0) {
-        return MPI_ERR_ARG;
+    /* num_notifications is a local argument -- MPI-5.1 12.6.1 allows it to
+     * differ between MPI processes -- but this is a synchronizing collective.
+     * A rank that rejected its own value and returned here would leave every
+     * other rank blocked in the allgather below, turning an erroneous argument
+     * into a hang.  So the validity rides through the collective as a sentinel
+     * and all ranks fail together.  A multi-process window defers the decision;
+     * a single-process one has nobody to agree with and can answer now. */
+    bad = (num_notifications < 0)
+          || (0 != module->notify_max_assert &&
+              requested > (unsigned long) module->notify_max_assert);
+
+    if (bad) {
+        if (1 == comm_size) {
+            return MPI_ERR_ARG;
+        }
+        /* Valid counts come from an int, so they can never equal ULONG_MAX */
+        requested = ULONG_MAX;
+        goto agree;
     }
 
-    /* mpi_assert_max_num_notify is the user asserting what will be requested,
-     * not a limit on what osc/sm supports (MPI-5.1 section 12.2.3).  It sized
-     * the reservation made at window creation; a request above it is served
-     * exactly like one above the default reservation, by growing into a new
-     * shared segment. */
+    /* mpi_assert_max_num_notify is this rank's promise not to ask for more
+     * (MPI-5.1 section 12.2.3); a request above it was rejected above.  A
+     * request above the current capacity but within the promise is served by
+     * growing into a new shared segment. */
 
     memset((void *) module->notify_bases[rank], 0,
            module->node_states[rank].notify_counter_capacity * sizeof(int64_t));
@@ -267,6 +282,7 @@ ompi_osc_sm_win_set_num_notify(struct ompi_win_t *win,
         return OMPI_SUCCESS;
     }
 
+agree:
     new_caps = malloc(sizeof(*new_caps) * comm_size);
     if (NULL == new_caps) {
         return OMPI_ERR_TEMP_OUT_OF_RESOURCE;
@@ -281,6 +297,16 @@ ompi_osc_sm_win_set_num_notify(struct ompi_win_t *win,
         return ret;
     }
 
+    for (i = 0 ; i < comm_size ; ++i) {
+        if (ULONG_MAX == new_caps[i]) {
+            /* Some rank supplied an invalid count.  Every rank sees the same
+             * gathered array, so they all report the same error and none of
+             * them reconfigures. */
+            free(new_caps);
+            return MPI_ERR_ARG;
+        }
+    }
+
     for (i = 0 ; i < comm_size ; ++i) {
         if (new_caps[i] > module->node_states[i].notify_counter_capacity) {
             grow = true;

ompi/mca/osc/sm/osc_sm_component.c

@@ -160,9 +160,11 @@ static int component_register (void)
     opal_asprintf(&description_str,
                   "Number of RMA notification counters reserved per MPI process "
                   "in the shared memory segment of each window.  Windows whose "
-                  "info gives an mpi_assert_max_num_notify value use that "
-                  "instead.  MPI_Win_set_num_notify may exceed this value, at "
-                  "the cost of allocating a new shared segment (default: %u)",
+                  "info gives an mpi_assert_max_num_notify value reserve that "
+                  "many instead, and MPI_Win_set_num_notify may not exceed it.  "
+                  "Without the assertion MPI_Win_set_num_notify may exceed this "
+                  "value, at the cost of allocating a new shared segment "
+                  "(default: %u)",
                   mca_osc_sm_component.num_notify_counters);
     (void) mca_base_component_var_register(&mca_osc_sm_component.super.osc_version,
                                            "num_notify_counters", description_str,
@@ -218,8 +220,8 @@ static int osc_sm_reserved_notify_counters(opal_info_t *info, unsigned int *asse
 /* Report the mpi_assert_max_num_notify actually in effect on the window.  The
  * counter reservation is fixed when the window is created, so a value handed to
  * MPI_Win_set_info afterwards cannot change it and is deliberately ignored --
- * returning our own value leaves MPI_Win_get_info describing the reservation
- * osc/sm really made rather than what was last asked for. */
+ * returning our own value leaves MPI_Win_get_info describing what osc/sm really
+ * enforces rather than what was last asked for. */
 static const char *
 osc_sm_notify_assert_info(opal_infosubscriber_t *obj,
                           const char *key __opal_attribute_unused__,

ompi/mpi/c/win_set_num_notify.c.in

@@ -30,10 +30,9 @@ PROTOTYPE ERROR_CLASS win_set_num_notify(WIN win, INFO info, INT num_notificatio
             return OMPI_ERRHANDLER_NOHANDLE_INVOKE(MPI_ERR_WIN, FUNC_NAME);
         } else if (NULL != info && MPI_INFO_NULL != info && ompi_info_is_freed(info)) {
             rc = MPI_ERR_INFO;
-        } else if (num_notifications < 0) {
-            rc = MPI_ERR_ARG;
         }
 
+
         OMPI_ERRHANDLER_CHECK(rc, win, rc, FUNC_NAME);
     }
 

ompi/test/general/win_notify.c

@@ -489,9 +489,8 @@ static void test_counter_growth(void)
 }
 
 /* A non-zero mpi_assert_max_num_notify is the user promising not to ask for
- * more counters than that, so osc/sm reserves exactly that many.  It is an
- * allocation hint and not a cap: a request above it still grows the window,
- * which is what MPI_WIN_NOTIFICATION_NUM_UB advertises. */
+ * more counters than that.  osc/sm reserves exactly that many and holds the
+ * user to the promise rather than silently reallocating. */
 static void test_max_num_notify_assertion(void)
 {
     int *base = NULL;
@@ -520,23 +519,23 @@ static void test_max_num_notify_assertion(void)
     test_verify("Win_get_num_notify returns the asserted maximum",
                 MPI_SUCCESS == rc && 8 == num);
 
-    /* Past it is a reallocation rather than an error: the assertion sized the
-     * window, it does not bound it. */
+    /* Past it is an error rather than a reallocation: the window was sized on
+     * the strength of the assertion. */
     rc = MPI_Win_set_num_notify(win, MPI_INFO_NULL, 9);
-    test_verify("Win_set_num_notify grows past the asserted maximum",
-                MPI_SUCCESS == rc);
+    test_verify("Win_set_num_notify refuses to exceed the asserted maximum",
+                MPI_ERR_ARG == rc);
 
+    /* The refused call must not have disturbed the counters that do exist. */
     rc = MPI_Win_get_num_notify(win, 0, &num);
-    test_verify("Win_get_num_notify returns the grown count",
-                MPI_SUCCESS == rc && 9 == num);
+    test_verify("a refused Win_set_num_notify leaves the count alone",
+                MPI_SUCCESS == rc && 8 == num);
 
-    /* The counter that only the growth provided must work end to end. */
     MPI_Win_lock_all(0, win);
     int src = 7;
-    rc = MPI_Put_notify(&src, 1, MPI_INT, 0, 0, 1, MPI_INT, 8, win);
-    test_verify("Put_notify works on the counter past the assertion",
+    rc = MPI_Put_notify(&src, 1, MPI_INT, 0, 0, 1, MPI_INT, 7, win);
+    test_verify("Put_notify works on the last asserted counter",
                 MPI_SUCCESS == rc);
-    check_counter(win, 8, 1, "the counter past the assertion advanced");
+    check_counter(win, 7, 1, "the last asserted counter advanced");
     MPI_Win_unlock_all(win);
 
     MPI_Win_free(&win);
@@ -583,12 +582,6 @@ static void test_notify_attributes(void)
     test_verify("VALUE_UB is the full range of the counter type",
                 INT64_MAX == *value_ub);
 
-    /* Asking for exactly NUM_SB counters must not need a reallocation, and
-     * must be accepted. */
-    int sb = *num_sb;
-    rc = MPI_Win_set_num_notify(win, MPI_INFO_NULL, sb);
-    test_verify("Win_set_num_notify accepts NUM_SB counters", MPI_SUCCESS == rc);
-
     MPI_Win_free(&win);
 
     /* An assertion sizes the reservation, so it moves the suggested bound.  It
@@ -677,10 +670,10 @@ static void test_notify_info_reporting(void)
     test_verify("Win_set_info cannot raise the assertion",
                 0 == strcmp(value, "8"));
 
-    /* The assertion the window was sized for still is not a cap. */
+    /* And the cap the window was sized for is still the one enforced. */
     rc = MPI_Win_set_num_notify(win, MPI_INFO_NULL, 9);
-    test_verify("set_num_notify grows past the reported assertion",
-                MPI_SUCCESS == rc);
+    test_verify("the original assertion still caps set_num_notify",
+                MPI_ERR_ARG == rc);
 
     MPI_Win_free(&win);
 }

test/simple/win_notify_multi.c

@@ -319,8 +319,8 @@ static void test_info_and_attributes(void)
     MPI_Win_free(&win);
     MPI_Barrier(MPI_COMM_WORLD);
 
-    /* With an assertion the window is sized for exactly that many at every
-     * rank, and growing past it stays collective. */
+    /* With an assertion the window is sized for exactly that many, and the
+     * promise is held against every rank. */
     MPI_Info_create(&info);
     MPI_Info_set(info, "mpi_assert_max_num_notify", "8");
     win = make_window(info, &base);
@@ -344,11 +344,8 @@ static void test_info_and_attributes(void)
           MPI_SUCCESS == rc && 8 == num);
 
     rc = MPI_Win_set_num_notify(win, MPI_INFO_NULL, 9);
-    check("Win_set_num_notify grows past the assertion", MPI_SUCCESS == rc);
-
-    rc = MPI_Win_get_num_notify(win, (rank + 1) % nprocs, &num);
-    check("a peer grew past the assertion too",
-          MPI_SUCCESS == rc && 9 == num);
+    check("Win_set_num_notify refuses to exceed the assertion",
+          MPI_ERR_ARG == rc);
 
     MPI_Win_free(&win);
     MPI_Barrier(MPI_COMM_WORLD);