PR feedback

Signed-off-by: Howard Pritchard <[email protected]>

Author: hppritcha

ompi/communicator/comm_cid.c

@@ -952,6 +952,7 @@ int ompi_comm_activate_nb (ompi_communicator_t **newcomm, ompi_communicator_t *c
     ompi_comm_cid_context_t *context;
     ompi_comm_request_t *request;
     ompi_request_t *subreq;
+    uint32_t comm_size;
     int ret = 0;
 
     /* the caller should not pass NULL for comm (it may be the same as *newcomm) */
@@ -973,6 +974,25 @@ int ompi_comm_activate_nb (ompi_communicator_t **newcomm, ompi_communicator_t *c
 
     request->context = &context->super;
 
+    /* Prep communicator for handling remote cids if needed */
+
+    if (!OMPI_COMM_IS_GLOBAL_INDEX(*newcomm)) {
+        if (OMPI_COMM_IS_INTER(*newcomm)) {
+            comm_size = ompi_comm_remote_size(*newcomm);
+        } else {
+            comm_size = ompi_comm_size(*newcomm);
+        }
+
+        (*newcomm)->c_index_vec = (uint32_t *)calloc(comm_size, sizeof(uint32_t));
+        if (NULL == (*newcomm)->c_index_vec) {
+            return OMPI_ERR_OUT_OF_RESOURCE;
+        }
+
+        if (OMPI_COMM_IS_INTRA(*newcomm)) {
+            (*newcomm)->c_index_vec[(*newcomm)->c_my_rank] = (*newcomm)->c_index;
+        }
+    }
+
     if (MPI_UNDEFINED != (*newcomm)->c_local_group->grp_my_rank) {
         /* Initialize the PML stuff in the newcomm  */
         if ( OMPI_SUCCESS != (ret = MCA_PML_CALL(add_comm(*newcomm))) ) {
@@ -1041,9 +1061,16 @@ int ompi_comm_get_remote_cid (ompi_communicator_t *comm, int dest, uint32_t *rem
 
     assert(NULL != remote_cid);
 
-    if (OMPI_COMM_IS_GLOBAL_INDEX(comm)) {
+    if (OPAL_LIKELY(OMPI_COMM_IS_GLOBAL_INDEX(comm))) {
+
         *remote_cid = comm->c_index;
+
+    } else if (0 != comm->c_index_vec[dest]) {
+
+        *remote_cid = comm->c_index_vec[dest];
+
     } else {
+
         ompi_proc = ompi_comm_peer_lookup(comm, dest);
         OPAL_PMIX_CONVERT_NAME(&pmix_proc, &ompi_proc->super.proc_name);
 
@@ -1075,8 +1102,10 @@ int ompi_comm_get_remote_cid (ompi_communicator_t *comm, int dest, uint32_t *rem
             PMIX_VALUE_GET_NUMBER(rc, val, remote_cid64, size_t);
             rc = OMPI_SUCCESS;
             *remote_cid = (uint32_t)remote_cid64;
+            comm->c_index_vec[dest] = (uint32_t)remote_cid64;
             OPAL_OUTPUT_VERBOSE((10, ompi_comm_output, "PMIx_Get PMIX_GROUP_LOCAL_CID %d for cid_base %ld", *remote_cid, excid.cid_base));
         } 
+
     }
 
 done:

ompi/communicator/comm_init.c

@@ -432,6 +432,7 @@ static void ompi_comm_construct(ompi_communicator_t* comm)
     comm->c_coll         = NULL;
     comm->c_nbc_tag      = MCA_COLL_BASE_TAG_NONBLOCKING_BASE;
     comm->instance       = NULL;
+    comm->c_index_vec    = NULL;
 
     /*
      * magic numerology - see TOPDIR/ompi/include/mpif-values.pl
@@ -533,6 +534,11 @@ static void ompi_comm_destruct(ompi_communicator_t* comm)
         comm->c_name = NULL;
     }
 
+    if (NULL != comm->c_index_vec) {
+        free (comm->c_index_vec);
+        comm->c_index_vec = NULL;
+    }
+
 #if OPAL_ENABLE_FT_MPI
     if( NULL != comm->agreement_specific ) {
         OBJ_RELEASE( comm->agreement_specific );

ompi/communicator/communicator.h

@@ -286,6 +286,10 @@ struct ompi_communicator_t {
     uint32_t c_epoch;  /* Identifier used to differentiate between two communicators
                           using the same c_contextid (not at the same time, obviously) */
 #endif
+    /* vector used to store remote cid values for communicators not using
+     * a global cid, i.e. when OMPI_COMM_IS_GLOBAL_INDEX(comm) returns 0.
+     */
+    uint32_t                     *c_index_vec;
     /* Non-blocking collective tag. These tags might be shared between
      * all non-blocking collective modules (to avoid message collision
      * between them in the case where multiple outstanding non-blocking

ompi/mca/mtl/ofi/mtl_ofi.c

@@ -1,6 +1,6 @@
 /*
  * Copyright (c) 2013-2020 Intel, Inc.  All rights reserved.
- * Copyright (c) 2021-2022 Triad National Security, LLC. All rights
+ * Copyright (c) 2021-2024 Triad National Security, LLC. All rights
  *                         reserved.
  *
  * $COPYRIGHT$
@@ -14,8 +14,6 @@
 
 OMPI_DECLSPEC extern mca_mtl_ofi_component_t mca_mtl_ofi_component;
 
-OBJ_CLASS_INSTANCE(mca_mtl_comm_t, opal_object_t, NULL, NULL);
-
 mca_mtl_ofi_module_t ompi_mtl_ofi = {
     {
         (int)((1ULL << MTL_OFI_CID_BIT_COUNT_1) - 1), /* max cid */
@@ -346,38 +344,10 @@ int ompi_mtl_ofi_add_comm(struct mca_mtl_base_module_t *mtl,
                       struct ompi_communicator_t *comm)
 {
     int ret = OMPI_SUCCESS;
-    uint32_t comm_size;
-    mca_mtl_comm_t* mtl_comm;
 
     mca_mtl_ofi_ep_type ep_type = (0 == ompi_mtl_ofi.enable_sep) ?
                                   OFI_REGULAR_EP : OFI_SCALABLE_EP;
 
-    if (!OMPI_COMM_IS_GLOBAL_INDEX(comm)) {
-        mtl_comm = OBJ_NEW(mca_mtl_comm_t);
-
-        if (OMPI_COMM_IS_INTER(comm)) {
-            comm_size = ompi_comm_remote_size(comm);
-        } else {
-            comm_size = ompi_comm_size(comm);
-        }
-        mtl_comm->c_index_vec = (c_index_vec_t *)calloc(comm_size, sizeof(c_index_vec_t));
-        if (NULL == mtl_comm->c_index_vec) {
-            ret = OMPI_ERR_OUT_OF_RESOURCE;
-            OBJ_RELEASE(mtl_comm);
-            goto error;
-        }
-        if (OMPI_COMM_IS_INTRA(comm)) {
-            mtl_comm->c_index_vec[comm->c_my_rank].c_index = comm->c_index;
-        }
-
-        comm->c_mtl_comm = mtl_comm;
-
-    } else  {
-
-        comm->c_mtl_comm = NULL;
-
-    }
-
     /*
      * If thread grouping enabled, add new OFI context for each communicator
      * other than MPI_COMM_SELF.
@@ -407,12 +377,6 @@ int ompi_mtl_ofi_del_comm(struct mca_mtl_base_module_t *mtl,
     mca_mtl_ofi_ep_type ep_type = (0 == ompi_mtl_ofi.enable_sep) ?
                                   OFI_REGULAR_EP : OFI_SCALABLE_EP;
 
-    if(NULL != comm->c_mtl_comm) {
-        free(comm->c_mtl_comm->c_index_vec);
-        OBJ_RELEASE(comm->c_mtl_comm);
-        comm->c_mtl_comm = NULL;
-    }
-
     /*
      * Clean up OFI contexts information.
      */

ompi/mca/mtl/ofi/mtl_ofi.h

@@ -76,31 +76,6 @@ int ompi_mtl_ofi_progress_no_inline(void);
 extern opal_thread_local int ompi_mtl_ofi_per_thread_ctx;
 #endif
 
-#define MCA_MTL_OFI_CID_NOT_EXCHANGED 2
-#define MCA_MTL_OFI_CID_EXCHANGING    1 
-#define MCA_MTL_OFI_CID_EXCHANGED     0
-
-typedef struct {
-    uint32_t c_index;
-} c_index_vec_t;
-
-typedef struct mca_mtl_comm_t {
-    opal_object_t super;
-    c_index_vec_t *c_index_vec;
-} mca_mtl_comm_t;
-
-OBJ_CLASS_DECLARATION(mca_mtl_comm_t);
-
-struct mca_mtl_ofi_cid_hdr_t {
-    ompi_comm_extended_cid_t hdr_cid;
-    int16_t                  hdr_src_c_index;
-    int32_t                  hdr_src;
-    bool                     need_response;
-    bool                     ofi_cq_data;
-};
-
-typedef struct mca_mtl_ofi_cid_hdr_t mca_mtl_ofi_cid_hdr_t;
-
 /* Set OFI context for operations which generate completion events */
 __opal_attribute_always_inline__ static inline void
 set_thread_context(int ctxt)
@@ -572,7 +547,8 @@ ompi_mtl_ofi_send_generic(struct mca_mtl_base_module_t *mtl,
 {
     ssize_t ret = OMPI_SUCCESS;
     ompi_mtl_ofi_request_t ofi_req;
-    int ompi_ret, ctxt_id = 0, c_index_for_tag;
+    int ompi_ret, ctxt_id = 0;
+    uint32_t c_index_for_tag;
     void *start;
     bool free_after;
     size_t length;
@@ -582,25 +558,10 @@ ompi_mtl_ofi_send_generic(struct mca_mtl_base_module_t *mtl,
     ompi_mtl_ofi_request_t *ack_req = NULL; /* For synchronous send */
     fi_addr_t src_addr = 0;
     fi_addr_t sep_peer_fiaddr = 0;
-    mca_mtl_comm_t *mtl_comm;
-    uint32_t remote_cid;
 
-    if (OPAL_LIKELY(OMPI_COMM_IS_GLOBAL_INDEX(comm))) {
-        c_index_for_tag = comm->c_index;
-    } else {
-        mtl_comm = comm->c_mtl_comm;
-        /*
-         * if we do not have the local cid for the target receiver,
-         * retrieve via the PMIx modex
-         */
-        if (mtl_comm->c_index_vec[dest].c_index == 0) {
-            ompi_ret = ompi_comm_get_remote_cid(comm, dest, &remote_cid);
-            if (OPAL_UNLIKELY(OMPI_SUCCESS != ompi_ret)) {
-                return ompi_ret;
-            }
-            mtl_comm->c_index_vec[dest].c_index = remote_cid;
-        }
-        c_index_for_tag = mtl_comm->c_index_vec[dest].c_index;
+    ompi_ret = ompi_comm_get_remote_cid(comm, dest, &c_index_for_tag);
+    if (OPAL_UNLIKELY(OMPI_SUCCESS != ompi_ret)) {
+        return ompi_ret;
     }
 
     ompi_mtl_ofi_set_mr_null(&ofi_req);
@@ -828,7 +789,8 @@ ompi_mtl_ofi_isend_generic(struct mca_mtl_base_module_t *mtl,
 {
     ssize_t ret = OMPI_SUCCESS;
     ompi_mtl_ofi_request_t *ofi_req = (ompi_mtl_ofi_request_t *) mtl_request;
-    int ompi_ret, ctxt_id = 0, c_index_for_tag;
+    int ompi_ret, ctxt_id = 0;
+    uint32_t c_index_for_tag;
     void *start;
     size_t length;
     bool free_after;
@@ -837,28 +799,10 @@ ompi_mtl_ofi_isend_generic(struct mca_mtl_base_module_t *mtl,
     mca_mtl_ofi_endpoint_t *endpoint = NULL;
     ompi_mtl_ofi_request_t *ack_req = NULL; /* For synchronous send */
     fi_addr_t sep_peer_fiaddr = 0;
-    mca_mtl_comm_t *mtl_comm;
-    uint32_t remote_cid;
 
     ompi_mtl_ofi_set_mr_null(ofi_req);
 
-    if (OMPI_COMM_IS_GLOBAL_INDEX(comm)) {
-        c_index_for_tag = comm->c_index;
-    } else {
-        mtl_comm = comm->c_mtl_comm;
-        /*
-         * if we do not have the local cid for the target receiver,
-         * retrive via the PMIx modex 
-         */
-        if (mtl_comm->c_index_vec[dest].c_index == 0) {    
-            ompi_ret = ompi_comm_get_remote_cid(comm, dest, &remote_cid);
-            if (OPAL_UNLIKELY(OMPI_SUCCESS != ompi_ret)) {
-                return ompi_ret;
-            }   
-            mtl_comm->c_index_vec[dest].c_index = remote_cid;
-        }
-        c_index_for_tag = mtl_comm->c_index_vec[dest].c_index;
-    }
+    ompi_ret = ompi_comm_get_remote_cid(comm, dest, &c_index_for_tag);
 
     if (ompi_mtl_ofi.total_ctxts_used > 0) {
         ctxt_id = comm->c_contextid.cid_sub.u64 % ompi_mtl_ofi.total_ctxts_used;