UCP/WIREUP/IB: provide a reason when a resource's lane is unreachable #9995

amastbaum · 2024-07-07T15:55:49Z

What

During the wireup process, provide a reason when a resource's lane is unreachable.

Why

Users need more information about why a device is not reachable after an unsuccessful connection establishment. This information should be passed from UCT to UCP.

How ?

Pass a string to select/search_lane functions so in case the wireup process fails, the reason will be printed out in an upper layer.

gleon99 · 2024-07-11T16:16:29Z

Please fix the PR title to match the main commit (conventions + capitals).

gleon99 · 2024-07-11T16:19:36Z

Please edit PR description:

Capitals, grammar, styling.
Add the "Why?" section, as per our conventions.

src/ucp/wireup/select.c

gleon99 · 2024-07-08T10:56:13Z

src/ucp/wireup/wireup.c

+ if (info_str != NULL) {
+ params.field_mask |= UCT_IFACE_IS_REACHABLE_FIELD_INFO_STRING |
+ UCT_IFACE_IS_REACHABLE_FIELD_INFO_STRING_LENGTH;
+ params.info_string = info_str;


src/uct/ib/rc/accel/rc_mlx5_iface.c

src/uct/ib/base/ib_iface.c

src/ucp/wireup/select.c

src/uct/base/uct_iface.c

src/ucp/wireup/select.c

src/uct/ib/base/ib_iface.c

gleon99 · 2024-07-11T16:42:06Z

src/uct/base/uct_iface.c

+ va_start(args, fmt);
+
+ if (info_str_buf && info_str_buf_len > 0 && info_str_buf[0] == '\0') {
+ snprintf(info_str_buf, info_str_buf_len, fmt, args);


ucs_snprintf_safe

AFAIU, it might be called several times in a single flow (e.g uct_ib_iface_dev_addr_is_reachable then in the caller uct_ib_iface_is_reachable_v2). The strings overwrite each other?

@gleon99 This is why I am checking info_str_buf is empty (info_str_buf[0] == '\0'), so it won't override an existing reason. I assume the first time info_str_buf is filled happens in the deepest level of this check (== the most descriptive one), yet I want to provide a more basic reason in case it wasn't filled earlier in the flow.

@yosefe it answers your other question about why we need info_str_buf[0] == '\0' .

yosefe

can you pls post example error messages from UCP level?
Maybe add guest for it?

src/ucp/wireup/select.c

yosefe · 2024-07-14T14:01:19Z

src/ucp/wireup/select.c

+ ucs_diag(UCT_TL_RESOURCE_DESC_FMT" : unreachable [%s]",
+ UCT_TL_RESOURCE_DESC_ARG(resource), wireup_info);


can wireup_info be empty string here?

It can't be empty unless we choose to not return info string in some cases (such invalid params)..
At first I was making sure it's filled in every scenario but you're right that the code doesn't ensure that

src/ucp/wireup/select.c

src/ucp/wireup/wireup.c

src/uct/ib/base/ib_iface.c

src/uct/ib/dc/dc_mlx5.c

yosefe · 2024-07-14T14:18:41Z

src/uct/ib/rc/accel/rc_mlx5_iface.c

@@ -713,6 +713,9 @@ uct_rc_mlx5_iface_is_reachable_v2(const uct_iface_h tl_iface,
 iface_addr, IFACE_ADDR, NULL);
 /* Check hardware tag matching compatibility */
 if ((iface_addr != NULL) && (my_type != *(uint8_t*)iface_addr)) {
+ uct_iface_populate_info_str_buf(
+ params, "incompatible hardware tag matching");


need to print what is not compatible

… a reason

…f fails to connect)

src/uct/base/uct_iface.h

src/ucp/wireup/select.c

src/uct/ib/base/ib_iface.c

src/uct/ib/dc/dc_mlx5.c

src/uct/ib/rc/accel/rc_mlx5_iface.c

…previous commit a9279be)

src/uct/base/uct_iface.c

src/uct/ib/mlx5/rc/rc_mlx5_iface.c

src/uct/base/uct_iface.c

yosefe · 2024-07-21T13:25:45Z

src/uct/ib/mlx5/rc/rc_mlx5_iface.c

@@ -647,6 +646,7 @@ uct_rc_mlx5_iface_is_reachable_v2(const uct_iface_h tl_iface,
 uint8_t my_type = uct_rc_mlx5_iface_get_address_type(tl_iface);
 uint8_t remote_type;
 const uct_iface_addr_t *iface_addr;
+ static const char *tm_type_to_str[] = {"basic", "tag matching"};


pls move it before my_type since initialized vars should be before uninitialized ones

gleon99 · 2024-07-22T08:10:02Z

src/uct/ib/base/ib_iface.c

 return 0;
 }

- if (!uct_ib_iface_dev_addr_is_reachable(iface, device_addr)) {
+ if (!uct_ib_iface_dev_addr_is_reachable(iface, device_addr, params)) {
+ uct_iface_fill_info_str_buf(params, "unreachable ib device address");


ib -> IB

gleon99 · 2024-07-22T08:10:31Z

src/uct/base/uct_iface.c

@@ -289,6 +307,15 @@ int uct_iface_is_reachable_v2(const uct_iface_h tl_iface,
 const uct_iface_is_reachable_params_t *params)
 {
 const uct_base_iface_t *iface = ucs_derived_of(tl_iface, uct_base_iface_t);
+ char *info_str_buf = UCS_PARAM_VALUE(UCT_IFACE_IS_REACHABLE_FIELD, params,


gleon99 · 2024-07-22T08:13:29Z

src/uct/base/uct_iface.c

+ INFO_STRING_LENGTH, 0);
+ va_list ap;
+
+ va_start(ap, fmt);


Maybe instead:

if (buf == NULL) { return; } va_start(..); ..

wdyt?

Yes, it's a good idea

gleon99 · 2024-07-22T08:21:35Z

src/uct/ib/mlx5/rc/rc_mlx5_iface.c

 const uct_iface_addr_t *iface_addr;

 iface_addr = UCS_PARAM_VALUE(UCT_IFACE_IS_REACHABLE_FIELD, params,
 iface_addr, IFACE_ADDR, NULL);
+ remote_type = *(uint8_t*)iface_addr;


Maybe dereference only after checking != NULL below?

amastbaum · 2024-07-22T08:51:02Z

src/uct/ib/mlx5/rc/rc_mlx5_iface.c

- remote_type = *(uint8_t*)iface_addr;
+
 /* Check hardware tag matching compatibility */
- if ((iface_addr != NULL) && (my_type != remote_type)) {
+ if ((iface_addr != NULL) &&
+ (remote_type = *(uint8_t*)iface_addr != my_type)) {


@gleon99 if it's too messy I can just dereference twice..

amastbaum added WIP-DNM Work in progress / Do not review and removed WIP-DNM Work in progress / Do not review labels Jul 7, 2024

amastbaum requested a review from gleon99 July 11, 2024 07:52

UCP/WIREUP/IB: provide a reason when a resource's lane is unreachable

6fba9fb

amastbaum force-pushed the specify_a_reason_for_wireup_failure branch from d8857c2 to 6fba9fb Compare July 11, 2024 08:01

amastbaum marked this pull request as ready for review July 11, 2024 08:55

UCP/WIREUP/IB: free wireup_info

a10f6d7

amastbaum changed the title ~~print out a reason when a resource's lane is unreachable~~ UCP/WIREUP/IB: provide a reason when a resource's lane is unreachable Jul 11, 2024

gleon99 reviewed Jul 11, 2024

View reviewed changes

yosefe reviewed Jul 14, 2024

View reviewed changes

UCP/WIREUP/IB: replace snprintf with vsnprintf (ucs_vsnprintf_safe)

335d7a3

amastbaum force-pushed the specify_a_reason_for_wireup_failure branch from 8b05c4f to 335d7a3 Compare July 16, 2024 10:44

amastbaum added 5 commits July 16, 2024 18:18

UCP/WIREUP/IB: CR fixes no 1

65ca828

UCP/WIREUP/IB: use static allocation for wireup_info

6ab6c39

UCP/WIREUP/IB: print unreachable message only if wireup_info contains…

b4c376d

… a reason

UCP/WIREUP/IB: print reason in ucp_wireup_select_lanes (prints only i…

b12c884

…f fails to connect)

UCP/WIREUP/IB: make uct_iface_fill_info_str_buf a macro

a9279be

yosefe reviewed Jul 18, 2024

View reviewed changes

amastbaum added 3 commits July 21, 2024 13:57

UCP/WIREUP/IB: CR fixes

35ce9db

UCP/WIREUP/IB: make uct_iface_fill_info_str_buf a function (revert a …

effdd16

…previous commit a9279be)

Merge branch 'master' into specify_a_reason_for_wireup_failure

19148e7

yosefe reviewed Jul 21, 2024

View reviewed changes

UCP/WIREUP/IB: CR fixes

94264e0

yosefe reviewed Jul 21, 2024

View reviewed changes

amastbaum added 2 commits July 21, 2024 16:58

UCP/WIREUP/IB: CR fixes

29777ee

UCP/WIREUP/IB: CR fixes

da2d2fb

amastbaum force-pushed the specify_a_reason_for_wireup_failure branch from 37d0f41 to da2d2fb Compare July 22, 2024 06:42

gleon99 reviewed Jul 22, 2024

View reviewed changes

UCP/WIREUP/IB: CR fixes + static check fix

26ff5d3

amastbaum commented Jul 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCP/WIREUP/IB: provide a reason when a resource's lane is unreachable #9995

UCP/WIREUP/IB: provide a reason when a resource's lane is unreachable #9995

amastbaum commented Jul 7, 2024 •

edited

Loading

gleon99 commented Jul 11, 2024

gleon99 commented Jul 11, 2024

gleon99 Jul 8, 2024

gleon99 Jul 22, 2024

gleon99 Jul 11, 2024

gleon99 Jul 11, 2024

amastbaum Jul 16, 2024

yosefe left a comment

yosefe Jul 14, 2024

amastbaum Jul 16, 2024

yosefe Jul 14, 2024

yosefe Jul 21, 2024

gleon99 Jul 22, 2024

gleon99 Jul 22, 2024

gleon99 Jul 22, 2024

amastbaum Jul 22, 2024

gleon99 Jul 22, 2024

amastbaum Jul 22, 2024

		ucs_diag(UCT_TL_RESOURCE_DESC_FMT" : unreachable [%s]",
		UCT_TL_RESOURCE_DESC_ARG(resource), wireup_info);

UCP/WIREUP/IB: provide a reason when a resource's lane is unreachable #9995

Are you sure you want to change the base?

UCP/WIREUP/IB: provide a reason when a resource's lane is unreachable #9995

Conversation

amastbaum commented Jul 7, 2024 • edited Loading

What

Why

How ?

gleon99 commented Jul 11, 2024

gleon99 commented Jul 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yosefe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amastbaum commented Jul 7, 2024 •

edited

Loading