Page MenuHomePhabricator

[sanitizer][fuchsia] Fix VMAR leak
ClosedPublic

Authored by cryptoad on Sep 18 2018, 11:47 AM.

Details

Summary

Destroy and close a range's vmar if all its memory was unmapped.

This addresses some performance regression due to the proliferation of vmars
when Secondary backed allocations are concerned with Scudo on Fuchsia.

When a Secondary backed allocation was freed, the associated
ReservedAddressRange was going away after unmapping the entirety of the
mapping, but without getting rid of the associated vmar properly (which
was created specifically for that mapping). This resulted in an increase of
defunct vmars, that in turn slowed down further new vmar allocations.

This appears to solve ZX-2560/ZX-2642, at least on QEMU.

Diff Detail

Event Timeline

cryptoad created this revision.Sep 18 2018, 11:47 AM
Herald added subscribers: Restricted Project, delcypher, kubamracek. · View Herald TranscriptSep 18 2018, 11:47 AM

Some before & after numbers for one of the benchmarks involved:

1552377     207294    1121659    2214385    1550481 nanoseconds              N/A Thread/CreateAndJoin
451883      35547     374475     984192     448331 nanoseconds              N/A Thread/CreateAndJoin
mseaborn added inline comments.Sep 18 2018, 12:06 PM
lib/sanitizer_common/sanitizer_fuchsia.cc
293

Note that if you're destroying the VMAR, you don't need to unmap any mappings from it first, because destroying the VMAR will remove those mappings. So you can avoid a syscall in that case.

cryptoad updated this revision to Diff 166020.Sep 18 2018, 12:40 PM
cryptoad marked an inline comment as done.

Skip the UnmapOrDieVmar call when unmapping the whole mapping, as
vmar_destroy will take care of this. We still have to do some bookkeeping
via DecreaseTotalMmap.

mcgrathr added inline comments.Sep 18 2018, 1:03 PM
lib/sanitizer_common/sanitizer_fuchsia.cc
283

This comment is a little ambiguous or misleading.
Destroying the VMAR is what unmaps the whole range.
Closing the VMAR handle just releases a now-useless resource; it has no effect itself on the state of mappings.

290

This seems questionable and at least needs comments.
By moving base_ up you are telling the local bookkeeping that the reservation starts at the new base.
But the actual VMAR still exists and will always cover the full range set at its creation.
So the address space between the old base_ and the new base_ is still reserved, but you've lost track of it.

cryptoad updated this revision to Diff 166023.Sep 18 2018, 1:13 PM
cryptoad marked an inline comment as done.

Correct a comment to reflect that it is the destruction of the vmar that is
responsible for the unmapping.

cryptoad added inline comments.Sep 18 2018, 1:32 PM
lib/sanitizer_common/sanitizer_fuchsia.cc
290

I agree with you, the ReservedAddressRange in their current state do not allow distinction between committed & reserved.
I think the original idea was to allow multiple Unmap to occur at the extremities of the mapping, but the use is currently confined to a full unmapping or a single pruning on either side in the event of Secondary aligned allocation.
I can skip the update of base_ & size_ to keep track of the reserved area if it feels more accurate to you, it shouldn't impact the current usage scenario.

mcgrathr added inline comments.Sep 18 2018, 2:10 PM
lib/sanitizer_common/sanitizer_fuchsia.cc
290

That does seem preferable, and with some comments about reserved regions that won't be ever be reused if that's the case.

cryptoad updated this revision to Diff 166036.Sep 18 2018, 2:24 PM
cryptoad marked 3 inline comments as done.

Do not update base_ & size_ to reflect the fact that the reserved range
remains unchanged. Adding a comment to clarify that partial unmapping still
leaves the memory reserved.

This revision is now accepted and ready to land.Sep 19 2018, 11:59 AM

Suggestion: you could start the commit message with:
"[sanitizer][fuchsia] Fix VMAR leak

Destroy and close a range's vmar if all its memory was unmapped.
..."

i.e. Start the commit message with the goal (fix the leak) and secondarily say how that is achieved. This should make it more obvious to the reader what the change does. Otherwise it's not so obvious that this fixes a leak.

I'm not very familiar with this code (and I'm not sure whether this fixes the leak in all cases) so I'll leave the rest of the review to Roland.

cryptoad retitled this revision from [sanitizer] Destroy and close a range's vmar if all its memory was unmapped to [sanitizer][fuchsia] Fix VMAR leak.Sep 19 2018, 12:04 PM
cryptoad edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.