v2.x mem hook update #1079

hjelmn · 2016-04-14T22:26:03Z

This commit brings the new patcher memory hook to v2.0.0. The requested changes to the master commits have been made. The linux memory manager has not been deleted but it will not always be used if --with-memory-manager=linux is specified and will not be built otherwise.

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

This commit changes the opal_fifo_push code to use opal_update_counted_pointer to set the head. This fixes a data race that occurs possibly because the read of the fifo head in opal_fifo_pop requires two instructions. This combined with the non-atomic update in opal_fifo_push can lead to an ABA issue that puts the fifo in an inconsistant state. There are other ways this problem could be fixed. One way would be to introduce an opal_atomic_read_128 implementation. On x86_64 this would have to use the cmpxchg16b instruction. Since this instruction would have to be in the pop path (and always executed) it would be slower than the fix in this commit. Closes open-mpi/ompi#1460. Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@dc00021) Signed-off-by: Nathan Hjelm <[email protected]>

This commit adds support for runtime binary patching. The support is broken down into two parts: util/opal_patcher.[ch] which contains the functionality for runtime patching of symbols, and mca/memory/patcher which patches the various symbols needed to provide support for memory hooks. This work is preliminary and is based off work donated by IBM. The patcher code is disabled if dlopen is disabled. Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@7aa03d6) Signed-off-by: Nathan Hjelm <[email protected]>

This commit makes it possible to set relative priorities for components. Before the addition of the patched component there was only one component that would run on any system but that is no longer the case. When determining which component to open each component's query function is called and the one that returns the highest priority is opened. The default priority of the patcher component is set slightly higher than the old ptmalloc2/ummunotify component. This commit fixes a long-standing break in the abstration of the memory components. ompi_mpi_init.c was referencing the linux malloc hook initilize function to ensure the hooks are initialized for libmpi.so. The abstraction break has been fixed by adding a memory base function that calls the open memory component's malloc hook init function if it has one. The code is not yet complete but is intended to support ptmalloc in 2.0.0. In that case the base function will always call the ptmalloc hook init if exists. Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@11e2d78) Signed-off-by: Nathan Hjelm <[email protected]>

Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@4cac623) Signed-off-by: Nathan Hjelm <[email protected]>

The --enable-static gives us what we want: statically linked components. Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@b1670f8) Signed-off-by: Nathan Hjelm <[email protected]>

This commit adds a framework to abstract runtime code patching. Components in the new framework can provide functions for either patching a named function or a function pointer. The later functionality is not being used but may provide a way to allow memory hooks when dlopen functionality is disabled. This commit adds two different flavors of code patching. The first is provided by the overwrite component. This component overwrites the first several instructions of the target function with code to jump to the provided hook function. The hook is expected to provide the full functionality of the hooked function. The linux patcher component is based on the memory hooks in ucx. It only works on linux and operates by overwriting function pointers in the symbol table. In this case the hook is free to call the original function using the function pointer returned by dlsym. Both components restore the original functions when the patcher framework closes. Changes had to be made to support Power/PowerPC with the Linux dynamic loader patcher. Some of the changes: - Move code necessary for powerpc/power support to the patcher base. The code is needed by both the overwrite and linux components. - Move patch structure down to base and move the patch list to mca_patcher_base_module_t. The structure has been modified to include a function pointer to the function that will unapply the patch. This allows the mixing of multiple different types of patches in the patch_list. - Update linux patching code to keep track of the matching between got entry and original (unpatched) address. This allows us to completely clean up the patch on finalize. All patchers keep track of the changes they made so that they can be reversed when the patcher framework is closed. At this time there are bugs in the Linux dynamic loader patcher so its priority is lower than the overwrite patcher. Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@27f8a4e) Signed-off-by: Nathan Hjelm <[email protected]>

This commit fixes bugs that can cause crashes and memory corruption when the mremap hook is called. The problem occurs because of the ellipses (...) in the mremap intercept function. The ellipses cover the optional new_addr argument on Linux. This commit removes the ellipses and adds an explicit 5th argument. This commit also adds a hook for shmdt. The code only works on Linux at the moment as it needs to read /proc/self/maps to determine the size of the shared memory segment. Additionally, this commit removes the mmap hook. There is no apparent benefit for detecting mmap(..., PROT_NONE, ...) and it seems to cause problems when threads are in use. Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@80ec79c) Signed-off-by: Nathan Hjelm <[email protected]>

