prov/efa: Improve efa_rdm_pke definition

[darrylabbate]

This removes the hardcoded padding bytes from the struct definition, which were ostensibly used as a mechanism to enforce alignment of wiredata to the 128 byte boundary.

While the static_asserts somewhat protect against changes made to the members which the struct is composed of, the manual calculation is a maintenance headache which is prone to developer error, as well as implementation-defined quirks.

More importantly, this change more clearly indicates the intent to align to the 128-byte boundary.

This commit also makes the struct size uniform when debug mode is enabled. This wasn't (currently) necessary, as the 32 bytes of padding were already sufficient for the 16-byte dlist_entry struct.

This is the C11-compliant version, utilizing _Alignas (stdalign.h)

For sake of comparison, I also pushed a (less-preferred) C99 version: darrylabbate@b479b5c

[shijin-aws]

Do we need to make EFA_RDM_PKE_ALIGNMENT 256 when ENABLE_DEBUG=1?

[darrylabbate]

No. This change makes the size of struct efa_rdm_pke 128 bytes regardless of whether debug mode is enabled or not. The extra struct dlist_entry when debug mode is enabled is currently smaller than the unused space added by _Alignas(128).

In the event the preceding struct members exceed 128 bytes, wiredata would be at the 256 byte offset, since _Alignas(128) specifies it be aligned to the nearest 128 byte boundary (Try it online!). So EFA_RDM_PKE_ALIGNMENT should remain as the intended alignment for wiredata

[sunkuamzn]

Then why was there a uint8_t pad[112] in the ENABLE_DEBUG condition? It seems like we were trying to align to 256 when ENABLE_DEBUG=1

[sunkuamzn]

But performance doesn't matter in debug mode, so I think it's OK to align to 128 all the time

[darrylabbate]

Then why was there a uint8_t pad[112] in the ENABLE_DEBUG condition? It seems like we were trying to align to 256 when ENABLE_DEBUG=1

You can look through the blame/commit history for context. From my view, it's a side-effect of:

1. Padding was originally added just before wiredata to align it to 128 bytes.
2. The debug struct dlist_entry was later added further up the struct, which naturally modifies the size/padding. Instead of modifying the original padding at the end of the struct, ENABLE_DEBUG-only padding was added to align it to 192 bytes.
3. 64 more ENABLE_DEBUG-only bytes were added to align it to 256 due to cache misses.

...which illustrates precisely why manual struct alignment should be avoided when possible (packing is a different story). _Alignas(128) will always keep wiredata aligned to a 128 byte offset/boundary without relying on future developers to manually maintain the memory layout of a struct.

[shijin-aws]

Then why was there a uint8_t pad[112] in the ENABLE_DEBUG condition? It seems like we were trying to align to 256 when ENABLE_DEBUG=1

It was aimed to make both debug and non-debug builds to be 128 aligned. There are more members introduced in debug build which makes efa_rdm_pke exceed 128 bytes, so we seek for the next 128 aligned integer (256)

[shijin-aws]

We are safe as long as the total size of efa_rdm_pke can be multiples of 128 bytes for both builds

[darrylabbate]

AWS CI failing due to Windows build test, which uses msbuild.exe to compile as suggested by aws/efawin. Likely due to the C11 feature. Curious if it can be configured to play nicely with C11.

[darrylabbate]

AWS CI failing due to Windows build test, which uses msbuild.exe to compile as suggested by aws/efawin. Likely due to the C11 feature. Curious if it can be configured to play nicely with C11.

Reached out to @gurugubs for some guidance here. We can either override the VisualStudioVersion by specifying -property:VisualStudioVersion=... for msbuild.exe, or redefine it in the .sln files themselves. I'm not a Windows/VS expert though, so I may be wrong.

[darrylabbate]

Added a (test) commit to bump VisualStudioVersion to 17 just to see what happens.

[darrylabbate]

Per @gurugubs, the VS-related files are fine to edit manually (and have been in the past). Specifying /std:c11 in AdditionalOptions seems to do the trick wrt C11 features, along with a new-enough VS version. Currently testing and trying out various changes to get a sufficient set of minimal changes.

[darrylabbate]

bot:aws:retest

[darrylabbate]

Capturing an internal Slack conversation: GCC 4.7 is the earliest version with _Alignas support. The few older OSs I tested all had GCC 4.8+ installed by default. From a cursory glance, [this list] also seems to indicate 4.8+ as the minimum default version for a respective gcc package.

[darrylabbate]

Rebased commits

[darrylabbate]

Rebasing again

[shijin-aws]

We are safe as long as the total size of efa_rdm_pke can be multiples of 128 bytes for both builds

[shijin-aws]

Do you think this need to be part of 1.20 release? @darrylabbate