Add new patcher memory hooks

[hjelmn]

This PR will add support for binary patching on ppc, x86, x86_64, and ia64. The work is not yet complete.

[lanl-ompi]

Test FAILed.

[hjelmn]

Looks like some work will be needed to handle the disable dlopen case. Just need to ensure -ldl is still there.

[hjelmn]

@gpaulsen Does Mark have a github account?

[lanl-ompi]

Test FAILed.

[lanl-ompi]

Test FAILed.

[jsquyres]

On Mar 25, 2016, at 4:21 PM, rhc54 notifications@github.com wrote:

Looks like some work will be needed to handle the disable dlopen case. Just need to ensure -ldl is still there.

It would really be good if we didn't have to do that (always ensure that -ldl is there).

You can't build statically if you have to -ldl.

[hjelmn]

Is there an alternate way to look up a symbol without using dlsym?

[jsquyres]

Not that I'm aware of.

Put differently: is it ok to say that static builds can't use leave_pinned?

[hjelmn]

Not sure. lanl has always used static builds to limit the overhead on the filesystem.

[hjelmn]

Maybe static components should be different than disable dlopen?

[hjelmn]

How about we add a configure option like --enable-mca-static-components? Just changes all the build modes to static.... If there isn't already an option like that that isn't --disable-dlopen.

[jsquyres]

Do you static builds, or disable-dlopen builds? Remember:

• --disable-dlopen disables the dlopen() code (as the name implies) and slurps all the components into their project libraries
• --enable-static --disable-shared does the same thing as --disable-dlopen and it builds .a files instead of .so files

If you're building .a files, the run-time linker can't share read-only copies of the libraries between processes (i.e., you eat up more RAM when you run num_cores MPI processes per server because you consume num_cores * (sizeof(libopen-pal) + sizeof(libopen-rte) + sizeof(libmpi)) instead of just (sizeof(libopen-pal) + sizeof(libopen-rte) + sizeof(libmpi))).

[hjelmn]

Ideally we want .so's for the projects but .a's for the components. The only way to get that now is --disable-dlopen.

[hjelmn]

Well. I could make a list of all static components but that is a pain.

[jsquyres]

I'm confused -- how do you get .a's for the components?

• --disable-dlopen doesn't install the individual components, because they're all slurped into the project libraries
• --enable-static --disable-shared similarly doesn't install individual components

We only install .so versions of the components when we build OMPI with dlopen() support. We never install .a versions of the components.

[hjelmn]

No, I don't want .a's installed. I want all components to be part of their respective .so. That is not possible unless --disable-dlopen is used.

[jsquyres]

Sounds like --enable-static does most of what you want:

• slurps plugins into project libraries
• builds .a and .so libraries
• leaves dlopen() enabled

My whole point here is: we really, really shouldn't require -ldl for all cases. It will mess with the static build cases (i.e., building project .a libraries).

[markalle]

I ran an experiment using &munmap instead of dlsym(RTLD_NEXT, "munmap") and on -static binaries it worked. The opal_patch_symbol() function could take a function pointer instead of a string as input and the caller could decide which to pass in.

Is that something that would fit with the Open MPI build-time options? I think a -static build is the only situation where using &munmap instead of dlsym() would work though.

[jsquyres]

@hjelmn Looks like there's a legit segv in the Mellanox jenkins. Is this due to some kind of interaction with UCX?

[hjelmn]

@markalle I refactored the code a little but I the linux patcher component is not working on Power8 with RHELL 6.7. I am working on the fix now.

[hjelmn]

:bot:retest:

[jsquyres]

We had a call today with me, @hjelmn @gpaulsen @markalle to discuss.

tl;dr

Everything looks good so far. We need a few more things that @hjelmn thinks he can finish tomorrow.

More detail

Here's the items left:

1. We need delayed patcher setup / during first rcache usage. This will allow OMPI to effectively guarantee that it is setup as the last memory intercepter (e.g., since UCX's current memory interception routine may not play nice if we're first... we're not 100% sure that it doesn't play nice, though -- it may be fine for OMPI to be first, but let's play safe and be last).
2. We will need to delete memory/linux (i.e., the ptmalloc component) because it can't handle delayed setup.
3. Need to look at current (old) UCX to confirm that OMPI registering 2nd is always going to be ok.
   • NOTE: if orte starts using BTLs, we might not be able to force 2nd registration
4. Add a patcher base downcall to the UCX PML that basically indicates "I'm using UCX-style patching", which can therefore intercept the selection logic in patcher/linux and/or patcher/overwrite)
5. Similarly, the upcoming PAMI PML can do the same thing.
   • @markalle will check with the PAMI people about all of this.
6. @hjelmn is going to open a PR against UCX with the save/restore functionality that it is currently missing (but is included in this PR).
7. LANL and IBM should run tests to make sure layering runs as expected.

[markalle]

I downloaded and built at 36abc43. I don't have a UCX setup readily available but I ran openib. For me the 'overwrite' patcher worked, but 'linux' failed.

I ran one rank on each of two hosts, with
--mca pml ob1 --mca btl openib,self --mca patcher linux

When I run a test that involves a lot of malloc/free of the send/recv buffers the 'overwrite' patcher gave data corruption.

I haven't thoroughly reviewed the code updates on 'overwrite' for putting the bits in a struct and allowing save/restore, but it seems right.

