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

Fix for atomicv on CXI #1147

Merged
merged 2 commits into from
Sep 19, 2024
Merged

Conversation

wrrobin
Copy link
Collaborator

@wrrobin wrrobin commented Sep 18, 2024

No description provided.

@@ -336,6 +336,7 @@ shmem_internal_atomicv(shmem_ctx_t ctx, void *target, const void *source,
shmem_internal_fetch_atomic(ctx, ((uint8_t *) target) + (i * type_size),
((uint8_t *) source) + (i * type_size), &tmp_fetch, type_size,
pe, op, datatype);
shmem_transport_get_wait((shmem_transport_ctx_t *)ctx);
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this outside the loop over count? I don't quite see why we need to wait after every 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.

Nope, we cant as the loop itself can go beyond the number of atomics that causes the issue on CXI.

src/shmem_comm.h Outdated
@@ -336,6 +336,7 @@ shmem_internal_atomicv(shmem_ctx_t ctx, void *target, const void *source,
shmem_internal_fetch_atomic(ctx, ((uint8_t *) target) + (i * type_size),
((uint8_t *) source) + (i * type_size), &tmp_fetch, type_size,
pe, op, datatype);
shmem_transport_get_wait((shmem_transport_ctx_t *)ctx);
#ifdef SOS_OFI_CXI_QUIET_RATE
Copy link
Member

Choose a reason for hiding this comment

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

Is there any chance we can remove this after the above fix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think based on the atomic OP, we cant get rid of this completely. But, will check.

Copy link
Member

@davidozog davidozog left a comment

Choose a reason for hiding this comment

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

As discussed offline, perhaps SHMEM_REDUCE_ALGORITHM=recdbl (maybe ring?) is a better workaround (when disabling non-fetching AMO) given this could dramatically impair the performance of linear and tree algorithms.

@wrrobin wrrobin merged commit 34b5727 into Sandia-OpenSHMEM:main Sep 19, 2024
36 checks passed
davidozog pushed a commit to davidozog/sandia-shmem that referenced this pull request Oct 9, 2024
* Fix for atomicv on CXI

* Removing periodic quiet
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants