UCT/API: Add new MD resource query API implementation #10161

tvegas1 · 2024-09-19T08:29:46Z

What

Implement UCT MD transport resource descriptor query v2.

Why ?

We need to introduce a new flag to allow sending address of some device of type SHM, even with net address only restriction.

How ?

Add to ucx_info -d and add UT. All users initialize the uct resource descriptor to zero except TCP.

src/uct/api/v2/uct_v2.h

iyastreb · 2024-09-19T09:14:46Z

test/gtest/uct/test_md.cc

+
+ ASSERT_UCS_OK(uct_md_query_tl_resources(md(), &v1, &num_v1));
+ ASSERT_UCS_OK(uct_md_query_tl_resources_v2(md(), &v2, &num_v2, &params));
+ ASSERT_TRUE(num_v2 > 0);


iyastreb · 2024-09-19T09:15:07Z

test/gtest/uct/test_md.cc

+ ASSERT_UCS_OK(uct_md_query_tl_resources(md(), &v1, &num_v1));
+ ASSERT_UCS_OK(uct_md_query_tl_resources_v2(md(), &v2, &num_v2, &params));
+ ASSERT_TRUE(num_v2 > 0);
+ ASSERT_EQ(num_v2, num_v1);


maybe EXPECT_EQ, to avoid resource leak on failure

iyastreb · 2024-09-19T09:18:51Z

src/uct/api/v2/uct_v2.h

+ * @ingroup UCT_RESOURCE
+ * @brief Parameters passed to @ref uct_md_query_tl_resources_v2.
+ */
+typedef struct uct_md_query_tl_resources_params {


struct name seems redundant

iyastreb · 2024-09-19T09:19:32Z

src/uct/api/v2/uct_v2.h

+ * The enumeration defines bit mask of capabilities in @ref
+ * uct_tl_resource_desc_v2_t::flags, set by @ref uct_md_query_tl_resources_v2.
+ */
+enum {


maybe give it a name and @reference in uct_tl_resource_desc_t

iyastreb · 2024-09-19T09:20:54Z

src/uct/api/v2/uct_v2.h

+uct_md_query_tl_resources_v2(uct_md_h md,
+ uct_tl_resource_desc_v2_t **resources_p,
+ unsigned *num_resources_p,
+ uct_md_query_tl_resources_params_t *params);


can params be const?

iyastreb · 2024-09-19T09:23:54Z

src/uct/base/uct_md.c

+ return status;
+ }
+
+ *resources_p = malloc(*num_resources_p * sizeof(**resources_p));


src/uct/base/uct_md.c

iyastreb · 2024-09-19T09:28:49Z

src/tools/info/tl_info.c

@@ -348,7 +349,7 @@ static void print_iface_info(uct_worker_h worker, uct_md_h md,
 }

 static ucs_status_t print_tl_info(uct_md_h md, const char *tl_name,
- uct_tl_resource_desc_t *resources,
+ uct_tl_resource_desc_v2_t *resources,


pass by const?

iyastreb · 2024-09-19T09:29:02Z

src/tools/info/tl_info.c

@@ -122,7 +122,7 @@ static const char *size_limit_to_str(size_t min_size, size_t max_size)
 }

 static void print_iface_info(uct_worker_h worker, uct_md_h md,
- uct_tl_resource_desc_t *resource)
+ uct_tl_resource_desc_v2_t *resource)


brminich · 2024-09-19T15:37:03Z

src/tools/info/tl_info.c

