OSC/UCX: Adding the following optimizations (nonblocking accumulate and reusing resources) #10709

MamziB · 2022-08-23T23:42:22Z

Adding the following optimizations: 1) Reuse the same workers/eps in
single-threaded applications, this is helpful if an application
creates many windows, therefore, we avoid the unnecessary overheads and 2) adding the truly nonblocking
MPI_Accumulate/Get_Accumulate.

Signed-off-by: Mamzi Bayatpour [email protected]
Co-authored-by: Tomislav Janjusic [email protected]
Co-authored-by: Joseph Schuchart [email protected]

devreal · 2022-08-24T21:15:45Z

Can we split this into two PRs? The two changes seem to be independent.

janjust · 2022-08-24T21:22:37Z

@devreal The features are independent, but the changes are intertwined, we'd prefer to leave it as is. FWIW, these are significant improvements in NWCHEM performance, now at least on small scale, we seem to outperform IMPI by about 20%, need to test larger scales to see effects.

devreal

Here is a preliminary review. There is good stuff in this PR. Some things aren't clear to me and I'd like to see some documentation on how the default ep mechanism works, as there seems to be some special handling depending on whether multiple-thread support is enabled.

opal/mca/common/ucx/common_ucx_wpool.c

devreal · 2022-08-26T12:22:56Z

ompi/mca/osc/ucx/osc_ucx.h

@@ -150,6 +152,17 @@ typedef struct ompi_osc_ucx_lock {
 #define OSC_UCX_GET_EP(comm_, rank_) (ompi_comm_peer_lookup(comm_, rank_)->proc_endpoints[OMPI_PROC_ENDPOINT_TAG_UCX])
 #define OSC_UCX_GET_DISP(module_, rank_) ((module_->disp_unit < 0) ? module_->disp_units[rank_] : module_->disp_unit)

+extern bool mpi_thread_multiple_enabled;


This looks like it has the same meaning opal_using_threads(). Is it different?

there are two reasons that for now, we did not set mpi_thread_multiple_enabled the same as opal_using_threads():

We need access to comm world in OSC Finalize, to destroy all the created default EPs. Therefore, we need to fix the following issue before making this change:

#10629

We want to support high multi-threading performance for applications that call init with thread-single, but they still use threads for RMA.

We want to support high multi-threading performance for applications that call init with thread-single, but they still use threads for RMA.

I'm not sure I understand. There shouldn't be multiple threads if MPI was initialized with thread-single. Can you elaborate? Did you mean thread-serialized? opal_using_threads should be false for thread-serialized, so it has the semantics you're looking for, no?

This is good to know. If this is a requirement, we can rely on opal_using_threads(). I found a workaround for the first issue, and now the osc finalize is fixed.

The symbol is still here. Can it be removed?

Can you please check the latest commits and let me know if you have more thoughts about it?
@devreal

ompi/mca/osc/ucx/osc_ucx_comm.c

opal/mca/common/ucx/common_ucx_wpool.c

ompi/mca/osc/ucx/osc_ucx_request.h

ompi/mca/osc/ucx/osc_ucx_component.c

devreal · 2022-08-26T12:55:56Z

ompi/mca/osc/ucx/osc_ucx_component.c

+                                           MCA_BASE_VAR_SCOPE_GROUP, &enable_nonblocking_accumulate);
+    (void) mca_base_component_var_register(&mca_osc_ucx_component.super.osc_version, "enable_wpool_thread_multiple",
+                                           description_str, MCA_BASE_VAR_TYPE_BOOL, NULL, 0, 0, OPAL_INFO_LVL_5,
+                                           MCA_BASE_VAR_SCOPE_GROUP, &mpi_thread_multiple_enabled);


Why is that an MCA parameter? That should be determined from how MPI is initialized (see my other comment about the opal variable)

Answered, please see above.

I'm still not sure why this is an MCA parameter? Why would a user have to control thread-safety from the command line?

