UCT/CUDA: Update cuda_copy perf estimates for Grace #10155

SeyedMir · 2024-09-17T22:51:57Z

What

Update cuda_copy perf estimates for Grace-Hopper

Why ?

The bandwidth and latency values will be different for PCIe versus C2C links that connect CPU and GPU.

How ?

Update the cuda_copy bw config and UCX_CUDA_COPY_BW.

src/uct/cuda/cuda_copy/cuda_copy_iface.c

ivankochin · 2024-09-19T06:46:22Z

src/uct/cuda/cuda_copy/cuda_copy_iface.c

+          ucs_offsetof(uct_cuda_copy_iface_config_t, bw.d2h)},
+         {"d2d", "device to device bandwidth",
+          ucs_offsetof(uct_cuda_copy_iface_config_t, bw.d2d)},
+         {"other", "any other src-dest memory types bandwidth",


Minor

Suggested change

{"other", "any other src-dest memory types bandwidth",

{"other", "any other memory types combinations bandwidth",

ivankochin · 2024-09-19T06:47:23Z

src/uct/cuda/cuda_copy/cuda_copy_iface.c

-     ucs_offsetof(uct_cuda_copy_iface_config_t, bandwidth), UCS_CONFIG_TYPE_BW},
+    /* TODO: 1. Add separate keys for shared and dedicated bandwidth
+             2. Remove the "other" key (use pref_loc for managed memory) */
+    {"BW", "h2d:8300MBs,d2h:11660MBs,d2d:320GBs,other:10000MBs",


Is that possible to remove other and define value for other memory types combinations through value without label?

Suggested change

{"BW", "h2d:8300MBs,d2h:11660MBs,d2d:320GBs,other:10000MBs",

{"BW", "10000MBs,h2d:8300MBs,d2h:11660MBs,d2d:320GBs",

Akshay-Venkatesh · 2024-09-26T17:16:58Z

src/uct/cuda/cuda_copy/cuda_copy_iface.h

@@ -87,7 +92,12 @@ typedef struct uct_cuda_copy_iface_config {
    uct_iface_config_t      super;
    unsigned                max_poll;
    unsigned                max_cuda_events;
-    double                  bandwidth;
+    struct {


@SeyedMir why not use bw[UCS_MEMORY_TYPE_LAST][UCS_MEMORY_TYPE_LAST] and avoid explicit fields for each direction? This way we don't have to introduce other field and we can populate the bandwidths by referring to the specific source-destination combination. For example, replace:

{"h2d", "host to device bandwidth", ucs_offsetof(uct_cuda_copy_iface_config_t, bw.h2d)}, ...

with

{"h2d", "host to device bandwidth", ucs_offsetof(uct_cuda_copy_iface_config_t, bw.[UCS_MEMORY_TYPE_UNKNOWN][UCS_MEMORY_TYPE_CUDA])},

I thought about doing it that way initially in fact. But, then I thought that bw matrix will have entries for types that are completely irrelevant to CUDA, and there will be much more entries than the four in this struct. Having said that, I'm not strongly against using the matrix.

maybe define a common struct type to also use it in uct_cuda_copy_iface_t?

brminich · 2024-11-27T17:00:43Z

src/uct/cuda/cuda_copy/cuda_copy_iface.c

+          ucs_offsetof(uct_cuda_copy_iface_config_t, bw.d2h)},
+         {"d2d", "device to device bandwidth",
+          ucs_offsetof(uct_cuda_copy_iface_config_t, bw.d2d)},
+         {"other", "any other memory types combinations bandwidth",


no need for other key - you can set default value without any key (see https://github.com/openucx/ucx/blob/master/src/uct/cuda/gdr_copy/gdr_copy_iface.c#L27)

brminich · 2024-11-27T17:01:14Z

src/uct/cuda/cuda_copy/cuda_copy_iface.c

-     "Effective memory bandwidth",
-     ucs_offsetof(uct_cuda_copy_iface_config_t, bandwidth), UCS_CONFIG_TYPE_BW},
+    /* TODO: 1. Add separate keys for shared and dedicated bandwidth
+             2. Remove the "other" key (use pref_loc for managed memory) */


other can be removed

@SeyedMir do we still need "other"?
maybe add h2h explicitly?
or rename it to "default"?

We need "other" because the existing code has a branch for each of h2d, d2h, d2d, and "else". The bw value set for h2d and "else" are not the same. My understanding is that we don't want this PR to change the already existing bw settings for non-grace platforms.
I can change it from "other" to "default", but I personally find "default" a bit confusing because the default value of the keys are set in line 42.

copying my old comment here

no need for other key - you can set default value without any key (see https://github.com/openucx/ucx/blob/master/src/uct/cuda/gdr_copy/gdr_copy_iface.c#L27)

@brminich
as @SeyedMir already mentioned: we need a key to set the value, that is used to initialize bw performance attribute in "else" case (not in [h2d, d2h, d2d]).

You can set default values for the existing tags. E.g. UCX_CUDA_COPY_BW=12345 sets 12345 to all declared tags.

We have four values to configure in the existing code of performance attributes query. If we want to keep old behavior, and just extend it, then we need to add four tags to the existing configuration variable.

ok with 4-th tag then. IMO either is ok (other or default)

@SeyedMir can you pls change "other" to "default"?
the Grace config can be probably specified as
UCX_CUDA_COPY_BW=800GBs,d2d:3TBs

Can't use default because it's a keyword. What else do you suggest?

brminich · 2024-11-27T17:03:54Z

src/uct/cuda/cuda_copy/cuda_copy_iface.c

@@ -134,7 +144,7 @@ static ucs_status_t uct_cuda_copy_iface_query(uct_iface_h tl_iface,

    iface_attr->latency                 = UCT_CUDA_COPY_IFACE_LATENCY;
    iface_attr->bandwidth.dedicated     = 0;
-    iface_attr->bandwidth.shared        = iface->config.bandwidth;
+    iface_attr->bandwidth.shared        = iface->config.bw.other;


maybe we can use just constant here to preserve wire compatibility and avoid adding other key?
@yosefe, wdyt?

@brminich I removed the other key and added a constant instead.

src/uct/cuda/cuda_copy/cuda_copy_iface.c

brminich · 2025-01-31T12:26:22Z

src/uct/cuda/cuda_copy/cuda_copy_iface.c

@@ -407,16 +416,17 @@ uct_cuda_copy_estimate_perf(uct_iface_h tl_iface, uct_perf_attr_t *perf_attr)
        perf_attr->bandwidth.dedicated = 0;
        if ((src_mem_type == UCS_MEMORY_TYPE_HOST) &&
            (dst_mem_type == UCS_MEMORY_TYPE_CUDA)) {
-            perf_attr->bandwidth.shared = (zcopy ? 8300.0 : 7900.0) * UCS_MBYTE;
+            perf_attr->bandwidth.shared = zcopy ? iface->config.bw.h2d :
+                                          iface->config.bw.h2d * 0.95;


does 0.95 come from overhead of cuStreamSynchronize for short protocols?

maybe smth like
stream_sync_factor = zcopy ? 1 : 0.95;
...
perf_attr->bandwidth.shared = iface->config.bw.h2d * stream_sync_factor;

yosefe · 2025-02-24T16:11:44Z

/azp run UCX PR

azure-pipelines · 2025-02-24T16:12:09Z

Azure Pipelines successfully started running 1 pipeline(s).

config/ucx.conf

Akshay-Venkatesh · 2025-02-24T21:59:27Z

src/uct/cuda/cuda_copy/cuda_copy_iface.c

-     "Effective memory bandwidth",
-     ucs_offsetof(uct_cuda_copy_iface_config_t, bandwidth), UCS_CONFIG_TYPE_BW},
+    /* TODO: Add separate keys for shared and dedicated bandwidth */
+    {"BW", "h2d:8300MBs,d2h:11660MBs,d2d:320GBs",


@SeyedMir can we introduce default field to ensure managed memory pipelines see c2c perf and not current default of 10 GB/s

Added the other key back to get c2c perf for managed memory. Also updated config values.
@brminich and @yosefe please let me know what you think.

src/uct/cuda/cuda_copy/cuda_copy_iface.c

SeyedMir · 2025-03-03T15:34:46Z

@yosefe @brminich some tests failed and they don't seem relevant to the changes in this PR AFAICT.

SeyedMir · 2025-03-12T17:26:31Z

@yosefe please let me know if you have more comments or I should squash.

src/uct/cuda/cuda_copy/cuda_copy_iface.c

yosefe · 2025-03-16T08:54:09Z

@SeyedMir pls squash

UCT/CUDA: Update cuda_copy perf estimates for Grace

SeyedMir requested review from ivankochin and brminich September 17, 2024 22:57

SeyedMir force-pushed the cuda-estimate-perf-conf branch from 9f8dc24 to f6964ee Compare September 18, 2024 17:24

ivankochin reviewed Sep 19, 2024

View reviewed changes

src/uct/cuda/cuda_copy/cuda_copy_iface.c Outdated Show resolved Hide resolved

ivankochin reviewed Sep 19, 2024

View reviewed changes

Akshay-Venkatesh reviewed Sep 26, 2024

View reviewed changes

SeyedMir force-pushed the cuda-estimate-perf-conf branch from 82eaa85 to b8ef03b Compare October 4, 2024 14:48

brminich reviewed Nov 27, 2024

View reviewed changes

SeyedMir force-pushed the cuda-estimate-perf-conf branch 2 times, most recently from 08e9d57 to ca7e266 Compare January 31, 2025 00:46

brminich reviewed Jan 31, 2025

View reviewed changes

brminich previously approved these changes Feb 24, 2025

View reviewed changes

SeyedMir dismissed brminich’s stale review via 620087b February 24, 2025 17:51

Akshay-Venkatesh reviewed Feb 24, 2025

View reviewed changes

SeyedMir force-pushed the cuda-estimate-perf-conf branch from 2de18cf to 62aef2f Compare February 28, 2025 16:07

rakhmets previously approved these changes Mar 1, 2025

View reviewed changes

src/uct/cuda/cuda_copy/cuda_copy_iface.c Outdated Show resolved Hide resolved

SeyedMir changed the title ~~UCT/CUDA: Update cuda_copy perf estimates for Grace-Hopper~~ UCT/CUDA: Update cuda_copy perf estimates for Grace Mar 3, 2025

SeyedMir dismissed rakhmets’s stale review via ccb30da March 3, 2025 16:26

SeyedMir force-pushed the cuda-estimate-perf-conf branch from ccb30da to 935719d Compare March 4, 2025 17:24

yosefe reviewed Mar 13, 2025

View reviewed changes

src/uct/cuda/cuda_copy/cuda_copy_iface.c Outdated Show resolved Hide resolved

SeyedMir force-pushed the cuda-estimate-perf-conf branch from c254298 to b3d018b Compare March 14, 2025 15:11

rakhmets approved these changes Mar 14, 2025

View reviewed changes

brminich approved these changes Mar 14, 2025

View reviewed changes

UCT/CUDA: Update cuda_copy perf estimates for Grace

adc9897

SeyedMir force-pushed the cuda-estimate-perf-conf branch from b3d018b to adc9897 Compare March 17, 2025 15:46

yosefe approved these changes Mar 18, 2025

View reviewed changes

yosefe enabled auto-merge March 18, 2025 06:13

yosefe merged commit e27378e into openucx:master Mar 19, 2025
151 checks passed

rakhmets mentioned this pull request Mar 19, 2025

UCT/CUDA_COPY: add multi-device support in cuda_copy #9645

Merged

SeyedMir pushed a commit to SeyedMir/ucx that referenced this pull request Mar 20, 2025

Merge pull request openucx#10155 from SeyedMir/cuda-estimate-perf-conf

6069ee1

UCT/CUDA: Update cuda_copy perf estimates for Grace

yosefe mentioned this pull request Mar 21, 2025

UCT/CUDA: Update cuda_copy perf estimates for Grace - v1.18.x #10563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCT/CUDA: Update cuda_copy perf estimates for Grace #10155

UCT/CUDA: Update cuda_copy perf estimates for Grace #10155

SeyedMir commented Sep 17, 2024

ivankochin Sep 19, 2024

ivankochin Sep 19, 2024

Akshay-Venkatesh Sep 26, 2024 •

edited

Loading

SeyedMir Sep 26, 2024

yosefe Feb 24, 2025

brminich Nov 27, 2024

brminich Nov 27, 2024

yosefe Feb 26, 2025

SeyedMir Feb 26, 2025

brminich Feb 27, 2025

rakhmets Mar 1, 2025

brminich Mar 2, 2025

yosefe Mar 5, 2025

SeyedMir Mar 5, 2025

brminich Nov 27, 2024

SeyedMir Jan 31, 2025

brminich Jan 31, 2025

SeyedMir Feb 4, 2025

yosefe Feb 24, 2025

yosefe commented Feb 24, 2025

azure-pipelines bot commented Feb 24, 2025

Akshay-Venkatesh Feb 24, 2025

SeyedMir Feb 25, 2025

SeyedMir commented Mar 3, 2025

SeyedMir commented Mar 12, 2025

yosefe commented Mar 16, 2025

	{"other", "any other src-dest memory types bandwidth",
	{"other", "any other memory types combinations bandwidth",

	{"BW", "h2d:8300MBs,d2h:11660MBs,d2d:320GBs,other:10000MBs",
	{"BW", "10000MBs,h2d:8300MBs,d2h:11660MBs,d2d:320GBs",

UCT/CUDA: Update cuda_copy perf estimates for Grace #10155

UCT/CUDA: Update cuda_copy perf estimates for Grace #10155

Conversation

SeyedMir commented Sep 17, 2024

What

Why ?

How ?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akshay-Venkatesh Sep 26, 2024 • edited Loading

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

yosefe commented Feb 24, 2025

azure-pipelines bot commented Feb 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeyedMir commented Mar 3, 2025

SeyedMir commented Mar 12, 2025

yosefe commented Mar 16, 2025

Akshay-Venkatesh Sep 26, 2024 •

edited

Loading