-
Notifications
You must be signed in to change notification settings - Fork 900
v4.0.x: RDMA OSC: initialize segment memory before registering the segment #5923
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.
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
Conversation
Signed-off-by: Joseph Schuchart <[email protected]> (cherry picked from commit d9dcdfd) Signed-off-by: Nathan Hjelm <[email protected]>
@bwbarrett Don't hold up v4.0.0 for this. Can wait for v4.0.1 if v4.0.0 is imminent. @jsquyres Looks good to me. This just ensures the memory is touched by the owning MPI process before its registered. |
According to #5921, the goal for this PR is to achieve NUMA locality. Do we really need to I know you can use first touch, but it's possible that a naieve approach (e.g., touch the first byte in each page) would have the same end effect of setting locality, yet have less of a performance impact. I'm not 100% sure of that, though, since touching the first byte of a page may cause a paging action, anyway...? Additionally, hwloc may be able to set the locality -- perhaps before you allocate. Check out https://www.open-mpi.org/projects/hwloc/doc/v2.0.2/a00155.php. |
@jsquyres Good point. I meant to perform some measurements on the window allocation performance this week (our system is currently in maintenance though). I would expect that the page allocation is more expensive than the actual |
@devreal I don't know if the page allocation actually brings the pages in to real memory. It may also be suitable to use hwloc to alloc the pages exactly where you want them...? |
The way I understand the documentation you linked, |
Ah, good point that this is allocated as shared memory. Duh. Ok, I guess it's just testing / reading code to see if there's any meaningful difference between touching 1 byte on each page vs. |
Moving to v4.0.1 |
See #5969 -- there may be a problem with this PR...? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per #5969, it looks like this PR isn't ready yet.
@jsquyres Yes, I think we can close this PR, I haven't had time to work on this again but I think there might be a better solution. |
Signed-off-by: Joseph Schuchart [email protected]
(cherry picked from commit d9dcdfd)
Signed-off-by: Nathan Hjelm [email protected]