CL/DOCA_UROM: Review feedback

nsarka · Jun 4, 2024 · 5d0397e · 5d0397e
1 parent 5719b37
commit 5d0397e
Show file tree

Hide file tree

Showing 13 changed files with 57 additions and 65 deletions.
diff --git a/config/m4/doca_urom.m4 b/config/m4/doca_urom.m4
@@ -11,16 +11,15 @@ 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"],
         [
             AS_IF([test ! -d $with_doca_urom],
                   [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])
-])])
+])])
diff --git 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])
-])])
+])])
diff --git 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
@@ -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
+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
@@ -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
@@ -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 <doca_dev.h>
 #include <doca_urom.h>
 #include <doca_pe.h>
@@ -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,33 +61,30 @@ 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;
 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 *,
                   const ucc_base_config_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
@@ -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 <doca_urom.h>
 #include <urom_ucc.h>
@@ -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
@@ -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
+#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
@@ -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
@@ -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,20 +42,20 @@ 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;
 }
 
 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
@@ -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
@@ -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
@@ -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";