Skip to content
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

Update atomic memory model text #204

Merged
merged 8 commits into from
Aug 23, 2018

Conversation

jdinan
Copy link
Collaborator

@jdinan jdinan commented Feb 6, 2018

Clarify the various cases that violate atomicity. There should be no semantic changes.

Closes #172

Clarify the various cases that violate atomicity.  There should be no
semantic changes.

Signed-off-by: James Dinan <[email protected]>
@jdinan jdinan added this to the OpenSHMEM 1.5 milestone Feb 6, 2018
@jdinan jdinan self-assigned this Feb 6, 2018
@jdinan jdinan requested a review from RaymondMichael February 6, 2018 01:16
exclusivity when concurrent accesses to the same location are performed using
\openshmem atomic operations using different datatypes, when non-atomic
\openshmem operations are used to access the given location, or when
non-\openshmem operations are performed on the given location. The result in
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add: "(e.g. load and store operations)"

concurrent accesses to the same location by \openshmem atomic operations using
the same datatype will be exclusive. However, they do not guarantee
exclusivity when concurrent accesses to the same location are performed using
\openshmem atomic operations using different datatypes, when non-atomic
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add wait/test with same datatype

Copy link
Contributor

Choose a reason for hiding this comment

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

@jdinan, you mean to add wait/test with same datatype to the allowed/defined list, correct?

outside.
\openshmem contains a number of routines that perform atomic operations on
symmetric data objects (Section \ref{sec:amo}). These routines guarantee that
concurrent accesses to the same location by \openshmem atomic operations using
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add: "and atomic-compatible operations"?

\openshmem contains a number of routines that perform atomic operations on
symmetric data objects (Section \ref{sec:amo}). These routines guarantee that
concurrent accesses to the same location by \openshmem atomic operations using
the same datatype will be exclusive. However, they do not guarantee
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Instead of "datatype" reference the table in the AMO section and use consistent terminology.

@jdinan
Copy link
Collaborator Author

jdinan commented Apr 26, 2018

Requesting committee feedback on the updated draft: osh-6ad7676.pdf.
Flagged reviewers: @shamisp, @spotluri, @manjugv, @npark

@jdinan
Copy link
Collaborator Author

jdinan commented May 21, 2018

Feedback from May 21 reading:

  • Follow up with @manjugv about shmem_barrier / interprocess synchronization in concurrency definition.
  • Examples: add comment above each incorrect line of code to clarify that it causes undefined behavior
  • Example 3: Look into using C11 atomic operation (is the type compatible with SHMEM operations?)

@jdinan
Copy link
Collaborator Author

jdinan commented May 29, 2018

C11 specifies that "The size, representation, and alignment of an atomic type need not be the same as those of the corresponding unqualified type." OpenSHMEM operations on such types are therefore not well-defined, so C11 atomics won't work in example 3.

@jdinan
Copy link
Collaborator Author

jdinan commented Jul 3, 2018

Special ballot changes: 252a6a1 and 04a876d

Section~\ref{subsec:p2p_intro}. The atomic and atomic compatible routines
guarantee that concurrent accesses by any of these routines to the same
location and using the same datatype (specified in Tables~\ref{stdamotypes} and
\ref{extamotypes}) will be exclusive. In contrast to atomic operations, atomic

Choose a reason for hiding this comment

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

short and unsigned short is supported by wait_until (not supported in amotypes); float and double are the other non-overlapping types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This text is intended to specify semantics for atomics, so those types shouldn't really come into play. But, I see your point that as it's written, it also specifies semantics for overlapping wait operations without atomics. How about splitting these cases up:

The atomic routines guarantee that concurrent accesses by any of these routines to the same location and using the same datatype (specified in Tables~\ref{stdamotypes} and \ref{extamotypes}) will be exclusive. Similarly, concurrent accesses by atomic and atomic compatible routines to the same location and using the same datatype will also be exclusive.

Choose a reason for hiding this comment

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

