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

spdk: -march=core2 instead of -march=native #23078

Merged
merged 1 commit into from Jul 20, 2018

Conversation

smithfarm
Copy link
Contributor

-march=native causes gcc to use opcodes according to whichever CPU happens to
be installed in the build host, which can be different for every build. This
makes it impossible to achieve a reproducible build.

Also, if the build host has a very new CPU, running the resulting binaries on
older CPUs (of the same family, i.e. x86_64) could result in segmentation
fault.

Hopefully nobody will be running Ceph on x86_64 CPUs older than Core2 (?)

Fixes: http://tracker.ceph.com/issues/24948
Signed-off-by: Nathan Cutler ncutler@suse.com

-march=native causes gcc to use opcodes according to whichever CPU happens to
be installed in the build host, which can be different for every build. This
makes it impossible to achieve a reproducible build.

Also, if the build host has a very new CPU, running the resulting binaries on
older CPUs (of the same family, i.e. x86_64) could result in segmentation
fault.

Hopefully nobody will be running Ceph on x86_64 CPUs older than Core2 (?)

Fixes: http://tracker.ceph.com/issues/24948
Signed-off-by: Nathan Cutler <ncutler@suse.com>
@bmwiedemann
Copy link

It is also worth noting that the current code throws a compile error when built with a X86 CPU or -march older than core2.

According to Wikipedia, core2 CPUs were released 2006-2009.
On the AMD side, CPUs with equivalent SSSE3 support appeared 2011 in Bulldozer and Bobcat.

@smithfarm
Copy link
Contributor Author

@bmwiedemann Indeed. In case anyone is interested, that error was reproduced in the CI, and quoted at #23076 (comment)

@tchaikov
Copy link
Contributor

it does address the problem, but i am not sure it is the right route in long term to patch SPDK. i'd suggest add an option named --machine allowing configure to override the TARGET_MACHINE, which comes from gcc --dumpmachine by default. and send the patch to SPDK upstream.

@bmwiedemann
Copy link

this?

# gcc -dumpmachine
x86_64-suse-linux

@ifed01
Copy link
Contributor

ifed01 commented Jul 17, 2018

+1 to @tchaikov on introducing additional explicit option to denote target machine. One of the drawback of this proposed solution - it might negatively impact the performance as we stop benefiting from new CPU features.

@smithfarm
Copy link
Contributor Author

smithfarm commented Jul 17, 2018

@tchaikov did you mean add an option (calling it "x86_64-flavor" here) to configure allowing to override the -march setting when TARGET_MACHINE equals "x86_64", and make it default to -march=native? Then we (i.e. Ceph in its SPDK build) could call configure with --x86_64-flavor=core2.

I agree that would probably be more acceptable to SPDK upstream.

@ifed01 I agree that there is a potential negative performance impact, which is the price we are paying here for a reproducible build.

@ifed01
Copy link
Contributor

ifed01 commented Jul 17, 2018

@smithfarm - my major concern is that such impact is unavoidable and well-hidden with this patch.

@tchaikov
Copy link
Contributor

@smithfarm i mean, a --machine option, so one can pass --machine=default to dpdk's configure script to prevent it from adding -march=native to CFLAGS. it's not specific to amd64. i believe that the downstream packagers would want to use this option as well, if they plan to package SPDK.

@smithfarm
Copy link
Contributor Author

smithfarm commented Jul 17, 2018

@tchaikov Wouldn't SPDK have to first fix the bug where it fails to compile (on x86_64) if -march=native is removed, though?

UPDATE: Also, I assume completely removing -march would cause gcc to build for the oldest possible x86_64 CPU, which might not be desirable. . . . In this PR we are making a compromise by going with -march=core2 which is old, but not that old.

@smithfarm
Copy link
Contributor Author

@ifed01 I understand, and would consider adding something to the documentation if we knew what the performance impact really was.

The problem I see here is we are shipping only one set of packages. If I understand this issue correctly, if we build SPDK with -march=native on a very new CPU, the binaries shipped in the resulting packages (i.e. official Ceph packages) might segfault on older CPUs.

If we had an option to publish different flavors of the build, we could try to build on a very new CPU and publish a flavor for that type of CPU. But that's beyond the scope of this PR.

@tchaikov
Copy link
Contributor

@smithfarm yeah, that issue should be addressed also. might want to report it back to upstream, though.

@liewegas liewegas merged commit e2104f7 into ceph:master Jul 20, 2018
liewegas added a commit that referenced this pull request Jul 20, 2018
* refs/pull/23078/head:
	spdk: -march=core2 instead of -march=native

Reviewed-by: Josh Durgin <jdurgin@redhat.com>
@smithfarm smithfarm deleted the wip-spdk-march-core2 branch July 21, 2018 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants