Skip to content

lower_half/mmap64.c: mmaps[]: add sentinel#351

Open
gc00 wants to merge 3 commits into
mpickpt:mainfrom
gc00:mmaps-add-sentinel
Open

lower_half/mmap64.c: mmaps[]: add sentinel#351
gc00 wants to merge 3 commits into
mpickpt:mainfrom
gc00:mmaps-add-sentinel

Conversation

@gc00

@gc00 gc00 commented Aug 17, 2023

Copy link
Copy Markdown
Collaborator

This adds a sentinel (MAP_FAILED) to the mmaps[] array, and then checks for it in the mpi_plugin.cpp:DMTCP_EVENT_PRECHECKPOINT.

It also renames numRegions to numMmapRegions in lower_half, and improves the documentation in mmap64.c where we reserve a large mmap'ed region for mmap64.c,, starting at 0x2aab00000000;.

@gc00 gc00 added the enhancement New feature or request label Aug 17, 2023
@gc00 gc00 requested review from JainTwinkle and aeblyve August 17, 2023 18:52
@gc00 gc00 force-pushed the mmaps-add-sentinel branch from 82eef31 to 8e53f1d Compare August 17, 2023 19:18
Comment thread mpi-proxy-split/lower-half/mmap64.c Outdated
Comment thread mpi-proxy-split/lower-half/mmap64.c Outdated
for (int i = 0; i < numRegions; i++) {
for (int i = 0; i < numMmapRegions; i++) {
memset(&mmaps[i], 0, sizeof(mmaps[i]));
mmaps[i].addr = MAP_FAILED; // Mark those mmaps past numMmapRegions, for GDB

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

memset should take care of setting all the variables to 0x0. We do not need to set MAP_FAILED separately. Also, the comment does not reflect the code. The loop is only till numMmapRegions, not past that.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Did you mean to add MAP_FAILED after the for loop? The value of i would then be numMmapRegions.

@gc00 gc00 force-pushed the mmaps-add-sentinel branch from 8e53f1d to eb55bed Compare August 17, 2023 19:47
Comment thread mpi-proxy-split/lower-half/mmap64.c Outdated
Comment thread mpi-proxy-split/lower-half/mmap64.c Outdated
for (int i = 0; i < numRegions; i++) {
for (int i = 0; i < numMmapRegions; i++) {
memset(&mmaps[i], 0, sizeof(mmaps[i]));
mmaps[i].addr = MAP_FAILED; // Mark those mmaps past numMmapRegions, for GDB

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Did you mean to add MAP_FAILED after the for loop? The value of i would then be numMmapRegions.

@gc00 gc00 force-pushed the mmaps-add-sentinel branch from eb55bed to 69eedbb Compare August 17, 2023 21:11
@gc00 gc00 force-pushed the mmaps-add-sentinel branch from 69eedbb to 2a5cfbf Compare August 17, 2023 21:16
@gc00

gc00 commented Aug 17, 2023

Copy link
Copy Markdown
Collaborator Author

@JainTwinkle, Thanks for the comment about:

Did you mean to add MAP_FAILED after the for loop? The value of i would then be numMmapRegions.

Good catch! I've now changed it to set the 'addr' to MAP_FAILED for all mmaps elements. Since 'numMmapRegions' is 0 at the end, I could set it just for the zero-th element. But it's safer to set it for all 'mmaps' elements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants