Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add DMA-BUF support #618

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

feat: add DMA-BUF support #618

wants to merge 5 commits into from
+655 −453

Conversation

aws-nslick
Copy link
Contributor

@aws-nslick aws-nslick commented Sep 21, 2024 •
edited
Loading 

feat: add DMA-BUF support

This adds DMA-BUF support to the plugin and enables it under the
following conditions:

At build time, libfabric>=1.20 is required (build checks for FI_MR_DMABUF).

At runtime:
 + The specific version of NCCL being used supports DMA-BUF and passes
   valid dmabuf fds to the plugin.
 + FI_HMEM must be supported.
  + For CUDA accelerators, CU_DEVICE_ATTRIBUTE_DMA_BUF_SUPPORTED is
   queried and FI_HMEM_CUDA is requested.
  + For Neuron, we assume all nrt versions are viable of dmabuf
    export.

Libfabric as of today provides no hints in at init time that allow the
plugin to differentiate between a provider that merely has FI_HMEM
support, and one that has dmabuf support. In the case that the plugin is
built against libfabric>=1.20 but libfabric is unable to handle dmabuf
registrations, a new environment variable (OFI_NCCL_DMABUF_DISABLE=1) is
introduced to force the legacy path. When set, dmabuf support is not
advertised to nccl and this ensures that the plugin remains in the
legacy iovec path.

Testing: Various combinations of
  + OFI_NCCL_DISABLE_DMABUF=0/1
  + OFI_NCCL_PROTOCOL=RDMA/SENDRECV
  + FI_HMEM_CUDA_USE_GDRCOPY=0/1

Signed-off-by: Nicholas Sielicki <nslick@amazon.com>

Note that currently this requires a patch for libfabric or explicitly disabling gdrcopy: aws-nslick/libfabric@a475d9140

prov/efa: entirely avoid gdrcopy on FI_MR_DMABUF
efa_mr_hmem_setup previously always called ofi_hmem_dev_register on
FI_HMEM_CUDA arguments when gdrcopy was enabled, regardless of whether
FI_MR_DMABUF was also present and gdrcopy was not to be involved. Apps
who wish to conditionally use fi_regattr with dmabuf descriptors by
passing FI_MR_DMABUF, but who reserve the right to also provide iovec
args that require pinning (provided in libfabric by libgdrcopy), find
themselves in a position of needing to explicitly disable gdrcopy to
support DMABUF prior to this patch.

When FI_MR_DMABUF flags are passed here, just exit early.

Signed-off-by: Nicholas Sielicki <nslick@amazon.com>

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

@aws-nslick
feat(cuda): use cudart_static
d0040f9 
check at build time for at least cuda 11.7, remove runtime checks
revalidating this. Prefer cudart functions for things like checking
versions for simplicity. Drop cuda_check functional test as its
intention was to ensure that CUDA was not linked, but CUDA is now
linked.

This also needed to fix nvtx's m4 autodetection, and our github actions
needed to use the actual cuda repos (previously were using ancient 11.x
toolkits from ubuntu universe repos).

Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
@aws-nslick
refactor(api): delete previous DMA-BUF stub support
ab8fd91 
Along the interface with NCCL, continue to return -ENOTSUP, but for the
internal api remove dmabuf fnptrs within the communicators and delete
the impls they pointed at.

Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
@aws-nslick
refactor(mr): add tagged union for mr registration
448b311 
The MR cache needs to be capable of handling triplets of {base, offset,
len} in addition to the current arguments of {base, len}. Add a tagged
union with a functional interface that can represent this generically.
Tagged union members are struct iovec and struct fi_mr_dmabuf, which
matches the union within struct fi_mr_attr;

Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
@aws-nslick
refactor(api): replace addr+len pairs with MRs
a0c1dfe 
Immediately on any top-level NCCL call, construct an immutable
nccl_ofi_mr_input_t on the stack, then pass that to communicator regmr
implementations. Add a flags argument to internal regmr functions such
that the input can be inspected and may add FI_MR_DMABUF if the input
arguments correspond to a file descriptor.

Implement top-level nccl_net_ofi_regMr in terms of
nccl_net_ofi_regMrDmaBuf, simply forwarding arguments alongside an
invalid file descriptor (-1) and a zero offset.

DMA-BUF remains unsupported as of this commit, but only due to not
advertising support back to NCCL/nccom.

Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
@aws-nslick aws-nslick marked this pull request as ready for review September 22, 2024 00:36
@aws-nslick aws-nslick mentioned this pull request Sep 22, 2024
Closed
@aws-nslick
feat: add DMA-BUF support
2200b81 
This adds DMA-BUF support to the plugin and enables it under the
following conditions:

At build time, libfabric>=1.20 is required (build checks for FI_MR_DMABUF).

At runtime:
 + The specific version of NCCL being used supports DMA-BUF and passes
   valid dmabuf fds to the plugin.
 + FI_HMEM must be supported.
  + For CUDA accelerators, CU_DEVICE_ATTRIBUTE_DMA_BUF_SUPPORTED is
   queried and FI_HMEM_CUDA is requested.
  + For Neuron, we assume all nrt versions are viable of dmabuf
    export.

Libfabric as of today provides no hints in at init time that allow the
plugin to differentiate between a provider that merely has FI_HMEM
support, and one that has dmabuf support. In the case that the plugin is
built against libfabric>=1.20 but libfabric is unable to handle dmabuf
registrations, a new environment variable (OFI_NCCL_DMABUF_DISABLE=1) is
introduced to force the legacy path. When set, dmabuf support is not
advertised to nccl and this ensures that the plugin remains in the
legacy iovec path.

Testing: Various combinations of
  + OFI_NCCL_DISABLE_DMABUF=0/1
  + OFI_NCCL_PROTOCOL=RDMA/SENDRECV
  + FI_HMEM_CUDA_USE_GDRCOPY=0/1

Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rajachan rajachan Awaiting requested review from rajachan rajachan is a code owner

@bwbarrett bwbarrett Awaiting requested review from bwbarrett bwbarrett is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@aws-nslick