Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mpi/finalized: don't hang if called during MPI_FINALIZE #5092

Merged
merged 2 commits into from
Jun 3, 2018
Merged

mpi/finalized: don't hang if called during MPI_FINALIZE #5092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
35438ae
mpi/finalized: revamp INITIALIZED/FINALIZED
jsquyres Apr 24, 2018
38ed70d
ompi_mpi_finalize: remove some dead code
jsquyres Jun 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions ompi/errhandler/errhandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
* University of Stuttgart. All rights reserved.
* Copyright (c) 2004-2005 The Regents of the University of California.
* All rights reserved.
* Copyright (c) 2008-2012 Cisco Systems, Inc. All rights reserved.
* Copyright (c) 2008-2018 Cisco Systems, Inc. All rights reserved
* Copyright (c) 2008-2009 Sun Microsystems, Inc. All rights reserved.
* Copyright (c) 2015-2016 Intel, Inc. All rights reserved.
* Copyright (c) 2016 Los Alamos National Security, LLC. All rights
Expand Down Expand Up @@ -193,11 +193,22 @@ struct ompi_request_t;
* This macro directly invokes the ompi_mpi_errors_are_fatal_handler()
* when an error occurs because MPI_COMM_WORLD does not exist (because
* we're before MPI_Init() or after MPI_Finalize()).
*
* NOTE: The ompi_mpi_state variable is a volatile that is set
* atomically in ompi_mpi_init() and ompi_mpi_finalize(). The
* appropriate memory barriers are done in those 2 functions such that
* we do not need to do a read memory barrier here (in
* potentially-performance-critical code paths) before reading the
* variable.
*/
#define OMPI_ERR_INIT_FINALIZE(name) \
if( OPAL_UNLIKELY(!ompi_mpi_initialized || ompi_mpi_finalized) ) { \
ompi_mpi_errors_are_fatal_comm_handler(NULL, NULL, name); \
}
#define OMPI_ERR_INIT_FINALIZE(name) \
{ \
int32_t state = ompi_mpi_state; \
if (OPAL_UNLIKELY(state < OMPI_MPI_STATE_INIT_COMPLETED || \
state > OMPI_MPI_STATE_FINALIZE_PAST_COMM_SELF_DESTRUCT)) { \
ompi_mpi_errors_are_fatal_comm_handler(NULL, NULL, name); \
} \
}

/**
* This is the macro to invoke to directly invoke an MPI error
Expand Down
13 changes: 8 additions & 5 deletions ompi/errhandler/errhandler_predefined.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
* Copyright (c) 2004-2005 The Regents of the University of California.
* All rights reserved.
* Copyright (c) 2006 University of Houston. All rights reserved.
* Copyright (c) 2008-2013 Cisco Systems, Inc. All rights reserved.
* Copyright (c) 2008-2018 Cisco Systems, Inc. All rights reserved
* Copyright (c) 2009 Sun Microsystems, Inc. All rights reserved.
* Copyright (c) 2010-2011 Oak Ridge National Labs. All rights reserved.
* Copyright (c) 2012 Los Alamos National Security, LLC.
Expand Down Expand Up @@ -149,7 +149,8 @@ void ompi_mpi_errors_return_win_handler(struct ompi_win_t **win,

static void out(char *str, char *arg)
{
if (ompi_rte_initialized && !ompi_mpi_finalized) {
if (ompi_rte_initialized &&
ompi_mpi_state < OMPI_MPI_STATE_FINALIZE_PAST_COMM_SELF_DESTRUCT) {
if (NULL != arg) {
opal_output(0, str, arg);
} else {
Expand Down Expand Up @@ -280,7 +281,9 @@ static void backend_fatal_no_aggregate(char *type,
{
char *arg;

assert(!ompi_mpi_initialized || ompi_mpi_finalized);
int32_t state = ompi_mpi_state;
assert(state < OMPI_MPI_STATE_INIT_COMPLETED ||
state >= OMPI_MPI_STATE_FINALIZE_PAST_COMM_SELF_DESTRUCT);

fflush(stdout);
fflush(stderr);
Expand All @@ -289,7 +292,7 @@ static void backend_fatal_no_aggregate(char *type,

/* Per #2152, print out in plain english if something was invoked
before MPI_INIT* or after MPI_FINALIZE */
if (!ompi_mpi_init_started && !ompi_mpi_initialized) {
if (state < OMPI_MPI_STATE_INIT_STARTED) {
if (NULL != arg) {
out("*** The %s() function was called before MPI_INIT was invoked.\n"
"*** This is disallowed by the MPI standard.\n", arg);
Expand All @@ -300,7 +303,7 @@ static void backend_fatal_no_aggregate(char *type,
"*** function was invoked, sorry. :-(\n", NULL);
}
out("*** Your MPI job will now abort.\n", NULL);
} else if (ompi_mpi_finalized) {
} else if (state >= OMPI_MPI_STATE_FINALIZE_PAST_COMM_SELF_DESTRUCT) {
if (NULL != arg) {
out("*** The %s() function was called after MPI_FINALIZE was invoked.\n"
"*** This is disallowed by the MPI standard.\n", arg);
Expand Down
3 changes: 2 additions & 1 deletion ompi/mca/coll/fca/coll_fca_ops.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* Copyright (c) 2011 Mellanox Technologies. All rights reserved.
* Copyright (c) 2015 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
* Copyright (c) 2018 Cisco Systems, Inc. All rights reserved
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -159,7 +160,7 @@ int mca_coll_fca_barrier(struct ompi_communicator_t *comm,
int ret;

FCA_VERBOSE(5,"Using FCA Barrier");
if (OPAL_UNLIKELY(ompi_mpi_finalize_started)) {
if (OPAL_UNLIKELY(ompi_mpi_state >= OMPI_MPI_STATE_FINALIZE_STARTED)) {
FCA_VERBOSE(5, "In finalize, reverting to previous barrier");
goto orig_barrier;
}
Expand Down
3 changes: 2 additions & 1 deletion ompi/mca/coll/hcoll/coll_hcoll_module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* Copyright (c) 2017 The University of Tennessee and The University
* of Tennessee Research Foundation. All rights
* reserved.
* Copyright (c) 2018 Cisco Systems, Inc. All rights reserved
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -241,7 +242,7 @@ static int mca_coll_hcoll_module_enable(mca_coll_base_module_t *module,

int mca_coll_hcoll_progress(void)
{
if (ompi_mpi_finalized){
if (ompi_mpi_state >= OMPI_MPI_STATE_FINALIZE_STARTED) {
hcoll_rte_p2p_disabled_notify();
}

Expand Down
3 changes: 2 additions & 1 deletion ompi/mca/coll/hcoll/coll_hcoll_ops.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
Copyright (c) 2011 Mellanox Technologies. All rights reserved.
Copyright (c) 2015 Research Organization for Information Science
and Technology (RIST). All rights reserved.
Copyright (c) 2018 Cisco Systems, Inc. All rights reserved
$COPYRIGHT$

Additional copyrights may follow
Expand All @@ -21,7 +22,7 @@ int mca_coll_hcoll_barrier(struct ompi_communicator_t *comm,
mca_coll_hcoll_module_t *hcoll_module = (mca_coll_hcoll_module_t*)module;
HCOL_VERBOSE(20,"RUNNING HCOL BARRIER");

if (OPAL_UNLIKELY(ompi_mpi_finalize_started)) {
if (OPAL_UNLIKELY(ompi_mpi_state >= OMPI_MPI_STATE_FINALIZE_STARTED)) {
HCOL_VERBOSE(5, "In finalize, reverting to previous barrier");
goto orig_barrier;
}
Expand Down
3 changes: 2 additions & 1 deletion ompi/mca/io/romio314/src/io_romio314_file_open.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* Copyright (c) 2015 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
* Copyright (c) 2016-2017 IBM Corporation. All rights reserved.
* Copyright (c) 2018 Cisco Systems, Inc. All rights reserved
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -69,7 +70,7 @@ mca_io_romio314_file_close (ompi_file_t *fh)
which we obviously can't do if we've started to MPI_Finalize).
The user didn't close the file, so they should expect
unexpected behavior. */
if (ompi_mpi_finalized) {
if (ompi_mpi_state >= OMPI_MPI_STATE_FINALIZE_PAST_COMM_SELF_DESTRUCT) {
return OMPI_SUCCESS;
}

Expand Down
3 changes: 2 additions & 1 deletion ompi/mca/pml/yalla/pml_yalla.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* Copyright (C) 2001-2011 Mellanox Technologies Ltd. ALL RIGHTS RESERVED.
* Copyright (c) 2015 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
* Copyright (c) 2018 Cisco Systems, Inc. All rights reserved
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -265,7 +266,7 @@ int mca_pml_yalla_del_procs(struct ompi_proc_t **procs, size_t nprocs)
{
size_t i;

if (ompi_mpi_finalized) {
if (ompi_mpi_state >= OMPI_MPI_STATE_FINALIZE_STARTED) {
PML_YALLA_VERBOSE(3, "%s", "using bulk powerdown");
mxm_ep_powerdown(ompi_pml_yalla.mxm_ep);
}
Expand Down
18 changes: 5 additions & 13 deletions ompi/mpi/c/finalized.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
* All rights reserved.
* Copyright (c) 2015 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
* Copyright (c) 2015 Cisco Systems, Inc. All rights reserved.
* Copyright (c) 2015-2018 Cisco Systems, Inc. All rights reserved
* Copyright (c) 2015 Intel, Inc. All rights reserved
* $COPYRIGHT$
*
Expand Down Expand Up @@ -44,13 +44,7 @@ int MPI_Finalized(int *flag)

ompi_hook_base_mpi_finalized_top(flag);

/* We must obtain the lock to guarnatee consistent values of
ompi_mpi_initialized and ompi_mpi_finalized. Note, too, that
this lock is held for the bulk of the duration of
ompi_mpi_init() and ompi_mpi_finalize(), so when we get the
lock, we are guaranteed that some other thread is not part way
through initialization or finalization. */
opal_mutex_lock(&ompi_mpi_bootstrap_mutex);
int32_t state = ompi_mpi_state;

if (MPI_PARAM_CHECK) {
if (NULL == flag) {
Expand All @@ -59,12 +53,11 @@ int MPI_Finalized(int *flag)
whether we're currently (after MPI_Init and before
MPI_Finalize) or not */

if (ompi_mpi_initialized && !ompi_mpi_finalized) {
opal_mutex_unlock(&ompi_mpi_bootstrap_mutex);
if (state >= OMPI_MPI_STATE_INIT_COMPLETED &&
state < OMPI_MPI_STATE_FINALIZE_PAST_COMM_SELF_DESTRUCT) {
return OMPI_ERRHANDLER_INVOKE(MPI_COMM_WORLD, MPI_ERR_ARG,
FUNC_NAME);
} else {
opal_mutex_unlock(&ompi_mpi_bootstrap_mutex);
/* We have no MPI object here so call ompi_errhandle_invoke
* directly */
return ompi_errhandler_invoke(NULL, NULL, -1,
Expand All @@ -74,8 +67,7 @@ int MPI_Finalized(int *flag)
}
}

*flag = ompi_mpi_finalized;
opal_mutex_unlock(&ompi_mpi_bootstrap_mutex);
*flag = (state >= OMPI_MPI_STATE_FINALIZE_PAST_COMM_SELF_DESTRUCT);

ompi_hook_base_mpi_finalized_bottom(flag);

Expand Down
6 changes: 4 additions & 2 deletions ompi/mpi/c/get_library_version.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* University of Stuttgart. All rights reserved.
* Copyright (c) 2004-2005 The Regents of the University of California.
* All rights reserved.
* Copyright (c) 2014-2015 Cisco Systems, Inc. All rights reserved.
* Copyright (c) 2014-2018 Cisco Systems, Inc. All rights reserved
* Copyright (c) 2015 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
* Copyright (c) 2015 Intel, Inc. All rights reserved
Expand Down Expand Up @@ -58,7 +58,9 @@ int MPI_Get_library_version(char *version, int *resultlen)
(i.e., use a NULL communicator, which will end up at the
default errhandler, which is abort). */

if (ompi_mpi_initialized && !ompi_mpi_finalized) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is not related to your patch, it is more general about the real meaning of the get_library_version function. In OMPI in particular why are we protecting this function ? All the information used here is statically known, we don't have to be between MPI_Init and MPI_Finalize.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not - see the comment above this that explains (short version: we're protecting the invocation of error handling if version or resultlen is NULL -- we have to handle the error differently if we're before INIT, afterINITbeforeFINALIZE, or after FINALIZE).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that. thanks.

int32_t state = ompi_mpi_state;
if (state >= OMPI_MPI_STATE_INIT_COMPLETED &&
state < OMPI_MPI_STATE_FINALIZE_PAST_COMM_SELF_DESTRUCT) {
return OMPI_ERRHANDLER_INVOKE(MPI_COMM_WORLD, MPI_ERR_ARG,
FUNC_NAME);
} else {
Expand Down
5 changes: 4 additions & 1 deletion ompi/mpi/c/get_version.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* Copyright (c) 2015 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
* Copyright (c) 2015 Intel, Inc. All rights reserved
* Copyright (c) 2018 Cisco Systems, Inc. All rights reserved
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -54,7 +55,9 @@ int MPI_Get_version(int *version, int *subversion)
(i.e., use a NULL communicator, which will end up at the
default errhandler, which is abort). */

if (ompi_mpi_initialized && !ompi_mpi_finalized) {
int32_t state = ompi_mpi_state;
if (state >= OMPI_MPI_STATE_INIT_COMPLETED &&
state < OMPI_MPI_STATE_FINALIZE_PAST_COMM_SELF_DESTRUCT) {
return OMPI_ERRHANDLER_INVOKE(MPI_COMM_WORLD, MPI_ERR_ARG,
FUNC_NAME);
} else {
Expand Down
18 changes: 5 additions & 13 deletions ompi/mpi/c/initialized.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
* All rights reserved.
* Copyright (c) 2015 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
* Copyright (c) 2015 Cisco Systems, Inc. All rights reserved.
* Copyright (c) 2015-2018 Cisco Systems, Inc. All rights reserved
* Copyright (c) 2015 Intel, Inc. All rights reserved
* $COPYRIGHT$
*
Expand Down Expand Up @@ -44,13 +44,7 @@ int MPI_Initialized(int *flag)

ompi_hook_base_mpi_initialized_top(flag);

/* We must obtain the lock to guarnatee consistent values of
ompi_mpi_initialized and ompi_mpi_finalized. Note, too, that
this lock is held for the bulk of the duration of
ompi_mpi_init() and ompi_mpi_finalize(), so when we get the
lock, we are guaranteed that some other thread is not part way
through initialization or finalization. */
opal_mutex_lock(&ompi_mpi_bootstrap_mutex);
int32_t state = ompi_mpi_state;

if (MPI_PARAM_CHECK) {
if (NULL == flag) {
Expand All @@ -59,12 +53,11 @@ int MPI_Initialized(int *flag)
whether we're currently (after MPI_Init and before
MPI_Finalize) or not */

if (ompi_mpi_initialized && !ompi_mpi_finalized) {
opal_mutex_unlock(&ompi_mpi_bootstrap_mutex);
if (state >= OMPI_MPI_STATE_INIT_COMPLETED &&
state < OMPI_MPI_STATE_FINALIZE_PAST_COMM_SELF_DESTRUCT) {
return OMPI_ERRHANDLER_INVOKE(MPI_COMM_WORLD, MPI_ERR_ARG,
FUNC_NAME);
} else {
opal_mutex_unlock(&ompi_mpi_bootstrap_mutex);
/* We have no MPI object here so call ompi_errhandle_invoke
* directly */
return ompi_errhandler_invoke(NULL, NULL, -1,
Expand All @@ -74,8 +67,7 @@ int MPI_Initialized(int *flag)
}
}

*flag = ompi_mpi_initialized;
opal_mutex_unlock(&ompi_mpi_bootstrap_mutex);
*flag = (state >= OMPI_MPI_STATE_INIT_COMPLETED);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming the param check is off, Is this really enough or should we check that the state is smaller than OMPI_MPI_STATE_FINALIZE_PAST_COMM_SELF_DESTRUCT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The param check is only checking if they passed NULL for flag.

This function is supposed to return if MPI_INIT has completed or not. It says nothing about MPI_FINALIZE.

...am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, my bad.


ompi_hook_base_mpi_initialized_bottom(flag);

Expand Down
6 changes: 4 additions & 2 deletions ompi/mpi/tool/finalize.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
/*
* Copyright (c) 2012-2015 Los Alamos National Security, LLC. All rights
* reserved.
* Copyright (c) 2014 Cisco Systems, Inc. All rights reserved.
* Copyright (c) 2014-2018 Cisco Systems, Inc. All rights reserved
* Copyright (c) 2017 IBM Corporation. All rights reserved.
* $COPYRIGHT$
*
Expand Down Expand Up @@ -39,7 +39,9 @@ int MPI_T_finalize (void)
if (0 == --ompi_mpit_init_count) {
(void) ompi_info_close_components ();

if ((!ompi_mpi_initialized || ompi_mpi_finalized) &&
int32_t state = ompi_mpi_state;
if ((state < OMPI_MPI_STATE_INIT_COMPLETED ||
state >= OMPI_MPI_STATE_FINALIZE_PAST_COMM_SELF_DESTRUCT) &&
(NULL != ompi_mpi_main_thread)) {
/* we are not between MPI_Init and MPI_Finalize so we
* have to free the ompi_mpi_main_thread */
Expand Down
6 changes: 5 additions & 1 deletion ompi/peruse/peruse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* reserved.
* Copyright (c) 2004-2008 High Performance Computing Center Stuttgart,
* University of Stuttgart. All rights reserved.
* Copyright (c) 2018 Cisco Systems, Inc. All rights reserved
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -64,8 +65,11 @@ const int PERUSE_num_events = (sizeof(PERUSE_events) / sizeof(peruse_event_assoc
int PERUSE_Init (void)
{
if (MPI_PARAM_CHECK) {
if (!ompi_mpi_initialized || ompi_mpi_finalized)
int32_t state = ompi_mpi_state;
if (state < OMPI_MPI_STATE_INIT_COMPLETED ||
state >= OMPI_MPI_STATE_FINALIZE_STARTED) {
return PERUSE_ERR_INIT;
}
}
ompi_peruse_init ();
return PERUSE_SUCCESS;
Expand Down
Loading