osc/rdma: adjustment on btl selection logic #9696

wzamazon · 2021-11-23T14:03:02Z

This PR contain 3 commits:

first one update btl's active message so it always support remote completion

The other 2 update the btl selection logic in osc/rdma.

wzamazon · 2021-11-23T14:05:16Z

CI will fail because this PR depends on #9694

ibm-ompi · 2021-11-23T14:48:09Z

The IBM CI (GNU/Scale) build failed! Please review the log, linked below.

Gist: https://gist.github.com/ed6555c1b3042a027157d39ff4589bde

ibm-ompi · 2021-11-23T14:50:47Z

The IBM CI (XL) build failed! Please review the log, linked below.

Gist: https://gist.github.com/86e0709aa974c58a53976db5cf22c439

ibm-ompi · 2021-11-23T14:57:11Z

The IBM CI (PGI) build failed! Please review the log, linked below.

Gist: https://gist.github.com/23fe703dd200c2de96ce74ec90ba8dbd

bwbarrett · 2021-11-23T19:41:09Z

ompi/mca/osc/rdma/osc_rdma_component.c

- * use of registration-based RDMA (these BTLs will not be used) and will use
- * any remaining BTL. By default the BTLs used will be tcp and sm but any single
- * (or pair) of BTLs may be used.
+ * This function is used when there ompi_osc_rdm_query_btls() failed to find


failed to find a single btl that can communicate with all peers and supports remote completion.

Thank you! fixed in new revision.

bwbarrett · 2021-11-23T19:54:02Z

ompi/mca/osc/rdma/osc_rdma_component.c

-            }
-
+        ++btls_found;
+        if (module) {


I know Nathan did this wrong, but we explicitly compare to NULL in Open MPI.

Thank you! fixed in new revision.

bwbarrett · 2021-11-23T19:54:58Z

ompi/mca/osc/rdma/osc_rdma_component.c


-            OSC_RDMA_VERBOSE(MCA_BASE_VERBOSE_INFO, "found alternate btl %s", btls_to_use[i]);
+        OSC_RDMA_VERBOSE(MCA_BASE_VERBOSE_INFO, "disabing btl's native support of RDMA and ATOMIC");
+        item->btl_module->btl_flags &= ~(MCA_BTL_FLAGS_RDMA | MCA_BTL_FLAGS_GET | MCA_BTL_FLAGS_PUT | MCA_BTL_FLAGS_ATOMIC_FOPS );


I'm really not comfortable with this approach; we should not be modifying the BTL module's flags. Those are owned by the BTL itself. Why do we need to do this (rather than use our own state for capabilities)?

This is necessary because mca_am_rdma_init will only fill the gaps, e.g. if a btl does not support put/get/atomics, mca_am_rdma_init add the support. mca_am_rdma_init will not overwrite btl's native support.

but what osc/rdma want is for an alternate btl to always use AM rdma, do not use its native put/get/atomics.

Maybe we can add an flag to mca_am_rdma_init to force it overwrite a btl's native implementation of rdma/atomics?

I think we should go the flags to am_rdma_init route. I also don't know that we want to overwrite the btl's native implementation; remember that the PML is using the BTL at the same time.

I think we want a proxy btl for the one-sided to use, which is just the underlying BTL and all the AM structures. So less am_rdma_init() and more btl* btl_base_am_rdma_alloc(base_btl);

Thanks for the advice. I will add a function btl_base_am_rdma_alloc then

@bwbarrett I tried to allocate a new btl module, but encountered an issue.

The newly created btls are stored in "module->selected_btls. Later when creating peer, osc/rdma need to pick a btl (from module->selected_btls) for each peer. This is currently done by comparing pointer in module->selected_btlswith pointer of btl registered with bml. Because the newly created btls are not registered with bml, osc/rdma cannot find a btl (and an endpoint) for the peer. (this happens in functionompi_osc_rdma_peer_btl_endpoint`)

I wonder what is the best way to handle this issue?

Yeah, we'd need to go the whole way and import into the BML if we go down that path. I sent a long email to you and Nathan about some alternatives that might be less costly. Let's run that to ground and come back to this discussion.

ibm-ompi · 2021-11-24T00:46:37Z

The IBM CI (GNU/Scale) build failed! Please review the log, linked below.

Gist: https://gist.github.com/4978a3cec5b53fa9b1b3a979a2ceb2f9

ibm-ompi · 2021-11-24T00:52:00Z

The IBM CI (XL) build failed! Please review the log, linked below.

Gist: https://gist.github.com/09c1eb4b879d0c01b6243d77e637922b

ibm-ompi · 2021-11-24T01:06:09Z

The IBM CI (PGI) build failed! Please review the log, linked below.

Gist: https://gist.github.com/17465f4ecc97b9fe4cf924273b1608f8

wzamazon · 2021-11-24T15:12:28Z

address #8983 and #7830

gpaulsen · 2021-12-02T16:13:38Z

@wzamazon

  CC       osc_rdma_component.lo
In file included from ../../../../opal/util/arch.h:25:0,
                 from osc_rdma_component.c:48:
osc_rdma_component.c: In function 'ompi_osc_rdma_query_alternate_btls':
osc_rdma_component.c:965:50: error: 'MCA_BTL_FLAGS_RDMA_REMOTE_COMPLETION' undeclared (first use in this function)
             assert(item->btl_module->btl_flags & MCA_BTL_FLAGS_RDMA_REMOTE_COMPLETION);
                                                  ^
osc_rdma_component.c:965:50: note: each undeclared identifier is reported only once for each function it appears in
osc_rdma_component.c: In function 'ompi_osc_rdma_query_btls':
osc_rdma_component.c:1008:108: error: 'MCA_BTL_FLAGS_RDMA_REMOTE_COMPLETION' undeclared (first use in this function)
                     (item->btl_module->btl_flags & (MCA_BTL_FLAGS_ATOMIC_FOPS | MCA_BTL_FLAGS_ATOMIC_OPS | MCA_BTL_FLAGS_RDMA_REMOTE_COMPLETION))) {
                                                                                                            ^
make[2]: *** [osc_rdma_component.lo] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory `/__w/1/ompi/ompi/mca/osc/rdma'

wzamazon · 2021-12-02T16:19:20Z

bot:aws:retest

wzamazon · 2021-12-02T16:22:04Z

@gpaulsen

The flag MCA_BTL_FLAGS_RDMA_REMOTE_COMPLETION was introduced in

#9694

which was not merged when this PR was opened, thus caused the compilation error.

Retest should pass

wzamazon · 2021-12-02T16:22:14Z

bot:ibm:retest

wzamazon · 2021-12-02T16:23:05Z

@gpaulsen I am not sure how to restart the ibm and mellanox CI. Can you help?

awlauria · 2021-12-02T16:24:57Z

bot:ibm:retest

awlauria · 2021-12-02T16:25:07Z

/azp run

azure-pipelines · 2021-12-02T16:25:18Z

Azure Pipelines successfully started running 1 pipeline(s).

wzamazon · 2021-12-02T16:25:56Z

@awlauria Thank you!

This patch makes active message (AM) to not use get to implement put, because AM put based on get does not support remote completion. By making this change, active message put will support remote completion. Other active message operations: get, fop and cswap support remote completion by the nature of the completion. Therefore, this patch ensures active message RDMA/atomics always support remote completion. Signed-off-by: Wei Zhang <[email protected]>

This patch introduced a new struct ompi_osc_rdma_btl_wrapper_t, which contains all the information osc/rdma uses in a mca_btl_base_module_t object. With this change, a ompi_osc_rdma_btl_wrapper_t struct will be created when a mca_btl_base_module was selected, and stored in "selected_btl" osc/rdma then operates on an ompi_osc_rdma_btl_wrapper_t object instead of the mca_btl_base_module_t object directly. This change is a preparation for an upcoming change on btl selection logic. Signed-off-by: Wei Zhang <[email protected]>

This patch makes the following adjustments to the btl selection logic. First, when selecting a primary btl, this patch requires the primary btl to support remote completion. Second, when selecting alternate btls, this patch allowes any btl to be used as an alternate btl. Prior to this patch, only a list of pre-defined btls can be used as alternate btls. Finally, when a btl is used as an atlernate btl, this patch create a btl wrapper such that osc/rdma will always use active message RDMA/atomics on this alternate btl. The reason for these changes are: First, these changes ensured the selected btls of osc/rdma always support remote completion (because active message RDMA/atomics supports remote completion). Remote completion is essential for several key components of osc/rdma: the usage of cpu atomics to update peer's state, the usage of local leader to update peer's state and, its fence implementation. Therefore the assurance of btl's support of remote completion greatly simplified osc/rdma's implementation. Second, these changes eliminated the need to save and exchange more than 1 memory reigstration, because active message RDMA/atomic does not require explicit memory registration. Third, these changes ensured the correctness of atomic operations. When multiple alternate btls are used, atomicity cannot be guarenteed accross each btl's native implementation of atomics. Finally, these changes allowed more btls to be used as alternate btls, especially btl/self. Signed-off-by: Wei Zhang <[email protected]>

wzamazon · 2021-12-12T05:27:03Z

/azp run

azure-pipelines · 2021-12-12T05:27:13Z

Azure Pipelines successfully started running 1 pipeline(s).

wzamazon mentioned this pull request Nov 23, 2021

osc/rdma, btl: fix two issues with one-sided #9594

Closed

wzamazon requested a review from bwbarrett November 23, 2021 15:30

bwbarrett requested changes Nov 23, 2021

View reviewed changes

wzamazon force-pushed the osc_rdma_adjust_btl_selection_logic branch from b334866 to 59a4528 Compare November 24, 2021 00:11

wzamazon requested a review from bwbarrett November 24, 2021 00:12

wzamazon mentioned this pull request Dec 2, 2021

osc rdma/tcp: Wrong answer with c_post_start in MTT #8983

Closed

bwbarrett marked this pull request as draft December 2, 2021 16:20

wzamazon mentioned this pull request Dec 2, 2021

Fix OSC RDMA to work in cases where one BTL cannot reach all communicator group members #7830

Closed

wzamazon added 3 commits December 12, 2021 01:49

wzamazon force-pushed the osc_rdma_adjust_btl_selection_logic branch from 59a4528 to b98bba5 Compare December 12, 2021 04:09

wzamazon marked this pull request as ready for review December 12, 2021 05:26

wzamazon requested review from jsquyres and removed request for bwbarrett December 12, 2021 05:27

jsquyres added the Target: main label Jan 3, 2022

wzamazon closed this Feb 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

osc/rdma: adjustment on btl selection logic #9696

osc/rdma: adjustment on btl selection logic #9696

wzamazon commented Nov 23, 2021 •

edited

Loading

wzamazon commented Nov 23, 2021

ibm-ompi commented Nov 23, 2021

ibm-ompi commented Nov 23, 2021

ibm-ompi commented Nov 23, 2021

bwbarrett Nov 23, 2021

wzamazon Nov 24, 2021

bwbarrett Nov 23, 2021

wzamazon Nov 24, 2021

bwbarrett Nov 23, 2021

wzamazon Nov 23, 2021

bwbarrett Nov 24, 2021

wzamazon Nov 24, 2021

wzamazon Nov 25, 2021

bwbarrett Nov 30, 2021

ibm-ompi commented Nov 24, 2021

ibm-ompi commented Nov 24, 2021

ibm-ompi commented Nov 24, 2021

wzamazon commented Nov 24, 2021

gpaulsen commented Dec 2, 2021 •

edited

Loading

wzamazon commented Dec 2, 2021

wzamazon commented Dec 2, 2021

wzamazon commented Dec 2, 2021

wzamazon commented Dec 2, 2021

awlauria commented Dec 2, 2021

awlauria commented Dec 2, 2021

azure-pipelines bot commented Dec 2, 2021

wzamazon commented Dec 2, 2021

wzamazon commented Dec 12, 2021

azure-pipelines bot commented Dec 12, 2021

osc/rdma: adjustment on btl selection logic #9696

osc/rdma: adjustment on btl selection logic #9696

Conversation

wzamazon commented Nov 23, 2021 • edited Loading

wzamazon commented Nov 23, 2021

ibm-ompi commented Nov 23, 2021

ibm-ompi commented Nov 23, 2021

ibm-ompi commented Nov 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibm-ompi commented Nov 24, 2021

ibm-ompi commented Nov 24, 2021

ibm-ompi commented Nov 24, 2021

wzamazon commented Nov 24, 2021

gpaulsen commented Dec 2, 2021 • edited Loading

wzamazon commented Dec 2, 2021

wzamazon commented Dec 2, 2021

wzamazon commented Dec 2, 2021

wzamazon commented Dec 2, 2021

awlauria commented Dec 2, 2021

awlauria commented Dec 2, 2021

azure-pipelines bot commented Dec 2, 2021

wzamazon commented Dec 2, 2021

wzamazon commented Dec 12, 2021

azure-pipelines bot commented Dec 12, 2021

wzamazon commented Nov 23, 2021 •

edited

Loading

gpaulsen commented Dec 2, 2021 •

edited

Loading