Skip to content

UCT/IB: Use per-device MR flags in dmabuf capability probe - #11826

Closed
vipulSharma18 wants to merge 2 commits into
openucx:masterfrom
vipulSharma18:fix/efa-dmabuf-probe-access-flags
Closed

vipulSharma18 wants to merge 2 commits into
openucx:masterfrom
vipulSharma18:fix/efa-dmabuf-probe-access-flags

Conversation

@vipulSharma18

Copy link
Copy Markdown

Summary

Use per-device MR access flags in the UCX dmabuf capability probe so AWS EFA does not silently disable GPUDirect RDMA.

uct_ib_md_check_dmabuf() probes dmabuf support by registering with a deliberately bad fd and treating any errno other than EBADF as "unsupported". The probe hardcoded UCT_IB_MEM_ACCESS_FLAGS, which includes IBV_ACCESS_REMOTE_ATOMIC.

AWS EFA validates access flags before the fd and supports only LOCAL_WRITE|REMOTE_READ|REMOTE_WRITE, so efa_alloc_mr returns -EOPNOTSUPP and UCX never sets UCT_MD_FLAG_REG_DMABUF. GPUDirect RDMA stays disabled on EFA, forcing GPU→host→SRD→host→GPU copies instead of direct transfers.

UCX already computes the correct per-device flags in md->dev.mr_access_flags (set for EFA in ib_efa_md.c, used for real registrations in ib_md.c). Use those flags in the probe as well.

Impact

No-op on mlx5/verbs, where dev->mr_access_flags == UCT_IB_MEM_ACCESS_FLAGS. EFA is the transport where the flags differ, and it assigns them before uct_ib_md_open_common() calls the probe.

Context

We observed this on B300 clusters running tiered-disaggregation KV transfers over EFA: dmabuf registration was never enabled, collapsing throughput.

Test plan

  • Patch applies cleanly to master
  • Existing UCT/IB unit tests
  • On EFA: verify UCT_MD_FLAG_REG_DMABUF is set and GPUDirect RDMA is enabled

@svc-ucx

svc-ucx commented Aug 25, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Codestyle AUTHORS file update check) · commit 02b72ac5

TL;DR: The "AUTHORS file check" codestyle job failed because PR #11826's commit author (Vipul Sharma) is a new contributor who is not listed in the AUTHORS file; the fix is to run ./contrib/authors_update.sh locally and commit the resulting AUTHORS change as part of the PR.

Full analysis

Summary: Azure Pipelines stage "AUTHORS file check" failed with Bash exited with code '1' because git diff --exit-code detected that contrib/authors_update.sh had to add a new author entry.

Root cause: Not a product/code defect. The codestyle gate runs:

base_revision=[REDACTED:Hex High Entropy String]
head_revision=[REDACTED:Hex High Entropy String]
./contrib/authors_update.sh $base..$head
git diff --exit-code

The script printed ++ Vipul Sharma <vipulSharma18@users.noreply.github.com> — i.e. neither the commit email nor a matching name exists in AUTHORS (the file jumps from Valentin Petrov at line 127 straight to Wenbin Lu at line 128). It therefore modified AUTHORS, and the subsequent git diff --exit-code returned 1, failing the job. The check is purely deterministic on commit metadata, so it will keep failing until AUTHORS is updated in the branch. No other stage failed; the checkout, container teardown and job finalize all completed normally.

Implicated commit: [REDACTED:Hex High Entropy String] (Vipul Sharma, branch fix/efa-dmabuf-probe-access-flags, PR #11826) — the PR's own commit, not a regression from another change.

File: AUTHORS:127 (insertion point, after Valentin Petrov <valentinp@mellanox.com>); check logic in contrib/authors_update.sh:18-46

Suggested fix: In the PR branch, run the same command the CI runs and commit the result:

./contrib/authors_update.sh origin/master..HEAD
git add AUTHORS && git commit -m "AUTHORS: add Vipul Sharma"

This adds Vipul Sharma <vipulSharma18@users.noreply.github.com> to AUTHORS in LC_COLLATE=C sort order. Two notes for the contributor:

  • If a real (non-users.noreply.github.com) address is preferred, fix git config user.email and amend/rebase the commits before regenerating AUTHORS, otherwise the noreply address is what gets recorded and the check will re-fail on any email mismatch.
  • Keep the AUTHORS update in the same PR; the gate compares the merge base to the PR head, so a separately-merged AUTHORS change won't satisfy it until this branch is rebased.

No change to contrib/authors_update.sh or the pipeline is warranted — the gate behaved as designed.

Related: PR #11826 (this PR); the check script was added for this purpose in contrib/authors_update.sh. No existing issue tracks this failure mode.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 06426a21-0afd-4dcd-9030-223efb0059b4 in the triage console for the audit trail.

uct_ib_md_check_dmabuf() probes dmabuf support by registering with a
deliberately bad fd and treating any errno other than EBADF as
"unsupported". The probe hardcoded UCT_IB_MEM_ACCESS_FLAGS, which
includes IBV_ACCESS_REMOTE_ATOMIC.

AWS EFA validates access flags before the fd and supports only
LOCAL_WRITE|REMOTE_READ|REMOTE_WRITE, so efa_alloc_mr returns
-EOPNOTSUPP and UCX never sets UCT_MD_FLAG_REG_DMABUF. GPUDirect RDMA
stays disabled on EFA.

UCX already computes the correct per-device flags in
md->dev.mr_access_flags (set for EFA in ib_efa_md.c, used for real
registrations in ib_md.c). Use those flags in the probe as well.

No-op on mlx5/verbs, where dev->mr_access_flags equals
UCT_IB_MEM_ACCESS_FLAGS.

Signed-off-by: Zack Cao <silverwindcao@gmail.com>
Signed-off-by: Zack Cao <silverwindcao@gmail.com>
@vipulSharma18
vipulSharma18 force-pushed the fix/efa-dmabuf-probe-access-flags branch 3 times, most recently from 108f842 to 8d07744 Compare August 25, 2026 23:57
@iyastreb
iyastreb requested review from brminich and tvegas1 August 26, 2026 15:30
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

@svc-nvidia-pr-review

Copy link
Copy Markdown

The fix correctly replaces the hardcoded UCT_IB_MEM_ACCESS_FLAGS with the device-specific md->dev.mr_access_flags in the dmabuf capability probe. For EFA, mr_access_flags is a subset (no REMOTE_ATOMIC, conditionally no REMOTE_READ/WRITE), so probing with the full flags could make ibv_reg_dmabuf_mr() fail with EINVAL instead of the expected EBADF, causing uct_ib_md_check_dmabuf() to wrongly conclude dmabuf is unsupported. Verified that mr_access_flags is initialized before uct_ib_md_open_common runs in all three MD open paths, and md->pd is allocated earlier during MD alloc — no lifetime or ordering issue.

No test coverage is added. This is EFA-specific dmabuf detection behavior that is difficult to exercise in CI without the specific NIC. Worth noting as a coverage gap; the author should note in the PR description why a focused test is not practical for the EFA dmabuf path.

@tvegas1

tvegas1 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

have you or your organization signed the CLA https://openucx.org/license/?

@tvegas1 tvegas1 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.

I verified advertised dmabuf support on AWS and non-AWS node.

@vipulSharma18

Copy link
Copy Markdown
Author

Hi, thanks for the quick review and approval. Our legal team is in the process of signing the CLA.

@tvegas1

tvegas1 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

hi @vipulSharma18, gentle ping if you have signed CLA?

@tvegas1

tvegas1 commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

@vipulSharma18 would you kindly have update on CLA signing process?

@tvegas1

tvegas1 commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

fixed as #12032

@tvegas1 tvegas1 closed this Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants