[SERVER-65664] assumptions about hardware_constructive_interference_size and hardware_constructive_interference_size Created: 14/Apr/22 Updated: 29/Oct/23 Resolved: 09/May/22 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 6.1.0-rc0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Khem Raj | Assignee: | Andrew Morrow (Inactive) |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||
| Operating System: | ALL | ||||||||
| Sprint: | Dev Platform 2022-04-18, Dev Platform 2022-05-02, Dev Platform 2022-05-16 | ||||||||
| Participants: | |||||||||
| Description |
|
GCC 12 has added a warning -Winterference-size when these are used without specifying its value. see https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Winterference-size
This is causing a build failure on aarch64 since hardware_constructive_interference_size defaults to 256 on this platform. we get error
In file included from src/mongo/s/commands/cluster_find_cmd.cpp:39: I think the assumption about these defaults are perhaps not right in mongodb, I also opened a gcc bug where there is some discussion around it.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105251
perhaps 'struct Together' should be aligned to hardware_constructive_interference_size to promote true sharing and not to CacheAligned which is aligned to hardware_destructive_interference_size |
| Comments |
| Comment by Githook User [ 06/May/22 ] | |||
|
Author: {'name': 'Andrew Morrow', 'email': 'acm@mongodb.com', 'username': 'acmorrow'}Message: | |||
| Comment by Andrew Morrow (Inactive) [ 06/May/22 ] | |||
|
raj.khem@gmail.com - That's great to hear, thanks for reporting the issue and for testing out the proposed fix. | |||
| Comment by Khem Raj [ 06/May/22 ] | |||
|
yes it seems to work in my limited testing with gcc-12. | |||
| Comment by Andrew Morrow (Inactive) [ 06/May/22 ] | |||
|
Hi raj.khem@gmail.com - I was curious if you had tried the above patches and could follow up about whether or not they fix the issue in your environment. | |||
| Comment by Andrew Morrow (Inactive) [ 29/Apr/22 ] | |||
|
raj.khem@gmail.com - Also, just in case you are interested, here is a side-by-side of the change I'm proposing: https://github.com/acmorrow/mongo/compare/a20861fbbdc72e721879da9a47a77887255dbcf7...SERVER-65664 | |||
| Comment by Andrew Morrow (Inactive) [ 29/Apr/22 ] | |||
|
Thanks raj.khem@gmail.com - You can find my work-in-progress branch here: https://github.com/acmorrow/mongo/commits/SERVER-65664
| |||
| Comment by Khem Raj [ 29/Apr/22 ] | |||
|
@andrew.morrow sure, let me know where to pick up the patches | |||
| Comment by Andrew Morrow (Inactive) [ 29/Apr/22 ] | |||
|
Hi raj.khem@gmail.com - We are working on a more polished fix for this, however, I don't have immediate handy access to GCC 12. Would you be willing to test out a work-in-progress branch with your toolchain and let me know the results? | |||
| Comment by Khem Raj [ 15/Apr/22 ] | |||
|
you solution works fiine with gcc12/aarch64 as well. | |||
| Comment by Andrew Morrow (Inactive) [ 14/Apr/22 ] | |||
|
No, I don't think you don't want to take the CacheAligned off _together: it is there to prevent false sharing of the two fields within _together with the other counters, like _physicalBytesOut and _logicalBytesOut. You could, I suppose, add an alignas(stdx::hardware_constructive_interference_size) to the Together type, but I don't think it matters in practice, since the value is already being wrapped in CacheAligned and that will ensure that it has larger alignment anyway. The quick fix is just to make the static_assert look at the size of the thing to be kept on one cache line, rather than the size of the thing to prevent other things from sharing that cache line. We will look into improving the naming of CacheAligned and providing some more detailed wrapper types that do this a little better. | |||
| Comment by Khem Raj [ 14/Apr/22 ] | |||
|
I will try this out. I had a local fix which is compling ok too
| |||
| Comment by Andrew Morrow (Inactive) [ 14/Apr/22 ] | |||
|
The purpose of CacheAligned is to prevent false sharing, so our use of std::destructive_hardware_interference_size is I believe correct for that. What is actually wrong is that the static_assert is checking the size of the destructively aligned type, and not the type itself. It is the inner type for which we care that it has a size smaller than std::hardware_constructive_interference_size in an effort to obtain true sharing. A secondary issue, as noted in the GCC ticket, is that we should probably be specifying that the inner type also has std::hardware_constructive_interference_size alignment. Can you see if the following change addresses your issue?
We will work on a fix to ensure that the proper alignment is affixed to the inner type. |