From cbce0e244e13e1dda73d799c0cf0597708331c1a Mon Sep 17 00:00:00 2001 From: ferrol aderholdt Date: Mon, 21 Oct 2024 14:53:15 -0700 Subject: [PATCH] REVIEW: address feedback (remove API changes) --- src/components/tl/ucp/tl_ucp.c | 5 --- src/components/tl/ucp/tl_ucp.h | 2 -- src/components/tl/ucp/tl_ucp_coll.c | 45 +++++-------------------- src/components/tl/ucp/tl_ucp_context.c | 28 ++++----------- src/components/tl/ucp/tl_ucp_sendrecv.h | 25 +++----------- src/ucc/api/ucc.h | 29 ++-------------- src/ucc/api/ucc_status.h | 1 - src/utils/ucc_status.c | 2 -- 8 files changed, 23 insertions(+), 114 deletions(-) diff --git a/src/components/tl/ucp/tl_ucp.c b/src/components/tl/ucp/tl_ucp.c index a7ca9862e2..72586dc5de 100644 --- a/src/components/tl/ucp/tl_ucp.c +++ b/src/components/tl/ucp/tl_ucp.c @@ -189,11 +189,6 @@ ucc_config_field_t ucc_tl_ucp_lib_config_table[] = { ucc_offsetof(ucc_tl_ucp_lib_config_t, use_reordering), UCC_CONFIG_TYPE_BOOL}, - {"USE_XGVMI", "n", - "Use XGVMI for onesided collectives", - ucc_offsetof(ucc_tl_ucp_lib_config_t, use_xgvmi), - UCC_CONFIG_TYPE_BOOL}, - {NULL}}; static ucs_config_field_t ucc_tl_ucp_context_config_table[] = { diff --git a/src/components/tl/ucp/tl_ucp.h b/src/components/tl/ucp/tl_ucp.h index 25da807f66..20fb4ceca9 100644 --- a/src/components/tl/ucp/tl_ucp.h +++ b/src/components/tl/ucp/tl_ucp.h @@ -80,7 +80,6 @@ typedef struct ucc_tl_ucp_lib_config { uint32_t alltoallv_hybrid_pairwise_num_posts; ucc_ternary_auto_value_t use_topo; int use_reordering; - int use_xgvmi; } ucc_tl_ucp_lib_config_t; typedef struct ucc_tl_ucp_context_config { @@ -107,7 +106,6 @@ typedef struct ucc_tl_ucp_remote_info { void * va_base; size_t len; void * mem_h; - void * packed_memh; void * packed_key; size_t packed_key_len; } ucc_tl_ucp_remote_info_t; diff --git a/src/components/tl/ucp/tl_ucp_coll.c b/src/components/tl/ucp/tl_ucp_coll.c index 3f13cb4e2a..05a415dc99 100644 --- a/src/components/tl/ucp/tl_ucp_coll.c +++ b/src/components/tl/ucp/tl_ucp_coll.c @@ -193,26 +193,16 @@ ucc_status_t ucc_tl_ucp_memmap_segment(ucc_tl_ucp_task_t *task, ucp_mem_h mh; /* map the memory */ - if (map->resource != NULL) { - mmap_params.field_mask = UCP_MEM_MAP_PARAM_FIELD_EXPORTED_MEMH_BUFFER; - mmap_params.exported_memh_buffer = map->resource; - tl_ctx->dynamic_remote_info[segid].packed_memh = map->resource; - } else { - mmap_params.field_mask = - UCP_MEM_MAP_PARAM_FIELD_ADDRESS | UCP_MEM_MAP_PARAM_FIELD_LENGTH; - mmap_params.address = map->address; - mmap_params.length = map->len; - tl_ctx->dynamic_remote_info[segid].packed_memh = NULL; - } - /* map exported memory handle */ + mmap_params.field_mask = + UCP_MEM_MAP_PARAM_FIELD_ADDRESS | UCP_MEM_MAP_PARAM_FIELD_LENGTH; + mmap_params.address = map->address; + mmap_params.length = map->len; + ucs_status = ucp_mem_map(tl_ctx->worker.ucp_context, &mmap_params, &mh); - if (ucs_status == UCS_ERR_UNREACHABLE) { - tl_error(tl_ctx->super.super.lib, "exported memh is unsupported"); - return UCC_ERR_MEM_MAP_FAILURE; - } else if (ucs_status < UCS_OK) { + if (ucs_status < UCS_OK) { tl_error(tl_ctx->super.super.lib, "ucp_mem_map failed with error code: %d", ucs_status); - return UCC_ERR_MEM_MAP_FAILURE; + return ucs_status_to_ucc_status(ucs_status); } /* generate rkeys / packed keys */ tl_ctx->dynamic_remote_info[segid].va_base = map->address; @@ -237,7 +227,6 @@ ucc_status_t ucc_tl_ucp_coll_dynamic_segment_init(ucc_coll_args_t *coll_args, ucc_tl_ucp_team_t *tl_team = UCC_TL_UCP_TASK_TEAM(task); ucc_tl_ucp_context_t *ctx = UCC_TL_UCP_TEAM_CTX(tl_team); int i = 0; - ucc_mem_map_t *maps = coll_args->mem_map.segments; ucc_mem_map_t *seg_maps = NULL; size_t n_segments = 3; uint64_t need_map = UCC_TL_UCP_DYN_SEG_UPDATE_SRC | @@ -286,18 +275,17 @@ ucc_status_t ucc_tl_ucp_coll_dynamic_segment_init(ucc_coll_args_t *coll_args, seg_maps[index].address = coll_args->src.info.buffer; seg_maps[index].len = (coll_args->src.info.count) * ucc_dt_size(coll_args->src.info.datatype); - seg_maps[index++].resource = NULL; + ++index; } if (need_map & UCC_TL_UCP_DYN_SEG_UPDATE_DST) { seg_maps[index].address = coll_args->dst.info.buffer; seg_maps[index].len = (coll_args->dst.info.count) * ucc_dt_size(coll_args->dst.info.datatype); - seg_maps[index++].resource = NULL; + ++index; } if (need_map & UCC_TL_UCP_DYN_SEG_UPDATE_GLOBAL) { seg_maps[index].address = coll_args->global_work_buffer; seg_maps[index].len = (ONESIDED_SYNC_SIZE + ONESIDED_REDUCE_SIZE) * sizeof(long); - seg_maps[index++].resource = NULL; } } @@ -318,14 +306,6 @@ ucc_status_t ucc_tl_ucp_coll_dynamic_segment_init(ucc_coll_args_t *coll_args, } ++ctx->n_dynrinfo_segs; } - for (i = 0; i < coll_args->mem_map.n_segments; i++) { - status = ucc_tl_ucp_memmap_segment(task, &maps[i], i + n_segments); - if (status != UCC_OK) { - tl_error(UCC_TASK_LIB(task), "failed to memory map a segment"); - goto failed_memory_map; - } - ++ctx->n_dynrinfo_segs; - } if (n_segments) { free(seg_maps); } @@ -343,9 +323,6 @@ ucc_status_t ucc_tl_ucp_coll_dynamic_segment_init(ucc_coll_args_t *coll_args, if (ctx->dynamic_remote_info[i].packed_key) { ucp_rkey_buffer_release(ctx->dynamic_remote_info[i].packed_key); } - if (ctx->dynamic_remote_info[i].packed_memh) { - ucp_rkey_buffer_release(ctx->dynamic_remote_info[i].packed_memh); - } } ctx->n_dynrinfo_segs = 0; if (n_segments) { @@ -488,10 +465,6 @@ ucc_status_t ucc_tl_ucp_coll_dynamic_segment_finalize(ucc_tl_ucp_task_t *task) if (ctx->dynamic_remote_info[i].packed_key) { ucp_rkey_buffer_release(ctx->dynamic_remote_info[i].packed_key); } - if (ctx->dynamic_remote_info[i].packed_memh) { - ucp_rkey_buffer_release( - ctx->dynamic_remote_info[i].packed_memh); - } } /* destroy rkeys */ for (i = 0; i < UCC_TL_TEAM_SIZE(tl_team); i++) { diff --git a/src/components/tl/ucp/tl_ucp_context.c b/src/components/tl/ucp/tl_ucp_context.c index b5d9955e29..813ce335f3 100644 --- a/src/components/tl/ucp/tl_ucp_context.c +++ b/src/components/tl/ucp/tl_ucp_context.c @@ -166,9 +166,6 @@ UCC_CLASS_INIT_FUNC(ucc_tl_ucp_context_t, UCP_PARAM_FIELD_NAME; ucp_params.features = UCP_FEATURE_TAG | UCP_FEATURE_AM | UCP_FEATURE_RMA | UCP_FEATURE_AMO64; - if (lib->cfg.use_xgvmi) { - ucp_params.features |= UCP_FEATURE_EXPORTED_MEMH; - } ucp_params.tag_sender_mask = UCC_TL_UCP_TAG_SENDER_MASK; ucp_params.name = "UCC_UCP_CONTEXT"; @@ -364,8 +361,8 @@ ucc_status_t ucc_tl_ucp_rinfo_destroy(ucc_tl_ucp_context_t *ctx) } ucc_free(ctx->remote_info); ucc_free(ctx->rkeys); - ctx->remote_info = NULL; - ctx->rkeys = NULL; + ctx->remote_info = NULL; + ctx->rkeys = NULL; return UCC_OK; } @@ -493,28 +490,17 @@ ucc_status_t ucc_tl_ucp_ctx_remote_populate(ucc_tl_ucp_context_t * ctx, } for (i = 0; i < nsegs; i++) { - if (lib->cfg.use_xgvmi == 0 || - (lib->cfg.use_xgvmi == 1 && map.segments[i].resource == NULL)) { - mmap_params.field_mask = UCP_MEM_MAP_PARAM_FIELD_ADDRESS | - UCP_MEM_MAP_PARAM_FIELD_LENGTH; - mmap_params.address = map.segments[i].address; - mmap_params.length = map.segments[i].len; - } else { - mmap_params.field_mask = - UCP_MEM_MAP_PARAM_FIELD_EXPORTED_MEMH_BUFFER; - mmap_params.exported_memh_buffer = map.segments[i].resource; - } + mmap_params.field_mask = UCP_MEM_MAP_PARAM_FIELD_ADDRESS | + UCP_MEM_MAP_PARAM_FIELD_LENGTH; + mmap_params.address = map.segments[i].address; + mmap_params.length = map.segments[i].len; + status = ucp_mem_map(ctx->worker.ucp_context, &mmap_params, &mh); if (UCS_OK != status) { tl_error(lib, "ucp_mem_map failed with error code: %d", status); ucc_status = ucs_status_to_ucc_status(status); goto fail_mem_map; } - if (lib->cfg.use_xgvmi && map.segments[i].resource != NULL) { - ctx->remote_info[i].packed_memh = map.segments[i].resource; - } else { - ctx->remote_info[i].packed_memh = NULL; - } ctx->remote_info[i].mem_h = (void *)mh; status = ucp_rkey_pack(ctx->worker.ucp_context, mh, diff --git a/src/components/tl/ucp/tl_ucp_sendrecv.h b/src/components/tl/ucp/tl_ucp_sendrecv.h index 26e5cf335c..eda691eec0 100644 --- a/src/components/tl/ucp/tl_ucp_sendrecv.h +++ b/src/components/tl/ucp/tl_ucp_sendrecv.h @@ -247,7 +247,7 @@ static inline int resolve_segment(const void *va, size_t *key_sizes, static inline ucc_status_t ucc_tl_ucp_resolve_p2p_by_va(ucc_tl_ucp_team_t *team, void *va, size_t msglen, ucp_ep_h *ep, ucc_rank_t peer, uint64_t *rva, - ucp_rkey_h *rkey, void **packed_memh, int *segment) + ucp_rkey_h *rkey, int *segment) { ucc_tl_ucp_context_t *ctx = UCC_TL_UCP_TEAM_CTX(team); ptrdiff_t key_offset = 0; @@ -276,9 +276,6 @@ ucc_tl_ucp_resolve_p2p_by_va(ucc_tl_ucp_team_t *team, void *va, size_t msglen, if (*segment >= 0) { *rva = rvas[*segment] + ((uint64_t)va - (uint64_t)ctx->remote_info[*segment].va_base); - *packed_memh = (ctx->remote_info[*segment].packed_memh) - ? ctx->remote_info[*segment].mem_h - : NULL; if (ucc_unlikely(NULL == UCC_TL_UCP_REMOTE_RKEY(ctx, peer, *segment))) { ucs_status_t ucs_status = ucp_ep_rkey_unpack( *ep, PTR_OFFSET(keys, key_offset), @@ -302,9 +299,6 @@ ucc_tl_ucp_resolve_p2p_by_va(ucc_tl_ucp_team_t *team, void *va, size_t msglen, *rva = rvas[*segment] + ((uint64_t)va - (uint64_t)ctx->dynamic_remote_info[*segment].va_base); - *packed_memh = (ctx->dynamic_remote_info[*segment].packed_memh) - ? ctx->dynamic_remote_info[*segment].mem_h - : NULL; if (ucc_unlikely(NULL == UCC_TL_UCP_DYN_REMOTE_RKEY(ctx, peer, *segment))) { ucs_status_t ucs_status = ucp_ep_rkey_unpack( @@ -374,7 +368,6 @@ static inline ucc_status_t ucc_tl_ucp_put_nb(void *buffer, void *target, int segment = 0; ucp_rkey_h rkey = NULL; uint64_t rva = 0; - void *packed_memh = NULL; ucs_status_ptr_t ucp_status; ucc_status_t status; ucp_ep_h ep; @@ -386,7 +379,7 @@ static inline ucc_status_t ucc_tl_ucp_put_nb(void *buffer, void *target, status = ucc_tl_ucp_resolve_p2p_by_va(team, target, msglen, &ep, dest_group_rank, - &rva, &rkey, &packed_memh, &segment); + &rva, &rkey, &segment); if (ucc_unlikely(UCC_OK != status)) { return status; } @@ -397,10 +390,6 @@ static inline ucc_status_t ucc_tl_ucp_put_nb(void *buffer, void *target, req_param.cb.send = ucc_tl_ucp_put_completion_cb; req_param.user_data = (void *)task; req_param.memory_type = ucc_memtype_to_ucs[mtype]; - if (packed_memh) { - req_param.op_attr_mask |= UCP_OP_ATTR_FIELD_MEMH; - req_param.memh = packed_memh; - } ucp_status = ucp_put_nbx(ep, buffer, msglen, rva, rkey, &req_param); task->onesided.put_posted++; @@ -425,7 +414,6 @@ static inline ucc_status_t ucc_tl_ucp_get_nb(void *buffer, void *target, int segment = 0; ucp_rkey_h rkey = NULL; uint64_t rva = 0; - void *packed_memh = NULL; ucs_status_ptr_t ucp_status; ucc_status_t status; ucp_ep_h ep; @@ -437,7 +425,7 @@ static inline ucc_status_t ucc_tl_ucp_get_nb(void *buffer, void *target, status = ucc_tl_ucp_resolve_p2p_by_va(team, target, msglen, &ep, dest_group_rank, - &rva, &rkey, &packed_memh, &segment); + &rva, &rkey, &segment); if (ucc_unlikely(UCC_OK != status)) { return status; } @@ -448,10 +436,6 @@ static inline ucc_status_t ucc_tl_ucp_get_nb(void *buffer, void *target, req_param.cb.send = ucc_tl_ucp_get_completion_cb; req_param.user_data = (void *)task; req_param.memory_type = ucc_memtype_to_ucs[mtype]; - if (packed_memh) { - req_param.op_attr_mask |= UCP_OP_ATTR_FIELD_MEMH; - req_param.memh = packed_memh; - } ucp_status = ucp_get_nbx(ep, buffer, msglen, rva, rkey, &req_param); task->onesided.get_posted++; @@ -475,7 +459,6 @@ static inline ucc_status_t ucc_tl_ucp_atomic_inc(void * target, uint64_t one = 1; ucp_rkey_h rkey = NULL; uint64_t rva = 0; - void *packed_memh = NULL; ucs_status_ptr_t ucp_status; ucc_status_t status; ucp_ep_h ep; @@ -487,7 +470,7 @@ static inline ucc_status_t ucc_tl_ucp_atomic_inc(void * target, status = ucc_tl_ucp_resolve_p2p_by_va(team, target, sizeof(uint64_t), &ep, dest_group_rank, &rva, &rkey, - &packed_memh, &segment); + &segment); if (ucc_unlikely(UCC_OK != status)) { return status; } diff --git a/src/ucc/api/ucc.h b/src/ucc/api/ucc.h index 49a21f7401..05b2286186 100644 --- a/src/ucc/api/ucc.h +++ b/src/ucc/api/ucc.h @@ -890,27 +890,13 @@ typedef struct ucc_oob_coll { typedef ucc_oob_coll_t ucc_context_oob_coll_t; typedef ucc_oob_coll_t ucc_team_oob_coll_t; -/** - * @ingroup UCC_CONTEXT_DT - */ -typedef enum { - UCC_MEM_MAP_TYPE_SEND_BUF, - UCC_MEM_MAP_TYPE_RECV_BUF, - UCC_MEM_MAP_TYPE_SEND_RECV_BUF, -} ucc_mem_map_usage_t; - /** * * @ingroup UCC_CONTEXT_DT */ typedef struct ucc_mem_map { - void *address; /*!< the address of a buffer to be attached to - a UCC context */ - size_t len; /*!< the length of the buffer */ - ucc_mem_map_usage_t type; /*!< the usage type of buffer being mapped. */ - void *resource; /*!< resource associated with the address. - examples of resources include memory - keys. */ + void *address; /*!< the address of a buffer to be attached to a UCC context */ + size_t len; /*!< the length of the buffer */ } ucc_mem_map_t; /** @@ -1812,8 +1798,7 @@ enum ucc_coll_args_field { UCC_COLL_ARGS_FIELD_TAG = UCC_BIT(1), UCC_COLL_ARGS_FIELD_CB = UCC_BIT(2), UCC_COLL_ARGS_FIELD_GLOBAL_WORK_BUFFER = UCC_BIT(3), - UCC_COLL_ARGS_FIELD_ACTIVE_SET = UCC_BIT(4), - UCC_COLL_ARGS_FIELD_MEM_MAP = UCC_BIT(5) + UCC_COLL_ARGS_FIELD_ACTIVE_SET = UCC_BIT(4) }; /** @@ -1888,14 +1873,6 @@ typedef struct ucc_coll_args { int64_t stride; uint64_t size; } active_set; - ucc_mem_map_params_t mem_map; /*!< Memory regions to be used - for the current collective. - If set, the designated regions - will be mapped and information - exchanged. Memory is unmapped - at collective completion. Not - necessary for two-sided - collectives. */ } ucc_coll_args_t; /** diff --git a/src/ucc/api/ucc_status.h b/src/ucc/api/ucc_status.h index 49ab4e1d7e..90d25b463b 100644 --- a/src/ucc/api/ucc_status.h +++ b/src/ucc/api/ucc_status.h @@ -40,7 +40,6 @@ typedef enum { UCC_ERR_NO_MESSAGE = -6, /*!< General purpose return code without specific error */ UCC_ERR_NOT_FOUND = -7, UCC_ERR_TIMED_OUT = -8, - UCC_ERR_MEM_MAP_FAILURE = -9, UCC_ERR_LAST = -100, } ucc_status_t; diff --git a/src/utils/ucc_status.c b/src/utils/ucc_status.c index 430d4a96d5..2e2285eb61 100644 --- a/src/utils/ucc_status.c +++ b/src/utils/ucc_status.c @@ -34,8 +34,6 @@ const char *ucc_status_string(ucc_status_t status) return "Not found"; case UCC_ERR_TIMED_OUT: return "Timeout expired"; - case UCC_ERR_MEM_MAP_FAILURE: - return "Failed to memory map address"; default: snprintf(error_str, sizeof(error_str) - 1, "Unknown error %d", status); return error_str;