Skip to content

RFC: Provide equivalence of MPICH_ASYNC_PROGRESS #13088

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

RFC: Provide equivalence of MPICH_ASYNC_PROGRESS #13088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 17 additions & 5 deletions config/opal_configure_options.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,9 +544,21 @@ fi
AC_DEFINE_UNQUOTED([OPAL_ENABLE_GETPWUID], [$opal_want_getpwuid],
[Disable getpwuid support (default: enabled)])

dnl We no longer support the old OPAL_ENABLE_PROGRESS_THREADS. At
dnl some point, this should die.
AC_DEFINE([OPAL_ENABLE_PROGRESS_THREADS],
[0],
[Whether we want BTL progress threads enabled])
#
# Disable progress threads
#
AC_MSG_CHECKING([if want asynchronous progress threads])
AC_ARG_ENABLE([progress_threads],
[AS_HELP_STRING([--disable-progress-threads],
[Disable asynchronous progress threads. Note that when enabled, for performance-related reasons, the progress thread is still not spawned by default. User must set env var OMPI_ASYNC_PROGRESS or OPAL_ASYNC_PROGRESS to 1 to really activate the progress thread at runtime. (default: enabled)])])
if test "$enable_progress_threads" = "no"; then
AC_MSG_RESULT([no])
opal_want_progress_threads=0
else
AC_MSG_RESULT([yes])
opal_want_progress_threads=1
fi
AC_DEFINE_UNQUOTED([OPAL_ENABLE_PROGRESS_THREADS], [$opal_want_progress_threads],
Copy link
Member

Choose a reason for hiding this comment

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

I saw the comment about maintaining the OPAL_ENABLE_PROGRESS_THREADS define but removing its uses all over the code, but the answer makes no sense to me. Not everything is protected by opal_async_progress_thread_spawned so the compiler will never be able to completely remove all extra code and variables without complaining (such as variable set but not used).

If there is no bug in the old support for progress thread maintain the code as much as possible.

[Disable BTL asynchronous progress threads (default: enabled)])

])dnl
8 changes: 4 additions & 4 deletions ompi/instance/instance.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ static int ompi_mpi_instance_init_common (int argc, char **argv)
ddt_init, but before mca_coll_base_open, since some collective
modules (e.g., the hierarchical coll component) may need ops in
their query function. */
if (OMPI_SUCCESS != (ret = ompi_op_base_find_available (OPAL_ENABLE_PROGRESS_THREADS, ompi_mpi_thread_multiple))) {
if (OMPI_SUCCESS != (ret = ompi_op_base_find_available (opal_async_progress_thread_spawned, ompi_mpi_thread_multiple))) {
return ompi_instance_print_error ("ompi_op_base_find_available() failed", ret);
}

Expand All @@ -532,7 +532,7 @@ static int ompi_mpi_instance_init_common (int argc, char **argv)
return ompi_instance_print_error ("mca_smsc_base_select() failed", ret);
}

if (OMPI_SUCCESS != (ret = mca_pml_base_select (OPAL_ENABLE_PROGRESS_THREADS, ompi_mpi_thread_multiple))) {
if (OMPI_SUCCESS != (ret = mca_pml_base_select (opal_async_progress_thread_spawned, ompi_mpi_thread_multiple))) {
return ompi_instance_print_error ("mca_pml_base_select() failed", ret);
}

Expand Down Expand Up @@ -613,11 +613,11 @@ static int ompi_mpi_instance_init_common (int argc, char **argv)
return ompi_instance_print_error ("mca_pml_base_bsend_init() failed", ret);
}

