Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ Swen Boehm <boehms@ornl.gov>
Thomas Vegas <tvegas@nvidia.com>
Tomer Gilad <tgilad@nvidia.com>
Tony Curtis <tonycurtis32@gmail.com>
Tristan Rice <rice@fn.lc>
Tushar Gohad <tushar.gohad@intel.com>
Tzafrir Cohen <tzafrirc@nvidia.com>
Valentin Petrov <valentinp@mellanox.com>
Expand Down
6 changes: 3 additions & 3 deletions src/uct/cuda/cuda_copy/cuda_copy_md.c
Original file line number Diff line number Diff line change
Expand Up @@ -610,9 +610,9 @@ static void uct_cuda_copy_md_sync_memops_get_address_range(
ucs_assert(md->config.alloc_whole_reg == UCS_CONFIG_ON);
}

/* For multi-handle VMM, the physical allocation may be smaller than the
* mapped virtual range; preserve the caller's requested extent. */
if (is_vmm && (alloc_length < length)) {
/* For multi-handle VMM, the physical allocation containing the address
Comment thread
d4l3k marked this conversation as resolved.
Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

minor: the zero-base case is unrelated to multi-handle VMM (and not VMM-specific), pls split it out so each condition carries its own comment:

Suggested change
/* For multi-handle VMM, the physical allocation containing the address
if (base_address == 0) {
/* Host-located VMM reports a zero base address */
goto out_ctx_pop;
}
/* For multi-handle VMM, the physical allocation containing the address
* may not cover the whole requested range */
if (is_vmm && ((base_address + alloc_length) < (address + length))) {
goto out_ctx_pop;
}

* may not cover the whole requested range; keep the caller's extent. */
if (is_vmm && ((base_address + alloc_length) < (address + length))) {
Comment thread
d4l3k marked this conversation as resolved.
Resolved
Comment thread
d4l3k marked this conversation as resolved.
Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why keep is_vmm here? The new check is just "the reported range must cover the requested range", which also holds for legacy allocations.

goto out_ctx_pop;
}

Expand Down
28 changes: 28 additions & 0 deletions test/gtest/uct/cuda/test_cuda_ipc_md.cc
Original file line number Diff line number Diff line change
Expand Up @@ -807,4 +807,32 @@ UCS_TEST_P(test_cuda_copy_md, vmm_locality_domain_mem_type) {
#endif
}

UCS_TEST_P(test_cuda_copy_md, vmm_multi_handle_unaligned_range,
"CUDA_COPY_REG_WHOLE_ALLOC=on") {
cuda_vmm_mem_buffer buffer(1, UCS_MEMORY_TYPE_CUDA, 2);
/* Starts in the middle of the first handle and ends in the second one,
* while being shorter than a single handle */
size_t chunk_size = buffer.size() / 2;
void *address = UCS_PTR_BYTE_OFFSET(buffer.ptr(), chunk_size / 2);
size_t length = (chunk_size / 2) + 1;
CUdeviceptr alloc_base;
size_t alloc_length;
uct_md_mem_attr_t mem_attr;

ASSERT_EQ(CUDA_SUCCESS, cuMemGetAddressRange(&alloc_base, &alloc_length,
Comment thread
d4l3k marked this conversation as resolved.
Resolved
(CUdeviceptr)address));
if ((alloc_base + alloc_length) >= ((CUdeviceptr)address + length)) {
UCS_TEST_SKIP_R("allocation range covers the requested range");
}

mem_attr.field_mask = UCT_MD_MEM_ATTR_FIELD_BASE_ADDRESS |
UCT_MD_MEM_ATTR_FIELD_ALLOC_LENGTH;
ASSERT_UCS_OK(uct_md_mem_query(md(), address, length, &mem_attr));

/* The containing allocation is too short, so the requested extent must be
* kept */
EXPECT_EQ(address, mem_attr.base_address);
EXPECT_EQ(length, mem_attr.alloc_length);
}

_UCT_MD_INSTANTIATE_TEST_CASE(test_cuda_copy_md, cuda_cpy);
29 changes: 20 additions & 9 deletions test/gtest/uct/cuda/test_switch_cuda_device.cc
Original file line number Diff line number Diff line change
Expand Up @@ -647,13 +647,8 @@ UCS_TEST_P(test_mem_alloc_device, no_current_context_vmm_mem_registrable,
}

#if CUDA_VERSION >= 12020
/* Host-located VMM is not dmabuf-exportable but is registrable by IB.
*
* TODO: REG_WHOLE_ALLOC=off is needed because of an existing issue with
* base_address being set to 0, causing cache pollution with this type of memory
*/
UCS_TEST_P(test_mem_alloc_device, host_vmm_mem_registrable,
"CUDA_COPY_REG_WHOLE_ALLOC=off")
/* Host-located VMM is not dmabuf-exportable but is registrable by IB */
UCS_TEST_P(test_mem_alloc_device, host_vmm_mem_registrable)
{
constexpr size_t size = 4 * UCS_MBYTE;
cuda_host_vmm_mem_buffer buffer(size, UCS_MEMORY_TYPE_CUDA);
Expand All @@ -663,14 +658,30 @@ UCS_TEST_P(test_mem_alloc_device, host_vmm_mem_registrable,

/* Host-located VMM is typed as CUDA memory, but cannot be allocated as
* GPUDirect RDMA capable */
UCS_TEST_P(test_mem_alloc_device, host_vmm_mem_not_gdr_capable,
"CUDA_COPY_REG_WHOLE_ALLOC=off")
UCS_TEST_P(test_mem_alloc_device, host_vmm_mem_not_gdr_capable)
Comment thread
d4l3k marked this conversation as resolved.
Resolved
{
constexpr size_t size = 4 * UCS_MBYTE;
cuda_host_vmm_mem_buffer buffer(size, UCS_MEMORY_TYPE_CUDA);

query_registrable_gdr_capable(buffer.ptr(), size, 1, 0);
}

/* cuMemGetAddressRange() reports a zero base address for host-located VMM, so
* the registration range must not be extended to it */
UCS_TEST_P(test_mem_alloc_device, host_vmm_mem_range,
Comment thread
tvegas1 marked this conversation as resolved.
Resolved
"CUDA_COPY_REG_WHOLE_ALLOC=on")
{
constexpr size_t size = 4 * UCS_MBYTE;
cuda_host_vmm_mem_buffer buffer(size, UCS_MEMORY_TYPE_CUDA);
uct_md_mem_attr_t mem_attr;

mem_attr.field_mask = UCT_MD_MEM_ATTR_FIELD_BASE_ADDRESS |
UCT_MD_MEM_ATTR_FIELD_ALLOC_LENGTH;
ASSERT_UCS_OK(uct_md_mem_query(md(), buffer.ptr(), buffer.size(),
&mem_attr));
EXPECT_EQ(buffer.ptr(), mem_attr.base_address);
EXPECT_EQ(buffer.size(), mem_attr.alloc_length);
}
#endif

UCS_TEST_P(test_mem_alloc_device, legacy_cuda_no_memtype_copy_inter_node)
Expand Down