With the latest commits, we are setting this variable as the same as the opal_thread_multiple().
I would like to keep this as an MCA variable for the following reason: Users will have the option to provide each window with an endpoint, instead of sharing it across all the windows. So far, I have not found such an application that can benefit from it, but it would be harmless to keep this option open. What do you think?

github-actions · 2022-10-06T20:47:41Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

janjust · 2022-10-06T20:49:36Z

@MamziB missing signed off in the last commit

MamziB · 2022-10-06T20:52:08Z

@MamziB missing signed off in the last commit

@janjust I am planning to squash all the new commits, and then add sign-off. Does that sound good to you?

github-actions · 2022-10-07T18:37:02Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

MamziB · 2022-10-07T21:04:13Z

@devreal, Thanks for your constructive comments. Can you please let us know if you have more comments?

github-actions · 2022-10-07T21:04:24Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

4afa6c7: code reorganization

check_signed_off: does not contain a valid Signed-off-by line

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

devreal · 2022-10-11T13:34:49Z

ompi/mca/osc/ucx/osc_ucx.h

@@ -150,6 +152,17 @@ typedef struct ompi_osc_ucx_lock {
 #define OSC_UCX_GET_EP(comm_, rank_) (ompi_comm_peer_lookup(comm_, rank_)->proc_endpoints[OMPI_PROC_ENDPOINT_TAG_UCX])
 #define OSC_UCX_GET_DISP(module_, rank_) ((module_->disp_unit < 0) ? module_->disp_units[rank_] : module_->disp_unit)

+extern bool mpi_thread_multiple_enabled;


We want to support high multi-threading performance for applications that call init with thread-single, but they still use threads for RMA.

I'm not sure I understand. There shouldn't be multiple threads if MPI was initialized with thread-single. Can you elaborate? Did you mean thread-serialized? opal_using_threads should be false for thread-serialized, so it has the semantics you're looking for, no?

ompi/mca/osc/ucx/osc_ucx.h

opal/mca/common/ucx/common_ucx_wpool.c

github-actions · 2022-10-13T18:40:35Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

6db6d31: Enhancing the osc finalize for resource utilizati...

check_signed_off: does not contain a valid Signed-off-by line

4afa6c7: code reorganization

check_signed_off: does not contain a valid Signed-off-by line

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

github-actions · 2022-10-13T20:55:23Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

e4072fe: fixing the macros

check_signed_off: does not contain a valid Signed-off-by line

5c28f90: adding the prefix

check_signed_off: does not contain a valid Signed-off-by line

6db6d31: Enhancing the osc finalize for resource utilizati...

check_signed_off: does not contain a valid Signed-off-by line

4afa6c7: code reorganization

check_signed_off: does not contain a valid Signed-off-by line

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

MamziB · 2022-10-13T20:58:31Z

hey @devreal please let us know your further comments. Again, thanks for your various help so far.

devreal

Thanks @MamziB! The issue with the thread-multiple variable still exists (either needs renaming or be replaced by opal_using_threads()) and a few more points I'm not clear about.

devreal · 2022-10-14T13:03:34Z

ompi/mca/osc/ucx/osc_ucx.h

@@ -150,6 +152,17 @@ typedef struct ompi_osc_ucx_lock {
 #define OSC_UCX_GET_EP(comm_, rank_) (ompi_comm_peer_lookup(comm_, rank_)->proc_endpoints[OMPI_PROC_ENDPOINT_TAG_UCX])
 #define OSC_UCX_GET_DISP(module_, rank_) ((module_->disp_unit < 0) ? module_->disp_units[rank_] : module_->disp_unit)

+extern bool mpi_thread_multiple_enabled;


The symbol is still here. Can it be removed?

ompi/mca/osc/ucx/osc_ucx_comm.c

ompi/mca/osc/ucx/osc_ucx_component.c

devreal · 2022-10-14T13:19:19Z

ompi/mca/osc/ucx/osc_ucx_component.c

+                                           MCA_BASE_VAR_SCOPE_GROUP, &enable_nonblocking_accumulate);
+    (void) mca_base_component_var_register(&mca_osc_ucx_component.super.osc_version, "enable_wpool_thread_multiple",
+                                           description_str, MCA_BASE_VAR_TYPE_BOOL, NULL, 0, 0, OPAL_INFO_LVL_5,
+                                           MCA_BASE_VAR_SCOPE_GROUP, &mpi_thread_multiple_enabled);


I'm still not sure why this is an MCA parameter? Why would a user have to control thread-safety from the command line?

opal/mca/common/ucx/common_ucx_wpool.c

opal/mca/common/ucx/common_ucx_wpool_int.h

github-actions · 2022-10-14T17:51:33Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

08a847c: use opal_uses_thread to set the mpi_thread_multipl...

check_signed_off: does not contain a valid Signed-off-by line

e4072fe: fixing the macros

check_signed_off: does not contain a valid Signed-off-by line

5c28f90: adding the prefix

check_signed_off: does not contain a valid Signed-off-by line

6db6d31: Enhancing the osc finalize for resource utilizati...

check_signed_off: does not contain a valid Signed-off-by line

4afa6c7: code reorganization

check_signed_off: does not contain a valid Signed-off-by line

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

github-actions · 2022-10-14T23:24:40Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

d91d76a: atomic add for nb outstanding ops and renaming the...

check_signed_off: does not contain a valid Signed-off-by line

37163ce: move num_incomplete_req_ops to osc ucx context

check_signed_off: does not contain a valid Signed-off-by line

08a847c: use opal_uses_thread to set the mpi_thread_multipl...

check_signed_off: does not contain a valid Signed-off-by line

e4072fe: fixing the macros

check_signed_off: does not contain a valid Signed-off-by line

5c28f90: adding the prefix

check_signed_off: does not contain a valid Signed-off-by line

6db6d31: Enhancing the osc finalize for resource utilizati...

check_signed_off: does not contain a valid Signed-off-by line

4afa6c7: code reorganization

check_signed_off: does not contain a valid Signed-off-by line

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

github-actions · 2022-10-17T18:51:57Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

8213d67: make nonblocking acc default

check_signed_off: does not contain a valid Signed-off-by line

d91d76a: atomic add for nb outstanding ops and renaming the...

check_signed_off: does not contain a valid Signed-off-by line

37163ce: move num_incomplete_req_ops to osc ucx context

check_signed_off: does not contain a valid Signed-off-by line

08a847c: use opal_uses_thread to set the mpi_thread_multipl...

check_signed_off: does not contain a valid Signed-off-by line

e4072fe: fixing the macros

check_signed_off: does not contain a valid Signed-off-by line

5c28f90: adding the prefix

check_signed_off: does not contain a valid Signed-off-by line

6db6d31: Enhancing the osc finalize for resource utilizati...

check_signed_off: does not contain a valid Signed-off-by line

4afa6c7: code reorganization

check_signed_off: does not contain a valid Signed-off-by line

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

opal/mca/common/ucx/common_ucx_wpool.c

ompi/mca/osc/ucx/osc_ucx_request.h

opal/mca/common/ucx/common_ucx_wpool.c

ompi/mca/osc/ucx/osc_ucx_comm.c

ompi/mca/osc/ucx/osc_ucx_component.c

opal/mca/common/ucx/common_ucx_wpool.c

github-actions · 2022-10-17T19:15:42Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

ddf5d70: cleanup

check_signed_off: does not contain a valid Signed-off-by line

dcb44cb: cleanup

check_signed_off: does not contain a valid Signed-off-by line

8213d67: make nonblocking acc default

check_signed_off: does not contain a valid Signed-off-by line

d91d76a: atomic add for nb outstanding ops and renaming the...

check_signed_off: does not contain a valid Signed-off-by line

37163ce: move num_incomplete_req_ops to osc ucx context

check_signed_off: does not contain a valid Signed-off-by line

08a847c: use opal_uses_thread to set the mpi_thread_multipl...

check_signed_off: does not contain a valid Signed-off-by line

e4072fe: fixing the macros

check_signed_off: does not contain a valid Signed-off-by line

5c28f90: adding the prefix

check_signed_off: does not contain a valid Signed-off-by line

6db6d31: Enhancing the osc finalize for resource utilizati...

check_signed_off: does not contain a valid Signed-off-by line

4afa6c7: code reorganization

check_signed_off: does not contain a valid Signed-off-by line

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

github-actions · 2022-10-18T17:50:08Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

9f46f48: create separate req obj for accumulate

check_signed_off: does not contain a valid Signed-off-by line

ddf5d70: cleanup

check_signed_off: does not contain a valid Signed-off-by line

dcb44cb: cleanup

check_signed_off: does not contain a valid Signed-off-by line

8213d67: make nonblocking acc default

check_signed_off: does not contain a valid Signed-off-by line

d91d76a: atomic add for nb outstanding ops and renaming the...

check_signed_off: does not contain a valid Signed-off-by line

37163ce: move num_incomplete_req_ops to osc ucx context

check_signed_off: does not contain a valid Signed-off-by line

08a847c: use opal_uses_thread to set the mpi_thread_multipl...

check_signed_off: does not contain a valid Signed-off-by line

e4072fe: fixing the macros

check_signed_off: does not contain a valid Signed-off-by line

5c28f90: adding the prefix

check_signed_off: does not contain a valid Signed-off-by line

6db6d31: Enhancing the osc finalize for resource utilizati...

check_signed_off: does not contain a valid Signed-off-by line

4afa6c7: code reorganization

check_signed_off: does not contain a valid Signed-off-by line

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

github-actions · 2022-10-18T18:05:49Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

63e6893: add description for mca vars

check_signed_off: does not contain a valid Signed-off-by line

9f46f48: create separate req obj for accumulate

check_signed_off: does not contain a valid Signed-off-by line

ddf5d70: cleanup

check_signed_off: does not contain a valid Signed-off-by line

dcb44cb: cleanup

check_signed_off: does not contain a valid Signed-off-by line

8213d67: make nonblocking acc default

check_signed_off: does not contain a valid Signed-off-by line

d91d76a: atomic add for nb outstanding ops and renaming the...

check_signed_off: does not contain a valid Signed-off-by line

37163ce: move num_incomplete_req_ops to osc ucx context

check_signed_off: does not contain a valid Signed-off-by line

08a847c: use opal_uses_thread to set the mpi_thread_multipl...

check_signed_off: does not contain a valid Signed-off-by line

e4072fe: fixing the macros

check_signed_off: does not contain a valid Signed-off-by line

5c28f90: adding the prefix

check_signed_off: does not contain a valid Signed-off-by line

6db6d31: Enhancing the osc finalize for resource utilizati...

check_signed_off: does not contain a valid Signed-off-by line

4afa6c7: code reorganization

check_signed_off: does not contain a valid Signed-off-by line

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

github-actions · 2022-10-18T18:20:38Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

18ff47b: fix the naming of req comp

check_signed_off: does not contain a valid Signed-off-by line

63e6893: add description for mca vars

check_signed_off: does not contain a valid Signed-off-by line

9f46f48: create separate req obj for accumulate

check_signed_off: does not contain a valid Signed-off-by line

ddf5d70: cleanup

check_signed_off: does not contain a valid Signed-off-by line

dcb44cb: cleanup

check_signed_off: does not contain a valid Signed-off-by line

8213d67: make nonblocking acc default

check_signed_off: does not contain a valid Signed-off-by line

d91d76a: atomic add for nb outstanding ops and renaming the...

check_signed_off: does not contain a valid Signed-off-by line

37163ce: move num_incomplete_req_ops to osc ucx context

check_signed_off: does not contain a valid Signed-off-by line

08a847c: use opal_uses_thread to set the mpi_thread_multipl...

check_signed_off: does not contain a valid Signed-off-by line

e4072fe: fixing the macros

check_signed_off: does not contain a valid Signed-off-by line

5c28f90: adding the prefix

check_signed_off: does not contain a valid Signed-off-by line

6db6d31: Enhancing the osc finalize for resource utilizati...

check_signed_off: does not contain a valid Signed-off-by line

4afa6c7: code reorganization

check_signed_off: does not contain a valid Signed-off-by line

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

github-actions · 2022-10-18T18:51:36Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

7651aa3: fix thread_enabled name

check_signed_off: does not contain a valid Signed-off-by line

18ff47b: fix the naming of req comp

check_signed_off: does not contain a valid Signed-off-by line

63e6893: add description for mca vars

check_signed_off: does not contain a valid Signed-off-by line

9f46f48: create separate req obj for accumulate

check_signed_off: does not contain a valid Signed-off-by line

ddf5d70: cleanup

check_signed_off: does not contain a valid Signed-off-by line

dcb44cb: cleanup

check_signed_off: does not contain a valid Signed-off-by line

8213d67: make nonblocking acc default

check_signed_off: does not contain a valid Signed-off-by line

d91d76a: atomic add for nb outstanding ops and renaming the...

check_signed_off: does not contain a valid Signed-off-by line

37163ce: move num_incomplete_req_ops to osc ucx context

check_signed_off: does not contain a valid Signed-off-by line

08a847c: use opal_uses_thread to set the mpi_thread_multipl...

check_signed_off: does not contain a valid Signed-off-by line

e4072fe: fixing the macros

check_signed_off: does not contain a valid Signed-off-by line

5c28f90: adding the prefix

check_signed_off: does not contain a valid Signed-off-by line

6db6d31: Enhancing the osc finalize for resource utilizati...

check_signed_off: does not contain a valid Signed-off-by line

4afa6c7: code reorganization

check_signed_off: does not contain a valid Signed-off-by line

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

github-actions · 2022-10-19T19:19:09Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

5444d2e: disable nb acc for dyn windows

check_signed_off: does not contain a valid Signed-off-by line

7651aa3: fix thread_enabled name

check_signed_off: does not contain a valid Signed-off-by line

18ff47b: fix the naming of req comp

check_signed_off: does not contain a valid Signed-off-by line

63e6893: add description for mca vars

check_signed_off: does not contain a valid Signed-off-by line

9f46f48: create separate req obj for accumulate

check_signed_off: does not contain a valid Signed-off-by line

ddf5d70: cleanup

check_signed_off: does not contain a valid Signed-off-by line

dcb44cb: cleanup

check_signed_off: does not contain a valid Signed-off-by line

8213d67: make nonblocking acc default

check_signed_off: does not contain a valid Signed-off-by line

d91d76a: atomic add for nb outstanding ops and renaming the...

check_signed_off: does not contain a valid Signed-off-by line

37163ce: move num_incomplete_req_ops to osc ucx context

check_signed_off: does not contain a valid Signed-off-by line

08a847c: use opal_uses_thread to set the mpi_thread_multipl...

check_signed_off: does not contain a valid Signed-off-by line

e4072fe: fixing the macros

check_signed_off: does not contain a valid Signed-off-by line

5c28f90: adding the prefix

check_signed_off: does not contain a valid Signed-off-by line

6db6d31: Enhancing the osc finalize for resource utilizati...

check_signed_off: does not contain a valid Signed-off-by line

4afa6c7: code reorganization

check_signed_off: does not contain a valid Signed-off-by line

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

github-actions · 2022-10-20T19:04:35Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

174b53c: adding ref count for accumulate lock

check_signed_off: does not contain a valid Signed-off-by line

5444d2e: disable nb acc for dyn windows

check_signed_off: does not contain a valid Signed-off-by line

7651aa3: fix thread_enabled name

check_signed_off: does not contain a valid Signed-off-by line

18ff47b: fix the naming of req comp

check_signed_off: does not contain a valid Signed-off-by line

63e6893: add description for mca vars

check_signed_off: does not contain a valid Signed-off-by line

9f46f48: create separate req obj for accumulate

check_signed_off: does not contain a valid Signed-off-by line

ddf5d70: cleanup

check_signed_off: does not contain a valid Signed-off-by line

dcb44cb: cleanup

check_signed_off: does not contain a valid Signed-off-by line

8213d67: make nonblocking acc default

check_signed_off: does not contain a valid Signed-off-by line

d91d76a: atomic add for nb outstanding ops and renaming the...

check_signed_off: does not contain a valid Signed-off-by line

37163ce: move num_incomplete_req_ops to osc ucx context

check_signed_off: does not contain a valid Signed-off-by line

08a847c: use opal_uses_thread to set the mpi_thread_multipl...

check_signed_off: does not contain a valid Signed-off-by line

e4072fe: fixing the macros

check_signed_off: does not contain a valid Signed-off-by line

5c28f90: adding the prefix

check_signed_off: does not contain a valid Signed-off-by line

6db6d31: Enhancing the osc finalize for resource utilizati...

check_signed_off: does not contain a valid Signed-off-by line

4afa6c7: code reorganization

check_signed_off: does not contain a valid Signed-off-by line

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

MamziB · 2022-10-20T19:05:34Z

@devreal Please kindly let us know if you have more comments. Thanks a lot.

github-actions · 2022-10-21T22:25:57Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

00ebe71: Enhance ref counters

check_signed_off: does not contain a valid Signed-off-by line

174b53c: adding ref count for accumulate lock

check_signed_off: does not contain a valid Signed-off-by line

5444d2e: disable nb acc for dyn windows

check_signed_off: does not contain a valid Signed-off-by line

7651aa3: fix thread_enabled name

check_signed_off: does not contain a valid Signed-off-by line

18ff47b: fix the naming of req comp

check_signed_off: does not contain a valid Signed-off-by line

63e6893: add description for mca vars

check_signed_off: does not contain a valid Signed-off-by line

9f46f48: create separate req obj for accumulate

check_signed_off: does not contain a valid Signed-off-by line

ddf5d70: cleanup

check_signed_off: does not contain a valid Signed-off-by line

dcb44cb: cleanup

check_signed_off: does not contain a valid Signed-off-by line

8213d67: make nonblocking acc default

check_signed_off: does not contain a valid Signed-off-by line

d91d76a: atomic add for nb outstanding ops and renaming the...

check_signed_off: does not contain a valid Signed-off-by line

37163ce: move num_incomplete_req_ops to osc ucx context

check_signed_off: does not contain a valid Signed-off-by line

08a847c: use opal_uses_thread to set the mpi_thread_multipl...

check_signed_off: does not contain a valid Signed-off-by line

e4072fe: fixing the macros

check_signed_off: does not contain a valid Signed-off-by line

5c28f90: adding the prefix

check_signed_off: does not contain a valid Signed-off-by line

6db6d31: Enhancing the osc finalize for resource utilizati...

check_signed_off: does not contain a valid Signed-off-by line

4afa6c7: code reorganization

check_signed_off: does not contain a valid Signed-off-by line

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

devreal

Thanks @MamziB, looks good to me now 👍

MamziB · 2022-10-24T23:45:32Z

@devreal I'm going to push the last commit. Please wait for that. I am working on pushing it.

github-actions · 2022-10-24T23:58:10Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

a2d2993: Using a separate lock for handling the dynamic win...

check_signed_off: does not contain a valid Signed-off-by line

00ebe71: Enhance ref counters

check_signed_off: does not contain a valid Signed-off-by line

174b53c: adding ref count for accumulate lock

check_signed_off: does not contain a valid Signed-off-by line

5444d2e: disable nb acc for dyn windows

check_signed_off: does not contain a valid Signed-off-by line

7651aa3: fix thread_enabled name

check_signed_off: does not contain a valid Signed-off-by line

18ff47b: fix the naming of req comp

check_signed_off: does not contain a valid Signed-off-by line

63e6893: add description for mca vars

check_signed_off: does not contain a valid Signed-off-by line

9f46f48: create separate req obj for accumulate

check_signed_off: does not contain a valid Signed-off-by line

ddf5d70: cleanup

check_signed_off: does not contain a valid Signed-off-by line

dcb44cb: cleanup

check_signed_off: does not contain a valid Signed-off-by line

8213d67: make nonblocking acc default

check_signed_off: does not contain a valid Signed-off-by line

d91d76a: atomic add for nb outstanding ops and renaming the...

check_signed_off: does not contain a valid Signed-off-by line

37163ce: move num_incomplete_req_ops to osc ucx context

check_signed_off: does not contain a valid Signed-off-by line

08a847c: use opal_uses_thread to set the mpi_thread_multipl...

check_signed_off: does not contain a valid Signed-off-by line

e4072fe: fixing the macros

check_signed_off: does not contain a valid Signed-off-by line

5c28f90: adding the prefix

check_signed_off: does not contain a valid Signed-off-by line

6db6d31: Enhancing the osc finalize for resource utilizati...

check_signed_off: does not contain a valid Signed-off-by line

4afa6c7: code reorganization

check_signed_off: does not contain a valid Signed-off-by line

addc6b8: enhance the datatype handling in nb acc

check_signed_off: does not contain a valid Signed-off-by line

6b537d1: Fixing some corner cases in nonblocking accumulate...

check_signed_off: does not contain a valid Signed-off-by line
check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: ae97b9792e246f4397fc175fde36f41e7c35637f

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

MamziB · 2022-10-25T00:00:52Z

@devreal Please take a look at the last commits. I reverted a couple of commits, as they were not MPI atomicity compliant. Basically, using those two commits, one process can initiate multiple on-the-flight nonblocking accumulate to the same target. This can violate the atomicity of each MPI_Accumulate/Get_accumulate call. Now everything should be good to go.

MamziB · 2022-10-26T18:24:16Z

I squashed the commits, however, since my branch is not based on the latest main branch, some unwanted commits sneaked in. I will rebase my branch on top of the latest main and I will try again. Thanks for your patience.

devreal

One more thing, otherwise it's looking good now 👍

devreal · 2022-10-26T20:05:52Z

ompi/mca/osc/ucx/osc_ucx.h

 #define OSC_UCX_GET_DISP(module_, rank_) ((module_->disp_unit < 0) ? module_->disp_units[rank_] : module_->disp_unit)

+extern bool opal_common_ucx_thread_enabled;


This is an opal variables and should be defined in an opal header.

Thanks for the comment, Actually, I noticed a warning message regarding this variable. I have fixed this warning in the latest push. Please kindly take look.
By the way, I added the definition to the opal header file, and I got many more warnings like below:

libopen-palmca_common_ucx_noinst_la-common_ucx_wpool.o: 0000000000000001 C opal_common_ucx_thread_enabled osc_ucx_component.o: 0000000000000001 C opal_common_ucx_thread_enabled osc_ucx_active_target.o: 0000000000000001 C opal_common_ucx_thread_enabled osc_ucx_comm.o: 0000000000000001 C opal_common_ucx_thread_enabled osc_ucx_passive_target.o: 0000000000000001 C opal_common_ucx_thread_enabled osc_ucx_request.o: 0000000000000001 C opal_common_

Therefore, instead, I defined it in an opal .c file, and extern'ed it in suitable places. This is because we do not need a copy of the variable on each file that includes the opal header, instead, I defined it in a c file and extern it on other files.

If you declare the variable in a header file you mark it as extern and define it once in a .c file. The messages suggest that you have definitions of the same variable in multiple files.

I always forget the details about this warning. Here is a thread about the warning you're seeing: https://users.open-mpi.narkive.com/fTvNIGxP/ompi-make-install-warns-about-common-symbols

Bottom line: global variables (in your case opal_common_ucx_thread_enabled) should always be initialized where they are defined.

Thanks @devreal I fixed this issue.
@janjust The branch is ready.

…eps in single threaded applications, this is helpful if an application creates many windows, therefore, we avoid the unnecessary overheads and 2) adding the truely nonblocking MPI_Accumulate/Get_Accumulate. Signed-off-by: Mamzi Bayatpour <[email protected]> Co-authored-by: Tomislav Janjusic <[email protected]> Co-authored-by: Joseph Schuchart <[email protected]>>

janjust · 2022-10-31T13:28:14Z

bot:retest

opal/mca/common/ucx/common_ucx_wpool.c

janjust · 2022-10-31T19:15:52Z

bot:retest

janjust · 2022-11-01T21:49:03Z

bot:ibm:retest

janjust · 2022-11-01T22:06:26Z

bot:retest

janjust · 2022-11-02T13:56:49Z

@MamziB please open up v5.0 PR asap

MamziB · 2022-11-02T18:48:27Z

@janjust I opened the v5 here:
#11025

github-actions bot added the Target: main label Aug 23, 2022

janjust requested review from devreal and janjust August 24, 2022 21:06

janjust added the ⚠️ WIP-DNM! label Aug 24, 2022

devreal requested changes Aug 26, 2022

View reviewed changes

devreal reviewed Oct 11, 2022

View reviewed changes

janjust removed the ⚠️ WIP-DNM! label Oct 13, 2022

devreal reviewed Oct 14, 2022

View reviewed changes

devreal reviewed Oct 17, 2022

View reviewed changes

devreal approved these changes Oct 24, 2022

View reviewed changes

MamziB force-pushed the mamzi/single-thread-enhancements-3 branch from a2d2993 to 5e6ba1a Compare October 26, 2022 18:21

MamziB force-pushed the mamzi/single-thread-enhancements-3 branch from 5e6ba1a to d1b0e37 Compare October 26, 2022 18:53

