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

cmake: identify the possible incompatibility of rocksdb and tcmalloc #17788

Merged
merged 6 commits into from Sep 20, 2017

Conversation

tchaikov
Copy link
Contributor

No description provided.

so it checks $ENV{GPERF_ROOT} first.

Signed-off-by: Kefu Chai <kchai@redhat.com>
Signed-off-by: Kefu Chai <kchai@redhat.com>
Signed-off-by: Kefu Chai <kchai@redhat.com>
Signed-off-by: Kefu Chai <kchai@redhat.com>
we've updated the rockdb wrapper on ceph side to be compatible with
the latest version of rocksdb upstream. so ceph is not compatible with
older version of rocksdb.

Signed-off-by: Kefu Chai <kchai@redhat.com>
@jtlayton
Copy link
Contributor

When trying to build that branch, I get this error:

-- Found cython
CMake Error at cmake/modules/BuildRocksDB.cmake:63 (message):
  Incompatible tcmalloc v2.5.93 and rocksdb v5.8.0, please use gperf-tools
  v2.5
Call Stack (most recent call first):
  src/CMakeLists.txt:818 (build_rocksdb)

Is that expected with these changes? If so, the error message is not particularly helpful. What does gperf-tools have to do with this? FWIW, I have gperftools-libs-2.5.93-1.fc26.x86_64, so I should be OK, right?

@tchaikov
Copy link
Contributor Author

tchaikov commented Sep 19, 2017

@jtlayton

Is that expected with these changes?

yes.

If so, the error message is not particularly helpful.

i am all ears.

What does gperf-tools have to do with this?

see the commit message of 94fc1f6.

FWIW, I have gperftools-libs-2.5.93-1.fc26.x86_64, so I should be OK, right?

no, you need to either upgrade your gperftools to the latest master or to downgrade it to 2.5 (without .93).

@badone
Copy link
Contributor

badone commented Sep 19, 2017

@jtlayton gperftools-libs contains libtcmalloc and this issue is a problem with the interaction of certain versions of rocksdb and tcmalloc. As for gperftools-libs-2.5.93-1.fc26.x86_64 that's the same version I'm running and it's definitely not OK. The reason this has remained undetected since @tchaikov updated rocksdb is that most OS are unaffected (I believe f26 is the only OS so far identified as having this problem). unittest_rocksdb_option fails in a similar fashion on f26 and yet that unit test has been passing consistently on jenkins systems and presumably others as well.

Copy link
Contributor

@badone badone left a comment

Choose a reason for hiding this comment

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

Agree with dmick. Apart from that LGTM.

the commit d406f228 in gperf implements a c11 feature used by a
recent change in rocksdb: 16e03882, which uses aligned_alloc().
and 16e03882 in rocksdb was merged after v5.7 was tagged, while
16e03882 in gperf was merged after v2.6.1 was tagged.

because aligned_alloc() is not implemented by tcmalloc until the
not-yet-released 2.6.2, if we call aligned_alloc() in an application
linked against tcmalloc, what gets called will be the glibc's
aligned_alloc(). but if we free() the memory chunk allocated by
aligned_alloc(), the tcmalloc's implementation kicks in, then
InvalidFree() is called, because the memory chunk being freed was
allocated by tcmalloc. in short, "mixing allocators", quote from
Dan Mick.

in rocksdb, aligned_alloc() is used if _ISOC11_SOURCE is defined, this
makes sense, because aligned_alloc() is a C11 function. we could avoid
using it by not defining _ISOC11_SOURCE. but as long as _GNU_SOURCE is
defined, glibc defines _ISOC11_SOURCE. and libstdc++ requires
_GNU_SOURCE, because it uses a fair amount of GNU extensions.

Fixes: http://tracker.ceph.com/issues/21422
Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov
Copy link
Contributor Author

@tchaikov tchaikov merged commit fa85dd0 into ceph:master Sep 20, 2017
@tchaikov tchaikov deleted the wip-cmake-rocksdb-tcmalloc branch September 20, 2017 02:32
@tchaikov
Copy link
Contributor Author

@jtlayton i put some details in the ticket, HTH.

w.r.t. to "If so, the error message is not particularly helpful." i will send a mail to ceph-devel to remind the fc25 users. i agree that the error message can be improved. if anybody can help with it. it will be much appreciated.

@jtlayton
Copy link
Contributor

jtlayton commented Sep 20, 2017

How about "use gperf-tools v2.6 or later" ? Your message says to use v2.5, but that's what I have and it doesn't work.

Ahh, nm...I see the patch now. Crap, then there is no working package for recent fedora releases. Can you open a bug for them to get the packages there updated? This is likely to break a lot of people that are building on Fedora.

@dmick
Copy link
Member

dmick commented Sep 20, 2017

I'm not saying this is a great idea, but we could maybe also make a local patch to rocksdb to not use alloc_aligned based on the version of libtcmalloc

@ivancich
Copy link
Member

@tchaikov The error message in cmake/modules/BuildRocksDB.cmake is off-by-1. It says "please install gperf-tools 2.5 or > 2.6.2" but the code accepts >= 2.6.2.

tchaikov added a commit to tchaikov/ceph that referenced this pull request Sep 22, 2017
it's a follow-up of ceph#17788

Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov
Copy link
Contributor Author

tchaikov commented Sep 22, 2017

@jtlayton @ivancich thanks for reviewing this PR , #17901 is posted to address your comments.

@dillaman
Copy link

Looks like Fedora 26 will release the fix under 2.6.1-5.fc26 so this change is will still prevent Ceph from being built on Fedora 26 since it expect 2.6.2.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1494309
[2] https://koji.fedoraproject.org/koji/buildinfo?buildID=983827

@tchaikov
Copy link
Contributor Author

@dillaman thanks for the heads-up. i will figure out a way to detect the missing aligned_alloc() in an updated cmake script.

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