fabtests/rdm_atomic, ubertest: move atomic verification to common code and use in functional test #10155
Conversation
fabtests/common/shared.c
Outdated
| ret = ft_hmem_copy_from(opts.iface, opts.device, dev_host_comp, | ||
| cmp, count * datatype_to_size(type)); | ||
| if (ret) { | ||
| FT_ERR("Failed to copy from atomic buffer\n"); | ||
| return ret; | ||
| } | ||
|
|
||
| check_comp = dev_host_comp; |
There was a problem hiding this comment.
Is check_comp needed when atomic != FI_ATOMIC_COMPARE?
There was a problem hiding this comment.
No, I think I left it because coverity gets really confused by the unused variables with the atomic cases. But I'm going to refactor this whole check which is looking really messy. I'll push an update when I figure out the other failures.
|
Looks like MS compiler doesn't like the complex types: |
aingerson
force-pushed
the
fabtests
branch
2 times, most recently
from
39730b5
to
be4ef93
Compare
fabtests/common/ofi_atomic.c
Outdated
| OFI_UNUSED(src); \ | ||
| for (i = 0; i < cnt; i++) \ | ||
| r[i] = d[i]; \ | ||
| memcpy(res, dst, sizeof(type) * cnt); \ |
There was a problem hiding this comment.
should use ofi_complex_##type here.
fabtests/common/ofi_atomic.c
Outdated
| ofi_complex_##type *r = (res); \ | ||
| for (i = 0; i < cnt; i++) { \ | ||
| r[i] = d[i]; \ | ||
| memcpy(res, dst, sizeof(type) * cnt); \ |
fabtests/common/ofi_atomic.c
Outdated
| ofi_complex_##type *r = (res); \ | ||
| for (i = 0; i < cnt; i++) { \ | ||
| r[i] = d[i]; \ | ||
| memcpy(res, dst, sizeof(type) * cnt); \ |
|
@acgoldma This PR is adding atomic verification to the regular fabtests runs. psm3 was failing a lot of them so I've added a few patches to fix most of them. The last patch disables the remaining failing combinations instead of fixing them because I couldn't completely root cause - long double and complex long double reads/swaps are failing and I'm not sure why. Switching the sets to a memcpy of the datatype size also fixes the issue but it takes a performance hit (and also affects all other data types for those ops) so I opted to disable instead of adding that fix. I did this by having all the reads and compares go through the PSM_CUDA | PSM_ONEAPI implementation which does the psmx3_memcpy (which internally switches to a memcpy for the same ifdef). Overall, something is off when doing |
We will take a look, but if you have time, it might be better to also adjust psmx3 to use Also, can you give the exact commands to replicate this so we can reproduce internally. |
|
@aingerson Can you provide some details on the failing PSM test cases, such as the specific commands executed and any additional details on the failure symptom? For example, it seems like |
|
@acgoldma @brooksmi Hello! Sorry for the late response, our clusters have all been down for the last few days so I haven't had any systems to test things on. |
Ah! NOTE: I don't see |
|
@brooksmi Ah shoot, missed adding that. Will add, thank you! |
|
@aingerson I'm curious if this is compliant or not, i.e. whether any spec says whether the remaining 48 bits are undefined or must be zero'd. But I also notice that, at least in trivial benchmarking, |
|
@brooksmi Thanks for that context, very helpful. Where do you see that a long double is 8 bits? (Sorry I'm not familiar with long double CPU operations). |
Both language standards and compiler documentation are flexible on the actual implementation. The C language simply states that GCC documentation has quite a few mentions of x86_64 using x87 80-bit in several places (as defined by Intel SDM vol 1), but ultimately it's target-specific, and could change if/when a CPU is released with native 128-bit FPU registers.
You could check If you're using GCC, you can do I suspect a language lawyer would argue that you should not be relying on any sort of The |
Note that logic would also imply avoiding It might be better to iterate elementwise, and write random valid typed floating point values in the fill, then iterate on a typed |
|
@brooksmi Yeah, that's what I'm going to move to. It was originally that way but I moved to the memset/memcmp because it was more performant and simplified the implementation for complex types. Back to the original method it is... thanks for your help! |
|
@brooksmi Thanks for the context with the long double issues, Michael! I've repushed a version that does an element by element set and check to be explicit with the datatype. I've kept the three psm3 patches though to resolve a few actual psm3 issues regarding complex compares, missing ops, and missing error checks. Let me know if those look ok to you. Thanks! |
@aingerson The other PSM changes look fine. |
aingerson
force-pushed
the
fabtests
branch
4 times, most recently
from
c4f8c71
to
9e5b90d
Compare
Fix incorrect atomic LOR on complex numbers. The values were incorrectly getting ANDed together instead of ORed. This went unnoticed because the code was very difficult to read. This also refactors the logical checks with a helper function to make it more readible and less prone to errors. Signed-off-by: Alexia Ingerson <[email protected]>
This allows fabtests to make use of atomic validation code
There were many Windows atomics bugs, inconsistencies, and missing
definitions. This patch also cleans up the entire ofi_atomic.c
implementation for unix and windows
The following changes are included:
- Separate fill and check based on real or complex types as setting
and reading complexes on windows is not allowed (not native datatype, abstracted).
Complex versions use eq and set functions specific for complexes defined in osd.h
- Remove duplicated ofi_complex definitions in ofi_atomic (already in osd.h file)
- Add general check_atomic and fill_atomic calls and use them in ubertest
- Add EXPAND ( x ) x define to work nicely with windows VA_ARGS handling
- Fix inconsistency with ofi_complex_type/or naming ('complex' always should come first)
- Fix inconsistency with op names "equ" and "mul" -> "eq" and "prod"
- Add missing lxor complex op definitions on Windows
Signed-off-by: Alexia Ingerson <[email protected]>
To properly validate atomic data, we need host bounce buffers for the result and compare buffers in addition to the regular bounce buffer for the tx/rx bufs. This adds two extra bufs allocated only for atomic purposes and adds hmem support to the common atomic validation path. It also renames the alloc/free_tx_buf calls to generic alloc/free_host_bufs which allocates all three buffers at once. Signed-off-by: Alexia Ingerson <[email protected]>
ft_post_atomic posted "buf" which is the base address for the entire send and recv buffer allocation. The first half of the allocation is the receive buffer and the second half is the send buffer. Posting just "buf" meant it was sending the receive buffer. This changes it to send the tx buf and do an atomic on the rx buf which allows us to properly do atomic validation Signed-off-by: Alexia Ingerson <[email protected]>
This allows us to post the rx buf without corrupting memory in case its needed for validation Signed-off-by: Alexia Ingerson <[email protected]>
Match the behavior of memset() where the value passed in is an int, but it is interpreted as a char. While ZE can technically handle this scenario, others may not so we need to standardize across ifaces Signed-off-by: Alexia Ingerson <[email protected]>
Add data validation to the atomic test by using the newly added atomic fill and check support imported from ubertest. This code uses a macro that switches on datatype for filling and checking the buffer contents. The atomic validation path requires an extra buffer to copy the contents of the original atomic buffer in order to recreate the atomic function locally and check the buffer against the simulated atomic operation. This patch also refactors the entire test to remove the extremely confusing macros used for the base/fetch/compare operations. The macros made the code extremely difficult to read and debug and also made it difficult to add data validation. Separating it into three explicit functions is about the same amount of code and significantly more readable Synchronization messages are added in the validation case to ensure the atomic operation completed on both sides before validation occurs. This requires the addition of the FI_ORDER_SAW and FI_ORDER_SAR message ordering to ensure that we get the completion for the send/recv sync after the atomic message is processed Signed-off-by: Alexia Ingerson <[email protected]>
Run fi_rdm_atomic with data validation in standard and short test suites Signed-off-by: Alexia Ingerson <[email protected]>
Signed-off-by: Alexia Ingerson <[email protected]>
Comparison of complex numbers is undefined and not a valid combination of atomic ops. Disable in psm3 Signed-off-by: Alexia Ingerson <[email protected]>
Report atomic op errors back to the application. Some datatype/op combinations were falsely being reported to the application but failing when the atomic was being performed. These failures were silently treated as successful because the errors were not passed back. Check the error code to catch future issues Signed-off-by: Alexia Ingerson <[email protected]>
psm3 advertises support for logical ops (lor, land, lxor) with all datatypes but the functions are only defined for integer types. When the atomic op is called with a non-integer type, it drops down to the default case and returns an error (FI_ENOTSUPP) Signed-off-by: Alexia Ingerson <[email protected]>
|
@j-xiong This is finally good to go! I added testing for all the atomics on windows to make sure the implementation was correct on both platforms. Intel CI failure is a known issue Zach is fixing soon and unrelated (MPI rxm IMB slurm timeout). Had to add a few more patches because of psm3 and windows bugs. Review or merge if it looks good to you, thanks! |
Ubertest copies the software implementation of atomics from libfabric in order to add atomic operation data validation. This is done by saving the sent data and performing a local atomic in order to validate against the actual data calculated by the libfabric provider.
This patch set moves that implementation from ubertest to the common code for other tests (specifically the fi_rdm_atomic test) to leverage it.
Ubertest atomics with validation is not run in the CI by default but he rdm_atomic test is. Because of the lack of validation in the functional fabtest, an incorrect fetched buffer pointer was never caught. This patch set also enables running rdm_atomic with validation in the standard and short test sets in runfabtests.sh so we can catch this in the future.
During testing of these changes, it properly caught the previously missed bug and properly passes when it was fixed.