[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:
Problem/Incident
causes SERVER-66334 Coverity analysis defect 122186: Unin... Closed
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:
src/mongo/db/stats/counters.h:185:47: error: static assertion failed: cache line spill
 185 |     static_assert(sizeof(decltype(_together)) <= stdx::hardware_constructive_interference_size,
     |                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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: SERVER-65664 Provide more specific types for cache alignment needs
Branch: master
https://github.com/mongodb/mongo/commit/5435f9585f857f6145beaf6d31daf336453ba86f

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

 

-    CacheAligned<Together> _together{};
+    alignas(std::hardware_constructive_interference_size) Together _together{};

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?

-    static_assert(sizeof(decltype(_together)) <= stdx::hardware_constructive_interference_size,
+    static_assert(sizeof(Together) <= stdx::hardware_constructive_interference_size,
                   "cache line spill");

We will work on a fix to ensure that the proper alignment is affixed to the inner type.

Generated at Thu Feb 08 06:03:16 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.