Skip to content
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

Add proc fix/v2.x #3012

Merged
merged 2 commits into from
Mar 1, 2017
Merged

Add proc fix/v2.x #3012

merged 2 commits into from
Mar 1, 2017

Conversation

artpol84
Copy link
Contributor

No description provided.

Ralph Castain and others added 2 commits February 22, 2017 07:41
@artpol84
Never collect data when doing the fence at the end of MPI_Init
10a0d0f 
Signed-off-by: Ralph Castain <rhc@open-mpi.org>
(cherry picked from commit 114e20a)

Conflicts:
	ompi/runtime/ompi_mpi_init.c
@artpol84
ompi: Avoid unnecessary PMIx lookups when adding procs.
9ae3aa0 
Signed-off-by: Artem Polyakov <artpol84@gmail.com>

(cherry-picked from 9f4464a)
@artpol84
Copy link
Contributor Author

Need in 9ae3aa0 explained in PR #3011.
On the same system 10a0d0f helps to reduce barrier time from 170ms to 69 ms with SLURM plugin.

@artpol84 artpol84 self-assigned this Feb 22, 2017
@artpol84 artpol84 modified the milestones: v2.x, v2.1.0 Feb 22, 2017
@artpol84
Copy link
Contributor Author

@hjelmn give this a try when you'll be testing at scale. May help 2.x to look better.

@hjelmn
Copy link
Member

hjelmn commented Feb 22, 2017 •
edited
Loading

Yeah, was only testing /bin/true. This is a performance bug that has existed since we made the add_procs changes. 👍 Please PR to 2.0.x if possible.

@artpol84
Copy link
Contributor Author

bot:mellanox:retest

hjelmn
hjelmn approved these changes Feb 22, 2017
View reviewed changes
Copy link
Member

@hjelmn hjelmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for fixing this.

rhc54
rhc54 requested changes Feb 22, 2017
View reviewed changes
Copy link
Contributor

@rhc54 rhc54 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we put this into the release branch, there are a couple of places that were missed. Let's get them correct in master and then worry about the PR to the release.

@jsquyres
Copy link
Member

This is not a blocker for v2.1.0.

@artpol84
Copy link
Contributor Author

It is, for us at least.

@jsquyres jsquyres modified the milestones: v2.1.0, v2.1.1 Feb 27, 2017
jjhursey
jjhursey approved these changes Feb 28, 2017
View reviewed changes
Copy link
Member

@jjhursey jjhursey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I'd like to see PR #3011 be merged into master first.

rhc54
rhc54 approved these changes Feb 28, 2017
View reviewed changes
Copy link
Contributor

@rhc54 rhc54 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will address the other locations for a later release

@rhc54
Copy link
Contributor

rhc54 commented Feb 28, 2017

Can come across after the master PR has a chance to cycle thru nightly MTT

@jsquyres
Copy link
Member

jsquyres commented Mar 1, 2017

It looks like #3011 went through ok last night in MTT. Merging this to v2.x.

@jsquyres jsquyres merged commit 5d2e5eb into open-mpi:v2.x Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hjelmn hjelmn hjelmn approved these changes

@rhc54 rhc54 rhc54 approved these changes

@jjhursey jjhursey jjhursey approved these changes

Assignees

@artpol84 artpol84

Labels
bug enhancement RM approved Severity: critical
Projects
None yet
Milestone
v2.1.1
Development

Successfully merging this pull request may close these issues.
6 participants
@artpol84 @hjelmn @jsquyres @rhc54 @jjhursey @jladd-mlnx