UCT/IB/MLX5: DV UAR alloc type NC support by Artemy-Mellanox · Pull Request #6082 · openucx/ucx

Artemy-Mellanox · 2020-12-31T15:30:10Z

No description provided.

yosefe · 2021-01-01T09:59:51Z

+    unsigned flags      = UCT_IB_MLX5_UAR_ALLOC_TYPE_WC;
+
+retry:
+    uar->uar            = mlx5dv_devx_alloc_uar(md->super.dev.ibv_context,
+                                                flags);
    if (uar->uar == NULL) {
+        if (flags != UCT_IB_MLX5_UAR_ALLOC_TYPE_NC) {
+            flags = UCT_IB_MLX5_UAR_ALLOC_TYPE_NC;
+            goto retry;
+        }


IMO, calling mlx5dv_devx_alloc_uar twice(instead of goto/retry) would be more readable

don't we want to cache the failure?

yosefe · 2021-01-03T09:42:46Z

 {
 #if HAVE_DEVX
-    uar->uar            = mlx5dv_devx_alloc_uar(md->super.dev.ibv_context, 0);
+    uar->uar            = mlx5dv_devx_alloc_uar(md->super.dev.ibv_context,


pls remove indentation where not needed

yosefe · 2021-01-03T09:43:51Z

-        ucs_error("mlx5dv_devx_alloc_uar() failed: %m");
-        return UCS_ERR_NO_MEMORY;
+        if (UCT_IB_MLX5_UAR_ALLOC_TYPE_WC == UCT_IB_MLX5_UAR_ALLOC_TYPE_NC) {
+            ucs_error("mlx5dv_devx_alloc_uar(WC) failed: %m");


print a debug message if WC failed and we're doing fallback

print actual flag value, ucs_error("mlx5dv_devx_alloc_uar(WC, flags=0x%x) failed: %m", UCT_IB_MLX5_UAR_ALLOC_TYPE_WC);

yosefe · 2021-01-03T09:45:38Z

+        uar->uar        = mlx5dv_devx_alloc_uar(md->super.dev.ibv_context,
+                                                UCT_IB_MLX5_UAR_ALLOC_TYPE_NC);
+        if (uar->uar == NULL) {
+            ucs_error("mlx5dv_devx_alloc_uar(NC) failed: %m");
+            return UCS_ERR_NO_MEMORY;
+        }


IMO, #define UCT_IB_MLX5_UAR_ALLOC_TYPE_NC 0 is not correct (but only #define UCT_IB_MLX5_UAR_ALLOC_TYPE_WC 0 is correct)
maybe add #ifdef HAVE_DECL_MLX5DV_UAR_ALLOC_TYPE_NC here?
and check for HAVE_DECL_MLX5DV_UAR_ALLOC_TYPE_WC separately in configure?

yosefe · 2021-01-03T12:41:07Z

-        uar->uar        = mlx5dv_devx_alloc_uar(md->super.dev.ibv_context,
-                                                UCT_IB_MLX5_UAR_ALLOC_TYPE_NC);
+#if HAVE_DECL_MLX5DV_UAR_ALLOC_TYPE_NC
+        ucs_debug("UAR alloc WC failed. Fallback to NC\n");


ucs_debug("mlx5dv_devx_alloc_uar(device=%s, flags=0x%x) failed: %m, fallback to NC\n");

yosefe · 2021-01-03T12:41:33Z

+                                         UCT_IB_MLX5_UAR_ALLOC_TYPE_NC);
        if (uar->uar == NULL) {
-            ucs_error("mlx5dv_devx_alloc_uar(NC) failed: %m");
+            ucs_error("mlx5dv_devx_alloc_uar(NC, flags=0x%x) failed: %m",


yosefe · 2021-01-03T12:42:45Z

also, revert #6061 in this PR

yosefe · 2021-01-03T14:11:43Z

+        ucs_debug("mlx5dv_devx_alloc_uar(device=%s, flags=0x%x(WC)) "
+                  "failed: %m, fallback to NC",
+                  uct_ib_device_name(&md->super.dev),
+                  UCT_IB_MLX5_UAR_ALLOC_TYPE_WC);


maybe add function uct_ib_mlx5_devx_alloc_uar(md, flags, err_level=ERROR/DEBUG, char *title="WC"/"NC") which will wrap the debug/error print?

yosefe

pls squash

yosefe · 2021-01-03T19:13:06Z

+        sprintf(buf, "mlx5dv_devx_alloc_uar(device=%s, flags=0x%x(%s)) "
+                "failed: %m", uct_ib_device_name(&md->super.dev), flags, title);
+        if (fallback == NULL) {
+            ucs_log(log_level, "%s", buf);
+        } else {
+            ucs_log(log_level, "%s, fallback to %s", buf, fallback);
+        }
+


what was wrong with the previous version?

yosefe · 2021-01-04T11:40:15Z

@Artemy-Mellanox can u pls squash?

Artemy-Mellanox force-pushed the topic/dv_uar_nc branch from e494da0 to a07e487 Compare December 31, 2020 19:47

yosefe reviewed Jan 1, 2021

View reviewed changes

yosefe reviewed Jan 3, 2021

View reviewed changes

Artemy-Mellanox force-pushed the topic/dv_uar_nc branch from cbeb7b5 to c7ad67e Compare January 3, 2021 14:13

yosefe approved these changes Jan 3, 2021

View reviewed changes

Artemy-Mellanox force-pushed the topic/dv_uar_nc branch from 617b186 to ef338d4 Compare January 3, 2021 15:28

yosefe reviewed Jan 3, 2021

View reviewed changes

yosefe approved these changes Jan 4, 2021

View reviewed changes

UCT/IB/MLX5: DV UAR alloc type NC support

31c9220

Artemy-Mellanox force-pushed the topic/dv_uar_nc branch from 985a3c4 to 31c9220 Compare January 4, 2021 11:45

yosefe merged commit 4e4f44f into openucx:master Jan 5, 2021

yosefe mentioned this pull request Jan 28, 2021

mlx5dv_devx_alloc_uar "Operation not supported" and ucp_ep_create failed "Out of memory" #6223

Closed

Artemy-Mellanox deleted the topic/dv_uar_nc branch March 28, 2022 10:05

Conversation

Artemy-Mellanox commented Dec 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yosefe commented Jan 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yosefe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yosefe commented Jan 4, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yosefe commented Jan 3, 2021 •

edited

Loading