Repository navigation
UCT/CUDA_COPY: Skip dmabuf probe for async managed memory #11922
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.
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
Changes from 2 commits
d72bcfb
65e7d2c
1d53937
458aeca
31617b6
0fb53b3
1b27615
12b3116
07409ae
cfc5a8e
a041773
ad87c78
13e872f
2b73283
3773519
15e7781
16a911b
f473f14
6ece985
a7cbb96
437da17
fae7ff1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -730,6 +730,9 @@ uct_cuda_copy_md_query_attributes(const uct_cuda_copy_md_t *md, | |
| * provided address and length as base address and alloc length | ||
| * respectively */ | ||
| mem_info->type = UCS_MEMORY_TYPE_CUDA_MANAGED; | ||
| if (cuda_mem_ctx == NULL) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor: pls add a short comment here explaining why a NULL context indicates an async/stream-ordered (pool) managed allocation; the sibling else-if branch has this reasoning but it's non-obvious in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, added a short comment explaining that managed pool allocations are stream-ordered and have no owning CUDA context. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why does a NULL context on genuine managed memory imply it's not registrable? regular
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added positive coverage for ordinary |
||
| *is_async_managed = 1; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with the default
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a valid cache-performance concern. The current cache format cannot distinguish an incomplete UCM allocation event from a completed MD query when both have unknown system device and zero flags. I kept that broader UCS/UCP cache representation change out of this focused bug-fix PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cached entry becomes indistinguishable from "not detected", forcing a per-operation slow path. When the preferred location resolves to the CPU ( |
||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove unrelated change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
| cu_err = cuMemRangeGetAttribute( | ||
| (void*)&pref_loc, sizeof(pref_loc), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -441,6 +441,69 @@ UCS_TEST_P(test_mem_alloc_device, no_current_context_user_mem_registrable, | |
| EXPECT_EQ(CUDA_SUCCESS, cuMemFree(dptr)); | ||
| } | ||
|
|
||
| #if CUDART_VERSION >= 13000 | ||
| UCS_TEST_P(test_mem_alloc_device, async_managed_mem_pool_not_registrable, | ||
| "CUDA_COPY_DMABUF=try") | ||
| { | ||
| constexpr size_t size = 192; | ||
| cudaMemLocation location = {}; | ||
| cudaStream_t stream = nullptr; | ||
| cudaMemPool_t pool = nullptr; | ||
| void *buffer = nullptr; | ||
| int cuda_device; | ||
| int concurrent_managed_access; | ||
| uct_md_mem_attr_v2_t mem_attr = {}; | ||
|
|
||
| ASSERT_EQ(cudaSuccess, cudaGetDevice(&cuda_device)); | ||
| ASSERT_EQ(cudaSuccess, | ||
| cudaDeviceGetAttribute(&concurrent_managed_access, | ||
| cudaDevAttrConcurrentManagedAccess, | ||
| cuda_device)); | ||
| if (!concurrent_managed_access) { | ||
| UCS_TEST_SKIP_R("device does not support concurrent managed access"); | ||
| } | ||
|
|
||
| location.type = cudaMemLocationTypeDevice; | ||
| location.id = cuda_device; | ||
| ASSERT_EQ(cudaSuccess, | ||
| cudaMemGetDefaultMemPool(&pool, &location, | ||
| cudaMemAllocationTypeManaged)); | ||
| ASSERT_EQ(cudaSuccess, cudaStreamCreate(&stream)); | ||
|
|
||
| cudaError_t cuda_status = cudaMallocFromPoolAsync(&buffer, size, pool, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe skip instead of asserting here? a device/driver may not support managed mem pools even on CUDART>=13000, and this would turn into a hard test failure.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, the test now skips if the default managed mempool cannot be queried or async managed mempool allocation is unsupported. |
||
| stream); | ||
| if (cuda_status != cudaSuccess) { | ||
| EXPECT_EQ(cudaSuccess, cudaStreamDestroy(stream)); | ||
| ASSERT_EQ(cudaSuccess, cuda_status); | ||
| } | ||
|
|
||
| cuda_status = cudaMemsetAsync(buffer, 0, size, stream); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor: the three copies of the free/sync/destroy cleanup hide what the test checks; can we wrap the stream and the pool buffer in RAII helpers and keep only the query here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wrapped the stream-ordered managed-pool allocation and stream teardown in |
||
| if (cuda_status != cudaSuccess) { | ||
| EXPECT_EQ(cudaSuccess, cudaFreeAsync(buffer, stream)); | ||
| EXPECT_EQ(cudaSuccess, cudaStreamSynchronize(stream)); | ||
| EXPECT_EQ(cudaSuccess, cudaStreamDestroy(stream)); | ||
| ASSERT_EQ(cudaSuccess, cuda_status); | ||
| } | ||
|
|
||
| cuda_status = cudaStreamSynchronize(stream); | ||
| if (cuda_status != cudaSuccess) { | ||
| EXPECT_EQ(cudaSuccess, cudaFreeAsync(buffer, stream)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor: pls cudaStreamSynchronize before cudaStreamDestroy here, to match the error path above and drain the async free.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| EXPECT_EQ(cudaSuccess, cudaStreamDestroy(stream)); | ||
| ASSERT_EQ(cudaSuccess, cuda_status); | ||
| } | ||
|
|
||
| mem_attr.field_mask = UCT_MD_MEM_ATTR_V2_FIELD_MEM_TYPE | | ||
| UCT_MD_MEM_ATTR_V2_FIELD_MEM_FLAGS; | ||
| EXPECT_UCS_OK(uct_md_mem_query_v2(md(), buffer, size, &mem_attr)); | ||
| EXPECT_EQ(UCS_MEMORY_TYPE_CUDA_MANAGED, mem_attr.mem_type); | ||
| EXPECT_FALSE(mem_attr.mem_flags & UCS_MEM_FLAG_REGISTRABLE); | ||
|
|
||
| EXPECT_EQ(cudaSuccess, cudaFreeAsync(buffer, stream)); | ||
| EXPECT_EQ(cudaSuccess, cudaStreamSynchronize(stream)); | ||
| EXPECT_EQ(cudaSuccess, cudaStreamDestroy(stream)); | ||
| } | ||
| #endif | ||
|
|
||
| UCS_TEST_P(test_mem_alloc_device, no_current_context_vmm_mem_registrable, | ||
| "CUDA_COPY_ASYNC_MEM_TYPE=cuda") | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not gate on md->config.cuda_async_managed here as well, like the sibling branch below? otherwise pool-managed memory is marked non-registrable even when the user sets CUDA_COPY_ASYNC_MEM_TYPE=cuda.
Also worth confirming that ordinary cuMemAllocManaged memory never reports a NULL context (which would otherwise become a real registration regression), and that the config-mismatch above is intentional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, added the same
md->config.cuda_async_managedgate soCUDA_COPY_ASYNC_MEM_TYPE=cudakeeps this memory registrable.