New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

spml/ucx: shuffle EPs creation #12907

Open

michal-shalev wants to merge 5 commits into open-mpi:main from michal-shalev:shuffle-eps-creation

michal-shalev commented Nov 4, 2024

What?

This PR randomize the order in which endpoints (EPs) are created in the mca_spml_ucx_add_procs function. Each new EP is placed at a random position instead of cyclic order.

Why?

Recently, a customer raised concerns about incast behavior during SHMEM quiet operations, where many EPs communicating in a fixed order could lead to network congestion and performance degradation. They believe that randomizing the order of EPs could reduce the likelihood of incast collisions.
The customer is interested in testing a patch to address this.

How?

Used the Fisher-Yates shuffle algorithm to randomize indices and modified the loop to handle all indices, including 0, within a single pass.

github-actions bot added the Target: main label

github-actions bot commented Nov 4, 2024

Hello! The Git Commit Checker CI bot found a few problems with this PR:

b734683: spml/ucx: shuffle EPs creation

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

michal-shalev force-pushed the shuffle-eps-creation branch from b734683 to 4d562ac Compare

November 4, 2024 03:29

github-actions bot commented Nov 4, 2024

Hello! The Git Commit Checker CI bot found a few problems with this PR:

4d562ac: spml/ucx: shuffle EPs creation

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

michal-shalev force-pushed the shuffle-eps-creation branch from 4d562ac to 2345c1f Compare

November 4, 2024 03:30

github-actions bot commented Nov 4, 2024

Hello! The Git Commit Checker CI bot found a few problems with this PR:

2345c1f: spml/ucx: shuffle EPs creation

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

michal-shalev force-pushed the shuffle-eps-creation branch from 2345c1f to 489dbcf Compare

November 4, 2024 03:31

github-actions bot commented Nov 4, 2024

Hello! The Git Commit Checker CI bot found a few problems with this PR:

489dbcf: spml/ucx: shuffle EPs creation

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

michal-shalev force-pushed the shuffle-eps-creation branch from 489dbcf to eb303bf Compare

November 4, 2024 03:32

github-actions bot commented Nov 4, 2024

Hello! The Git Commit Checker CI bot found a few problems with this PR:

eb303bf: spml/ucx: shuffle EPs creation

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

michal-shalev force-pushed the shuffle-eps-creation branch from eb303bf to 25c6b86 Compare

November 4, 2024 03:33

github-actions bot commented Nov 4, 2024

Hello! The Git Commit Checker CI bot found a few problems with this PR:

25c6b86: spml/ucx: shuffle EPs creation

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

michal-shalev force-pushed the shuffle-eps-creation branch from 25c6b86 to 893a73e Compare

November 4, 2024 03:36

github-actions bot commented Nov 4, 2024

Hello! The Git Commit Checker CI bot found a few problems with this PR:

893a73e: spml/ucx: shuffle EPs creation

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

michal-shalev force-pushed the shuffle-eps-creation branch from 893a73e to 00610cc Compare

November 4, 2024 03:41


          spml/ucx: shuffle EPs creation

03c59cd

Signed-off-by: Michal Shalev <mshalev.nvidia.com>

michal-shalev force-pushed the shuffle-eps-creation branch from 00610cc to 03c59cd Compare

November 4, 2024 06:43

gleon99 reviewed

View reviewed changes

oshmem/mca/spml/ucx/spml_ucx.c Show resolved Hide resolved

tvegas1 reviewed

View reviewed changes

Contributor

tvegas1 left a comment

minor comments

oshmem/mca/spml/ucx/spml_ucx.c Outdated

                       }
                   }
+                  indices = malloc(nprocs * sizeof(int));

Contributor

tvegas1 Nov 4, 2024

minor: sizeof(*indices)

Author

michal-shalev Nov 6, 2024

done

oshmem/mca/spml/ucx/spml_ucx.c Outdated

                   /* Get the EP connection requests for all the processes from modex */
-                  for (n = 0; n < nprocs; ++n) {
-                      i = (my_rank + n) % nprocs;
+                  for (i = nprocs - 1; i >= 0; --i) {

Contributor

tvegas1 Nov 4, 2024

maybe perform randomization as a separate function/loop

Author

michal-shalev Nov 6, 2024

The idea here is to iterate over the EPs once, and to save another iteration

Contributor

tvegas1 Nov 6, 2024

ok for me, but i guess one extra loop on sequential memory on ep creation would not impact perf

oshmem/mca/spml/ucx/spml_ucx.c Outdated

    
                      err = ucp_ep_create(mca_spml_ucx_ctx_default.ucp_worker[0], &ep_params,

                              &mca_spml_ucx_ctx_default.ucp_peers[i].ucp_conn);

                                          &mca_spml_ucx_ctx_default.ucp_peers[indices[i]].ucp_conn);

                      if (UCS_OK != err) {

                          SPML_UCX_ERROR("ucp_ep_create(proc=%zu/%zu) failed: %s", n, nprocs,

Contributor

tvegas1 Nov 4, 2024

also update log line index (i and indices[i])

Author

michal-shalev Nov 6, 2024

Notice I'm using proc_index instead of i.
I changed n to proc_index, why indices[proc_index]? the loop still iterates until nproc.

Contributor

tvegas1 Nov 6, 2024

ah right, already up-to-date. we could add indices[proc_index] to the log along with iteration since it is randomized.

Author

michal-shalev Nov 13, 2024

done

oshmem/mca/spml/ucx/spml_ucx.c Show resolved Hide resolved

michal-shalev added 2 commits

November 5, 2024 09:59


          spml/ucx: fix for loop

78d0a63

Signed-off-by: Michal Shalev <mshalev.nvidia.com>


          spml/ucx: PR fixes

32a86a4

Signed-off-by: Michal Shalev <mshalev.nvidia.com>

michal-shalev requested review from tvegas1 and gleon99

November 6, 2024 01:30

gleon99 reviewed

View reviewed changes

oshmem/mca/spml/ucx/spml_ucx.c

                   ucp_address_t **wk_local_addr;
                   unsigned int *wk_addr_len;
                   ucp_ep_params_t ep_params;
+                  int *indices;

Contributor

gleon99 Nov 7, 2024

unsigned int?

Author

michal-shalev Nov 13, 2024

Please notice proc_index >= 0;

Contributor

gleon99 Nov 13, 2024

Exactly..

oshmem/mca/spml/ucx/spml_ucx.c Outdated Show resolved Hide resolved

oshmem/mca/spml/ucx/spml_ucx.c Outdated Show resolved Hide resolved

oshmem/mca/spml/ucx/spml_ucx.c Outdated Show resolved Hide resolved

michal-shalev added 2 commits

November 13, 2024 11:09


          spml/ucx: fix swap

6dc87d1

Signed-off-by: Michal Shalev <mshalev.nvidia.com>


          spml/ucx: PR fixes 2.0

2ad0f14

Signed-off-by: Michal Shalev <mshalev.nvidia.com>

michal-shalev force-pushed the shuffle-eps-creation branch from c421b4a to 2ad0f14 Compare

November 13, 2024 09:10

michal-shalev requested a review from gleon99

November 13, 2024 09:10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels