Skip to content

rdma: simplify MR handle to two owned arrays, eliminate mr_ctrl_owned - #1252

Open
ryanhankins wants to merge 1 commit into
aws:masterfrom
ryanhankins:separate_ep
Open

rdma: simplify MR handle to two owned arrays, eliminate mr_ctrl_owned#1252
ryanhankins wants to merge 1 commit into
aws:masterfrom
ryanhankins:separate_ep

Conversation

@ryanhankins

@ryanhankins ryanhankins commented May 18, 2026

Copy link
Copy Markdown
Contributor

Remove the mr_ctrl_owned[] intermediate and the raw-pointer view mr_ctrl[]. Promote mr_ctrl[] to a fully-owned std::array<ofi_mr_ptr> mirroring mr_data[].

Registration (reg_mr_on_device):

  • In FI_MR_ENDPOINT mode, store ctrl-rail fid_mr objects directly in mr_ctrl[] rather than in mr_ctrl_owned[] then copying the raw pointer.
  • In non-endpoint-MR mode, set nctrl = 0 (no ctrl-ep binding needed); the old alias loop that pointed mr_ctrl[i] at mr_data[i] is removed.

IO path: update the four call sites that consumed mr_ctrl[] as a raw fid_mr* pointer to call .get() on the owning element instead.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ryanhankins
ryanhankins requested a review from a team as a code owner May 18, 2026 22:28
@ryanhankins
ryanhankins force-pushed the separate_ep branch 3 times, most recently from 8283293 to ef4521a Compare May 21, 2026 13:25

@bwbarrett bwbarrett left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, although you forgot the signed-off-by line in your commit message.

… view

Remove the mr_ctrl_owned[] intermediate and the raw-pointer view
mr_ctrl[].  Promote mr_ctrl[] to a fully-owned std::array<ofi_mr_ptr>
mirroring mr_data[].

Registration (reg_mr_on_device):
- In FI_MR_ENDPOINT mode, store ctrl-rail fid_mr objects directly in
  mr_ctrl[] rather than in mr_ctrl_owned[] then copying the raw pointer.
- In non-endpoint-MR mode, set nctrl = 0 (no ctrl-ep binding needed);
  the old alias loop that pointed mr_ctrl[i] at mr_data[i] is removed.

IO path: update the four call sites that consumed mr_ctrl[] as a raw
fid_mr* pointer to call .get() on the owning element instead.

Signed-off-by: Ryan Hankins <ryan.hankins@hpe.com>
@ryanhankins

Copy link
Copy Markdown
Contributor Author

Fixed, thank you.

@Zhenye-Na

Copy link
Copy Markdown
Member

bot:aws:retest

Comment thread src/nccl_ofi_rdma.cpp
Comment on lines +2696 to +2697
/* Allocate rdma memory registration handle */
uint16_t nctrl = ep ? ep->num_control_rails : num_rails;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line does not seem to match what the commit message says.

In non-endpoint-MR mode, set nctrl = 0 (no ctrl-ep binding needed);

Here in the non-endpoint-mr case you set nctrl = num_rails, which means you will make duplicate registrations for the control rails, even though they aren't needed for non-endpoint-mr providers.

@asbrodsky-amzn

Copy link
Copy Markdown

Any updates on this PR?

@ryanhankins

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Looking back at this, I'm not sure what the cleanest resolution is.

The goal of this PR was to eliminate the mr_ctrl_owned[] + mr_ctrl[] dual representation and reduce the MR handle to two owned arrays (mr_data[] and mr_ctrl[]).

However, in the non-FI_MR_ENDPOINT case the previous implementation relied on mr_ctrl[] aliasing mr_data[]. Once mr_ctrl[] becomes an owning array, avoiding duplicate registrations is no longer obvious to me.

At this point I see two options:

Keep separate ownership for mr_ctrl[], which implies additional registrations in the non-endpoint-MR path.
Preserve the aliasing behavior, which effectively requires retaining some form of separate view/access layer.

Is there a preferred direction here? I'm happy to update the PR, but I'm not currently seeing a clean way to both eliminate the owned+view split and avoid the duplicate registrations you pointed out. If this cleanup is no longer considered worthwhile, I'm also happy to close the PR.

@rauteric

rauteric commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Sorry, I lost track of this PR.

Coming back to this, I think the existing behavior is better. Adopting the change in this PR basically doubles the number of memory registrations we do for the EFA provider, and other providers that don't require MR_ENDPOINT, and we may hold these duplications registrations for the entire duration of the run. Neither the MR cache built into the plugin, nor Libfabric's MR cache will prevent this duplication.

So, I suggest we close this PR and keep the existing behavior. @bwbarrett do you agree?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants