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

Add support for building on s390x platform #8962

Closed

Conversation

jonathan-albrecht-ibm
Copy link
Contributor

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?

  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.

adamretter and others added 10 commits September 23, 2021 14:44
…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.
@adamretter
Copy link
Collaborator

@jonathan-albrecht-ibm Hi there, yes I can certainly try and help...

A prebuilt https://rocksdb-deps.s3-us-west-2.amazonaws.com/cmake/cmake-3.14.5-Linux-s390x.deb package

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...

A https://hub.docker.com/r/evolvedbinary/rocksjava s390x image

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

@jonathan-albrecht-ibm
Copy link
Contributor Author

jonathan-albrecht-ibm commented Sep 28, 2021

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 -march=native. I'll try to push a commit to fix that soon.

@jonathan-albrecht-ibm
Copy link
Contributor Author

@adamretter I was able to build a cmake package locally. I've sent you a link with details when you get a chance.

@pdillinger pdillinger self-requested a review October 4, 2021 19:14
Copy link
Contributor

@pdillinger pdillinger left a 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.

@jonathan-albrecht-ibm
Copy link
Contributor Author

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.

@jonathan-albrecht-ibm
Copy link
Contributor Author

@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.

Copy link
Contributor

@pdillinger pdillinger left a 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why

Copy link
Contributor Author

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) {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx, make sense

@facebook-github-bot
Copy link
Contributor

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@jonathan-albrecht-ibm
Copy link
Contributor Author

LGTM

Thanks @pdillinger!

@facebook-github-bot
Copy link
Contributor

@pdillinger merged this pull request in e970248.

@mdcallag
Copy link
Contributor

Wow, thank you. While I might never be hands on with an s390 I think it is great that RocksDB can run there.

@riversand963
Copy link
Contributor

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.

Hi @jonathan-albrecht-ibm, thanks for adding support for s390x.
Before @adamretter uploads the cmake deb to S3, would it be possible to disable only the cmake + s390x combination temporarily?

@jonathan-albrecht-ibm
Copy link
Contributor Author

Hi @jonathan-albrecht-ibm, thanks for adding support for s390x. Before @adamretter uploads the cmake deb to S3, would it be possible to disable only the cmake + s390x combination temporarily?

Hi @riversand963, yes, just need to make sure those jobs are unconditionally excluded in .travis.yml. Would you like me to open a PR for that? I won't be able to get to it until tomorrow though.

@riversand963
Copy link
Contributor

@jonathan-albrecht-ibm that will be great if you can contribute a fix! Tmr is fine.

@jonathan-albrecht-ibm
Copy link
Contributor Author

@riversand963 I've opened #9095 to exclude cmake + s390x in Travis

yoori pushed a commit to yoori/rocksdb that referenced this pull request Nov 26, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants