CUDA_IPC: Don't show CUDA error on exit, from static destroy - #11846
CUDA_IPC: Don't show CUDA error on exit, from static destroy#11846iyastreb wants to merge 1 commit into
Conversation
|
🤖 Starting review — findings will be posted here when done. |
| status = uct_cuda_ipc_primary_ctx_retain_and_push(region->cu_dev, | ||
| log_level); | ||
| if (status != UCS_OK) { | ||
| return status; |
There was a problem hiding this comment.
uct_cuda_ipc_close_memhandle_legacy() now takes log_level, but this cuIpcCloseMemHandle() still logs at WARN. This is the call the STATIC_CLEANUP comment says is expected to fail during driver shutdown, so pls use UCT_CUDADRV_FUNC(..., log_level) here, otherwise the warning is not silenced.
| @@ -157,11 +158,13 @@ static void uct_cuda_ipc_primary_ctx_pop_and_release(CUdevice cuda_device) | |||
| } | |||
There was a problem hiding this comment.
uct_cuda_ipc_primary_ctx_pop_and_release runs in the shutdown close path too, but ignores log_level and both cuCtxPopCurrent / cuDevicePrimaryCtxRelease log at WARN. pls thread log_level here as well so the driver-deinit failures are demoted consistently.
| static ucs_status_t uct_cuda_ipc_close_memhandle(uct_cuda_ipc_cache_region_t *region) | ||
| static ucs_status_t | ||
| uct_cuda_ipc_close_memhandle(uct_cuda_ipc_cache_region_t *region, | ||
| ucs_log_level_t log_level) |
There was a problem hiding this comment.
close_memhandle() takes log_level but only the legacy branch forwards it; the VMM/POSIX_FD (cuMemUnmap, cuMemAddressFree) and mempool (cuMemFree) branches still log at WARN. pls pass log_level here too, otherwise VMM/mempool regions still warn during driver shutdown.
|
|
||
| status = UCT_CUDADRV_FUNC_LOG_ERR(cuCtxPushCurrent(cuda_ctx)); | ||
| status = UCT_CUDADRV_FUNC(cuCtxPushCurrent(cuda_ctx), log_level); | ||
| if (status != UCS_OK) { |
There was a problem hiding this comment.
minor: after cuCtxPushCurrent fails, the compensating cuDevicePrimaryCtxRelease still logs at WARN even though the function now has log_level. pass log_level to this release too for consistency with the push above.
|
Overall: the refactor threads log_level through the API correctly, but the implementation is incomplete relative to its stated goal. The operations that fail during CUDA driver shutdown (cuIpcCloseMemHandle, the ctx pop/release, and the VMM/mempool unmap/free calls) still log at WARN, so UCS_STATIC_CLEANUP will still emit warnings and the change does not fully deliver the intended log suppression. Test coverage note: the only test update is a signature fix in test_cuda_ipc_md.cc; there is no test verifying the shutdown log level. This is hard to test in gtest, so the gap is acceptable but worth noting. |
| * releases all the mappings during its shutdown. */ | ||
| kh_foreach_value(&uct_cuda_ipc_remote_cache.hash, rem_cache, { | ||
| uct_cuda_ipc_destroy_cache(rem_cache); | ||
| uct_cuda_ipc_destroy_cache(rem_cache, UCS_LOG_LEVEL_DEBUG); |
There was a problem hiding this comment.
Maybe to reduce the amount of changes do the following:
- call some dummy cuda driver api method, e.g. cuDeviceGetCount
- if it returns CUDA_ERROR_DEINITIALIZED pass an option to uct_cuda_ipc_destroy_cache
- in uct_cuda_ipc_cache_purge close mem handles only if the option is true.
3 participants
What?
Address ai-dynamo/nixl#2171
Why?
We should ignore this diagnostic on cleanup, because it's not a real issue, as all this mem handles will be destroyed anyway, as the cuda driver is shutting down
How?
Add log_level param