From ee5a31cbf2055cda8f3743d724a3135bbb6c42db Mon Sep 17 00:00:00 2001 From: Nick Sarkauskas Date: Fri, 17 May 2024 12:01:06 -0700 Subject: [PATCH] CL/DOCA_UROM: Review feedback --- config/m4/doca_urom.m4 | 8 ++--- config/m4/doca_urom_ucc.m4 | 12 ++++---- configure.ac | 3 ++ src/components/cl/doca_urom/Makefile.am | 2 +- src/components/cl/doca_urom/cl_doca_urom.c | 26 ++++++++--------- src/components/cl/doca_urom/cl_doca_urom.h | 29 ++++++++----------- .../cl/doca_urom/cl_doca_urom_coll.c | 3 +- .../cl/doca_urom/cl_doca_urom_coll.h | 5 ++-- .../cl/doca_urom/cl_doca_urom_context.c | 2 -- .../cl/doca_urom/cl_doca_urom_lib.c | 12 ++++---- .../cl/doca_urom/cl_doca_urom_team.c | 6 ++-- src/components/cl/ucc_cl.c | 8 ++--- src/components/tl/ucp/tl_ucp_context.c | 6 ++-- 13 files changed, 57 insertions(+), 65 deletions(-) diff --git a/config/m4/doca_urom.m4 b/config/m4/doca_urom.m4 index 7037c6f645..8295c946c0 100644 --- a/config/m4/doca_urom.m4 +++ b/config/m4/doca_urom.m4 @@ -11,8 +11,7 @@ AS_IF([test "x$doca_urom_checked" != "xyes"],[ [], [with_doca_urom=guess]) AS_IF([test "x$with_doca_urom" != "xno"], [ - save_CPPFLAGS="$CPPFLAGS $UCS_CPPFLAGS" - save_CFLAGS="$CFLAGS" + save_CPPFLAGS="$CPPFLAGS" save_LDFLAGS="$LDFLAGS" AS_IF([test ! -z "$with_doca_urom" -a "x$with_doca_urom" != "xyes" -a "x$with_doca_urom" != "xguess"], [ @@ -20,7 +19,7 @@ AS_IF([test "x$doca_urom_checked" != "xyes"],[ [AC_MSG_ERROR([Provided "--with-doca_urom=${with_doca_urom}" location does not exist])]) check_doca_urom_dir="$with_doca_urom" check_doca_urom_libdir="$with_doca_urom/lib64" - CPPFLAGS="-I$with_doca_urom/include $save_CPPFLAGS" + CPPFLAGS="-I$with_doca_urom/include $UCS_CPPFLAGS $save_CPPFLAGS" LDFLAGS="-L$check_doca_urom_libdir $save_LDFLAGS" ]) AS_IF([test ! -z "$with_doca_urom_libdir" -a "x$with_doca_urom_libdir" != "xyes"], @@ -65,7 +64,6 @@ AS_IF([test "x$doca_urom_checked" != "xyes"],[ AC_MSG_WARN([DOCA_UROM not found]) ]) ]) - CFLAGS="$save_CFLAGS" CPPFLAGS="$save_CPPFLAGS" LDFLAGS="$save_LDFLAGS" ], @@ -74,4 +72,4 @@ AS_IF([test "x$doca_urom_checked" != "xyes"],[ ]) doca_urom_checked=yes AM_CONDITIONAL([HAVE_DOCA_UROM], [test "x$doca_urom_happy" != xno]) -])]) \ No newline at end of file +])]) diff --git a/config/m4/doca_urom_ucc.m4 b/config/m4/doca_urom_ucc.m4 index 020bc33cc0..56ba0610af 100644 --- a/config/m4/doca_urom_ucc.m4 +++ b/config/m4/doca_urom_ucc.m4 @@ -13,17 +13,16 @@ AS_IF([test "x$doca_urom_ucc_checked" != "xyes"],[ [ AS_IF([test "x$doca_urom_happy" == "xyes"], [ - save_CPPFLAGS="$CPPFLAGS $UCS_CPPFLAGS $DOCA_UROM_CPPFLAGS" - save_CFLAGS="$CFLAGS" - save_LDFLAGS="$LDFLAGS $DOCA_UROM_LDFLAGS" + save_CPPFLAGS="$CPPFLAGS" + save_LDFLAGS="$LDFLAGS" AS_IF([test ! -z "$with_doca_urom_ucc" -a "x$with_doca_urom_ucc" != "xyes" -a "x$with_doca_urom_ucc" != "xguess"], [ AS_IF([test ! -d $with_doca_urom_ucc], [AC_MSG_ERROR([Provided "--with-doca_urom_ucc=${with_doca_urom_ucc}" location does not exist])]) check_doca_urom_ucc_dir="$with_doca_urom_ucc" check_doca_urom_ucc_libdir="$with_doca_urom_ucc/lib64" - CPPFLAGS="-I$with_doca_urom_ucc/include $save_CPPFLAGS" - LDFLAGS="-L$check_doca_urom_ucc_libdir $save_LDFLAGS" + CPPFLAGS="-I$with_doca_urom_ucc/include $UCS_CPPFLAGS $DOCA_UROM_CPPFLAGS $save_CPPFLAGS" + LDFLAGS="-L$check_doca_urom_ucc_libdir $DOCA_UROM_LDFLAGS $save_LDFLAGS" ]) AS_IF([test ! -z "$with_doca_urom_ucc_libdir" -a "x$with_doca_urom_ucc_libdir" != "xyes"], [ @@ -67,7 +66,6 @@ AS_IF([test "x$doca_urom_ucc_checked" != "xyes"],[ AC_MSG_WARN([DOCA_UROM_UCC not found]) ]) ]) - CFLAGS="$save_CFLAGS" CPPFLAGS="$save_CPPFLAGS" LDFLAGS="$save_LDFLAGS" ], @@ -81,4 +79,4 @@ AS_IF([test "x$doca_urom_ucc_checked" != "xyes"],[ ]) doca_urom_ucc_checked=yes AM_CONDITIONAL([HAVE_DOCA_UROM_UCC], [test "x$doca_urom_ucc_happy" != xno]) -])]) \ No newline at end of file +])]) diff --git a/configure.ac b/configure.ac index 936bfe3750..490e484d4d 100644 --- a/configure.ac +++ b/configure.ac @@ -162,6 +162,8 @@ AS_IF([test "x$with_docs_only" = xyes], AM_CONDITIONAL([HAVE_IBVERBS],[false]) AM_CONDITIONAL([HAVE_RDMACM],[false]) AM_CONDITIONAL([HAVE_MLX5DV],[false]) + AM_CONDITIONAL([HAVE_DOCA_UROM], [false]) + AM_CONDITIONAL([HAVE_DOCA_UROM_UCC], [false]) ], [ AM_CONDITIONAL([DOCS_ONLY], [false]) @@ -274,6 +276,7 @@ AC_MSG_NOTICE([ C++ compiler: ${CXX} ${CXXFLAGS} ${BASE_CXXFLAGS}]) AS_IF([test "x$cuda_happy" = "xyes"],[ AC_MSG_NOTICE([ NVCC gencodes: ${NVCC_ARCH}]) ]) +AC_MSG_NOTICE([ DOCA UROM enabled: ${doca_urom_happy}]) AC_MSG_NOTICE([ Perftest: ${mpi_enable}]) AC_MSG_NOTICE([ Gtest: ${gtest_enable}]) AC_MSG_NOTICE([ MC modules: <$(echo ${mc_modules}|tr ':' ' ') >]) diff --git a/src/components/cl/doca_urom/Makefile.am b/src/components/cl/doca_urom/Makefile.am index 6f23e5281c..d70339dd7b 100644 --- a/src/components/cl/doca_urom/Makefile.am +++ b/src/components/cl/doca_urom/Makefile.am @@ -17,4 +17,4 @@ libucc_cl_doca_urom_la_CFLAGS = $(BASE_CFLAGS) libucc_cl_doca_urom_la_LDFLAGS = -version-info $(SOVERSION) --as-needed $(DOCA_UROM_UCC_LDFLAGS) $(DOCA_UROM_LDFLAGS) libucc_cl_doca_urom_la_LIBADD = $(DOCA_UROM_UCC_LIBADD) $(DOCA_UROM_LIBADD) $(UCC_TOP_BUILDDIR)/src/libucc.la -include $(top_srcdir)/config/module.am \ No newline at end of file +include $(top_srcdir)/config/module.am diff --git a/src/components/cl/doca_urom/cl_doca_urom.c b/src/components/cl/doca_urom/cl_doca_urom.c index 8be335527d..f676108acd 100644 --- a/src/components/cl/doca_urom/cl_doca_urom.c +++ b/src/components/cl/doca_urom/cl_doca_urom.c @@ -19,32 +19,32 @@ static ucc_config_field_t ucc_cl_doca_urom_lib_config_table[] = { {"", "", NULL, ucc_offsetof(ucc_cl_doca_urom_lib_config_t, super), UCC_CONFIG_TYPE_TABLE(ucc_cl_lib_config_table)}, + {NULL}}; + +static ucs_config_field_t ucc_cl_doca_urom_context_config_table[] = { + {"", "", NULL, ucc_offsetof(ucc_cl_doca_urom_context_config_t, super), + UCC_CONFIG_TYPE_TABLE(ucc_cl_context_config_table)}, + {"DEVICE", "mlx5_0", "DPU device", - ucc_offsetof(ucc_cl_doca_urom_lib_config_t, device), + ucc_offsetof(ucc_cl_doca_urom_context_config_t, device), UCC_CONFIG_TYPE_STRING}, {"PLUGIN_NAME", "libdoca_urom_ucc_plugin", "Name of plugin library", - ucc_offsetof(ucc_cl_doca_urom_lib_config_t, plugin_name), + ucc_offsetof(ucc_cl_doca_urom_context_config_t, plugin_name), UCC_CONFIG_TYPE_STRING}, - {"DOCA_LOG_LEVEL", "10", - "DOCA log level", - ucc_offsetof(ucc_cl_doca_urom_lib_config_t, doca_log_level), - UCC_CONFIG_TYPE_INT}, - - {NULL}}; - -static ucs_config_field_t ucc_cl_doca_urom_context_config_table[] = { - {"", "", NULL, ucc_offsetof(ucc_cl_doca_urom_context_config_t, super), - UCC_CONFIG_TYPE_TABLE(ucc_cl_context_config_table)}, - {"PLUGIN_ENVS", "", "Comma separated envs to pass to the worker plugin", ucc_offsetof(ucc_cl_doca_urom_context_config_t, plugin_envs), UCC_CONFIG_TYPE_STRING_ARRAY}, + {"DOCA_LOG_LEVEL", "10", + "DOCA log level", + ucc_offsetof(ucc_cl_doca_urom_context_config_t, doca_log_level), + UCC_CONFIG_TYPE_INT}, + {NULL}}; UCC_CLASS_DEFINE_NEW_FUNC(ucc_cl_doca_urom_lib_t, ucc_base_lib_t, diff --git a/src/components/cl/doca_urom/cl_doca_urom.h b/src/components/cl/doca_urom/cl_doca_urom.h index 1e92246d6e..ae566591d4 100644 --- a/src/components/cl/doca_urom/cl_doca_urom.h +++ b/src/components/cl/doca_urom/cl_doca_urom.h @@ -1,5 +1,5 @@ /** - * Copyright (c) 2020, NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * Copyright (c) 2020-2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * * See file LICENSE for terms. */ @@ -13,8 +13,6 @@ #include "coll_score/ucc_coll_score.h" #include "utils/ucc_mpool.h" -#include "../../tl/ucp/tl_ucp.h" - #include #include #include @@ -42,17 +40,17 @@ extern ucc_cl_doca_urom_iface_t ucc_cl_doca_urom; typedef struct ucc_cl_doca_urom_lib_config { ucc_cl_lib_config_t super; - char *device; - char *plugin_name; - int doca_log_level; } ucc_cl_doca_urom_lib_config_t; typedef struct ucc_cl_doca_urom_context_config { - ucc_cl_context_config_t super; + ucc_cl_context_config_t super; + char *device; + char *plugin_name; ucs_config_names_array_t plugin_envs; + int doca_log_level; } ucc_cl_doca_urom_context_config_t; -typedef struct urom_ctx { +typedef struct ucc_cl_doca_urom_ctx { struct doca_urom_service *urom_service; struct doca_urom_worker *urom_worker; struct doca_urom_domain *urom_domain; @@ -63,12 +61,11 @@ typedef struct urom_ctx { uint64_t worker_id; void *urom_ucc_context; ucc_rank_t ctx_rank; -} urom_ctx_t; +} ucc_cl_doca_urom_ctx_t; typedef struct ucc_cl_doca_urom_lib { ucc_cl_lib_t super; ucc_cl_doca_urom_lib_config_t cfg; - urom_ctx_t urom_ctx; struct doca_dev *dev; int tl_ucp_index; } ucc_cl_doca_urom_lib_t; @@ -76,12 +73,11 @@ UCC_CLASS_DECLARE(ucc_cl_doca_urom_lib_t, const ucc_base_lib_params_t *, const ucc_base_config_t *); typedef struct ucc_cl_doca_urom_context { - ucc_cl_context_t super; - void *urom_ucc_ctx_h; - ucc_mpool_t sched_mp; - void *old_dest; - void *old_src; - ucp_context_h ucp_context; + ucc_cl_context_t super; + void *urom_ucc_ctx_h; + ucc_mpool_t sched_mp; + ucp_context_h ucp_context; + ucc_cl_doca_urom_ctx_t urom_ctx; ucc_cl_doca_urom_context_config_t cfg; } ucc_cl_doca_urom_context_t; UCC_CLASS_DECLARE(ucc_cl_doca_urom_context_t, const ucc_base_context_params_t *, @@ -89,7 +85,6 @@ UCC_CLASS_DECLARE(ucc_cl_doca_urom_context_t, const ucc_base_context_params_t *, typedef struct ucc_cl_doca_urom_team { ucc_cl_team_t super; - int team_posted; ucc_team_h **teams; unsigned n_teams; ucc_coll_score_t *score; diff --git a/src/components/cl/doca_urom/cl_doca_urom_coll.c b/src/components/cl/doca_urom/cl_doca_urom_coll.c index d34e635edf..e32df77126 100644 --- a/src/components/cl/doca_urom/cl_doca_urom_coll.c +++ b/src/components/cl/doca_urom/cl_doca_urom_coll.c @@ -7,7 +7,6 @@ #include "cl_doca_urom.h" #include "cl_doca_urom_coll.h" #include "utils/ucc_coll_utils.h" -#include "components/tl/ucp/tl_ucp.h" #include #include @@ -50,5 +49,5 @@ ucc_status_t ucc_cl_doca_urom_coll_init(ucc_base_coll_args_t *coll_args, ucc_cl_doca_urom_coll_full_start(*task); ucc_cl_doca_urom_triggered_post_setup(*task); - return UCC_OK; + return UCC_ERR_NOT_IMPLEMENTED; } diff --git a/src/components/cl/doca_urom/cl_doca_urom_coll.h b/src/components/cl/doca_urom/cl_doca_urom_coll.h index 3894adc5b9..fc33b5b027 100644 --- a/src/components/cl/doca_urom/cl_doca_urom_coll.h +++ b/src/components/cl/doca_urom/cl_doca_urom_coll.h @@ -1,5 +1,5 @@ /** - * Copyright (c) 2020-2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * Copyright (c) 2020-2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * * See file LICENSE for terms. */ @@ -18,7 +18,6 @@ extern const char typedef struct ucc_cl_doca_urom_schedule_t { ucc_schedule_pipelined_t super; struct ucc_result res; - ucc_mc_buffer_header_t *scratch; struct export_buf src_ebuf; struct export_buf dst_ebuf; } ucc_cl_doca_urom_schedule_t; @@ -37,4 +36,4 @@ static inline void ucc_cl_doca_urom_put_schedule(ucc_schedule_t *schedule) ucc_mpool_put(schedule); } -#endif \ No newline at end of file +#endif diff --git a/src/components/cl/doca_urom/cl_doca_urom_context.c b/src/components/cl/doca_urom/cl_doca_urom_context.c index d877ff9c91..bff3fee2a2 100644 --- a/src/components/cl/doca_urom/cl_doca_urom_context.c +++ b/src/components/cl/doca_urom/cl_doca_urom_context.c @@ -8,8 +8,6 @@ #include "cl_doca_urom_coll.h" #include "utils/ucc_malloc.h" -#include "components/tl/ucp/tl_ucp.h" - UCC_CLASS_INIT_FUNC(ucc_cl_doca_urom_context_t, const ucc_base_context_params_t *params, const ucc_base_config_t *config) diff --git a/src/components/cl/doca_urom/cl_doca_urom_lib.c b/src/components/cl/doca_urom/cl_doca_urom_lib.c index 5d2019a6d7..ef245a3239 100644 --- a/src/components/cl/doca_urom/cl_doca_urom_lib.c +++ b/src/components/cl/doca_urom/cl_doca_urom_lib.c @@ -1,5 +1,5 @@ /** - * Copyright (c) 2020-2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * Copyright (c) 2020-2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * * See file LICENSE for terms. */ @@ -42,7 +42,7 @@ static inline ucc_status_t check_tl_lib_attr(const ucc_base_lib_t *lib, attr->super.attr.thread_mode = ucc_min(attr->super.attr.thread_mode, tl_attr.super.attr.thread_mode); attr->super.attr.coll_types |= tl_attr.super.attr.coll_types; - attr->super.flags |= tl_attr.super.flags; + attr->super.flags |= tl_attr.super.flags; return UCC_OK; } @@ -50,12 +50,12 @@ static inline ucc_status_t check_tl_lib_attr(const ucc_base_lib_t *lib, ucc_status_t ucc_cl_doca_urom_get_lib_attr(const ucc_base_lib_t *lib, ucc_base_lib_attr_t *base_attr) { - ucc_tl_iface_t *tl_iface; - ucc_status_t status; - int i; ucc_cl_doca_urom_lib_t *cl_lib = ucc_derived_of(lib, ucc_cl_doca_urom_lib_t); ucc_cl_lib_attr_t *attr = ucc_derived_of(base_attr, ucc_cl_lib_attr_t); ucc_config_names_list_t *tls = &cl_lib->super.tls; + ucc_tl_iface_t *tl_iface; + ucc_status_t status; + int i; attr->tls = &cl_lib->super.tls.array; @@ -75,7 +75,7 @@ ucc_status_t ucc_cl_doca_urom_get_lib_attr(const ucc_base_lib_t *lib, ucc_assert(tls->array.count >= 1); for (i = 0; i < tls->array.count; i++) { - /* Check TLs provided in CL_BASIC_TLS. Not all of them could be + /* Check TLs provided in CL_DOCA_UROM_TLS. Not all of them could be available, check for NULL. */ tl_iface = ucc_derived_of(ucc_get_component(&ucc_global_config.tl_framework, diff --git a/src/components/cl/doca_urom/cl_doca_urom_team.c b/src/components/cl/doca_urom/cl_doca_urom_team.c index c43ff56a35..355794f97c 100644 --- a/src/components/cl/doca_urom/cl_doca_urom_team.c +++ b/src/components/cl/doca_urom/cl_doca_urom_team.c @@ -1,5 +1,5 @@ /** - * Copyright (c) 2020-2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * Copyright (c) 2020-2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * * See file LICENSE for terms. */ @@ -35,11 +35,11 @@ ucc_status_t ucc_cl_doca_urom_team_create_test(ucc_base_team_t *cl_team) ucc_status_t ucc_cl_doca_urom_team_get_scores(ucc_base_team_t *cl_team, ucc_coll_score_t **score) { - ucc_coll_score_team_info_t team_info; - ucc_status_t status; ucc_cl_doca_urom_team_t *team = ucc_derived_of(cl_team, ucc_cl_doca_urom_team_t); ucc_base_context_t *ctx = UCC_CL_TEAM_CTX(team); + ucc_coll_score_team_info_t team_info; + ucc_status_t status; status = ucc_coll_score_dup(team->score, score); if (UCC_OK != status) { diff --git a/src/components/cl/ucc_cl.c b/src/components/cl/ucc_cl.c index a87a9a1d18..a1dd0f518f 100644 --- a/src/components/cl/ucc_cl.c +++ b/src/components/cl/ucc_cl.c @@ -30,11 +30,11 @@ ucc_config_field_t ucc_cl_context_config_table[] = { }; const char *ucc_cl_names[] = { - [UCC_CL_BASIC] = "basic", - [UCC_CL_HIER] = "hier", + [UCC_CL_BASIC] = "basic", + [UCC_CL_HIER] = "hier", [UCC_CL_DOCA_UROM] = "doca_urom", - [UCC_CL_ALL] = "all", - [UCC_CL_LAST] = NULL + [UCC_CL_ALL] = "all", + [UCC_CL_LAST] = NULL }; UCC_CLASS_INIT_FUNC(ucc_cl_lib_t, ucc_cl_iface_t *cl_iface, diff --git a/src/components/tl/ucp/tl_ucp_context.c b/src/components/tl/ucp/tl_ucp_context.c index d35360e90d..5dac2c4fc9 100644 --- a/src/components/tl/ucp/tl_ucp_context.c +++ b/src/components/tl/ucp/tl_ucp_context.c @@ -166,9 +166,11 @@ UCC_CLASS_INIT_FUNC(ucc_tl_ucp_context_t, ucp_params.field_mask = UCP_PARAM_FIELD_FEATURES | UCP_PARAM_FIELD_TAG_SENDER_MASK | UCP_PARAM_FIELD_NAME; - ucp_params.features = UCP_FEATURE_TAG | UCP_FEATURE_AM | UCP_FEATURE_EXPORTED_MEMH | UCP_FEATURE_RMA; + ucp_params.features = UCP_FEATURE_TAG | UCP_FEATURE_AM; if (params->params.mask & UCC_CONTEXT_PARAM_FIELD_MEM_PARAMS) { - ucp_params.features |= UCP_FEATURE_RMA | UCP_FEATURE_AMO64; + ucp_params.features |= UCP_FEATURE_RMA | + UCP_FEATURE_AMO64 | + UCP_FEATURE_EXPORTED_MEMH; } ucp_params.tag_sender_mask = UCC_TL_UCP_TAG_SENDER_MASK; ucp_params.name = "UCC_UCP_CONTEXT";