IB/BASE: Fixes #8595 #8603
IB/BASE: Fixes #8595 #8603
Conversation
src/uct/ib/base/ib_device.c
Outdated
| uint8_t port_num; | ||
| uint8_t usable_port_cnt = 0; | ||
|
|
||
| if (uct_ib_device_is_iwarp(dev)) { |
There was a problem hiding this comment.
IMO we should modify
ucx/src/uct/ib/base/ib_device.c
Lines 777 to 787 in a767fac
- move uct_ib_device_is_iwarp() from uct_ib_device_port_check to
Line 1345 in ee5fa05
- check that port is RoCE or IB in
ucx/src/uct/ib/base/ib_device.c
Line 783 in a767fac
There was a problem hiding this comment.
I agree that the solution does not look super clean. The logic between MD creating and the rest of the code is not unified. I was really puzzled when I went through the code.
There was a problem hiding this comment.
@yosefe I implemented what you have suggested and it does not look good.
1 - Will work but ...
2 - It is not going to work because uct_ib_device_port_check that you referenced is invoked after MD creation, which too late.
Just addressing #1 is possible but then you only execute the iwarp part of the check in uct_ib_device_is_accessible and the second half (not roce or IB) in uct_ib_device_init which makes code much more complex to understand. Essentially the logical check "not verbs compliant device that has verbs" gets spread over different places. IMHO current implementation is more reasonable.
There was a problem hiding this comment.
we are checking the port type after creating the md because in case some ports are supported and some are not, it makes more sense to disable only the unsupported ports and not the entire device.
if the device does not have any supported ports we would eventually close the md anyway.
There was a problem hiding this comment.
@yosefe I understand why you are checking port by port and the patch is doing the same in much earlier stage so we don't create MD for "non-verbs" devices.
There was a problem hiding this comment.
@yosefe Because the TL does not work on transport layers other than IBV_TRANSPORT_IB, which cover both ROCE and IB.
We know that IBV_TRANSPORT_USNIC, IBV_TRANSPORT_USNIC_UDP, and IBV_TRANSPORT_UNSPECIFIED. So, why to enable and then cause failures and leaks when on such systems only TCP has to be used in default mode.
There was a problem hiding this comment.
@yosefe , what to do thing about the patch bellow (please ignore my ifdef 0 debug section). On AWS machine with EFA I have all tests passing.
@@ -1280,6 +1280,32 @@ static uct_md_ops_t UCS_V_UNUSED uct_ib_md_global_odp_ops = {
.detect_memory_type = ucs_empty_function_return_unsupported,
};
+#if 0
+static const char *uct_ib_device_transport_to_name(struct ibv_device *device)
+{
+ switch (device->transport_type) {
+ case IBV_TRANSPORT_IB:
+ return "InfiniBand";
+ case IBV_TRANSPORT_IWARP:
+ return "iWARP";
+ case IBV_TRANSPORT_USNIC:
+ return "usNIC";
+ case IBV_TRANSPORT_USNIC_UDP:
+ return "usNIC UDP";
+ case IBV_TRANSPORT_UNSPECIFIED:
+ return "Unspecified";
+ default:
+ return "Unknown";
+ }
+}
+#endif
+
+static int uct_ib_device_is_supported(struct ibv_device *device)
+{
+ /* TODO: enable additional transport types when ready */
+ return device->transport_type == IBV_TRANSPORT_IB;
+}
+
int uct_ib_device_is_accessible(struct ibv_device *device)
{
/* Enough place to hold the full path */
@@ -1298,7 +1324,7 @@ int uct_ib_device_is_accessible(struct ibv_device *device)
return 0;
}
- return 1;
+ return uct_ib_device_is_supported(device);
}
|
I will take a look but IMHO what I proposed here is cleaner - we should not create these MD all together. |
|
@yosefe #8466 fixes ucx_perftest run but with my patch I see less failures in gtest with #8466 only |
|
I see the difference: these failures couldn't be related to #8466 |
|
@dmitrygx I applied this patch on top of your patch and I don't see these two errors. |
src/uct/ib/base/ib_md.c
Outdated
| static const char *uct_ib_device_transport_to_name(struct ibv_device *device) | ||
| { | ||
| switch (device->transport_type) { | ||
| case IBV_TRANSPORT_IB: |
There was a problem hiding this comment.
code style - no indent here
src/uct/ib/base/ib_md.c
Outdated
| @@ -1230,6 +1230,36 @@ static uct_md_ops_t UCS_V_UNUSED uct_ib_md_global_odp_ops = { | |||
| .detect_memory_type = ucs_empty_function_return_unsupported, | |||
| }; | |||
|
|
|||
| static const char *uct_ib_device_transport_to_name(struct ibv_device *device) | |||
There was a problem hiding this comment.
uct_ib_device_transport_type_name
src/uct/ib/base/ib_md.c
Outdated
| /* TODO: enable additional transport types when ready */ | ||
| int ret = device->transport_type == IBV_TRANSPORT_IB; | ||
| if (!ret) { | ||
| ucs_debug("Device %s of type %s is not supported", |
There was a problem hiding this comment.
small case (Device->device)
| } | ||
| } | ||
|
|
||
| static int uct_ib_device_is_supported(struct ibv_device *device) |
There was a problem hiding this comment.
this function is small and used once, IMO we can move its code to uct_ib_device_is_accessible
There was a problem hiding this comment.
IMO stand alone static functions look cleaner and makes code easier to read.
|
@yosefe It seems there are pre-2014 rdma-core builds that don't have post 2014 defines. To workaround this, instead of introducing bunch of m4 config scripts I'm thinking to go with numerical values. These are not expected to change. What do you thing ? |
|
@shamisp I don't think it's a clean solution, IMO adding these to AC_CHECK_DECLS and using |
IMO, numerical values are in debug messages are not super useful to debug. I went with numerical cases as for now. AC_CHECK_DECLS is possible but IMHO just too many ifdef for a simple print message. |
The code move RDMA transport check to MD level and prevents MD creation if the underlying NIC is not supported.
shamisp
force-pushed
the
topic/fix_8595
branch
from
2fbd35f to
b3252f0
Compare
done |
What
The code prevents MD to create PD if the underlying transport is not supported by UCX.
Why ?
Fixes #8595
How ?
If the verbs transport layer is not ethernet, roce, or IB - skip the device