UCP/API: Work with offsets and channel id. #10920

ofirfarjun7 · 2025-09-29T08:02:25Z

What?

Change UCP device API to work with offsets instead of address.
Add channel_id to API to support use of multiple QPs per destination.

Why?

User requirements.

src/ucp/api/device/ucp_device_impl.h

Sergei-Lebedev · 2025-09-29T21:12:07Z

src/ucp/api/device/ucp_device_impl.h

 {
-    return ucs_device_atomic64_write(
-            reinterpret_cast<uint64_t*>(counter_ptr), value);
+    return ucs_device_atomic64_write(reinterpret_cast<uint64_t*>(counter_ptr),


this is void function, return is not needed

src/tools/perf/cuda/ucp_cuda_kernel.cu

yosefe · 2025-09-30T14:15:17Z

src/tools/perf/cuda/ucp_cuda_kernel.cu

+                                   UCP_DEVICE_MEM_LIST_ELEM_FIELD_LENGTH;
+            elems[i].memh        = perf.ucp.send_memh;
+            elems[i].rkey        = perf.ucp.rkey;
+            elems[i].local_addr  = (char*)perf.send_buffer + offset;


minor: UCS_PTR_BYTE_OFFSET

yosefe · 2025-09-30T14:16:54Z

src/ucp/api/device/ucp_device_impl.h

-    uct_elem    = (uct_device_mem_element_t*)UCS_PTR_BYTE_OFFSET(mem_list_h + 1,
-                                                                 elem_offset);
+    uct_elem    = (uct_device_mem_element_t*)
+            UCS_PTR_BYTE_OFFSET(mem_list_h->uct_mem_elements, elem_offset);


maybe save uct_mem_elements per lane, and remove the field uct_mem_element_size ?

Maybe I'm missing something, how we can calculate offset to specific element without saving the size? type of element is unknown to UCP.

BTW: to support multi lanes we do need to add uct_mem_elements per lane. Not sure I will do it in this PR, it's already large.

yosefe · 2025-09-30T14:17:40Z

src/ucp/api/device/ucp_device_impl.h

-        ucp_device_mem_list_handle_h mem_list_h, unsigned mem_list_index,
-        const void *address, uint64_t remote_address, size_t length,
-        uint64_t flags, ucp_device_request_t *req)
+        ucp_device_mem_list_handle_h mem_list_h, unsigned channel_id,


IMO channel_id param should be between "length" and "flags"

yosefe · 2025-09-30T14:19:30Z

src/ucp/api/device/ucp_device_types.h

+        /**
+         * Array of local addresses for the device transfer operations.
+         */
+        void     **local_addr;


local_addrs

yosefe · 2025-09-30T14:19:39Z

src/ucp/api/device/ucp_device_types.h

+        /**
+         * Array of remote addresses for the device transfer operations.
+         */
+        uint64_t *remote_addr;


remote_addrs

yosefe · 2025-09-30T14:19:55Z

src/ucp/api/device/ucp_device_types.h

+         * Array of lengths of the local buffers in bytes.
+         */
+        size_t *length;
+    } ucp_mem_elements;


IMO we can remove this wrapping struct

yosefe · 2025-09-30T14:20:30Z

src/ucp/core/ucp_device.c

+
+    handle_size += sizeof(*handle.ucp_mem_elements.local_addr);
+    handle_size += sizeof(*handle.ucp_mem_elements.remote_addr);
+    handle_size += sizeof(*handle.ucp_mem_elements.length);


space line after

maybe combine to single calculation

yosefe · 2025-09-30T14:21:27Z

src/ucp/core/ucp_device.c

+        ucp_mem_type_unpack(ep->worker, &local_addresses[i], &local_addr,
+                            sizeof(local_addresses[i]), mem_type);
+        ucp_mem_type_unpack(ep->worker, &remote_addresses[i], &remote_addr,
+                            sizeof(remote_addresses[i]), mem_type);
+        ucp_mem_type_unpack(ep->worker, &lengths[i], &length,
+                            sizeof(lengths[i]), mem_type);


can we "unpack" all these in single/several calls, and not for each element?

remote_addrs array follow local_addrs array, so only way I see to do it not for each element is to allocate some tmp array, copy to it and then use unpack.

yes, can be next pr

iyastreb · 2025-09-30T14:12:31Z

src/tools/perf/cuda/ucp_cuda_kernel.cu

+            elems[i].local_addr  = (char*)perf.send_buffer + offset;
+            elems[i].remote_addr = perf.ucp.remote_addr + offset;
+            elems[i].length      = (i == count - 1) ? ONESIDED_SIGNAL_SIZE :
+                                                          perf.params.msg_size_list[i];


next pr: align

iyastreb · 2025-09-30T14:12:41Z

src/tools/perf/cuda/ucp_cuda_kernel.cu

+            local_offsets[i]  = 0;
+            remote_offsets[i] = 0;
+            lengths[i]        = (i == count - 1) ? ONESIDED_SIGNAL_SIZE :
+                                                          perf.params.msg_size_list[i];


next pr: align

iyastreb · 2025-09-30T14:13:52Z

