Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 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
8 changes: 5 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,11 @@ 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, and host-located VMM reports a
* zero base address; keep the caller's extent in both cases. */
if (is_vmm && ((base_address == 0) ||
Comment thread
d4l3k marked this conversation as resolved.
Resolved
((base_address + alloc_length) < (address + length)))) {
goto out_ctx_pop;
}

Expand Down
29 changes: 29 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,33 @@ 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;

/* If the query fails, the MD keeps the requested extent as well */
if ((cuMemGetAddressRange(&alloc_base, &alloc_length,
(CUdeviceptr)address) == CUDA_SUCCESS) &&
((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);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let's adapt the test to cover both cases and remove skipping the the rest in case of allocation range covers the requested range.
and also it'd make code free from the duplicated condition which we already have in the code for checking ranges.

Suggested change
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;
/* If the query fails, the MD keeps the requested extent as well */
if ((cuMemGetAddressRange(&alloc_base, &alloc_length,
(CUdeviceptr)address) == CUDA_SUCCESS) &&
((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);
}
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);
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 = 0;
size_t alloc_length = 0;
uct_md_mem_attr_t mem_attr;
bool covers_range =
(cuMemGetAddressRange(&alloc_base, &alloc_length,
(CUdeviceptr)address) == CUDA_SUCCESS) &&
((alloc_base + alloc_length) >= ((CUdeviceptr)address + length));
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));
if (covers_range) {
/* Allocation covers the whole range: MD should widen to the full handle */
EXPECT_EQ((void*)alloc_base, mem_attr.base_address);
EXPECT_EQ(alloc_length, mem_attr.alloc_length);
} else {
/* Allocation doesn't cover the range: MD must keep the requested extent */
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
Loading