opal/memory: Move Memory Allocation Hooks usage from openib #967

igor-ivanov · 2016-02-15T10:06:12Z

See related PR in ompi/trunk open-mpi/ompi#1351

:bot:assign: @jsquyres
:bot🏷️bug
:bot:milestone:v2.0.0

ompi-trunk:
open-mpi/ompi@d9eefef
open-mpi/ompi@477991b
open-mpi/ompi@5c685e23

mellanox-github · 2016-02-15T10:43:01Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1339/ for details.

jsquyres · 2016-02-15T15:07:20Z

Per open-mpi/ompi#1351 (diff), this PR contains an abstraction violation that needs to be fixed. 👎

igor-ivanov · 2016-02-15T15:26:14Z

@jsquyres could you please propose correct way.

jsquyres · 2016-02-15T16:03:02Z

Typically, you have to do these kinds of things via indirection (e.g., via the framework base). This will also allow you to get rid of the #if in the BTL code.

mellanox-github · 2016-02-16T10:22:27Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1345/ for details.

igor-ivanov · 2016-02-16T11:06:33Z

@jsquyres could you check this update

jsquyres · 2016-02-16T14:19:18Z

opal/mca/btl/base/btl_base_select.c

@@ -107,6 +122,16 @@ int mca_btl_base_select(bool enable_progress_threads,
                                "select: no init function; ignoring component %s",
                                component->btl_version.mca_component_name);
        } else {
+#if MEMORY_LINUX_MALLOC_ALIGN_ENABLED
+            if (strcmp(component->btl_version.mca_component_name, "openib") == 0) {


You can't put a check for a specific component in a framework base.

Ah -- I see the problem: I think I wasn't clear in my comment to you before about doing this "through the framework base." Sorry. It was clear in my head! 😉 Here's what I meant:

I was actually referring to the opal/mca/memory base (not the BTL base). I.e., make an OPAL memory framework function to set a memalign hook. This function would be located in opal/mca/memory/base, and openib (and oshmem, and... whoever else needs it) can just call it directly, without needing to directly include opal/mca/memory/linux/something.h or directly invoke a function in opal/mca/memory/linux. Instead, the memory/base function will lookup and see if the selected memory component supports setting a memalign hook, and if so, set it (e.g., perhaps by calling a new member function on the memory module structure). The linux memory hook component will support this functionality; I don't know if the solaris one will (i.e., it needs to be safe for the solaris memory component to not support this functionality).

Does that make more sense?

@jsquyres thank you for sharing details. Actually my initial local version did something you described. Reason I did not push one was in necessity to use #ifdef inside memory/base that relates memory/linux component only. It seems that solaris does not use hooks.

Fair enough. Perhaps this could be exposed as a function pointer on the module -- hence, the solaris module can provide a NULL pointer, and the linux one can provide a non-NULL pointer.

jsquyres · 2016-02-16T14:19:45Z

@igor-ivanov Can you please revert the master commit? The abstraction violation is causing problems for others: http://www.open-mpi.org/community/lists/devel/2016/02/18598.php. Thanks.

jsquyres · 2016-02-16T14:24:08Z

opal/mca/memory/linux/memory_linux_component.c

+    mca_memory_linux_component.use_memalign = -1;
+    ret = mca_base_component_var_register(&mca_memory_linux_component.super.memoryc_version,
+                                 "memalign",
+                                 "[64 | 32 | 0] - Enable memory alignment for all malloc calls (default: disabled).",


Is there a reason not to use an MCA var enumeration here?

seems @ggouaillardet solved issue in master. Thanks.

only reason is to minimize changes with original code. Moreover using enumeration requires additional variable.

True, but this is exactly the case that enumerated MCA vars were intended for.

hppritcha · 2016-02-16T15:06:24Z

Any idea why Jenkins/Travis did not catch this?

@igor-ivanov Can you please revert the master commit? The abstraction violation is causing problems for others: http://www.open-mpi.org/community/lists/devel/2016/02/18598.php. Thanks.

—
Reply to this email directly or view it on GitHub.

jsquyres · 2016-02-16T18:16:55Z

I think @ggouaillardet did a workaround on master that's probably good enough for the moment, but the final result should probably be synchronized between master and v2.x.

igor-ivanov · 2016-02-17T15:21:04Z

@jsquyres Could you advice.
There is memory/linux component ant it looks as suitable for keeping malloc hook operation. Now memory/linux component can be disabled in case --with-memory-manager=no is selected but we would like to have hook operation under Linux despite this (original openib is able to support this capability in both situations). So impementing this stuff inside memory/linux does not help completely.
I see a moving mca variables and hook operations on memory/base layer as a way that could solve this issue (note: there is 'empty' component inside 'base' layer). configure check if memory hook exists should be moved to memory top layer also.
Do you see any abstraction violation issues in this approach. What is your opinion.

hppritcha · 2016-02-17T18:13:29Z

Does this also need to go in to 1.10.X?

…t_alignment

ggouaillardet · 2016-02-18T11:00:58Z

once open-mpi/ompi#1376 is merged, my last three commits can be cherry-picked into this PR and it should be good to go

igor-ivanov · 2016-02-18T11:32:51Z

One point here.
Original code (before open-mpi/ompi#1351) was able to use aligned malloc in case no memory-manager is enabled (actually memory/empty is activated that is hidden in memory/base). This ability was lost after the PR (my apologies). See #967 (comment).
I have no idea how to solve this w/o breaking abstraction. There is 'linux' component that is suitable for this but it can be disabled. I can not add mostly linux specific feature in 'empty' component too.

ggouaillardet · 2016-02-18T11:56:38Z

if memory alignment is not specific to the Linux component, then it should be moved to base.
an other option is to write an other minimal component that does alignment only.

igor-ivanov · 2016-02-18T12:09:19Z

minimal component can be disabled as --memory-manager=none up layer option. So it seems not good way. As for the second option that I asked @jsquyres to check.

ggouaillardet · 2016-02-18T12:55:00Z

well, if the only role of the minimal memory manager is to align memory, then you cannot have memory aligned with --memory-manager=none
that sounds like a feature to me...

igor-ivanov · 2016-02-18T15:18:20Z

After looking at old openib code more I realized that malloc hook did not work w/o memory manager. Setting such hook was controled by opal_mem_hooks_support_level(). Only memory/linux and memory/malloc_solaris set hooks support level calling opal_mem_hooks_set_support().
So changes from @ggouaillardet should be enough.

mike-dubman · 2016-02-19T06:58:13Z

@hppritcha - yes, we would like to port it into v1.10.x once ready.

mike-dubman · 2016-02-28T07:24:46Z

👍

lanl-ompi · 2016-02-29T07:07:02Z

Test FAILed.

These changes fix issue open-mpi/ompi#1336 - improve abstractions: opal/memory/linux component should be single place that opeartes with Memory Allocation Hooks. - avoid collisions in case dynamic component open/close: it is safe because it is linked statically. - does not change original behaivour.

…t_alignment

mellanox-github · 2016-02-29T07:52:20Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1382/ for details.

mike-dubman · 2016-02-29T08:38:55Z

👍

mike-dubman · 2016-02-29T08:39:09Z

👎

mike-dubman · 2016-02-29T08:39:17Z

👍

mike-dubman · 2016-02-29T08:39:26Z

coool :)

igor-ivanov · 2016-02-29T08:54:05Z

@ggouaillardet could you check if you see any issue

jsquyres · 2016-03-01T23:34:24Z

@igor-ivanov I think this looks good; nice job. I'd still like to see the MCA enumeration, though.

@ggouaillardet Does it look good to you?

ggouaillardet · 2016-03-02T05:28:24Z

@jsquyres so far so good, i made open-mpi/ompi#1420 for the MCA enumeration

hppritcha · 2016-03-02T15:06:37Z

@jsquyres i pronounce ready to go.

jsquyres · 2016-03-02T16:24:25Z

@hppritcha I'd like to see a v2.0.0 PR corresponding to open-mpi/ompi#1420 first.

Thanks Igor Ivanov for the review.

mellanox-github · 2016-03-03T18:06:00Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1397/ for details.

jsquyres · 2016-03-07T20:25:17Z

👍

ompiteam-bot · 2016-03-07T20:25:18Z

OMPIBot error: Label "reviewed" is already set on issue 967.

opal/memory: Move Memory Allocation Hooks usage from openib

ompiteam-bot assigned jsquyres Feb 15, 2016

ompiteam-bot added this to the v2.0.0 milestone Feb 15, 2016

ompiteam-bot added the bug label Feb 15, 2016

igor-ivanov mentioned this pull request Feb 15, 2016

opal/memory: Move Memory Allocation Hooks usage from openib open-mpi/ompi#1351

Merged

ompiteam-bot added the pushed-back label Feb 15, 2016

igor-ivanov force-pushed the pr/issue-1336 branch from 868e077 to 22f0a75 Compare February 16, 2016 09:44

jsquyres reviewed Feb 16, 2016
View reviewed changes

igor-ivanov referenced this pull request in ggouaillardet/ompi Feb 18, 2016

btl/openib: fix abstraction violation and use opal_memory->memoryc_se…

c3edc8b

…t_alignment

jsquyres added the blocker label Feb 25, 2016

ompiteam-bot removed the pushed-back label Feb 28, 2016

igor-ivanov and others added 2 commits February 29, 2016 09:16

btl/openib: fix abstraction violation and use opal_memory->memoryc_se…

b5f8583

…t_alignment

igor-ivanov force-pushed the pr/issue-1336 branch from cc69baf to b5f8583 Compare February 29, 2016 07:16

ompiteam-bot added reviewed and removed pushed-back labels Feb 29, 2016

ompiteam-bot added pushed-back and removed reviewed labels Feb 29, 2016

ompiteam-bot added reviewed and removed pushed-back labels Feb 29, 2016

hppritcha added the RM-Approved label Mar 2, 2016

jsquyres removed the RM-Approved label Mar 2, 2016

ggouaillardet mentioned this pull request Mar 3, 2016

memory/linux: make memory_linux_memalign an enum open-mpi/ompi#1420

Merged

memory/linux: make memory_linux_memalign an enum

6780bb0

Thanks Igor Ivanov for the review.

hppritcha added the RM-Approved label Mar 7, 2016

hppritcha added a commit that referenced this pull request Mar 7, 2016

Merge pull request #967 from igor-ivanov/pr/issue-1336

43e860c

opal/memory: Move Memory Allocation Hooks usage from openib

hppritcha merged commit 43e860c into open-mpi:v2.x Mar 7, 2016

opal/memory: Move Memory Allocation Hooks usage from openib #967

opal/memory: Move Memory Allocation Hooks usage from openib #967

Uh oh!

Conversation

igor-ivanov commented Feb 15, 2016

Uh oh!

mellanox-github commented Feb 15, 2016

Uh oh!

jsquyres commented Feb 15, 2016

Uh oh!

igor-ivanov commented Feb 15, 2016

Uh oh!

jsquyres commented Feb 15, 2016

Uh oh!

mellanox-github commented Feb 16, 2016

Uh oh!

igor-ivanov commented Feb 16, 2016

Uh oh!

jsquyres Feb 16, 2016

Choose a reason for hiding this comment

Uh oh!

jsquyres Feb 16, 2016

Choose a reason for hiding this comment

Uh oh!

igor-ivanov Feb 16, 2016

Choose a reason for hiding this comment

Uh oh!

jsquyres Feb 16, 2016

Choose a reason for hiding this comment

Uh oh!

jsquyres commented Feb 16, 2016

Uh oh!

jsquyres Feb 16, 2016

Choose a reason for hiding this comment

Uh oh!

igor-ivanov Feb 16, 2016

Choose a reason for hiding this comment

Uh oh!

jsquyres Feb 16, 2016

Choose a reason for hiding this comment

Uh oh!

hppritcha commented Feb 16, 2016

Uh oh!

jsquyres commented Feb 16, 2016

Uh oh!

igor-ivanov commented Feb 17, 2016

Uh oh!

hppritcha commented Feb 17, 2016

Uh oh!

ggouaillardet commented Feb 18, 2016

Uh oh!

igor-ivanov commented Feb 18, 2016

Uh oh!

ggouaillardet commented Feb 18, 2016

Uh oh!

igor-ivanov commented Feb 18, 2016

Uh oh!

ggouaillardet commented Feb 18, 2016

Uh oh!

igor-ivanov commented Feb 18, 2016

Uh oh!

mike-dubman commented Feb 19, 2016

Uh oh!

mike-dubman commented Feb 28, 2016

Uh oh!

lanl-ompi commented Feb 29, 2016

Uh oh!

mellanox-github commented Feb 29, 2016

Uh oh!

mike-dubman commented Feb 29, 2016

Uh oh!

mike-dubman commented Feb 29, 2016

Uh oh!

mike-dubman commented Feb 29, 2016

Uh oh!

mike-dubman commented Feb 29, 2016

Uh oh!

igor-ivanov commented Feb 29, 2016

Uh oh!

jsquyres commented Mar 1, 2016