Follow-up question : will there be a usecase for short and unsigned short if the amo APIs end up being the only way to signal wait_until?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IIRC, InfiniBand doesn't have hardware support for 8- and 16-bit atomics. That's one argument in favor supporting scalar put with the wait operations..

@manjugv
Copy link
Collaborator

manjugv commented Jul 23, 2018

July 23rd Meeting Notes: Ballot postponed

@BryantLam
Copy link
Collaborator

Atomic completion of shmem_wait_until

@shamisp expressed concern that shmem_wait_until is not efficiently implementable if it is to both guarantee atomic and non-atomic completion. He claimed shmem_wait_until should only work on non-atomic operations or only on atomic operations, but not both.

OpenSHMEM 1.4 already specifies that atomic and non-atomic completion can be achieved with shmem_wait_until:

shmem_wait and shmem_wait_until wait for ivar to be changed by a write or an atomic operation issued by a PE. ...

Note to implementors -- Implementations must ensure that shmem_wait and shmem_wait_until do not return before the update of the memory indicated by ivar is fully complete. Partial updates to the memory must not cause shmem_wait or shmem_wait_until to return.

This led into a discussion about whether the semantics for wait-until are effectively broken.

Definition of concurrent

04a876d specifies generic statements on concurrency that could apply to elsewhere in the document. One suggestion was to move this definition to a higher level/another section and simply reference it here.

@jdinan
Copy link
Collaborator Author

jdinan commented Jul 26, 2018

@shamisp @BryantLam Let's discuss this at the next RMA WG meeting. The purpose of this ticket is only to clarify existing semantics (broken or not). Restricting the behavior of shmem_wait is being pursued in separate proposal led by @anshumang and @spotluri. One of the goals of this ticket was to raise awareness of this issue; glad it's working. :neckbeard:

I'm fine with moving the concurrency definition to a terms section. Can we handle this separately as part of #228?

@manjugv
Copy link
Collaborator

manjugv commented Jul 26, 2018

I'm fine with moving the concurrency definition to a terms section. Can we handle this separately as part of #228?

Agree with this. As I mentioned on the call, we need to converge this definition with what @anshumang and @spotluri are proposing in #232 and #233 .

@jdinan
Copy link
Collaborator Author

jdinan commented Jul 26, 2018

@manjugv Which definition are you referring to? The definition of concurrency?

Update: I see #229 mentions incorporating this definition for concurrency. Given that the memory model has not yet gelled, I'd like to suggest ratifying/merging this ticket to close out these changes and allow that ticket to integrate them in time for their reading.

@shamisp
Copy link
Contributor

shamisp commented Aug 1, 2018

@jdinan I see it as an integral part of memory model.

@jdinan
Copy link
Collaborator Author

jdinan commented Aug 1, 2018

@shamisp Can you spot me a couple more words? I'm not sure how to interpret your feedback, and github comments are wonderfully context-free.

@jdinan
Copy link
Collaborator Author

jdinan commented Aug 2, 2018

Feedback from Aug. 2 WG meeting: remove concurrency definition and atomic compatible; handle these as part of the memory model proposal.

To be handled in a future proposal.

Signed-off-by: James Dinan <[email protected]>
@jdinan
Copy link
Collaborator Author

jdinan commented Aug 2, 2018

BTW, the "left shift" comment in example 1 strikes me as questionable advice given that the original code has different behaviors depending on the endianness of the system it's running on. Do we want to change that bit to something like: "For example, the 32-bit value can be left-shifted and a 64-bit atomic OR operation can be used."?

@jdinan
Copy link
Collaborator Author

jdinan commented Aug 7, 2018

Special ballot changes: 252a6a1, 04a876d, 12e0687, and b3b5cfb.

Combined diff of special ballot changes

@shamisp
Copy link
Contributor

shamisp commented Aug 10, 2018

@jdinan looks good to me.

@jdinan jdinan merged commit 973ab5c into openshmem-org:master Aug 23, 2018
@jdinan jdinan deleted the pr/atomicity-clarification branch September 11, 2018 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants