Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Signal operation type for put with signal #929
Signal operation type for put with signal #929
Changes from 4 commits
1b471e5
be9e259
67912f5
42a61cf
217c430
bada86b
32bf17d
3af033e
e24e99a
432835a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This needs to be an atomic operation, use
shmem_shr_transport_atomic
.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.
@jdinan
shmem_shr_transport_atomic
is enabled throughUSE_SHR_ATOMICS
. Unless we use--enable-shr-atomics
flag, we will not be able to use this API, right? Please let me know if I am missing something.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.
If the target is waiting for several messages using the same signal variable, this will not work, right?
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.
Right, you would need to write code to support builds with and without shared memory atomics.
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.
Thanks @jdinan. One question, from the current code, there seems to be no relation between the flags
USE_SHR_ATOMICS
andUSE_XPMEM
/USE_CMA
. Was there any particular reason behind this? I am thinking shouldn'tUSE_SHR_ATOMICS
be defined when either of these on-node transports is defined?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.
For nearly all builds, we can't enable shared memory atomics because the networking layer is not coherent with processor atomics. By default, we only enable shared memory copy to implement put/get. This is why shared memory atomics are a separate option, and disabled by default. CMA is essentially memory copy performed by the Linux kernel and it cannot support atomics.
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.
For CMA, we should directly use the transport layer atomics then, as you also suggested later. But, for XPMEM, can we say that enabling USE_XPMEM should enable USE_SHR_ATOMICS as well? We can use the shared memory atomics if
shmem_internal_get_shr_rank
returns something other than -1, right? But, currently,shmem_shr_transport_use_atomic
also seems to be guarded byUSE_SHR_ATOMICS
.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.
Correct, it is always safe to use shared memory put/get because OpenSHMEM says overlapping operations (e.g. between local shared memory and remote network transport accesses) leads to undefined behavior. Atomics do allow this overlapping. Therefore, it is only safe to use shared memory atomics when you know the transport layer is coherent with processor atomics. Because of this, enabling XPMEM only enables shared memory put/get and there is an additional flag to enable shared memory atomics.
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.
This needs to be an atomic. CMA does not support atomics; use
shmem_transport_atomic
.