if (OMPI_SUCCESS != (ret = mca_coll_base_find_available (OPAL_ENABLE_PROGRESS_THREADS, ompi_mpi_thread_multiple))) {
if (OMPI_SUCCESS != (ret = mca_coll_base_find_available (opal_async_progress_thread_spawned, ompi_mpi_thread_multiple))) {
return ompi_instance_print_error ("mca_coll_base_find_available() failed", ret);
}

if (OMPI_SUCCESS != (ret = ompi_osc_base_find_available (OPAL_ENABLE_PROGRESS_THREADS, ompi_mpi_thread_multiple))) {
if (OMPI_SUCCESS != (ret = ompi_osc_base_find_available (opal_async_progress_thread_spawned, ompi_mpi_thread_multiple))) {
return ompi_instance_print_error ("ompi_osc_base_find_available() failed", ret);
}

Expand Down
10 changes: 3 additions & 7 deletions ompi/mpi/c/request_get_status.c.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,7 @@
PROTOTYPE ERROR_CLASS request_get_status(REQUEST request, INT_OUT flag,
STATUS_OUT status)
{
#if OPAL_ENABLE_PROGRESS_THREADS == 0
int do_it_once = 0;
#endif

MEMCHECKER(
memchecker_request(&request);
Expand All @@ -56,9 +54,7 @@ PROTOTYPE ERROR_CLASS request_get_status(REQUEST request, INT_OUT flag,
}
}

#if OPAL_ENABLE_PROGRESS_THREADS == 0
recheck_request_status:
#endif
opal_atomic_mb();
if( (request == MPI_REQUEST_NULL) || (request->req_state == OMPI_REQUEST_INACTIVE) ) {
*flag = true;
Expand All @@ -80,16 +76,16 @@ PROTOTYPE ERROR_CLASS request_get_status(REQUEST request, INT_OUT flag,
}
return MPI_SUCCESS;
}
#if OPAL_ENABLE_PROGRESS_THREADS == 0
if( 0 == do_it_once ) {

if( 0 == do_it_once && !opal_async_progress_thread_spawned ) {
/* If we run the opal_progress then check the status of the
request before leaving. We will call the opal_progress only
once per call. */
opal_progress();
do_it_once++;
goto recheck_request_status;
}
#endif

*flag = false;
return MPI_SUCCESS;
}
26 changes: 12 additions & 14 deletions ompi/request/req_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,9 @@ int ompi_request_default_test(ompi_request_t ** rptr,
{
ompi_request_t *request = *rptr;

#if OPAL_ENABLE_PROGRESS_THREADS == 0
int do_it_once = 0;

recheck_request_status:
#endif
if( request->req_state == OMPI_REQUEST_INACTIVE ) {
*completed = true;
if (MPI_STATUS_IGNORE != status) {
Expand Down Expand Up @@ -81,8 +79,8 @@ int ompi_request_default_test(ompi_request_t ** rptr,
return MPI_ERR_PROC_FAILED_PENDING;
}
#endif
#if OPAL_ENABLE_PROGRESS_THREADS == 0
if( 0 == do_it_once ) {

if( 0 == do_it_once && !opal_async_progress_thread_spawned ) {
/**
* If we run the opal_progress then check the status of the request before
* leaving. We will call the opal_progress only once per call.
Expand All @@ -92,7 +90,7 @@ int ompi_request_default_test(ompi_request_t ** rptr,
goto recheck_request_status;
}
}
#endif

*completed = false;
return OMPI_SUCCESS;
}
Expand Down Expand Up @@ -163,9 +161,9 @@ int ompi_request_default_test_any(
*index = MPI_UNDEFINED;
if(num_requests_null_inactive != count) {
*completed = false;
#if OPAL_ENABLE_PROGRESS_THREADS == 0
opal_progress();
#endif
if (!opal_async_progress_thread_spawned) {
opal_progress();
}
} else {
*completed = true;
if (MPI_STATUS_IGNORE != status) {
Expand Down Expand Up @@ -208,16 +206,16 @@ int ompi_request_default_test_all(
return MPI_ERR_PROC_FAILED_PENDING;
}
#endif /* OPAL_ENABLE_FT_MPI */
#if OPAL_ENABLE_PROGRESS_THREADS == 0
if (0 == do_it_once) {

if (0 == do_it_once && !opal_async_progress_thread_spawned) {
++do_it_once;
if (0 != opal_progress()) {
/* continue walking the list, retest the current request */
--i;
continue;
}
}
#endif /* OPAL_ENABLE_PROGRESS_THREADS */

/* short-circuit */
break;
}
Expand Down Expand Up @@ -353,9 +351,9 @@ int ompi_request_default_test_some(
*outcount = num_requests_done;

if (num_requests_done == 0) {
#if OPAL_ENABLE_PROGRESS_THREADS == 0
opal_progress();
#endif
if (!opal_async_progress_thread_spawned) {
opal_progress();
}
return OMPI_SUCCESS;
}

Expand Down
7 changes: 4 additions & 3 deletions ompi/runtime/ompi_mpi_finalize.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,10 @@ int ompi_mpi_finalize(void)
opal_atomic_swap_32(&ompi_mpi_state,
OMPI_MPI_STATE_FINALIZE_PAST_COMM_SELF_DESTRUCT);

#if OPAL_ENABLE_PROGRESS_THREADS == 0
opal_progress_set_event_flag(OPAL_EVLOOP_ONCE | OPAL_EVLOOP_NONBLOCK);
#endif
/* shutdown async progress thread before tearing down further services */
if (opal_async_progress_thread_spawned) {
Copy link
Member

Choose a reason for hiding this comment

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

This is okay for now but does leave a hole to plug for the sessions model, but since this is an buy-in option for the application user it should be okay for how.

Copy link
Member

Choose a reason for hiding this comment

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

Do you mind elaborate on this @hppritcha, I fail to see the issue with the session model.

opal_progress_shutdown_async_progress_thread();
}

/* NOTE: MPI-2.1 requires that MPI_FINALIZE is "collective" across
*all* connected processes. This only means that all processes
Expand Down
8 changes: 4 additions & 4 deletions ompi/runtime/ompi_mpi_init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,16 +522,16 @@ int ompi_mpi_init(int argc, char **argv, int requested, int *provided,
time if so, then start the clock again */
OMPI_TIMING_NEXT("barrier");

#if OPAL_ENABLE_PROGRESS_THREADS == 0
/* Start setting up the event engine for MPI operations. Don't
block in the event library, so that communications don't take
forever between procs in the dynamic code. This will increase
CPU utilization for the remainder of MPI_INIT when we are
blocking on RTE-level events, but may greatly reduce non-TCP
latency. */
int old_event_flags = opal_progress_set_event_flag(0);
opal_progress_set_event_flag(old_event_flags | OPAL_EVLOOP_NONBLOCK);
#endif
if (!opal_async_progress_thread_spawned) {
int old_event_flags = opal_progress_set_event_flag(0);
opal_progress_set_event_flag(old_event_flags | OPAL_EVLOOP_NONBLOCK);
}

/* wire up the mpi interface, if requested. Do this after the
non-block switch for non-TCP performance. Do before the
Expand Down
2 changes: 1 addition & 1 deletion opal/mca/btl/smcuda/btl_smcuda.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ static struct mca_btl_base_endpoint_t *create_sm_endpoint(int local_proc, struct
OBJ_CONSTRUCT(&ep->endpoint_lock, opal_mutex_t);
#if OPAL_ENABLE_PROGRESS_THREADS == 1
sprintf(path, "%s" OPAL_PATH_SEP "sm_fifo.%lu", opal_process_info.job_session_dir,
(unsigned long) proc->proc_name);
(unsigned long) proc->proc_name.vpid);
ep->fifo_fd = open(path, O_WRONLY);
if (ep->fifo_fd < 0) {
opal_output(0, "mca_btl_smcuda_add_procs: open(%s) failed with errno=%d\n", path, errno);
Expand Down
2 changes: 1 addition & 1 deletion opal/mca/btl/smcuda/btl_smcuda_component.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -860,7 +860,7 @@ mca_btl_smcuda_component_init(int *num_btls, bool enable_progress_threads, bool
#if OPAL_ENABLE_PROGRESS_THREADS == 1
/* create a named pipe to receive events */
sprintf(mca_btl_smcuda_component.sm_fifo_path, "%s" OPAL_PATH_SEP "sm_fifo.%lu",
opal_process_info.job_session_dir, (unsigned long) OPAL_PROC_MY_NAME->vpid);
opal_process_info.job_session_dir, (unsigned long) OPAL_PROC_MY_NAME.vpid);
if (mkfifo(mca_btl_smcuda_component.sm_fifo_path, 0660) < 0) {
opal_output(0, "mca_btl_smcuda_component_init: mkfifo failed with errno=%d\n", errno);
return NULL;
Expand Down
10 changes: 10 additions & 0 deletions opal/runtime/opal_params_core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,16 @@ OPAL_DECLSPEC extern bool opal_built_with_cuda_support;
OPAL_DECLSPEC extern bool opal_built_with_rocm_support;
OPAL_DECLSPEC extern bool opal_built_with_ze_support;

# if OPAL_ENABLE_PROGRESS_THREADS == 1
OPAL_DECLSPEC extern bool opal_async_progress_thread_spawned;
Copy link
Contributor

Choose a reason for hiding this comment

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

This patch removes the OPAL_ENABLE_PROGRESS_THREADS pre-processor guard from many places (like https://github.com/open-mpi/ompi/pull/13088/files#diff-b23aacc904fb4f13b003d1c716bf6db21c5c490a319b58a3ab70e8f3a8226885L48). Can we define opal_async_progress_thread_spawned to false in the header if progress threads are explicitly disabled at compile time? That makes the code cleaner and allows the compiler to optimize away code that is explicitly disabled.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, done in new updated version. Thanks for proposal.

# else
/* When disabled at configure, using 'static const' will allow compilers to
* not only evaluate the boolean at compile time, but also to remove its
* storage within any object file which is including this header.
* See https://godbolt.org/z/hxc8EjPa4 */
OPAL_DECLSPEC static const bool opal_async_progress_thread_spawned = false;
# endif

/**
* * Whether we want to enable CUDA GPU buffer send and receive support.
* */
Expand Down
Loading