UCT/IB/MD: retry memory registration with reduced access flags on failure #10341

amastbaum · 2024-12-01T15:49:40Z

What?

Enable memory registration for read-only addresses.

Why?

A customer reported an issue when attempting to send read-only memory using RDMA. Details can be found in the related discussion: https://github.com/openucx/ucx/issues/10085#issuecomment-2377368540.

…lure

gleon99 · 2024-12-02T16:36:59Z

src/ucp/rndv/proto_rndv.inl

@@ -49,7 +49,8 @@ ucp_proto_rndv_rts_request_init(ucp_request_t *req)
    status = ucp_datatype_iter_mem_reg(ep->worker->context,
                                       &req->send.state.dt_iter,
                                       rpriv->md_map,
-                                       UCT_MD_MEM_ACCESS_RMA |
+                                       UCT_MD_MEM_ACCESS_REMOTE_GET |
+                                       UCT_MD_MEM_ACCESS_LOCAL_READ |


It's just the most basic option so I thought I can keep mention it as well, but on the other hand it doesn't matter as local read is enabled by default in ibv_reg_mr (there is no such flag in verbs.h because it's always enabled)

gleon99 · 2024-12-02T16:41:54Z

src/uct/ib/mlx5/dv/ib_mlx5dv_md.c

+                                                     md->super.relaxed_order,
+                                                     1, flags) &
+                            access_mask;
+    int attempt_cnt       = 0;


Avoid abbreviations (cnt)

gleon99 · 2024-12-02T16:43:48Z

src/uct/ib/base/ib_md.c

@@ -626,9 +627,39 @@ ucs_status_t uct_ib_memh_alloc(uct_ib_md_t *md, size_t length,
    return UCS_OK;
 }

-uint64_t uct_ib_memh_access_flags(uct_ib_mem_t *memh, int relaxed_order)
+uint64_t uct_flags_to_ibv_mem_access_flags(uint64_t uct_flags)


uct_ib_..

gleon99 · 2024-12-02T16:45:18Z

src/uct/ib/base/ib_md.c

@@ -460,7 +460,7 @@ uct_ib_md_handle_mr_list_mt(uct_ib_md_t *md, void *address, size_t length,
 ucs_status_t uct_ib_reg_mr(uct_ib_md_t *md, void *address, size_t length,
                           const uct_md_mem_reg_params_t *params,
                           uint64_t access_flags, struct ibv_dm *dm,
-                           struct ibv_mr **mr_p)
+                           struct ibv_mr **mr_p, int first_attempt)


first_attempt -> ignore_errors or smth similar.
This func doesn't care about "attempts" and their number, the meaning of this flag here is only to "force" ignore errors.

gleon99 · 2024-12-02T16:56:54Z

src/uct/ib/base/ib_md.c

+}
+
+uint64_t uct_ib_memh_access_flags(uct_ib_mem_t *memh, int relaxed_order,
+                                  int first_attempt, uint64_t uct_flags)


Here as well, I'd rename first_attempt to its real meaning in this context.
Maybe smth like use_uct_flags?

gleon99 · 2024-12-02T16:58:25Z

src/uct/ib/base/ib_md.c