[hjelmn]

Yeah, one-sided is only through BTLs at the moment. Leave pinned it used for large user buffers in that path or large buffers in the ob1 send path.

[hjelmn]

Ok, got this working with ucx and the overwrite component. That will be sufficient for working with UCX. I will delete the linux component and re-add it once I figure out why it isn't quite working.

[hjelmn]

Ok, that Mellanox failure is interesting. I can reproduce on my systems. Using gdb I was able to determine what is happening. During pthread_create the heap is extended and as part of the process mmap is called on the heap with PROT_NONE (which is triggering an invalidate). The protection is removed by a called to mprotect. I think the right answer is not to hook mmap since cache entries within the heap are still valid even if they are protected PROT_NONE they just can't be used for communication. @markalle Thoughts?

[hjelmn]

@hppritcha I think this is converging on a working solution. I hope to move this into master on Sunday so it starts going through MTT in preparation of the PR to v2.x.

[lanl-ompi]

Test FAILed.

[markalle]

Between mmap(,,PROT_NONE,,,) and madvise(,,MADV_DONTNEED) I guess I had a lot of unnecessary interception. I can't get any testcase to prove it's needed. Both were added out of excessive caution of the unknown rather than a known issue. And unnecessary clearing of cached pins, especially in cases that don't happen much, doesn't really hurt anything generally.

I didn't follow the particulars of why the mmap(,,PROT_NONE,,,) interception failed, and I'm a little concerned since in general we should expect the interception to happen from "practically anywhere". But having said that I don't have any objection to removing it. I think it was indeed just another unnecessary interception.

[hjelmn]

The intercept failed in a threaded test. One thread was communicating out of a heap pointer during a call to pthread_create. pthread_create calls mmap (PROT_NONE) on the head the calls mprotect(). Not sure why it does that but it caused an abort because of the ongoing communication.

[hjelmn]

BTW, the mremap fix seems to have fixed the linux patcher. I am leaving the overwrite patcher as the default because it stacks with UCX.... Or not. Was running with UCX. Weird that it works when ucx is in use.

[hjelmn]

BTW, the way I get the linux patcher component to work with UCX is to patch dlsym to look for UCX's attempt to get the original symbol. Requires that our hooks are first but it appears to work every time.

[markalle]

But is there any reason the thread couldn't call munmap() on some other piece of memory at the same location where you saw the mmap(,,PROT_NONE,,,)? It sounds like you're saying it's a bad spot to handle a memory-event from, and happily the PROT_NONE was an event we can safely ignore, but in general if legitimate memory events could happen at this location and our processing can break because it's too early in thread creation that's still a concern.

I don't follow what happens very far down in opal_mem_hooks_release_hook(), but if there's much at all going on there I'd be tempted to whittle it into an atomic FIFO that gets processed later. I did a little experimenting with the opal struct for that with multiple threads and was pretty impressed. Eg I used

typedef struct item_s {
    opal_list_item_t super;
    void *addr;
    size_t len;
} item_t;
opal_lifo_t free_list;
opal_fifo_t memevent_list;

several threads generate fake memory events and adding them to the fifo:

        item = (item_t*) opal_lifo_pop_atomic(&free_list);
        item->addr  and  item->len  =  fake memory event;
        opal_fifo_push_atomic(&memevent_list, &item->super);

while another thread reads off the events, eg

        item = (item_t*) opal_fifo_pop_atomic(&memevent_list);
        opal_lifo_push_atomic(&free_list, &item->super);

I guess I shouldn't be surprised the calls did what they claim. But I always feel like I need to test things like that.

The data structure did well, and I'm hoping ultimately there's not much more than cmpxchg and maybe short sections of lock-holding going on in their implementation. In general I'd want as little as possible going on upon receiving a memory event.

[hjelmn]

Ok, squished this down quite a bit. Will merge after Jenkins.

[jsquyres]

@markalle Did @gpaulsen answer your questions IRL? We talked about the status of these hooks in detail on the Tuesday call.

[markalle]

Mmm, kind of. I'm satisfied that being in a thread whose heap is PROT_NONE is a special state from which we can't function normally. But I'm not positive that by not intercepting mmap(PROT_NONE) we guarantee we'll never intercept a memory event from a time when a thread is in this extra special bad state.

If I thought the state was detectable and the interception routines could do something other than segv, I'd still push for that since I like to be extra robust in case unexpected things happen. But I do tend to agree that I don't actually expect an munmap() to happen while the heap is PROT_NONE.

[markalle]

I was looking at the "from_alloc" field, and asked Yossi/etc what MXM's requirements were. They indicated false positives would be okay but not false negatives. So we need to set it any time it's possible we're inside malloc/free/etc.

So I think intercept_munmap() needs to use true.

[markalle]

@hjelmn Thoughts on opal_mem_hooks_release_hook (,,true) in intercept_munmap()? I think "true" is required since it could be in malloc/free/etc at the time. And generally I'd expect most munmap()s happen from inside a free().

[hjelmn]

@markalle Yeah, a malloc implementation could munmap a pointer. Will update.

[jsquyres]

Followup: @hjelmn filed #1557 to fix this issue.

[jsquyres]

Also -- just to link everything together: here's the corresponding 2.x PR: open-mpi/ompi-release#1079