Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
19 changes: 19 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,23 @@ 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;
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(), address, length, &mem_attr));

EXPECT_LE(mem_attr.base_address, address);
EXPECT_GE(UCS_PTR_BYTE_OFFSET(mem_attr.base_address, mem_attr.alloc_length),
UCS_PTR_BYTE_OFFSET(address, length));
}

_UCT_MD_INSTANTIATE_TEST_CASE(test_cuda_copy_md, cuda_cpy);