opal/memory: Move Memory Allocation Hooks usage from openib #1351
Conversation
| @@ -98,7 +98,7 @@ | |||
| #endif | |||
| #include "ompi/runtime/ompi_cr.h" | |||
|
|
|||
| #if defined(MEMORY_LINUX_PTMALLOC2) && MEMORY_LINUX_PTMALLOC2 | |||
| #if defined(MEMORY_LINUX_HAVE_MALLOC_HOOK_SUPPORT) && (MEMORY_LINUX_HAVE_MALLOC_HOOK_SUPPORT == 1) | |||
There was a problem hiding this comment.
This should be replaced with
#if MEMORY_LINUX_HAVE_MALLOC_HOOK_SUPPORT
If this macro is not defined, then this is a configure issue
igor-ivanov
force-pushed
the
pr/issue-1336
branch
from
803d19a to
9f2a9ed
Compare
|
|
||
| /* This function is called from MPI_Init in case system has Memory Allocation Hooks | ||
| * (see ompi/runtime/ompi_mpi_init.c) | ||
| */ |
There was a problem hiding this comment.
Does OSHMEM need this functionality, too?
There was a problem hiding this comment.
Ok. Should probably update the comment to be a bit more generic. And per my other comments, we might want to make this functionality opt-in vs. global.
|
General comment on this PR: I see why you moved it from the BTL down to here -- that makes sense. But it does make a global change: if memory hooks are enabled, then this memalign functionality will be enabled, even if you're using a network that doesn't care about aligned memory. Shouldn't the BTL (or whatever transport) have to request to turn this functionality on if it wants it (vs. unconditionally turning it on for all BTLs/etc.)? |
|
@jsquyres what milestone would you suggest for this PR. Its not going in to 2.0.0 |
|
@jsquyres __malloc_hook is a global variable so |
|
@igor-ivanov That's not quite what I mean. What I mean is: if openib (or UCX or ...) wants this functionality, they should request that it be enabled. It shouldn't be enabled unconditionally if the support for malloc hooks is compiled in to Open MPI. Second note: @hppritcha and I talked in IM. This is a bug fix, and therefore is suitable for v2.0.0. Howard updated the labels and milestone. |
|
@jsquyres this capability is OFF by default. You must enable this from command line. |
igor-ivanov
force-pushed
the
pr/issue-1336
branch
from
9f2a9ed to
6d6ea9e
Compare
|
@igor-ivanov Gotcha. Should it be on by default for BTLs/etc. that would benefit from its use? |
igor-ivanov
force-pushed
the
pr/issue-1336
branch
2 times, most recently
from
1af7c9b to
4e64ad8
Compare
These changes fix issue open-mpi#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.
igor-ivanov
force-pushed
the
pr/issue-1336
branch
from
4e64ad8 to
8b05f30
Compare
|
@jsquyres You are right. I realized that changes broke default logic for btl/opeinb. So I am updating the commit to save original btl/openib behaivour.
|
opal/memory: Move Memory Allocation Hooks usage from openib
|
thanks @igor-ivanov |
| #endif | ||
| want most of the allocated resources be aligned. | ||
| */ | ||
| opal_memory_linux_malloc_set_alignment(32, mca_btl_openib_module.super.btl_eager_limit); |
There was a problem hiding this comment.
This is a huge abstraction violation: you cannot have one component directly call another component.
|
fwiw, there is a similar abstraction violation in |
…dates v1.10 README: update for new mailman and main web sites
These changes fix issue #1336
opal/memory/linux component should be single place that opeartes with
Memory Allocation Hooks. It is safe because it is linked statically.
@ggouaillardet @yosefe could you please look at it.