issue: 3884801 Check for resident hugepages to avoid SIGBUS #204
Conversation
This file contains 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
pasis
changed the title
pasis
added
enhancement
AlexanderGrissik
force-pushed
the
vNext
branch
from
cfce2e8
to
51c2340
Compare
pasis
force-pushed
the
hugepage-mincore
branch
from
b04003d
to
437bb39
Compare
In a containerized environment mmap() beyond the hugepages limit can be successful, but a further memory access triggers SIGBUS and terminates the process. Check that all the allocated hugepages are resident with mincore() to avoid the SIGBUS issue. Cost of the feature depends on the allocation size and hugepage size: - 32GB with 2MB pages takes 4400 us - 32GB with 1GB pages takes 11 us - 32MB with 2MB pages takes 5-6 us We expect a big memory preallocation at the start (2GB by default) and rare 32MB allocations during warmup period. Therefore, this feature can add several additional 5us latency spikes during warmup period. Signed-off-by: Dmytro Podgornyi <[email protected]>
pasis
force-pushed
the
hugepage-mincore
branch
from
437bb39
to
eedc045
Compare
The parameter disables the check for resident hugepages. The check can be expensive and, on the other hand, can be irrelevant based on the system configuration. Therefore, disabling it reduces the initialization time which can be crucial in some scenarios. The name for the parameter is chosen to extend the idea of reducing initialization time in the future. Signed-off-by: Dmytro Podgornyi <[email protected]>
| for (size_t i = 0; rc == 0 && i < pages_nr; ++i) { | ||
| unsigned char vec; | ||
| rc = mincore(page_ptr, 1, &vec); | ||
| resident_nr += rc == 0 ? (vec & 1U) : 0; |
There was a problem hiding this comment.
Do we really need to count or we can just fail on the first bad hugepage ptr?
There was a problem hiding this comment.
we can stop, however, this is expected to be very unlikely case, so no point in this optimization and I'd suggest choosing the more readable variant (not necessarily current variant)
There was a problem hiding this comment.
i think that stopping on the first one may create simpler impl, maybe even without many different dbg_logs.
There was a problem hiding this comment.
Also we can loop while ptr < end_ptr ... and avoid many different vars and stuff
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
In a containerized environment mmap() beyond the hugepages limit can be successful, but a further memory access triggers SIGBUS and terminates the process. Check that all the allocated hugepages are resident with mincore() to avoid the SIGBUS issue.
Cost of the feature depends on the allocation size and hugepage size:
We expect a big memory preallocation at the start (2GB by default) and
rare 32MB allocations during warmup period. Therefore, this feature can
add several additional 5us latency spikes during warmup period.
What
Check for resident hugepages after allocation.
Why ?
Avoid SIGBUS in containerized environments.
Change type
What kind of change does this PR introduce?
Check list