This is an archive of the discontinued LLVM Phabricator instance.

Release memory to OS only when the requested range covers the entire page
ClosedPublic

Authored by alekseyshl on Nov 29 2016, 2:34 PM.

Details

Summary

The current code was sometimes attempting to release huge chunks of
memory due to undesired RoundUp/RoundDown interaction when the requested
range is fully contained within one memory page.

Event Timeline

alekseyshl updated this revision to Diff 79645.Nov 29 2016, 2:34 PM
alekseyshl retitled this revision from to Release memory to OS only when the requested range covers the entire page.
alekseyshl updated this object.
alekseyshl added a reviewer: eugenis.
alekseyshl added a subscriber: llvm-commits.
eugenis edited edge metadata.Nov 29 2016, 2:51 PM

Nice.
I understand this is never a problem in practice, because the kernel would never release a region of size (uptr)(-4096).

lib/sanitizer_common/sanitizer_posix_libcdep.cc
58–59

Is this function still used?

It is not _really_a problem, but making pontless call, the one we know is going to fail, is not a good idea to me.

lib/sanitizer_common/sanitizer_posix_libcdep.cc
58–59

Yes, it is, tsan has a bit more elaborated logic and I decided not to touch it, at least in this patch.

I just don't like that now we have two functions doing the same thing, one takes (start, end) pointers, the other - (start, size), and the difference in function names (the word "Pages") has nothing to do with the difference in their behavior.

How about we add the rounding logic to ReleaseMemoryToOS() and remove it from the call sites?

alekseyshl updated this revision to Diff 79676.Nov 29 2016, 4:49 PM
alekseyshl edited edge metadata.
  • Remove unused ReleaseMemoryToOS
alekseyshl marked 2 inline comments as done.Nov 29 2016, 4:50 PM

Good point, removed it.

eugenis accepted this revision.Nov 29 2016, 4:58 PM
eugenis edited edge metadata.

LGTM

This revision is now accepted and ready to land.Nov 29 2016, 4:58 PM
This revision was automatically updated to reflect the committed changes.