+        access_flags = uct_ib_memh_access_flags(&memh->super, md->relaxed_order,
+                                                attempt_cnt == 0, uct_flags);
+
+        status = uct_ib_reg_mr(md, address, length, params, access_flags, NULL,


gleon99 · 2024-12-02T16:59:17Z

src/uct/ib/mlx5/dv/ib_mlx5dv_md.c

+                               NULL, &memh->mrs[mr_type].super.ib,
+                               attempt_cnt == 0);
+        attempt_cnt++;
+        access_flags = uct_ib_memh_access_flags(&memh->super,


First reg_mr, then access_flags - is that intentional?
This line always has attempt_cnt >= 1...

Yes, it's intentional, though it might be written better..
access_flags is first calculated at the top of the function because it's used for another function, and only then it is used in the do while loop

yosefe · 2024-12-06T00:27:28Z

src/uct/ib/base/ib_md.c

@@ -626,9 +627,35 @@ ucs_status_t uct_ib_memh_alloc(uct_ib_md_t *md, size_t length,
    return UCS_OK;
 }

-uint64_t uct_ib_memh_access_flags(uct_ib_mem_t *memh, int relaxed_order)
+uint64_t uct_ib_flags_to_ibv_mem_access_flags(uint64_t uct_flags)


yosefe · 2024-12-06T00:29:29Z

src/uct/ib/base/ib_md.c

@@ -309,7 +309,7 @@ void *uct_ib_md_mem_handle_thread_func(void *arg)
    while (ctx->length > 0) {
        if (ctx->params != NULL) {
            status = uct_ib_reg_mr(ctx->md, ctx->address, length, ctx->params,
-                                   ctx->access_flags, NULL, &ctx->mrs[mr_idx]);
+                                   ctx->access_flags, NULL, &ctx->mrs[mr_idx], 1);


why hide_errors is true here?

@yosefe Oh, I actually wanted to put here a value that doesn't change the previous behavior and put 1 by mistake. I'll also change it in the other places.

yosefe · 2024-12-06T00:33:43Z

src/uct/ib/base/ib_md.c

+    memh = ucs_derived_of(ib_memh, uct_ib_verbs_mem_t);
+
+    do {
+        access_flags = uct_ib_memh_access_flags(&memh->super, md->relaxed_order,
+                                                attempt_count > 0, uct_flags);
+
+        status       = uct_ib_reg_mr(md, address, length, params, access_flags,
+                                     NULL, &mr_default, attempt_count == 0);
+        attempt_count++;


This retry should happen in UCP layer, otherwise IMO it can cause issues with UCP rcache region merges

@yosefe Where are the rcache region merges triggered from?
Do you think we should still try to register with full permissions and only then with specific ones?

Artemy-Mellanox · 2025-01-02T13:24:37Z

src/ucp/core/ucp_mm.c

@@ -462,7 +462,7 @@ static void ucp_memh_cleanup(ucp_context_h context, ucp_mem_h memh)
 }

 static ucs_status_t ucp_memh_register_gva(ucp_context_h context, ucp_mem_h memh,
-                                          ucp_md_map_t md_map)
+                                          ucp_md_map_t md_map, unsigned uct_flags)


I understand it's temp, but, just in case, it would be good to replace gva_mr with some structure and keep registrations for different access flags there.

… test_ucp_memheap

… capability is not set

UCT/IB/MD: retry memory registration with reduced access flags on fai…

e47f0ff

…lure

amastbaum added the WIP-DNM Work in progress / Do not review label Dec 1, 2024

amastbaum added 2 commits December 2, 2024 11:18

UCT/IB/MD: add myself to AUTHORS file

0ed3576

UCT/IB/MD: added some changes

5b5a4ba

amastbaum added Ready for Review and removed WIP-DNM Work in progress / Do not review labels Dec 2, 2024

amastbaum requested a review from gleon99 December 2, 2024 11:23

amastbaum marked this pull request as ready for review December 2, 2024 11:24

UCT/IB/MD: set mem reg flags to read-only in proto rndv rts

ebd6386

gleon99 reviewed Dec 2, 2024

View reviewed changes

amastbaum added 2 commits December 3, 2024 12:36

UCT/IB/MD: CR fixes

e2564b6

UCT/IB/MD: CR fixes 2

bcf4d11

amastbaum requested a review from gleon99 December 3, 2024 14:02

yosefe reviewed Dec 6, 2024

View reviewed changes

amastbaum added 3 commits December 10, 2024 18:13

UCT/IB/MD: add ucp_mem_map params->prot to uct_flags translation

7f0ad47

Merge branch 'master' into support_mem_reg_of_a_read_only_address

0be06fc

UCT/IB/MD: CR fixes

547432e

amastbaum requested a review from Artemy-Mellanox December 11, 2024 15:47

yosefe mentioned this pull request Dec 15, 2024

Address Registration Error in CUDA Aware MPICH 4.2.2 + UCX 1.17.0 Application #10085

Open

amastbaum added 4 commits December 16, 2024 15:42

UCT/IB/MD: use only ucp defined flags in ib mem reg

f45911c

UCT/IB/MD: revert changes in UCP

fdaa23a

Merge branch 'master' into support_mem_reg_of_a_read_only_address

a25d39b

UCT/IB/MD: pass uct_flags to gva mem_reg

251cd9a

amastbaum force-pushed the support_mem_reg_of_a_read_only_address branch from 364f3cf to 251cd9a Compare January 2, 2025 13:05

Artemy-Mellanox reviewed Jan 2, 2025

View reviewed changes

amastbaum added 5 commits January 6, 2025 15:30

UCT/IB/MD: set uct_flags to ALL when calling ucp_datatype_iter_mem_reg

2dc3f68

UCT/IB/MD: change mem_reg permissions in rndv_rtr to ALL

26e991e

UCT/IB/MD: change libperf memory registration permissions to ALL

bc3cf3c

UCT/IB/MD: add ucp_mem_map params->prot to uct_flags translation

d2b1b60

UCT/IB/MD: add full permissions to ucp_test mem_map

8aa1b4e

amastbaum added 5 commits January 9, 2025 18:21

UCT/IB/MD: decoupled atomic permission flags assignment from rma flags

0ad3eb4

UCT/IB/MD: added memory protection flags to map_buffer test function

22ec2c2

UCT/IB/MD: specified memory protection flags in test_ucp_mem_type and…

256bd62

… test_ucp_memheap

Merge branch 'master' into support_mem_reg_of_a_read_only_address

c6f8cbe

UCT/IB/MD: don't fail in dereg_invalidate_rkey_check if the requested…

e3aa7ed

… capability is not set

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCT/IB/MD: retry memory registration with reduced access flags on failure #10341

UCT/IB/MD: retry memory registration with reduced access flags on failure #10341

amastbaum commented Dec 1, 2024 •

edited

Loading

gleon99 Dec 2, 2024

amastbaum Dec 3, 2024

gleon99 Dec 2, 2024

gleon99 Dec 2, 2024

gleon99 Dec 2, 2024

gleon99 Dec 2, 2024

gleon99 Dec 2, 2024

gleon99 Dec 2, 2024

amastbaum Dec 3, 2024

yosefe Dec 6, 2024

yosefe Dec 6, 2024

amastbaum Dec 9, 2024

yosefe Dec 6, 2024

amastbaum Dec 9, 2024

Artemy-Mellanox Jan 2, 2025

UCT/IB/MD: retry memory registration with reduced access flags on failure #10341

Are you sure you want to change the base?

UCT/IB/MD: retry memory registration with reduced access flags on failure #10341

Conversation

amastbaum commented Dec 1, 2024 • edited Loading

What?

Why?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amastbaum commented Dec 1, 2024 •

edited

Loading