Skip to content

Fix some issues with dynamic algorithm selection in coll/tuned #8198

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
Merged
jsquyres merged 5 commits into from
Nov 12, 2020
Merged

Fix some issues with dynamic algorithm selection in coll/tuned #8198

jsquyres merged 5 commits into from
Nov 12, 2020

Conversation

@devreal
Copy link
Contributor

@devreal devreal commented Nov 10, 2020

This PR addresses a potential performance issue with the algorithm selection in coll/tuned and some minor issues found while digging into it:

  1. Performance: some bcast and allreduce algorithms require the number of elements to be larger than the number of ranks and will fall back to a linear implementation if that is not the case. This hit me bitterly with 4B bcast on 128 ranks on a single node where latency increased 10x compared to 127 ranks.
  2. The documentation for the coll_tuned_*_algorithm MCA variables should mention that they only take effect if the coll_tuned_use_dynamic_rules variable is set to true.
  3. Mark global static array used for the MCA parameters as const.
  4. Fix some glitches in comments.

Backport of #8186 to v4.1.x

@devreal
coll/tuned: add hint about dynamic rules to mca parameters
a9ede52 
The mca parameters coll_tuned_*_algorithm are ignored unless coll_tuned_use_dynamic_rules is true so mention that in the description.

Signed-off-by: Joseph Schuchart <[email protected]>
(cherry picked from commit 06f605c)
@devreal
coll/tuned: Mark global static algorithm as const
4a3f2af 
Signed-off-by: Joseph Schuchart <[email protected]>
(cherry picked from commit 7261255)
@devreal
coll/tuned: don't select algorithms knowing when it's clear they woul…
5e19de8 
…d fall back to linear

Bcast: scatter_allgather and scatter_allgather_ring expect N_elem >= N_procs
Allreduce: rabenseifner expects N_elem >= pow2 nearest to N_procs

In all cases, the implementations will fall back to a linear implementation,
which will most likely yield the worst performance (noted for 4B bcast on 128 ranks)

Signed-off-by: Joseph Schuchart <[email protected]>
(cherry picked from commit 04d198f)
@devreal
Copy link
Contributor Author

devreal commented Nov 11, 2020

I added a commit that removes the selection of linear algorithms in allreduce and allgather. In my measurements the latency for these ranges is higher than necessary and I don't see how that is motivated by previous measurements (it seems unlikely to me that linear algorithms perform well at several dozens or hundreds of ranks).

@rajachan
Copy link
Member

rajachan commented Nov 11, 2020

@devreal ICYMI, something was off with allgatherv too (I'd tested with the 4.1.x branch #8186 (comment)). Is that something you are seeing?

@devreal
Copy link
Contributor Author

devreal commented Nov 11, 2020

@rajachan I have not yet looked at allgatherv. I can run some tests for that over night and see. Do remember at what scales things were weird?

@rajachan
Copy link
Member

rajachan commented Nov 11, 2020

I was running with ~1K ranks (32 nodes with 36 ranks per node).

@rajachan
Copy link
Member

rajachan commented Nov 11, 2020

Btw, your master PR is missing the allreduce/allgather commit.

@devreal
Copy link
Contributor Author

devreal commented Nov 11, 2020

Btw, your master PR is missing the allreduce/allgather commit.

Oops, pushed to the wrong branch. Will fix in a minute

@wckzhang
Copy link
Contributor

wckzhang commented Nov 11, 2020

Btw, your master PR is missing the allreduce/allgather commit.

Nice catch

@devreal
coll/tuned: fix minor errors in comments
aec55f1 
Signed-off-by: Joseph Schuchart <[email protected]>
(cherry picked from commit 22e289b)
@devreal
COLL TUNED: remove stray selection of linear algs for alreduce and al…
3cae9f7 
…lgather

These selections seem harmful in my measurements and don't seem to be
motivated by previous measurement data.

Signed-off-by: Joseph Schuchart <[email protected]>
(cherry picked from commit a15e5dc)
@devreal devreal force-pushed the fix-tuned-dynamic-v4.1.x branch from 0f89397 to 3cae9f7 Compare November 11, 2020 17:43
@jsquyres jsquyres merged commit 6f21a39 into open-mpi:v4.1.x Nov 12, 2020
@it-is-a-robot it-is-a-robot mentioned this pull request Nov 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bosilca bosilca bosilca approved these changes

@rajachan rajachan Awaiting requested review from rajachan

+1 more reviewer

@wckzhang wckzhang wckzhang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RM approved Target: v4.1.x

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@devreal @rajachan @wckzhang @bosilca @jsquyres