Skip to content

Don't skip munmap of mtcp_restart regions. #353

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
karya0 wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Don't skip munmap of mtcp_restart regions. #353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e8db1b5
ADDED: MPI_Comm_internal_vgroup, creating a virtual group without log…
aeblyve Aug 16, 2023
a88dc36
extern function declaration
aeblyve Aug 16, 2023
f6e5001
Improve readability and commentary
aeblyve Aug 17, 2023
d3f1133
Don't skip munmap of mtcp_restart regions.
karya0 Aug 28, 2023
2c3aaa3
Munmap heap on restart.
karya0 Aug 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions mpi-proxy-split/mpi-wrappers/mpi_group_wrappers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,25 @@ USER_DEFINED_WRAPPER(int, Comm_group, (MPI_Comm) comm, (MPI_Group *) group)
return retval;
}

// Calls MPI_Comm_group to define a new group for internal purposes.
// See: p2p_drain_send_recv.cpp
int
MPI_Comm_internal_virt_group(MPI_Comm comm, MPI_Group *group)
{
int retval;
DMTCP_PLUGIN_DISABLE_CKPT();
MPI_Comm realComm = VIRTUAL_TO_REAL_COMM(comm);
JUMP_TO_LOWER_HALF(lh_info.fsaddr);
retval = NEXT_FUNC(Comm_group)(realComm, group);
RETURN_TO_UPPER_HALF();
if (retval == MPI_SUCCESS) {
MPI_Group virtGroup = ADD_NEW_GROUP(*group);
*group = virtGroup;
}
DMTCP_PLUGIN_ENABLE_CKPT();
return retval;
}

USER_DEFINED_WRAPPER(int, Group_size, (MPI_Group) group, (int *) size)
{
int retval;
Expand Down
11 changes: 10 additions & 1 deletion mpi-proxy-split/p2p_drain_send_recv.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ extern int MPI_Comm_create_group_internal(MPI_Comm comm, MPI_Group group,
extern int MPI_Comm_free_internal(MPI_Comm *comm);
extern int MPI_Comm_group_internal(MPI_Comm comm, MPI_Group *group);
extern int MPI_Group_free_internal(MPI_Group *group);
extern int MPI_Comm_internal_virt_group(MPI_Comm comm, MPI_Group *group);
int *g_sendBytesByRank; // Number of bytes sent to other ranks
int *g_rsendBytesByRank; // Number of bytes sent to other ranks by MPI_rsend
int *g_bytesSentToUsByRank; // Number of bytes other ranks sent to us
Expand Down Expand Up @@ -75,7 +76,7 @@ registerLocalSendsAndRecvs()
// Get a copy of MPI_COMM_WORLD
MPI_Group group_world;
MPI_Comm mana_comm;
MPI_Comm_group(MPI_COMM_WORLD, &group_world);
MPI_Comm_internal_virt_group(MPI_COMM_WORLD, &group_world);
MPI_Comm_create_group_internal(MPI_COMM_WORLD, group_world, 1, &mana_comm);

// broadcast sendBytes and recvBytes
Expand All @@ -84,7 +85,15 @@ registerLocalSendsAndRecvs()
g_bytesSentToUsByRank[g_world_rank] = 0;

// Free resources
// mana_comm is a real id, and MPI_Comm_free_internal expects a
// virtual id, but it works out because virtualToReal(real_id) is
// defined to be real_id.
MPI_Comm_free_internal(&mana_comm);

// Because group_world is a virtual group, we have to free both its
// virtual and real id to clean up correctly.
MPI_Group_free_internal(&group_world);
REMOVE_OLD_GROUP(group_world);
}

// status was received by MPI_Iprobe
Expand Down
6 changes: 6 additions & 0 deletions restart_plugin/mtcp_restart_plugin.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,6 +849,12 @@ mtcp_plugin_skip_memory_region_munmap(Area *area, RestoreInfo *rinfo)
LhCoreRegions_t *lh_regions_list = NULL;
int total_lh_regions = lh_info->numCoreRegions;

// Don't skip munmap of mtcp_restart regions.
Copy link
Collaborator

@gc00 gc00 Aug 28, 2023

Choose a reason for hiding this comment

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

Let's change the comment to remove the double negative:
// Do an munmap of mtcp_restart regions during restart. Don't skip this.

Also, please add a comment about why we need to munmap the mtcp_restart regions within MANA, but we don't need to do that within ordinary DMTCP. Where is the potential address conflict that we're trying to avoid?

Copy link
Collaborator

@jiamingz9925 jiamingz9925 Aug 28, 2023

Choose a reason for hiding this comment

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

I want to add that we should not skip all [heap] but only the [heap] right after the mtcp_restart region.

if (mtcp_strendswith(area->name, "/mtcp_restart") ||
mtcp_strendswith(area->name, "[heap]")) {
Copy link
Collaborator

@jiamingz9925 jiamingz9925 Aug 29, 2023

Choose a reason for hiding this comment

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

we only want to skip the [heap] right after mtcp_restart. Also in mpi_plugin.cpp we need to do the same to skip those area for libsStart consideration.

Copy link
Collaborator

@jiamingz9925 jiamingz9925 Aug 29, 2023

Choose a reason for hiding this comment

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

Update: For some reason unmap the heap region here would cause seg fault, but without unmapping it we will encounter conflict as well later. @karya0

return 0;
}

if (regionContains(rinfo->pluginInfo.memRange.start,
rinfo->pluginInfo.memRange.end,
area->addr, area->endAddr)) {
Expand Down