Skip to content

Conversation

@hjelmn
Copy link
Member

@hjelmn hjelmn commented Oct 12, 2016

In v2.0.x we added new memory hooks to replace ptmalloc2. We let the old hooks stick around but disabled them by default. We agreed to remove the hooks entirely in v2.1.x. If that is still the case please merge this PR.

Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
Because of the removal of the linux memory component it is no longer
necessary to initialize the memory component in opal_init(). This
commit moves the initialization to the creation of the first rcache
component.

Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
(cherry picked from commit c2b6fbb)
Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
This commit fixes an error in the failure path of leave_pinned. When
the rcache tries to enable leave_pinned but leave_pinned was not
specifically requested (opal_leave_pinned == -1) the code was
erroneously printing an error and returning NULL.

Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
(cherry picked from commit 25a97af)
Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
available, this will cause memory corruption. Refuse to run.
Right now, ptmalloc2 is the only memory manager that we have on
OS's that support OpenFabrics that provide both FREE and MUNMAP
support, so the following test is [currently] good enough... */
Copy link
Member

Choose a reason for hiding this comment

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

@hjelmn You just deleted the ptmalloc component, so is this still needed?

Copy link
Member

Choose a reason for hiding this comment

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

And it may not be true anymore.

@jsquyres
Copy link
Member

@hppritcha and I discussed: this is a code cleanup PR, and is ok for v2.1.0.

@jsquyres
Copy link
Member

bot:lanl:retest

@jsquyres
Copy link
Member

@gpaulsen Can IBM test / review this?

Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

Please add a Signed-off-by line to this PR's commit.

@hppritcha
Copy link
Member

bot:lanl:retest

jsquyres
jsquyres previously approved these changes Nov 7, 2016
Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

All commits signed off. Thanks!

@jsquyres jsquyres dismissed their stale review November 7, 2016 20:38

Waiting for IBM's review

@gpaulsen
Copy link
Member

Well, I read the code, but didn't fully understand it. I'll download it and give it a whirl.

@gpaulsen
Copy link
Member

gpaulsen commented Dec 6, 2016

Sorry, getting back to this. I'll do this today.

@jsquyres jsquyres requested a review from gpaulsen January 24, 2017 19:48
jsquyres added a commit to jsquyres/ompi that referenced this pull request Jan 24, 2017
Follow on to open-mpi#2217.

Signed-off-by: Jeff Squyres <jsquyres@cisco.com>
Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

Review addressed in #2184.

@jsquyres jsquyres merged commit 5a9ba57 into open-mpi:v2.x Jan 24, 2017
@jsquyres
Copy link
Member

Correction: review addressed in #2804.

Copy link
Member

@gpaulsen gpaulsen left a comment

Choose a reason for hiding this comment

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

+1

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants