-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Add support for building on s390x platform #8962
Add support for building on s390x platform #8962
Conversation
…llback arg This is required on big endian platforms which would truncate an int to 0.
only when PORTABLE=1, otherwise use default -march-native.
Summary: s390x has a CACHE_LINE_SIZE of 256 but the following tests failed with that size: * Full/FullBloomTest.OptimizeForMemory/0 * DBPropertiesTest.AggregatedTableProperties * DBPropertiesTest.AggregatedTablePropertiesAtLevel On an x86_64 build using TEST_CACHE_LINE_SIZE == 256, the same tests failed. The expected values in the tests were conditionally updated for CACHE_LINE_SIZE >= 256 on x86_64. Test Plan: make check on x86_64 with: * TEST_CACHE_LINE_SIZE unset * TEST_CACHE_LINE_SIZE=128 * TEST_CACHE_LINE_SIZE=256 make check on s390x with: * TEST_CACHE_LINE_SIZE unset * TEST_CACHE_LINE_SIZE=64 * TEST_CACHE_LINE_SIZE=128
Summary: LZ4_Uncompress only reads the first 4 bytes of input_data into output_len when format_version==1. This gets the wrong end of the bits on big endian platforms even in the case where the file was written on big endian. Fix reading the length so a file written on big endian can be read on big endian. This allows related tests to pass on big endian. Reading and writing across endianness is still not supported.
Summary: Older versions of g++ up to at least 5.4 did not understand -march=native. Check that flag on s390x and use -march=z196 instead.
@jonathan-albrecht-ibm Hi there, yes I can certainly try and help...
Do you have such a package? If so, I think we can get this uploaded? If not I will need to try and build one on an s390x which could take a little longer...
If I recall correctly, I think this is only needed if we decide to release RocksJava binaries for s390x. It isn't actually needed for CI. I did start on one of these Images here - https://github.com/evolvedbinary/docker-rocksjava/blob/master/centos7_s390x/Dockerfile |
Thanks @adamretter. I don't have a deb package for s390x cmake 3.14.5 yet. The distros I build on have a newer cmake. I can try and build one tomorrow and let you know how it goes. Thanks for the pointer to the Dockerfile. It would be really great to get s390x support released in the RocksJava binaries. We're also building other projects that depend on RocksJava so releases with s390x support would be a big help. I'll try out the scripts and see if I can get it working. Also, I see there is a problem with the older gcc version that travis (xenial) uses doesn't recognize |
@adamretter I was able to build a cmake package locally. I've sent you a link with details when you get a chance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look fine. I can't guarantee that our Travis integration will keep working, but add what you want while it is working.
Thanks for the review, @pdillinger! I think all that is still needed for travis is to upload a cmake-3.14.5-Linux-s390x.deb to the rocksdb-deps AWS s3 but I'll need help with that. @adamretter did you get the link to the github repo I sent with my local cmake deb build? Hope that works ok for you. I can't publicly publish binaries due to work policies but feel free to contact me at the email address on my profile https://github.com/jonathan-albrecht-ibm if there's an easier way. |
@adamretter just wanted to check if I could get some help uploading the cmake-3.14.5-Linux-s390x.deb to the rocksdb-deps AWS s3. I noticed there is a cmake-3.14.5-Linux-s390x.tar.gz already there so tried it out on a fork and it was all that was needed to get a clean build so I don't think anything else is needed for this PR to be ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -190,7 +190,11 @@ extern void InitOnce(OnceType* once, void (*initializer)()); | |||
#define ALIGN_AS(n) /*empty*/ | |||
#else | |||
#if defined(__s390__) | |||
#if defined(__GNUC__) && __GNUC__ < 6 | |||
#define CACHE_LINE_SIZE 64U |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a good explanation but there's a bit more info here: #4022. I think that older gcc incorrectly warned when the cache line size was set greater than 64.
@@ -1136,7 +1136,11 @@ inline CacheAllocationPtr LZ4_Uncompress(const UncompressionInfo& info, | |||
if (input_length < 8) { | |||
return nullptr; | |||
} | |||
memcpy(&output_len, input_data, sizeof(output_len)); | |||
if (port::kLittleEndian) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have copied into a uint64_t and cast down to uint32_t, which to me is more understandable. But OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx, make sense
@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Thanks @pdillinger! |
@pdillinger merged this pull request in e970248. |
Wow, thank you. While I might never be hands on with an s390 I think it is great that RocksDB can run there. |
Hi @jonathan-albrecht-ibm, thanks for adding support for s390x. |
Hi @riversand963, yes, just need to make sure those jobs are unconditionally excluded in |
@jonathan-albrecht-ibm that will be great if you can contribute a fix! Tmr is fine. |
@riversand963 I've opened #9095 to exclude cmake + s390x in Travis |
Summary: This PR adds support for building on s390x including updating travis CI. It uses the previous work in facebook/rocksdb#6168 and adds some more changes to get all current tests (make check and jni tests) to pass. The tests were run with snappy, lz4, bzip2 and zstd all compiled in. There are a few pieces still needed to get the travis build working that I don't think I can do. adamretter is this something you could help with? 1. A prebuilt https://rocksdb-deps.s3-us-west-2.amazonaws.com/cmake/cmake-3.14.5-Linux-s390x.deb package 2. A https://hub.docker.com/r/evolvedbinary/rocksjava s390x image Not sure if there is more required for travis. Happy to help in any way I can. Pull Request resolved: facebook/rocksdb#8962 Reviewed By: mrambacher Differential Revision: D31802198 Pulled By: pdillinger fbshipit-source-id: 683511466fa6b505f85ba5a9964a268c6151f0c2
This PR adds support for building on s390x including updating travis CI. It uses the previous work in #6168 and adds some more changes to get all current tests (make check and jni tests) to pass. The tests were run with snappy, lz4, bzip2 and zstd all compiled in.
There are a few pieces still needed to get the travis build working that I don't think I can do. @adamretter is this something you could help with?
Not sure if there is more required for travis. Happy to help in any way I can.