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

Add shmem_signal_{add,set} #489

Merged
merged 3 commits into from
Oct 4, 2023
Merged

Conversation

nspark
Copy link
Contributor

@nspark nspark commented Mar 23, 2022

  • Adds shmem_signal_{add,set}
  • Revises the signaling operations introduction and atomicity sections accordingly

Closes #382

@nspark nspark requested a review from naveen-rn March 23, 2022 19:49
@manjugv
Copy link
Collaborator

manjugv commented Mar 25, 2022

The difference between atomic operations and these {add, set} operations is their atomicity guarantees, correct?

@nspark
Copy link
Contributor Author

nspark commented Mar 25, 2022

Yes. AMOs are only guaranteed atomicity of updates w.r.t. other AMOs of the same data type (but for all operation types; e.g., set, add, xor). Signal operations are only guaranteed atomicity of updates w.r.t. other signal operations of the same operation type (i.e., add or set).

I think the intent was that, for performance reasons, shmem_signal_set and shmem_put_signal(SET) can provide (for example) single-copy atomic writes without needing the full strength of atomicity required by AMOs.

@shamisp
Copy link
Contributor

shamisp commented Mar 30, 2022

I think this adds quite a bit API bloat, while the semantical difference is very nuanced. I would suggest to add a parameter to shmalloc that lets you define desired AMO semantics, so you can reuse all existing AMO operations.

Copy link
Contributor Author

@nspark nspark left a comment

Choose a reason for hiding this comment

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

Remove erroneous const type qualifiers

\begin{apidefinition}

\begin{C11synopsis}
void @\FuncDecl{shmem\_signal\_add}@(shmem_ctx_t ctx, const uint64_t *sig_addr, uint64_t signal, int pe);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
void @\FuncDecl{shmem\_signal\_add}@(shmem_ctx_t ctx, const uint64_t *sig_addr, uint64_t signal, int pe);
void @\FuncDecl{shmem\_signal\_add}@(shmem_ctx_t ctx, uint64_t *sig_addr, uint64_t signal, int pe);

Comment on lines +12 to +13
void @\FuncDecl{shmem\_signal\_add}@(const uint64_t *sig_addr, uint64_t signal, int pe);
void @\FuncDecl{shmem\_ctx\_signal\_add}@(shmem_ctx_t ctx, const uint64_t *sig_addr, uint64_t signal, int pe);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
void @\FuncDecl{shmem\_signal\_add}@(const uint64_t *sig_addr, uint64_t signal, int pe);
void @\FuncDecl{shmem\_ctx\_signal\_add}@(shmem_ctx_t ctx, const uint64_t *sig_addr, uint64_t signal, int pe);
void @\FuncDecl{shmem\_signal\_add}@(uint64_t *sig_addr, uint64_t signal, int pe);
void @\FuncDecl{shmem\_ctx\_signal\_add}@(shmem_ctx_t ctx, uint64_t *sig_addr, uint64_t signal, int pe);

\begin{apidefinition}

\begin{C11synopsis}
void @\FuncDecl{shmem\_signal\_set}@(shmem_ctx_t ctx, const uint64_t *sig_addr, uint64_t signal, int pe);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
void @\FuncDecl{shmem\_signal\_set}@(shmem_ctx_t ctx, const uint64_t *sig_addr, uint64_t signal, int pe);
void @\FuncDecl{shmem\_signal\_set}@(shmem_ctx_t ctx, uint64_t *sig_addr, uint64_t signal, int pe);

Comment on lines +12 to +13
void @\FuncDecl{shmem\_signal\_set}@(const uint64_t *sig_addr, uint64_t signal, int pe);
void @\FuncDecl{shmem\_ctx\_signal\_set}@(shmem_ctx_t ctx, const uint64_t *sig_addr, uint64_t signal, int pe);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
void @\FuncDecl{shmem\_signal\_set}@(const uint64_t *sig_addr, uint64_t signal, int pe);
void @\FuncDecl{shmem\_ctx\_signal\_set}@(shmem_ctx_t ctx, const uint64_t *sig_addr, uint64_t signal, int pe);
void @\FuncDecl{shmem\_signal\_set}@(uint64_t *sig_addr, uint64_t signal, int pe);
void @\FuncDecl{shmem\_ctx\_signal\_set}@(shmem_ctx_t ctx, uint64_t *sig_addr, uint64_t signal, int pe);

@manjugv
Copy link
Collaborator

manjugv commented May 26, 2023

May spec meeting: Please add change log entry

@jdinan
Copy link
Collaborator

jdinan commented Sep 8, 2023

Special ballot: 1c254ce

@jdinan jdinan merged commit 91a2495 into openshmem-org:master Oct 4, 2023
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.

Add Signal API
5 participants