Move check of MADV_DONTNEED into runtime (fixes clickhouse under qemu)#18169
Closed
azat wants to merge 2 commits intoClickHouse:masterfrom
Closed
Move check of MADV_DONTNEED into runtime (fixes clickhouse under qemu)#18169azat wants to merge 2 commits intoClickHouse:masterfrom
azat wants to merge 2 commits intoClickHouse:masterfrom
Conversation
robot-clickhouse
added
the
pr-not-for-changelog
azat
force-pushed
the
MADV_DONTNEED-in-runtime
branch
3 times, most recently
from
be41611 to
9774c88
Compare
azat
force-pushed
the
MADV_DONTNEED-in-runtime
branch
from
9774c88 to
8a7027d
Compare
alexey-milovidov
approved these changes
Dec 16, 2020
Member
alexey-milovidov
left a comment
alexey-milovidov
left a comment
There was a problem hiding this comment.
LGTM.
BTW, extern "C" in c source file looks pointless (as it cannot be included in cpp file).
Member
Author
Indeed, this is a copy-paste from glibc-compatibility.c, let's remove it |
There is no need in extern "C" when the is not processed as c++ module.
Member
Author
DetailsThe problem is that sanitizer is not "initialized" yet, and sanitizers traits ( |
azat
commented
Dec 17, 2020
Member
Author
|
Closed in favor of #18238 |
azat
added a commit
to azat/ClickHouse
that referenced
this pull request
Dec 19, 2020
qemu does not support MADV_DONTNEED, and by not support it simply ignore it (i.e. return 0 -- no error). This issue has been "fixed" in ClickHouse#15590, however it just terminates the process, and completely breaks clickhouse under qemu (see also ClickHouse#15174). But there is no need in such strong protection, we can stop using madvise in jemalloc if MADV_DONTNEED does not work properly. And this is what ClickHouse#18169 was tried to do (by override madvise), however this will break sanitizers, at least TSAN and UBSAN. The problem there is that sanitizers initialization code uses madvise (and there is no way to turn this off with TSAN_OPTIONS) and overwritten madvise function will have sanitizers traits (__tsan_func_entry), while TSAN is not ready for this, and eventually it SIGSEGV. Interesting thing is that in the recent clang-12, madvise was replaced with direct syscall [1]. [1]: llvm/llvm-project@9f8c403 But it is better to make clickhouse compatible with clang < 12 too, so instead of override madvise completely, the runtime check was moved into the jemalloc code [2]. [2]: ClickHouse/jemalloc#1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Move check of MADV_DONTNEED (that is required by jemalloc) into runtime (fixes clickhouse under qemu)
Refs: #15174
Refs: #15590
Сс: @alexey-milovidov
Changelog category (leave one):
P.S. I've checked it manually