This is an archive of the discontinued LLVM Phabricator instance.

Release the shadow memory used by the mmap range at munmap
ClosedPublic

Authored by stephan.yichao.zhao on Oct 1 2020, 11:06 AM.

Details

Summary

When an application does a lot of pairs of mmap and munmap, if we did
not release shadoe memory used by mmap addresses, this would increase
memory usage.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2020, 11:06 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
stephan.yichao.zhao requested review of this revision.Oct 1 2020, 11:06 AM

Can we add a test that ensures RSS doesn't increase after a mmap + munmap pair?

compiler-rt/lib/dfsan/dfsan_interceptors.cpp
53

Nit: GetPageSizeCached

addressed comments
added a test case

stephan.yichao.zhao marked an inline comment as done.Oct 1 2020, 2:19 PM
morehouse added inline comments.Oct 1 2020, 3:35 PM
compiler-rt/test/dfsan/munmap_release_shadow.c
11

Nit: get_rss_kb seems simpler

17

Why ssize_t instead of size_t or long?

38

Rather than returning errors from this function, can we simplify by asserting inline?

42

I think the Linux kernel avoids reusing address space by default, so we don't need to give mmap hints.

I think I'd prefer removing the hints to simplify the test.

68

I'm not sure, but 4GB RSS might be too much on the buildbots. Can we simplify the test and reduce this by doing a single large mmap + munmap instead of many loops?

stephan.yichao.zhao marked 5 inline comments as done.

addressed comments

compiler-rt/lib/dfsan/dfsan_interceptors.cpp
37

With madvise by DONTNEED OR FREE (SANITIZER_MADVISE_DONTNEED) at munmap, is it possible to also remove these dfsan_set_labels ? Before the next access OS will zero-fill data.

https://www.man7.org/linux/man-pages/man2/madvise.2.html says
"""
MADV_DONTNEED

...
After a successful MADV_DONTNEED operation, the semantics of
memory access in the specified region are changed: subsequent
accesses of pages in the range will succeed, but will result
in either ... or zero-
fill-on-demand pages for anonymous private mappings.

MADV_FREE (since Linux 4.5)

...
Once pages in the range have been freed, the caller will see
zero-fill-on-demand pages upon subsequent page references.

"""

compiler-rt/test/dfsan/munmap_release_shadow.c
42

In our system, for some reason, the mmap likes to reuse the last address. Sometimes all returned addresses are the same from hundreds of runs.
With the hint, it is able to return different addresses.

68

reduced to 50 loops. This reduced the max RSS below 500M. This also reduces its test time from 20s to 2-3s.

morehouse added inline comments.Oct 2 2020, 8:58 AM
compiler-rt/lib/dfsan/dfsan_interceptors.cpp
37

Yes, it should work as an optimization. Happy to take a separate patch for it.

compiler-rt/test/dfsan/munmap_release_shadow.c
42

Ok. Then maybe let's make hint a static variable in mmap_track_and_munmap to simplify the logic.

68

Do we need *any* loops at all? Isn't a single mmap + touch the memory + munmap enough to test that RSS increases then goes back down?

The test seems overly complicated if it could be tested with something as simple as:

const size_t map_size = 100 << 20;
size_t before = get_rss_kb();

void *p = mmap(NULL, map_size, ...);
memset(p, 0xff, map_size);
size_t after_mmap = get_rss_kb();

munmap(p, map_size);
size_t after_munmap = get_rss_kb();

assert(after_mmap >= before + 3 * map_size);
assert(after_munmap <= after_mmap - 3 * map_size);
stephan.yichao.zhao retitled this revision from Release memory at munmap to Release the shadow memory used by the mmap range at munmap.Oct 2 2020, 11:40 AM
stephan.yichao.zhao edited the summary of this revision. (Show Details)

addressed comments

stephan.yichao.zhao marked 2 inline comments as done.Oct 2 2020, 11:51 AM
stephan.yichao.zhao added inline comments.
compiler-rt/test/dfsan/munmap_release_shadow.c
42

removed hint after simplifying the test.

68

Still called dfsan_set_label after memset because the dfsan_set_label in mmap may not touch pages if initial data are zero-filled.

Harbormaster completed remote builds in B73819: Diff 295883.
morehouse accepted this revision.Oct 2 2020, 1:05 PM
morehouse added inline comments.
compiler-rt/test/dfsan/munmap_release_shadow.c
28–31
This revision is now accepted and ready to land.Oct 2 2020, 1:05 PM
stephan.yichao.zhao marked 3 inline comments as done.

addressed comment

compiler-rt/test/dfsan/munmap_release_shadow.c
28–31

Thank you!

This revision was landed with ongoing or failed builds.Oct 2 2020, 1:18 PM
This revision was automatically updated to reflect the committed changes.
glider added a subscriber: glider.Oct 5 2020, 12:47 AM

I might be missing something, but if you release one of the shadow pages to the OS, couldn't it then accidentally use that page for one of the user anonymous mappings, thus breaking DFSan logic?

I might be missing something, but if you release one of the shadow pages to the OS, couldn't it then accidentally use that page for one of the user anonymous mappings, thus breaking DFSan logic?

The mmap interceptions zero-out shadow spaces and anonymous mappings zero-fill mapped pages. So returning shadow pages at munmap equals to the existing behavior.

eugenis added a subscriber: eugenis.Oct 5 2020, 9:18 AM

I might be missing something, but if you release one of the shadow pages to the OS, couldn't it then accidentally use that page for one of the user anonymous mappings, thus breaking DFSan logic?

ReleaseMemoryPagesToOS is not munmap, it's madvise.

glider added a comment.Oct 5 2020, 9:54 AM

Fine, thanks for explaining!