@@ -392,14 +393,15 @@ static void print_md_info(uct_component_h component,
 const char *req_tl_name)
 {
 UCS_STRING_BUFFER_ONSTACK(strb, 256);
- uct_tl_resource_desc_t *resources, tmp;
+ uct_tl_resource_desc_v2_t *resources, tmp;


seems we can use old API here

using the new one allows to dump the added flags content which can be helpful, so I'd keep it.

brminich · 2024-09-19T15:40:47Z

src/ucp/core/ucp_context.c

@@ -1112,31 +1112,36 @@ static int ucp_tl_resource_is_same_device(const uct_tl_resource_desc_t *resource
 static void ucp_add_tl_resource_if_enabled(
 ucp_context_h context, ucp_md_index_t md_index,
 const ucp_config_t *config, const ucs_string_set_t *aux_tls,
- const uct_tl_resource_desc_t *resource, unsigned *num_resources_p,
+ const uct_tl_resource_desc_v2_t *resource, unsigned *num_resources_p,


maybe you can name it tl_resource and define uct_tl_resource_desc_t *resource = &tl_resource->desc. Then only a couple of lines need to be changed in this func

brminich · 2024-09-19T16:31:02Z

src/ucp/core/ucp_context.c

@@ -1159,16 +1164,19 @@ ucp_add_tl_resources(ucp_context_h context, ucp_md_index_t md_index,
 uint64_t *tl_cfg_mask)
 {
 ucp_tl_md_t *md = &context->tl_mds[md_index];
- uct_tl_resource_desc_t *tl_resources;
+ uct_tl_resource_desc_v2_t *tl_resources;


seems we can use old API here

we need v2 because we call ucp_add_tl_resource_if_enabled() on the end of the funcction.

brminich · 2024-09-19T16:35:29Z

src/uct/cuda/cuda_ipc/cuda_ipc_iface.c

 }
 #endif
- return uct_cuda_base_query_devices_common(uct_md, dev_type,
- tl_devices_p, num_tl_devices_p);
+ status = uct_cuda_base_query_devices_common(uct_md, dev_type,


Suggested change

status = uct_cuda_base_query_devices_common(uct_md, dev_type,

status = uct_cuda_base_query_devices_common(uct_md, UCT_DEVICE_TYPE_SHM,

brminich · 2024-09-19T16:36:37Z

test/gtest/uct/test_md.cc

+ EXPECT_GT(num_v2, 0);
+ EXPECT_EQ(num_v2, num_v1);
+
+ for (auto i = 0; (i < num_v1) && (num_v1 == num_v2) ; ++i) {


imo, no need to check num_v1 == num_v2 again

I think we need it because otherwise we might use out of bound memory.

iyastreb · 2024-09-20T05:55:33Z

src/uct/api/v2/uct_v2.h

+ * Resource descriptor of a standalone communication resource with extraneous
+ * flags.
+ */
+typedef struct uct_tl_resource_desc_v2 {


redundant struct name

tvegas1 added 2 commits September 18, 2024 09:45

UCT/API: Add new MD resource query API

1e716b3

UCT/API: Add new MD resource query implementation

dce5abb

tvegas1 mentioned this pull request Sep 19, 2024

UCT/CUDA: Advertise MNNVL inter-node capability with shm device type #10141

Closed

rakhmets reviewed Sep 19, 2024

View reviewed changes

src/uct/api/v2/uct_v2.h Show resolved Hide resolved

iyastreb reviewed Sep 19, 2024

View reviewed changes

tvegas1 added 2 commits September 19, 2024 16:27

UCT/API: Add new MD resource query implementation

dd27566

UCT/API: Add new MD resource query implementation

7d61d6f

brminich reviewed Sep 19, 2024

View reviewed changes

UCT/API: Add new MD resource query implementation

5c65954

iyastreb approved these changes Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCT/API: Add new MD resource query API implementation #10161

UCT/API: Add new MD resource query API implementation #10161

tvegas1 commented Sep 19, 2024

iyastreb Sep 19, 2024

tvegas1 Sep 19, 2024

iyastreb Sep 19, 2024

tvegas1 Sep 19, 2024

iyastreb Sep 19, 2024

tvegas1 Sep 19, 2024

iyastreb Sep 19, 2024

tvegas1 Sep 19, 2024

iyastreb Sep 19, 2024

tvegas1 Sep 19, 2024

iyastreb Sep 19, 2024

tvegas1 Sep 19, 2024

iyastreb Sep 19, 2024

tvegas1 Sep 19, 2024

iyastreb Sep 19, 2024

tvegas1 Sep 19, 2024

brminich Sep 19, 2024

tvegas1 Sep 19, 2024

brminich Sep 19, 2024

tvegas1 Sep 19, 2024

brminich Sep 19, 2024

tvegas1 Sep 19, 2024

brminich Sep 19, 2024

tvegas1 Sep 19, 2024

brminich Sep 19, 2024

tvegas1 Sep 19, 2024

iyastreb Sep 20, 2024

	status = uct_cuda_base_query_devices_common(uct_md, dev_type,
	status = uct_cuda_base_query_devices_common(uct_md, UCT_DEVICE_TYPE_SHM,

UCT/API: Add new MD resource query API implementation #10161

Are you sure you want to change the base?

UCT/API: Add new MD resource query API implementation #10161

Conversation

tvegas1 commented Sep 19, 2024

What

Why ?

How ?

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

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