This is an archive of the discontinued LLVM Phabricator instance.

[scudo][standalone] Do not fill 32b regions at once
ClosedPublic

Authored by cryptoad on Mar 3 2020, 11:16 AM.

Details

Summary

For the 32b primary, whenever we created a region, we would fill it
all at once (eg: create all the transfer batches for all the blocks
in that region). This wasn't ideal as all the potential blocks in
a newly created region might not be consummed right away, and it was
using extra memory (and release cycles) to keep all those free
blocks.

So now we keep track of the current region for a given class, and
how filled it is, carving out at most MaxNumBatches worth of
blocks at a time.

Additionally, lower MaxNumBatches on Android from 8 to 4. This
lowers the randomness of blocks, which isn't ideal for security, but
keeps things more clumped up for PSS/RSS accounting purposes.

Diff Detail

Event Timeline

cryptoad created this revision.Mar 3 2020, 11:16 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 3 2020, 11:16 AM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript
cryptoad updated this revision to Diff 248002.Mar 3 2020, 1:02 PM

For the current region, only release up to the current size.

hctim accepted this revision.Mar 4 2020, 8:09 AM

This all looks reasonable to me, but the code in populateFreeList is dense. It might be useful to have some block-level comments to abstract over the nuance.

This revision is now accepted and ready to land.Mar 4 2020, 8:09 AM
cferris accepted this revision.Mar 4 2020, 9:32 AM

The performance after this change is slightly worse for 32 bit, but not by much. and a lot of time seems to be within the variance However, it dramatically reduces the RSS for dex2oat, where it's much closer to jemalloc. It also reduces some of the traces RSS, but not by a large margin.

cryptoad updated this revision to Diff 248235.Mar 4 2020, 10:23 AM

Commenting populateFreeList, with some additional white-line
separation for code blocks.

Additionally I removed the update to LastReleaseAtNs in said function,
we check for enough PushedBlocks in releaseToOS anyway, which is
not affected by populateFreeList, so updating the timer didn't add
anything.

This revision was automatically updated to reflect the committed changes.