UCP/WIREUP: Increase UCP_MAX_LANES to 64 #9814

ivankochin · 2024-04-11T09:38:08Z

What

Increase number of maximal lanes per EP to 64.

Why ?

To allow collecting information about all lanes on systems with many transports/devices.

edgargabriel · 2024-04-14T19:36:04Z

Would it make sense to have this parameter as a configure option?

yosefe · 2024-04-15T07:28:09Z

Would it make sense to have this parameter as a configure option?

I think not, we should be able to find a solution to set upper limit to 64 w/o extra overheads when the actual number of lanes is small

src/ucs/sys/compiler_def.h

ivankochin · 2024-04-16T14:07:53Z

src/ucp/rndv/rndv_rkey_ptr.c

+
+    ucs_assertv(rpriv->spriv.super.lane != UCP_NULL_LANE, "lane=%d",
+                rpriv->spriv.super.lane);
+    attr->lane_map = UCS_BIT(rpriv->spriv.super.lane);
+    if (rpriv->ack.lane != UCP_NULL_LANE) {
+       attr->lane_map |= UCS_BIT(rpriv->ack.lane);
+    }


This change is required since ack.lane can be equal to 255 (UCP_NULL_LANE) so after increasing lane_map size to 64 it started to set 63rd bit in lane_map.

why did it set 63d bit?

i think (1ul << 256) is anyway undefined behavior as 256 >= 64.

why did it set 63d bit?

To be honest, I don't know. I just detected this behavior and when I applied this change it gone. Do you want me to check it?

i was experimenting on similar case and got unpredictable result, i guess this should be ok

…test

src/ucs/sys/compiler_def.h

src/ucp/wireup/select.c

src/ucp/core/ucp_ep.inl

src/ucs/sys/compiler_def.h

src/ucp/core/ucp_ep.inl

src/ucp/proto/proto.h

src/ucp/rndv/rndv_rkey_ptr.c

brminich · 2024-04-19T12:53:24Z

test/gtest/common/test_obj_size.cc

@@ -57,7 +57,7 @@ UCS_TEST_F(test_obj_size, size) {
    EXPECTED_SIZE(ucp_rkey_t, 32);
 #endif
    /* TODO reduce request size to 240 or less after removing old protocols state */
-    EXPECTED_SIZE(ucp_request_t, 264);
+    EXPECTED_SIZE(ucp_request_t, 280);


can we avoid that?

I think we can consider increasing number of lanes to 32 instead of 64. Or allowing creation of 64 lanes only for wireup and restrict the max lanes number by 32 or 16 for RNDV protocols. What sounds better for you?

both sounds ok. Maybe it is better to restrict protocol;s to use up to 16 lanes? Then no need to keep large maps in the request

brminich · 2024-04-19T12:53:39Z

test/gtest/ucp/test_ucp_sockaddr.cc

+    ASSERT_LE(16, (int)ucp_ep_num_lanes(sender().ep()));
+    ASSERT_LE(16, (int)ucp_ep_num_lanes(receiver().ep()));


Because we have some platform which can create more than 16 lanes in that particular case.

i'd suggest to change this test or wtite a new one which tests creation of 64 lanes

brminich · 2024-04-19T12:54:43Z

test/gtest/ucp/test_ucp_tag_xfer.cc

@@ -1247,19 +1247,19 @@ UCS_TEST_P(multi_rail_max, max_lanes, "IB_NUM_PATHS?=16", "TM_SW_RNDV=y",

    ucp_lane_index_t num_lanes = ucp_ep_num_lanes(sender().ep());
    ASSERT_EQ(ucp_ep_num_lanes(receiver().ep()), num_lanes);
-    ASSERT_EQ(num_lanes, max_lanes);
+    ASSERT_GE(num_lanes, max_lanes);


Because we have some platform which can create more than 16 lanes in that particular case.

brminich · 2024-04-19T12:55:02Z

src/ucp/core/ucp_ep.inl

@@ -239,7 +239,7 @@ static inline ucp_rsc_index_t
 ucp_ep_config_get_dst_md_cmpt(const ucp_ep_config_key_t *key,
                              ucp_md_index_t dst_md_index)
 {
-    unsigned idx = ucs_popcount(key->reachable_md_map & UCS_MASK(dst_md_index));
+    unsigned idx = ucs_bitmap2idx(key->reachable_md_map, dst_md_index);


unrelated fix?

Yes, a little :)

I just discovered that there is a helper function for that logic, so decided to included it to the patch.

brminich · 2024-04-19T12:56:31Z

src/ucp/rndv/rndv_rkey_ptr.c

+
+    ucs_assertv(rpriv->spriv.super.lane != UCP_NULL_LANE, "lane=%d",
+                rpriv->spriv.super.lane);
+    attr->lane_map = UCS_BIT(rpriv->spriv.super.lane);
+    if (rpriv->ack.lane != UCP_NULL_LANE) {
+       attr->lane_map |= UCS_BIT(rpriv->ack.lane);
+    }


why did it set 63d bit?

…-max-lanes-to-64 Conflicts: src/ucp/proto/proto_select.c

…-max-lanes-to-64 Conflicts: src/ucp/proto/proto.h

brminich · 2024-05-02T09:30:41Z

test/gtest/common/mem_buffer.h

@@ -129,6 +130,7 @@ class mem_buffer {
                                     size_t length, size_t offset,
                                     const void *buffer, const void *orig_ptr)
    {
+        ucs_assertv(length <= 8, "length=%zu", length);


Since new implementation of UCS_MASK will still do undefined behavior if input value > 64. In that particular case length is size of one element that should be compared.

UCP/WIREUP: Increase UCP_MAX_LANES to 64

8b9a274

ivankochin self-assigned this Apr 11, 2024

ivankochin added 2 commits April 11, 2024 13:57

UCP/PROTO: Increase priv_buf size for ucp_proto_multi_priv_t

cb02d99

UCS/DEF: Increase UCS_ALLOCA_MAX_SIZE

b71a104

tvegas1 reviewed Apr 15, 2024

View reviewed changes

src/ucs/sys/compiler_def.h Outdated Show resolved Hide resolved

ivankochin marked this pull request as draft April 15, 2024 10:14

ivankochin added 4 commits April 15, 2024 13:26

UCP/WIREUP: Fix p2p lanes detection + fix ucp_request size expectations

d44cb31

UCP/PROTO: Move priv buffer allocation to heap

2706b04

UCP/WIREUP: Move sinfo_array to heap

3711e96

UCP/PROTO: Avoid adding NULL lane to lane_map

220f940

ivankochin commented Apr 16, 2024

View reviewed changes

UCP/WIREUP: Make UCS_MASK handle 64 input correctly + fix multi-rail …

325f606

…test

ivankochin commented Apr 19, 2024

View reviewed changes

src/ucs/sys/compiler_def.h Show resolved Hide resolved

ivankochin requested review from yosefe and brminich April 19, 2024 04:15

ivankochin marked this pull request as ready for review April 19, 2024 06:32

tvegas1 reviewed Apr 19, 2024

View reviewed changes

src/ucp/wireup/select.c Outdated Show resolved Hide resolved

tvegas1 reviewed Apr 19, 2024

View reviewed changes

src/ucp/core/ucp_ep.inl Show resolved Hide resolved

src/ucs/sys/compiler_def.h Show resolved Hide resolved

brminich reviewed Apr 19, 2024

View reviewed changes

ivankochin added 5 commits April 29, 2024 11:12

UCP/WIREUP: Apply review comments

3028900

Merge remote-tracking branch 'origin/master' into ucp/wireup/increase…

653fd7d

…-max-lanes-to-64 Conflicts: src/ucp/proto/proto_select.c

Merge remote-tracking branch 'origin/master' into ucp/wireup/increase…

e735c50

…-max-lanes-to-64 Conflicts: src/ucp/proto/proto.h

UCS/DEF: Always use safe version of UCS_MASK

eee7cc5

UCS/DEF: Use UCS_MASK implementation that works for all type sizes

9b84043

tvegas1 approved these changes May 1, 2024

View reviewed changes

brminich reviewed May 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCP/WIREUP: Increase UCP_MAX_LANES to 64 #9814

UCP/WIREUP: Increase UCP_MAX_LANES to 64 #9814

ivankochin commented Apr 11, 2024

edgargabriel commented Apr 14, 2024

yosefe commented Apr 15, 2024

ivankochin Apr 16, 2024

brminich Apr 19, 2024

tvegas1 Apr 19, 2024

ivankochin Apr 29, 2024

tvegas1 Apr 29, 2024

brminich Apr 19, 2024

ivankochin Apr 29, 2024

brminich Apr 30, 2024

brminich Apr 19, 2024

ivankochin Apr 29, 2024

brminich Apr 30, 2024

brminich Apr 19, 2024

ivankochin Apr 29, 2024

brminich Apr 19, 2024

ivankochin Apr 29, 2024

brminich Apr 19, 2024

brminich May 2, 2024

ivankochin May 2, 2024

		ASSERT_LE(16, (int)ucp_ep_num_lanes(sender().ep()));
		ASSERT_LE(16, (int)ucp_ep_num_lanes(receiver().ep()));

UCP/WIREUP: Increase UCP_MAX_LANES to 64 #9814

Are you sure you want to change the base?

UCP/WIREUP: Increase UCP_MAX_LANES to 64 #9814

Conversation

ivankochin commented Apr 11, 2024

What

Why ?

edgargabriel commented Apr 14, 2024

yosefe commented Apr 15, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment