UCT/CUDA_IPC: Detect different PID namespace to avoid non-reachable - #11514
Conversation
shasson5
changed the title
6ffbfcd to
d2c3e22
Compare
ba5f721 to
0c81629
Compare
shasson5
changed the title
|
🤖 Starting review — findings will be posted here when done. |
svc-nvidia-pr-review
left a comment
svc-nvidia-pr-review
left a comment
There was a problem hiding this comment.
Code Review
6 findings — 🚨 3 blockers,
6 findings posted as inline comments.
836cf3c to
f62130c
Compare
shasson5
changed the title
…D namespace to avoid non-reachable
f62130c to
33c4001
Compare
| int fabric_supported = 0; | ||
| CUdevice cu_device; | ||
|
|
||
| if ((UCT_CUDADRV_FUNC(cuCtxGetDevice(&cu_device), |
There was a problem hiding this comment.
If no current context -> fabric_supported=0 is cached forever -> cross-PID-ns peers that actually support fabric won't advertise that flag.
Maybe query device 0 / all visible devices, or otherwise make the cached value independent of a current context? @rakhmets WDYT?
There was a problem hiding this comment.
I think, query device 0 is enough.
| iface_addr_len = UCS_PARAM_VALUE(UCT_IFACE_IS_REACHABLE_FIELD, params, | ||
| iface_addr_length, IFACE_ADDR_LENGTH, | ||
| sizeof(remote_iface_addr.pid)); | ||
| /* Older peers do not send pid_ns, so preserve legacy same-node reachability |
There was a problem hiding this comment.
This misses the default/non-default pid ns case. A new peer in the default ns advertises only the legacy iface addr, so a non-default local peer enters this path w iface_addr_len < sizeof(remote_iface_addr) -> skips the fabric req.
That can mark legacy cuda ipc reachable across different pid ns's.
Need to distinguish default-ns peers from legacy ones, e.g can compare the unpacked default pid_ns against the local pid_ns
There was a problem hiding this comment.
Need to distinguish default-ns peers from legacy ones, e.g can compare the unpacked default pid_ns against the local pid_ns
this will cancel wire compatibility when local + remote are on the same non-default namespace.
@rakhmets is it safe to add pid_ns packing in cuda_ipc_iface address for default NS?
There was a problem hiding this comment.
Yes, it's safe to always pack pid ns to iface address. The only reason was to pack as less as possible. If it's the only way to implement the feature, then we have to pack this into the address.
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Starting review — findings will be posted here when done. |
|
@rakhmets can review? |
| .device_addr = ae->dev_addr, | ||
| .iface_addr = ae->iface_addr, | ||
| .device_addr_length = ae->dev_addr_len | ||
| .device_addr_length = ae->dev_addr_len, |
There was a problem hiding this comment.
shall we also update ucp_wireup_connect_lane_to_iface? and maybe other places that create ep and need to pass device/iface/ep address length?
There was a problem hiding this comment.
added to ucp_wireup_ep_connect_aux (no other relevant call site was found)
| int uct_cuda_base_device_supports_fabric(CUdevice cuda_device) | ||
| { | ||
| #if HAVE_CUDA_FABRIC && \ | ||
| HAVE_DECL_CU_DEVICE_ATTRIBUTE_HANDLE_TYPE_FABRIC_SUPPORTED | ||
| int supported; | ||
|
|
||
| if (UCT_CUDADRV_FUNC(cuDeviceGetAttribute( | ||
| &supported, | ||
| CU_DEVICE_ATTRIBUTE_HANDLE_TYPE_FABRIC_SUPPORTED, | ||
| cuda_device), UCS_LOG_LEVEL_DEBUG) != UCS_OK) { | ||
| return 0; | ||
| } | ||
|
|
||
| return supported; | ||
| #else | ||
| return 0; | ||
| #endif | ||
| } |
There was a problem hiding this comment.
why needed? we already check fabric support in current code , and the result is reflected in enabled_mnnvl field
There was a problem hiding this comment.
if you refer to uct_cuda_ipc_md_check_fabric_info, it checks MNNVL support
| typedef enum { | ||
| UCT_CUDA_IPC_DEVICE_ADDR_FLAG_MNNVL = UCS_BIT(0) | ||
| UCT_CUDA_IPC_DEVICE_ADDR_FLAG_MNNVL = UCS_BIT(0), | ||
| UCT_CUDA_IPC_DEVICE_ADDR_FLAG_FABRIC = UCS_BIT(1) |
There was a problem hiding this comment.
how is FABRIC support different from MNNVL support? can we unite them?
There was a problem hiding this comment.
Fabric API can be available regardless of MNNVL support (for example for intra node communication between containers). We can use it on setups without NVLink
| uct_cuda_ipc_iface_dev_addr_flags(const uct_cuda_ipc_device_addr_t *dev_addr, | ||
| size_t dev_addr_len) | ||
| { | ||
| if (dev_addr_len == sizeof(uint64_t)) { |
There was a problem hiding this comment.
it will fail in line 129:
ucs_assertv(dev_addr_len >= sizeof(uct_cuda_ipc_device_addr_t),
"dev_addr_len=%zu", dev_addr_len);
| /* Older peers do not send pid_ns, so preserve legacy same-node reachability | ||
| * and apply the cross-namespace fabric check only to extended addresses. */ | ||
| remote_iface_addr = uct_cuda_ipc_iface_address_unpack(params->iface_addr, | ||
| iface_addr_len); | ||
| if ((iface_addr_len >= sizeof(remote_iface_addr)) && |
There was a problem hiding this comment.
if remote pid_ns is not present, we can assume it's the default ns, like in rkey case
There was a problem hiding this comment.
we need to distinguish "old address" from "default NS address" because for default NS we'd want to confirm Fabric API is supported, o/w a connection between host process and "container process" may be identified as reachable
| uct_cuda_ipc_iface_address_t *cuda_ipc_iface_address; | ||
|
|
||
| *(pid_t*)iface_addr = getpid(); | ||
| if (ucs_sys_ns_is_default(UCS_SYS_NS_TYPE_PID)) { |
There was a problem hiding this comment.
why need to pack NS in case it is default? we could save on the address length
There was a problem hiding this comment.
we need to distinguish "old address" vs "default NS address":
- old address -> this is wire compatibility mode, we'd want to keep same behavour as today (reachable regardless to PID NS).
- default NS -> if remote is default NS and local is non-default (e.g. run in container), then Fabric API is required for communication
| } | ||
| #endif | ||
|
|
||
| static int uct_cuda_ipc_md_check_fabric_support(void) |
There was a problem hiding this comment.
why is it needed for reachability?
There was a problem hiding this comment.
because different PID namespaces are only reachable when using Fabric API
# Conflicts: # src/ucp/wireup/select.c # src/uct/cuda/cuda_ipc/cuda_ipc_iface.c
|
@yosefe please review |
|
@yosefe WDYT? |
|
@yosefe can take a look? |
5 participants
What?
Improve cuda_ipc reachability checks for peers in different PID namespaces by advertising CUDA fabric IPC support only when the local process can actually use it.
This PR adds:
CU_DEVICE_ATTRIBUTE_HANDLE_TYPE_FABRIC_SUPPORTEDcuDeviceGetAttribute()Why?
Legacy CUDA IPC handles cannot be exchanged across different PID namespaces. In that case cuda_ipc should be considered reachable only if both peers support fabric IPC handles.
The previous approach detected local fabric support by trying to allocate and release a small fabric VMM buffer. Querying the CUDA device attribute is cleaner and avoids using allocation as a capability probe.
How?
When building CUDA IPC device addresses, the transport now sets a FABRIC flag if the current CUDA device reports fabric-handle support and the process can access an IMEX channel. During reachability, peers in different PID namespaces require both local and remote FABRIC support.
Validation
git diff --check