Skip to content
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

zLinux build on s390x produce warning #4022

Open
koldat opened this issue Jun 19, 2018 · 10 comments
Open

zLinux build on s390x produce warning #4022

koldat opened this issue Jun 19, 2018 · 10 comments
Assignees

Comments

@koldat
Copy link
Contributor

koldat commented Jun 19, 2018

It seems that gcc for -march=z10 shows warning for predefined CACHE_LINE_SIZE 256 introduced in:
16e0388 and ccf5f08

 #ifndef CACHE_LINE_SIZE
-#define CACHE_LINE_SIZE 64U
+  #if defined(__s390__)
+    #define CACHE_LINE_SIZE 256U
...

I am not sure what is effect (only performance?) of aligning in RocksDB. Also I do not know what compiler was used so that it did not produced this warning. @grooverdan ?

Expected behavior

No warning

Actual behavior

00:01:01.208 CC jls/cache/lru_cache.o
00:01:01.503 In file included from cache/lru_cache.cc:14:0:
00:01:01.503 ./cache/lru_cache.h:157:33: warning: requested alignment 256 is larger than 64 [-Wattributes]
00:01:01.503 class ALIGN_AS(CACHE_LINE_SIZE) LRUCacheShard : public CacheShard {
00:01:01.503 ^
00:01:02.086 CC jls/cache/sharded_cache.o

Steps to reproduce the behavior

Compile on zLinux s390x with gcc:
[root@linux321 RocksDB_Build_s390x]# uname -a
Linux linux321.ca.com 3.10.0-693.21.1.el7.s390x #1 SMP Fri Mar 23 16:11:08 EDT 2018 s390x s390x s390x GNU/Linux

[root@linux321 RocksDB_Build_s390x]# gcc --version
gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-28)

@koldat koldat changed the title zLinux build on s390x zLinux build on s390x produce warning Jun 19, 2018
@yiwu-arbug
Copy link
Contributor

@koldat The aligning is only for performance sake. You can change CACHE_LINE_SIZE to 64 to get around. I'm not familiar with zLinux and s390. What should be the correct cache line size?

@yiwu-arbug yiwu-arbug self-assigned this Jun 19, 2018
@grooverdan
Copy link
Contributor

grooverdan commented Jun 19, 2018

While 256 is the correct cache size for zLinux/s390 t appears this is above the stdc / gcc defined maximum of sizeof(std::max_align_t)

I think the most compatible thing it to take the minimum of the existing cache size and std::max_align_t

refs:
https://stackoverflow.com/questions/15523537/alignas-specifier-vs-attribute-aligned-c11
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2341.pdf
https://reviews.llvm.org/rL201729
https://github.com/gcc-mirror/gcc/blob/master/gcc/ginclude/stddef.h#L426..L437
TBD - rhel have backported https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56019 ?

@grooverdan
Copy link
Contributor

ignore, was probably on wrong track. sizeof(::max_align_t) was 32 and and alignof(::max_align_t) varied was 8 or 16 depending on architecture. the standard specified alignof(::max_align_t) as the minimum however the maximum isn't defined.

I assume the warning is indicating the compiler is reducing it. On Power8 I had the compiler warning and pushing down to 128 when a 256 alignment was requested. Odd that s390 has a lower limit.

@koldat
Copy link
Contributor Author

koldat commented Jun 20, 2018

Does it really have that high impact (I assume that operations required to process the cache is much higher)? Isn't 64 bytes good compromise?

@grooverdan
Copy link
Contributor

There's a small loss in cache efficiency at higher values because some is empty however the aim is that concurrent accesses to adjacent memory structures get separated so they don't wait on cycles to obtain the cache line on concurrently updated that are being updated on different CPUs. Measurement depends on architecture so feel free to test (I should too). 'perf c2c' and the perf stat will be able to measure impacts of cache hardware events to measure contention.

@koldat
Copy link
Contributor Author

koldat commented Jun 20, 2018

Should we use BIGGEST_ALIGNMENT macro then?

@grooverdan
Copy link
Contributor

BIGGEST_ALIGNMENT appears to be alignof(::max_align_t) - quickly tested on multiple arches by doing int main() { return BIGGEST_ALIGNMENT; } https://gcc.godbolt.org/. Its the biggest alignment of a basic type rather than the maximum value for the compiler.

Looking at https://github.com/gcc-mirror/gcc/blob/master/gcc/varasm.c#L2106 i think the maximum alignment is equal to the max(size of the type, BIGGEST_ALIGNMENT). It seems it was a warning https://github.com/gcc-mirror/gcc/blob/gcc-4_8_5-release/gcc/varasm.c#L1849

Interestingly sizeof(rocksdb::LRUCacheShard)) is 256 bytes (sizeof rocksdb::StatisticsImpl 104 on x86 - gets bumped to 128 on Power).

So sorry, still don't have a good solution.

@koldat
Copy link
Contributor Author

koldat commented Jun 21, 2018

Sure take your time. Thanks a lot that you are looking into it. Do you think this warning can cause some issue other than slowdown? When I was googling it was not a exception to see words like "undefined". I am using DISABLE_WARNING_AS_ERROR=1 for now. But not sure if it can cause instability. It is always worse than slowdown. If you think it can cause troubles I would introduce "PREFER_SLOW" macro as temporary solution.

@adamretter
Copy link
Collaborator

Likewise compiling on s390x z15 with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.12) 5.4.0 20160609 I see:

In file included from cache/lru_cache.cc:10:0:
./cache/lru_cache.h:192:35: error: requested alignment 256 is larger than 64 [-Werror=attributes]
 class ALIGN_AS(CACHE_LINE_SIZE) LRUCacheShard final : public CacheShard {

@adamretter
Copy link
Collaborator

I believe I have recently fixed this in 6253dd1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants