-
Notifications
You must be signed in to change notification settings - Fork 93
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
Add Support for Dynamic Symmetric Memory #909
base: master
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
0451a7d
to
5c25b9c
Compare
b7610cc
to
3db29e9
Compare
@wfaderhold21 Is this ready for review? @janjust Does this work for you? |
@manjugv In general this works, but I'm still a little concerned if keeping mapped data until tl destruction is the right approach, or if we should enable symmetric buffer unmapping. |
@manjugv yes. It’s ready for review. |
ef33001
to
fef2a37
Compare
@wfaderhold21 Did we address the possibility of adding an imported key? What about the ability to important keys with an API? I think we did a follow up study and concluded that parallelization does not improve perf much, and requires a unique memory region to handle key imports otherwise we run into segfaults. |
OK - after WG meeting I think I realize that my point is moot. I think this is good as is, we just need to put a protection for when keys are imported by multiple threads. |
@Sergei-Lebedev @janjust was there any more feedback on this PR? |
@wfaderhold21 I don't think so - I think we're just really backlogged with other PRs and reviews - sorry for the slow progress but it's on my radar |
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.
LGTM!
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.
LGTM! thanks
@samnordmann @nsarka I think I made the corrections you all asked, but I also updated some code to bugs I found. Could you all take another look to make sure everything is OK there? |
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.
LGTM!
@Sergei-Lebedev @wfaderhold21 Hey guys, sorry to harp on this, but are we done with this PR? Is it ready to be merged, @Sergei-Lebedev any other input needed? |
@janjust AFIAK nothing to be added on my end. |
@Sergei-Lebedev @manjugv are there any outstanding comments? Can we push this forward? |
What
Currently, UCC has support for symmetric memory segments that are added during context creation for use with one-sided collectives. This PR adds support for memory segments to be added after a context and team have been created to better support programming models such as MPI. In addition, this adds support for attaching a key with the memory region for use with DPUs and offloading of collectives.
Why ?
With the current approach, a user will need to perform a memory copy of source and destination buffers to a known and already mapped memory region before a collective can begin using one-sided or offloaded collectives. This removes the need for the memory copy at the expense of memory exchanges on the first collective. This is useful if multiple collectives occur with the same buffers.