This commit changes the configury of memory/linux to only allow it to build if the user specifies --with-memory-manager=linux. If this is specified then the ptmalloc component is forced on. If --with-memory-manager=linux is not specified the patcher memory component will be used. Systems without patcher support will not get a memory manager. Signed-off-by: Nathan Hjelm <[email protected]>

rhc54 · 2016-04-14T22:34:02Z

@hjelmn Might want to add open-mpi/ompi@4b3995d to silence a warning

(cherry picked from open-mpi/ompi@4b3995d) Signed-off-by: Nathan Hjelm <[email protected]>

hjelmn · 2016-04-14T22:35:52Z

Done.

mellanox-github · 2016-04-14T23:05:35Z

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

mellanox-github · 2016-04-14T23:14:27Z

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

jsquyres · 2016-04-15T11:16:52Z

Coverity found a problem last night:

*** CID 1358512:  Error handling issues  (NEGATIVE_RETURNS)
/opal/mca/memory/patcher/memory_patcher_component.c: 283 in memory_patcher_get_shm_seg_size()
277         seg_size = 0;
278     
279         fd = open ("/proc/self/maps", O_RDONLY);
280         assert (fd >= 0);
281     
282         for (size_t read_offset = 0 ; ; ) {
   CID 1358512:  Error handling issues  (NEGATIVE_RETURNS)
   "fd" is passed to a parameter that cannot be negative.
283             ssize_t nread = read(fd, buffer + read_offset, sizeof(buffer) - 1 - read_offset);
284             if (nread <= 0) {
285                 if (errno == EINTR) {
286                     continue;
287                 }
288

@gpaulsen @markalle Can you confirm that this PR works properly on the v2.x branch?

hjelmn · 2016-04-15T15:02:05Z

Should be ok. The read will fail and errno will be set. Didn't think it was worth checking the return on open.

jsquyres · 2016-04-15T16:02:23Z

Can you fix anyway, please? 😃

Fix CID 1358512: Error handling issues (NEGATIVE_RETURNS): C libraries usually handle read (-1, ...) fine but it is safer to avoid calling read with a negative handle. Added negative file descriptor check. Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@7f27117) Signed-off-by: Nathan Hjelm <[email protected]>

mellanox-github · 2016-04-18T21:35:32Z

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

…plementation Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from commit open-mpi/ompi@a8e90e8) Signed-off-by: Nathan Hjelm <[email protected]>

mellanox-github · 2016-04-19T00:09:15Z

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

jsquyres · 2016-04-19T11:53:34Z

We're getting MTT compile warnings and errors on master from Absoft:

patcher_linux_module.c: In function 'mca_patcher_linux_get_dynentry':
patcher_linux_module.c:97: warning: comparison between signed and unsigned
patcher_linux_module.c: In function 'mca_patcher_linux_get_aux_phent':
patcher_linux_module.c:200: warning: format '%lu' expects type 'long unsigned int', but argument 4
has type 'unsigned int'
patcher_linux_module.c: In function 'mca_patcher_linux_apply_patch':
patcher_linux_module.c:322: warning: format '%lx' expects type 'long unsigned int', but argument 5
has type 'uintptr_t'
patcher_linux_module.c: In function 'mca_patcher_linux_remove_patch':
patcher_overwrite_module.c: In function 'mca_patcher_overwrite_apply_patch':
patcher_overwrite_module.c:93: error: 'func_old_addr' undeclared (first use in this function)
patcher_overwrite_module.c:93: error: (Each undeclared identifier is reported only once
patcher_overwrite_module.c:93: error: for each function it appears in.)
patcher_overwrite_module.c: At top level:
patcher_overwrite_module.c:306: warning: initialization from incompatible pointer type

jsquyres · 2016-04-19T14:00:26Z

Also, Durga cited a compile error on FreeBSD.

Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@1147fb3) Signed-off-by: Nathan Hjelm <[email protected]>

Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@5bc9d9d) Signed-off-by: Nathan Hjelm <[email protected]>

Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@d981a9f) Signed-off-by: Nathan Hjelm <[email protected]>

hjelmn · 2016-04-19T17:43:23Z

bot:nolabel:pushed-back

jsquyres · 2016-04-19T17:54:34Z

Do you need open-mpi/ompi@16c2839, too?

mellanox-github · 2016-04-19T18:08:32Z

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

hjelmn · 2016-04-19T20:56:55Z

@jsquyres Nope, that is part of the mpool rewrite. Different PR.

hjelmn · 2016-04-19T20:57:34Z

Looks like a false positive from mellanox.

:bot:retest:

mellanox-github · 2016-04-19T21:42:01Z

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

Fixes open-mpi/ompi#1565 Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@db854c3) Signed-off-by: Nathan Hjelm <[email protected]>

mellanox-github · 2016-04-20T17:09:31Z

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

jsquyres · 2016-04-23T14:01:38Z

bot:retest

mellanox-github · 2016-04-23T14:38:47Z

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

jsquyres · 2016-04-23T16:17:42Z

@hjelmn Is this PR now good to go?

hjelmn · 2016-04-23T18:06:18Z

Yup.

jsquyres · 2016-04-25T15:13:21Z

opal/mca/memory/base/memory_base_open.c


+void opal_memory_base_malloc_init_hook (void)
+{
+#if MEMORY_LINUX_PTMALLOC2


I find it a bit weird to have #if's for specific components in the base... is this really necessary?

Better than the #if being in ompi_mpi_init.c ;)

I guess I don't understand why they are needed at all...? Can't these callbacks be done in a way that doesn't break abstractions?

@hjelmn Tells me that there's no component selected at this point. Grumble. Well, it's not a regression, so I guess we'll let this go for v2.0.0...

I would call it a partial fix for the abstraction break. Now there is no check for ptmalloc2 in ompi_mpi_init.c.

This is complicated stuff: add some comments so that future maintainers have some rationale to understand the way things have been done. Signed-off-by: Jeff Squyres <[email protected]> (cherry picked from commit 78b367e)

This is complicated stuff: add some comments as to why there's an unfortunate-but-intentional abstraction violation in the memory base. (this commit did not come from master, because ptmalloc has been removed from master -- this commit is only for v2.x) Signed-off-by: Jeff Squyres <[email protected]>

Hjelmn v2.x mem hooks

…ponent. (cherry picked from open-mpi/ompi@ed6f508) Signed-off-by: Nathan Hjelm <[email protected]>

…component (cherry picked from open-mpi/ompi@55a15fb) Signed-off-by: Nathan Hjelm <[email protected]>

jsquyres · 2016-04-25T20:25:40Z

@hppritcha I think we're good to go here once CI finishes

mellanox-github · 2016-04-25T20:58:36Z

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

hppritcha · 2016-04-25T21:08:25Z

yep I'll merge once travis is done.

hjelmn added 8 commits April 14, 2016 10:22

opal/patch: add call to check if binary patching is supported

9abddc4

Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@4cac623) Signed-off-by: Nathan Hjelm <[email protected]>

contrib/platform: don't disable dlopen

f307571

The --enable-static gives us what we want: statically linked components. Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@b1670f8) Signed-off-by: Nathan Hjelm <[email protected]>

ompiteam-bot assigned jsquyres Apr 14, 2016

ompiteam-bot added this to the v2.0.0 milestone Apr 14, 2016

ompiteam-bot added bug blocker labels Apr 14, 2016

Trivial change to silence warning

e2042d8

(cherry picked from open-mpi/ompi@4b3995d) Signed-off-by: Nathan Hjelm <[email protected]>

This was referenced Apr 18, 2016

memory/patcher: munmap hook could be called from within a malloc() implementation open-mpi/ompi#1557

Merged

Add new patcher memory hooks open-mpi/ompi#1495

Merged

memory/patcher: munmap hook could be called from within a malloc() im…

2bae7f0

…plementation Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from commit open-mpi/ompi@a8e90e8) Signed-off-by: Nathan Hjelm <[email protected]>

jsquyres added the pushed-back label Apr 19, 2016

patcher/linux: ensure component is only enabled on Linux

72a9d51

Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@1147fb3) Signed-off-by: Nathan Hjelm <[email protected]>

hjelmn added 2 commits April 19, 2016 11:42

patcher/linux: fix compiler warnings

9a8f1e0

Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@5bc9d9d) Signed-off-by: Nathan Hjelm <[email protected]>

patcher/overwrite: fix compile error on x86

35d9105

Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@d981a9f) Signed-off-by: Nathan Hjelm <[email protected]>

ompiteam-bot removed the pushed-back label Apr 19, 2016

memory/patcher: do not hook madvise if the syscall doesn't exist

6c55bae

Fixes open-mpi/ompi#1565 Signed-off-by: Nathan Hjelm <[email protected]> (cherry picked from open-mpi/ompi@db854c3) Signed-off-by: Nathan Hjelm <[email protected]>

jsquyres reviewed Apr 25, 2016
View reviewed changes

jsquyres mentioned this pull request Apr 25, 2016

README: update list of frameworks #1096

Merged

jsquyres and others added 5 commits April 25, 2016 13:12

memory patcher: add some clarifying comments

19c1627

This is complicated stuff: add some comments so that future maintainers have some rationale to understand the way things have been done. Signed-off-by: Jeff Squyres <[email protected]> (cherry picked from commit 78b367e)

Merge pull request #1 from jsquyres/hjelmn-v2.x_mem_hooks

e1d70aa

Hjelmn v2.x mem hooks

Updated IBM Copyright message for contributions in memory patcher com…

0292a87

…ponent. (cherry picked from open-mpi/ompi@ed6f508) Signed-off-by: Nathan Hjelm <[email protected]>

Missed one IBM Copyright message for contributions in memory patcher …

bac3362

…component (cherry picked from open-mpi/ompi@55a15fb) Signed-off-by: Nathan Hjelm <[email protected]>

jsquyres added the RM-Approved label Apr 25, 2016

hppritcha merged commit 8e9647f into open-mpi:v2.x Apr 26, 2016

v2.x mem hook update #1079

v2.x mem hook update #1079

Uh oh!

Conversation

hjelmn commented Apr 14, 2016

Uh oh!

rhc54 commented Apr 14, 2016

Uh oh!

hjelmn commented Apr 14, 2016

Uh oh!

mellanox-github commented Apr 14, 2016

Uh oh!

mellanox-github commented Apr 14, 2016

Uh oh!

jsquyres commented Apr 15, 2016

Uh oh!

hjelmn commented Apr 15, 2016

Uh oh!

jsquyres commented Apr 15, 2016

Uh oh!

mellanox-github commented Apr 18, 2016

Uh oh!

mellanox-github commented Apr 19, 2016

Uh oh!

jsquyres commented Apr 19, 2016

Uh oh!

jsquyres commented Apr 19, 2016

Uh oh!

hjelmn commented Apr 19, 2016

Uh oh!

jsquyres commented Apr 19, 2016

Uh oh!

mellanox-github commented Apr 19, 2016

Uh oh!

hjelmn commented Apr 19, 2016

Uh oh!

hjelmn commented Apr 19, 2016

Uh oh!

mellanox-github commented Apr 19, 2016

Uh oh!

mellanox-github commented Apr 20, 2016

Uh oh!

jsquyres commented Apr 23, 2016

Uh oh!

mellanox-github commented Apr 23, 2016

Uh oh!

jsquyres commented Apr 23, 2016

Uh oh!

hjelmn commented Apr 23, 2016

Uh oh!

jsquyres Apr 25, 2016

Choose a reason for hiding this comment

Uh oh!

hjelmn Apr 25, 2016

Choose a reason for hiding this comment

Uh oh!

jsquyres Apr 25, 2016

Choose a reason for hiding this comment

Uh oh!

jsquyres Apr 25, 2016

Choose a reason for hiding this comment

Uh oh!

hjelmn Apr 25, 2016

Choose a reason for hiding this comment

Uh oh!

jsquyres commented Apr 25, 2016

Uh oh!

mellanox-github commented Apr 25, 2016

Uh oh!

hppritcha commented Apr 25, 2016

Uh oh!

Uh oh!