-
Notifications
You must be signed in to change notification settings - Fork 53
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
Fix for atomicv on CXI #1147
Conversation
@@ -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); |
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.
Can we move this outside the loop over count
? I don't quite see why we need to wait after every 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.
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 |
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.
Is there any chance we can remove this after the above fix?
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.
I think based on the atomic OP, we cant get rid of this completely. But, will check.
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.
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.
* Fix for atomicv on CXI * Removing periodic quiet
No description provided.