diff --git a/ompi/mca/osc/sm/osc_sm.h b/ompi/mca/osc/sm/osc_sm.h index 5ce0a18e791..6bec8c34d94 100644 --- a/ompi/mca/osc/sm/osc_sm.h +++ b/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. */ diff --git a/ompi/mca/osc/sm/osc_sm_comm.c b/ompi/mca/osc/sm/osc_sm_comm.c index f20c00c2517..6f0213641e4 100644 --- a/ompi/mca/osc/sm/osc_sm_comm.c +++ b/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; diff --git a/ompi/mca/osc/sm/osc_sm_component.c b/ompi/mca/osc/sm/osc_sm_component.c index d960ccf17ac..dc8403b3410 100644 --- a/ompi/mca/osc/sm/osc_sm_component.c +++ b/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__, diff --git a/ompi/mpi/c/win_set_num_notify.c.in b/ompi/mpi/c/win_set_num_notify.c.in index cc1d39a9e77..eccd4a4eebd 100644 --- a/ompi/mpi/c/win_set_num_notify.c.in +++ b/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); } diff --git a/ompi/test/general/win_notify.c b/ompi/test/general/win_notify.c index f3f5eeb1556..d1bf8a1837e 100644 --- a/ompi/test/general/win_notify.c +++ b/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); } diff --git a/test/simple/win_notify_multi.c b/test/simple/win_notify_multi.c index 2c8b48e3c60..c668cbfe565 100644 --- a/test/simple/win_notify_multi.c +++ b/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);