src/tools/perf/cuda/ucp_cuda_kernel.cu


    void init_counters(const ucx_perf_context_t &perf)
    {
        m_params.length         = ucx_perf_get_message_size(&perf.params);


next pr: align

iyastreb · 2025-09-30T14:17:57Z

src/ucp/api/device/ucp_device_impl.h

- * for the memory transfer. The addresses and length must be valid for the used
- * @a mem_list entry.
+ * for the memory transfer. The @a local_offset and @a remote_offset parameters
+ * specify byte offsets within the selected memory list entry.The @a length,


Suggested change

* specify byte offsets within the selected memory list entry.The @a length,

* specify byte offsets within the selected memory list entry. The @a length,

iyastreb · 2025-09-30T14:21:03Z

src/ucp/api/device/ucp_device_impl.h

 * @tparam      level  Level of cooperation of the transfer.
 * @param [in]  req    Request containing operations in progress.
- *
+ * @param [in]  channel_id Channel ID to progress.


next pr: align

iyastreb · 2025-09-30T14:24:07Z

src/ucp/api/device/ucp_device_types.h

+        /**
+         * Array of local addresses for the device transfer operations.
+         */
+        void     **local_addr;


Wouldn't be more logical and cache friendly to have an array of struct?
So that the fields of certain element are stored together

We can do that, it will also help to unpack the fields when creating the handle.
But it will require changing UCT API as well...(it expects array of local & remote addresses). I didn't want to do it now because it's urgent.

ofirfarjun7 added 2 commits September 29, 2025 08:00

UCP/API: Work with offsets.

78d0469

UCP/API: Add channel id.

77101e4

ofirfarjun7 changed the title ~~UCP/API: Work with offsets.~~ UCP/API: Work with offsets and channel id. Sep 29, 2025

ofirfarjun7 added 3 commits September 29, 2025 09:26

UCP/API: Fix.

ad4a11d

UCP/API: Add lengths and change multi API.

0c6c60f

UCP/API: Improve doc.

a4fa0cc

ofirfarjun7 requested review from yosefe, iyastreb, Artemy-Mellanox and Sergei-Lebedev September 29, 2025 16:07

ofirfarjun7 added 5 commits September 29, 2025 16:23

UCP/API: Remove unused param.

78f365f

UCP/API: Check elements fields.

719cd6c

UCP/API: Better contorol post atomic.

497fc22

UCP/API: adapt cuda_ipc.

0cb09ac

UCP/API: Fix.

e7ae20a

michal-shalev reviewed Sep 29, 2025

View reviewed changes

src/ucp/api/device/ucp_device_impl.h Outdated Show resolved Hide resolved

Sergei-Lebedev reviewed Sep 29, 2025

View reviewed changes

michal-shalev reviewed Sep 29, 2025

View reviewed changes

src/tools/perf/cuda/ucp_cuda_kernel.cu Outdated Show resolved Hide resolved

ofirfarjun7 added 7 commits September 30, 2025 11:12

UCP/API: add channel id to progress.

43302a5

UCP/API: Fix static check comment.

ec4a087

UCP/API: Fix comments.

31efa17

UCP/DEVICE/API: Merge branch 'master' into topic/device-api-use-offsets

a22e889

UCP/API: Improve.

bd14203

UCP/API: Improve.

d930c60

UCP/API: Check for elements fields value.

feb8ed6

yosefe reviewed Sep 30, 2025

View reviewed changes

iyastreb reviewed Sep 30, 2025

View reviewed changes

UCP/API: CR comments.

89c3c5e

michal-shalev mentioned this pull request Sep 30, 2025

DEVICE/API: Change addresses to offsets and add channel_id ai-dynamo/nixl#815

Open

UCP/API: Fix cuda_ipc test.

4ca86c8

ofirfarjun7 added 4 commits October 1, 2025 07:39

UCP/API: CR comments.

fa285c3

UCP/API: Format.

c4d5dfb

UCP/API: Minor.

12f6f98

UCP/API: Minor improve.

fd4aadf

ofirfarjun7 requested review from Sergei-Lebedev and iyastreb October 1, 2025 08:38

ofirfarjun7 added 4 commits October 1, 2025 08:44

UCP/API: Minor.

375ba74

UCP/API: Bug fix.

c0d188f

UCP/API: Improve.

efcfd72

UCP/API: Temporary disable perftest checks.

70fdc08

iyastreb previously approved these changes Oct 6, 2025

View reviewed changes

UCP/API: Merge branch 'master' into topic/device-api-use-offsets

0de577e

ofirfarjun7 dismissed iyastreb’s stale review via 0de577e October 6, 2025 08:18

iyastreb approved these changes Oct 6, 2025

View reviewed changes

michal-shalev merged commit 1537aa9 into openucx:master Oct 6, 2025
141 checks passed

	* specify byte offsets within the selected memory list entry.The @a length,
	* specify byte offsets within the selected memory list entry. The @a length,

UCP/API: Work with offsets and channel id. #10920

UCP/API: Work with offsets and channel id. #10920

Conversation

ofirfarjun7 commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What?

Why?

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ofirfarjun7 Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ofirfarjun7 Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ofirfarjun7 commented Sep 29, 2025 •

edited

Loading

ofirfarjun7 Sep 30, 2025 •

edited

Loading

ofirfarjun7 Sep 30, 2025 •

edited

Loading