Skip to content

Conversation

@gc00
Copy link
Collaborator

@gc00 gc00 commented May 26, 2020

[ This works now. Please review. ]

setLhMemRange() has been moved to src/mtcp/mtcp_split_process.c. (And another version of it is defined in contrib/mpi-proxy-split/split-process.cpp.) Please review this, so I can push it in, and build on top of it.

Once this is in 'refactoring', I can then test PR #2 and ask for that also to be reviewed. With that in place, I will then modify setLhMemRange() to set the lh_memory_range based on other memory layouts, including CentOS/Ubuntu.

Right now, MANA is not working on CentOS/Ubuntu because the lh_mem_range is conflicting with the stack on CentOS/Ubuntu. The libs and stack, there, have only 128 MB free between them.

We can then port to other layouts of the stack, vdso, libs, etc.. So, the choice of where to put the LhMemRange() will be based on an analysis of the current memory layout.

This code continues to work on Cori., and should not break anything.

Other simplifications were also made in the code:

  1. lh_proxy reads the lh_mem_range on its stdin, and writes back the lh_info struct on its stdout. So, lh_proxy (the child process) doesn't need special coding with pipes; that's only for the parent process now.
  2. This also means that we don't need to pass buf as arg[1] of lh_proxy, holding the pipe fd that will be used by lh_proxy.
  3. We now have only one routine, setLhMemRange() (in mtcp_restart), instead of two of them in the contrib/mpi-proxy-split plugin.
  4. Various comments were added to make clear the logic flow in splitProcess(), and how the lh_info struct and lh_mem_range are passed back and forth.
  5. 'info' and 'pdlsym' are now declared and allocated in lower-half-api.h and split-process.cpp. So, now split-process.cpp no longer depends on mpi_plugin.h.

@gc00 gc00 force-pushed the refactor-mtcp-does-setLhMemRange branch from 24a9c03 to f323c19 Compare May 26, 2020 20:22
@rohgarg rohgarg changed the base branch from master to refactoring May 27, 2020 06:32
@gc00 gc00 force-pushed the refactor-mtcp-does-setLhMemRange branch 2 times, most recently from dcd39ad to f200a54 Compare May 27, 2020 09:44
@rohgarg
Copy link
Contributor

rohgarg commented May 27, 2020

lh_proxy reads the lh_mem_range on its stdin, and writes back the lh_info struct on its stdout. So, lh_proxy (the child process) doesn't need special coding with pipes; that's only for the parent process now.

I don't understand the motivation for doing this. In particular, I don't understand why we need to exchange lh_mem_range information between the main process and the transient proxy process. The memory range info is relevant only after we've copied the lower half bits into the main process and we're initializing and calling functions in the lower half from the upper half.

We can simply define a new API in the lower half to set the memory range and call that function from the upper half, similarly to other APIs in lower_half_api.h.

Or is there a case where the lower half needs this information even while it's part of the transient proxy process?

We now have only one routine, setLhMemRange() (in mtcp_restart), instead of two of them in the contrib/mpi-proxy-split plugin.

I didn't understand this point. We still have the duplicate definitions in contrib/mpi-proxy-split/split_process.cpp and src/mtcp/mtcp_split_process.c.

@rohgarg rohgarg self-requested a review May 27, 2020 17:51
Copy link
Contributor

@rohgarg rohgarg left a comment

Choose a reason for hiding this comment

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

I didn't understand this change.

  • These two variables are not part of the lower half API. Moving the two variable declarations to lower_half_api.h would imply that anyone trying to use the lower-half API would need to define them. An upper-half entity's (library, binary, etc.) interaction with the lower half should not depend defining and using these two global variables.
  • These are global variables used only in the plugin library for calling the lower half functions. The library code is spread across multiple files where we need to refer to mydlsym() and other API's in the lower half. So, they were defined globally in the library (in mpi_plugin.h) for convenience of implementing the library. A different implementation might not require these two globals.

@@ -1,3 +1,4 @@
// FIXME: Why do we have all these libc headers if we're not using libc here?
Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, we don't need all the libc headers here. We only need a few of them in order to compile this file.

Here's a minimal list of header includes we do need in order to compile this file; every other include can be removed.

#include <asm/prctl.h> // For ARCH_GET_FS, ARCH_SET_FS
#include <sys/prctl.h> // For ARCH_GET_FS, ARCH_SET_FS
#include <sys/personality.h> // For ADDR_NO_RANDOMIZE
#include <ucontext.h> // For ucontext_t
#include <sys/uio.h> // For struct iovec
#include <link.h> // For ElfW, auxv_t

@gc00 gc00 force-pushed the refactor-mtcp-does-setLhMemRange branch 2 times, most recently from 0b9bdc6 to f183ca3 Compare May 28, 2020 17:53
@gc00 gc00 force-pushed the refactor-mtcp-does-setLhMemRange branch 2 times, most recently from a59be5c to 1c6202d Compare May 28, 2020 18:17
@gc00 gc00 force-pushed the refactoring branch 2 times, most recently from d4da85c to 59d8b3f Compare May 28, 2020 18:26
@gc00 gc00 force-pushed the refactor-mtcp-does-setLhMemRange branch from 1c6202d to c9c15b6 Compare May 28, 2020 18:27
@gc00 gc00 force-pushed the refactor-mtcp-does-setLhMemRange branch from c9c15b6 to b3d8191 Compare May 29, 2020 10:12
@gc00 gc00 force-pushed the refactor-mtcp-does-setLhMemRange branch from b3d8191 to 22e41f9 Compare May 29, 2020 11:08
@gc00 gc00 force-pushed the refactor-mtcp-does-setLhMemRange branch from 22e41f9 to 232ba07 Compare May 30, 2020 18:33
@gc00 gc00 force-pushed the refactor-mtcp-does-setLhMemRange branch from 232ba07 to 840dde0 Compare May 30, 2020 19:07
gc00 added 5 commits May 30, 2020 15:59
 * ADDING:  int getMappedArea(Area *area, char *name);
   'name' is input parameter, and 'area' is output parameter
   'area' is mapped segment; int result is 1 if name found, else 0
 * mtcp_restart writes to stdin of child, lh_proxy, and reads from stdout.
 * This happens only during "firstTime" in constructor of lh_proxy.
   (Later, we call initializeLowerHalf() w/o reading/writing to stdin/stdout.)
 * We write memRange to stdin of lh_proxy during "firstTime" in constructor.
 * We read lh_info from stdout of lh_proxy during "firstTime" in constructor.
 * (And lh_info now includes the struct MemRange_t; not just a pointer.)
 * For plugin for dmtcp_launch: contrib/mptcp-split-proxy/split-process.c
 * Previous commit was for mtcp_restart (for dmtcp_restart)
 * These global variables have information about the lower half.
   Logically, they belong in lower-half-api.h and split-process.cp
   (They were formally in mpi_plugin.cpp and mpi_plugin.h.)
@gc00 gc00 force-pushed the refactor-mtcp-does-setLhMemRange branch from 840dde0 to 395d767 Compare May 30, 2020 20:01
@gc00
Copy link
Collaborator Author

gc00 commented May 31, 2020

@rohgarg , because I'm basing my development on top of this PR, it was getting complicated. So, I'm pushing this in now. I looked back, and I think I placed info and pdlsym and setLhMemRange() in split-process.cpp in the plugin because their analogs were in mtcp_split_process.c. I agree that these should probably be in mptc_restart.c and mpi_plugin.cpp. I can do that later as part of a cleaning-up exercise. For now, I just want the same architecture for the two cases.

By the way, I think I can now make MANA work with MPICH on dekaksi using the test-dekaksi branch. I'll test that further before setting up a PR for it.

@gc00 gc00 merged commit 2205a0e into refactoring Jun 1, 2020
@karya0 karya0 deleted the refactor-mtcp-does-setLhMemRange branch May 15, 2022 23:41
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.

3 participants