devreal reviewed Oct 26, 2022

View reviewed changes

MamziB force-pushed the mamzi/single-thread-enhancements-3 branch 4 times, most recently from ee49d3b to 34a98f3 Compare October 27, 2022 18:37

MamziB force-pushed the mamzi/single-thread-enhancements-3 branch from 34a98f3 to 1ea6fb9 Compare October 27, 2022 18:44

janjust approved these changes Oct 31, 2022

View reviewed changes

opal/mca/common/ucx/common_ucx_wpool.c Show resolved Hide resolved

janjust merged commit 710ff57 into open-mpi:main Nov 2, 2022

s417-lama mentioned this pull request Nov 21, 2022

osc/ucx: Fix bugs with dynamic windows #11095

Merged

This was referenced Dec 16, 2022

ompi_comm_finalize is called before osc ucx component_finalize #10629

Open

UCX osc thread-safety concerns #6941

Closed

		#define OSC_UCX_GET_DISP(module_, rank_) ((module_->disp_unit < 0) ? module_->disp_units[rank_] : module_->disp_unit)

		extern bool opal_common_ucx_thread_enabled;

OSC/UCX: Adding the following optimizations (nonblocking accumulate and reusing resources) #10709

OSC/UCX: Adding the following optimizations (nonblocking accumulate and reusing resources) #10709

Uh oh!

Conversation

MamziB commented Aug 23, 2022 • edited by janjust Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devreal commented Aug 24, 2022

Uh oh!

janjust commented Aug 24, 2022

Uh oh!

devreal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MamziB Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MamziB Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 6, 2022

Uh oh!

janjust commented Oct 6, 2022

Uh oh!

MamziB commented Oct 6, 2022

Uh oh!

github-actions bot commented Oct 7, 2022

Uh oh!

MamziB commented Oct 7, 2022

Uh oh!

github-actions bot commented Oct 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 13, 2022

Uh oh!

github-actions bot commented Oct 13, 2022

Uh oh!

MamziB commented Oct 13, 2022

Uh oh!

devreal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MamziB commented Aug 23, 2022 •

edited by janjust

Loading

MamziB Oct 7, 2022 •

edited

Loading

MamziB Oct 17, 2022 •

edited

Loading