This is an archive of the discontinued LLVM Phabricator instance.

Differential D136898

[scudo] Fix the calculating of memory group usage
ClosedPublic

Authored by Chia-hungDuan on Oct 27 2022, 4:16 PM.

Details

Reviewers
cferris
cryptoad
Commits
rG6130d67f70ad: [scudo] Fix the calculating of memory group usage
Summary

In SizeClassAllocator64, the boundary of a memory group may not align to
the region begin. Which means the begin addr of a memory group may
smaller than region begin. This leads to wrong judgement of memory group
usage.

Diff Detail

Unit TestsFailed

TimeTest
210 msx64 debian > Clang.OpenMP::remarks_parallel_in_multiple_target_state_machines.c
200 msx64 debian > Clang.OpenMP::remarks_parallel_in_target_state_machine.c

Event Timeline

Chia-hungDuan created this revision.Oct 27 2022, 4:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 4:16 PM
Herald added a subscriber: Enna1. · View Herald Transcript
Chia-hungDuan requested review of this revision.Oct 27 2022, 4:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 4:16 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
Chia-hungDuan added a parent revision: D136873: [scudo] Lazy initialize the PageMap while page releasing.Oct 27 2022, 4:18 PM
cferris added a comment.Oct 27 2022, 4:32 PM

Only some comment nits.

compiler-rt/lib/scudo/standalone/primary64.h
705–713

does not necessarily have the same alignment as

706

a Region

Harbormaster completed remote builds in B194776: Diff 471319.Oct 27 2022, 4:36 PM
Chia-hungDuan updated this revision to Diff 471327.Oct 27 2022, 4:45 PM
Chia-hungDuan marked 2 inline comments as done.

Address review comment

Chia-hungDuan added a reviewer: cryptoad.Oct 27 2022, 4:46 PM
Harbormaster completed remote builds in B194782: Diff 471327.Oct 27 2022, 5:04 PM
cryptoad added inline comments.Oct 27 2022, 5:05 PM
compiler-rt/lib/scudo/standalone/primary32.h
711

s/fromt/front/

Chia-hungDuan updated this revision to Diff 471352.Oct 27 2022, 6:21 PM
Chia-hungDuan marked an inline comment as done.

Fix typo

cferris accepted this revision.Oct 27 2022, 6:23 PM

LGTM

This revision is now accepted and ready to land.Oct 27 2022, 6:23 PM
Harbormaster completed remote builds in B194800: Diff 471352.Oct 27 2022, 6:42 PM
Chia-hungDuan updated this revision to Diff 471396.Oct 27 2022, 10:13 PM

Add more comments and make the logic more clear

Harbormaster completed remote builds in B194830: Diff 471396.Oct 27 2022, 10:35 PM
Chia-hungDuan updated this revision to Diff 471579.Oct 28 2022, 9:37 AM

update the description

Harbormaster completed remote builds in B194961: Diff 471579.Oct 28 2022, 9:55 AM
cryptoad accepted this revision.Oct 28 2022, 12:13 PM
cferris accepted this revision.Oct 28 2022, 1:24 PM

LGTM

This revision was landed with ongoing or failed builds.Oct 28 2022, 2:13 PM
Closed by commit rG6130d67f70ad: [scudo] Fix the calculating of memory group usage (authored by Chia-hungDuan). · Explain Why
This revision was automatically updated to reflect the committed changes.
Chia-hungDuan added a commit: rG6130d67f70ad: [scudo] Fix the calculating of memory group usage.
Chia-hungDuan added a reverting change: rG69fe7abb393b: Revert "[scudo] Fix the calculating of memory group usage".Oct 28 2022, 3:16 PM

Revision Contents

PathSize
compiler-rt/
lib/
scudo/
standalone/
4 lines
18 lines

Diff 471352

compiler-rt/lib/scudo/standalone/primary32.h

Show First 20 Lines • Show All 702 Lines • ▼ Show 20 Lines for (BatchGroup &BG : Sci->FreeList) {
continue; continue;
uptr AllocatedGroupSize = BG.GroupId == CurRegionGroupId uptr AllocatedGroupSize = BG.GroupId == CurRegionGroupId
? Sci->CurrentRegionAllocated ? Sci->CurrentRegionAllocated
: GroupSize; : GroupSize;
if (AllocatedGroupSize == 0) if (AllocatedGroupSize == 0)
continue; continue;
// TransferBatches are pushed in front of BG.Batches. The first one may
cryptoadUnsubmitted
Done
ReplyInline Actions

s/fromt/front/

cryptoad: s/fromt/front/
// not have all caches used.
const uptr NumBlocks = (BG.Batches.size() - 1) * BG.MaxCachedPerBatch + const uptr NumBlocks = (BG.Batches.size() - 1) * BG.MaxCachedPerBatch +
BG.Batches.back()->getCount(); BG.Batches.front()->getCount();
const uptr BytesInBG = NumBlocks * BlockSize; const uptr BytesInBG = NumBlocks * BlockSize;
// Given the randomness property, we try to release the pages only if the // Given the randomness property, we try to release the pages only if the
// bytes used by free blocks exceed certain proportion of allocated // bytes used by free blocks exceed certain proportion of allocated
// spaces. // spaces.
if (CheckDensity && (BytesInBG * 100U) / AllocatedGroupSize < if (CheckDensity && (BytesInBG * 100U) / AllocatedGroupSize <
(100U - 1U - BlockSize / 16U)) { (100U - 1U - BlockSize / 16U)) {
continue; continue;
} }
▲ Show 20 Lines • Show All 42 Lines • Show Last 20 Lines

compiler-rt/lib/scudo/standalone/primary64.h

Show First 20 Lines • Show All 696 Lines • ▼ Show 20 Lines NOINLINE uptr releaseToOSMaybe(RegionInfo *Region, uptr ClassId,
auto DecompactPtr = [CompactPtrBase](CompactPtrT CompactPtr) { auto DecompactPtr = [CompactPtrBase](CompactPtrT CompactPtr) {
return decompactPtrInternal(CompactPtrBase, CompactPtr); return decompactPtrInternal(CompactPtrBase, CompactPtr);
}; };
for (BatchGroup &BG : Region->FreeList) { for (BatchGroup &BG : Region->FreeList) {
const uptr PushedBytesDelta = const uptr PushedBytesDelta =
BG.PushedBlocks - BG.PushedBlocksAtLastCheckpoint; BG.PushedBlocks - BG.PushedBlocksAtLastCheckpoint;
if (PushedBytesDelta * BlockSize < PageSize) if (PushedBytesDelta * BlockSize < PageSize)
continue; continue;
const uptr BatchGroupEnd = // Group boundary does not necessarily have the same alignment as Region.
batchGroupBase(BG.GroupId, CompactPtrBase) + GroupSize; // It may sit across a Region boundary.
cferrisUnsubmitted
Done
ReplyInline Actions

a Region

cferris: a Region
const uptr AllocatedGroupSize = AllocatedUserEnd >= BatchGroupEnd const uptr BatchGroupBeg =
Max(batchGroupBase(BG.GroupId, CompactPtrBase), Region->RegionBeg);
DCHECK_GE(AllocatedUserEnd, BatchGroupBeg);
const uptr AllocatedGroupSize =
AllocatedUserEnd >= BatchGroupBeg + GroupSize
? GroupSize ? GroupSize
: AllocatedUserEnd - BatchGroupEnd; : AllocatedUserEnd - BatchGroupBeg;
cferrisUnsubmitted
Done
ReplyInline Actions

does not necessarily have the same alignment as

cferris: does not necessarily have the same alignment as
if (AllocatedGroupSize == 0) if (AllocatedGroupSize == 0)
continue; continue;
// TransferBatches are pushed in front of BG.Batches. The first one may
// not have all caches used.
const uptr NumBlocks = (BG.Batches.size() - 1) * BG.MaxCachedPerBatch + const uptr NumBlocks = (BG.Batches.size() - 1) * BG.MaxCachedPerBatch +
BG.Batches.back()->getCount(); BG.Batches.front()->getCount();
const uptr BytesInBG = NumBlocks * BlockSize; const uptr BytesInBG = NumBlocks * BlockSize;
// Given the randomness property, we try to release the pages only if the // Given the randomness property, we try to release the pages only if the
// bytes used by free blocks exceed certain proportion of group size. Note // bytes used by free blocks exceed certain proportion of group size. Note
// that this heuristic only applies when all the spaces in a BatchGroup // that this heuristic only applies when all the spaces in a BatchGroup
// are allocated. // are allocated.
if (CheckDensity && (BytesInBG * 100U) / AllocatedGroupSize < if (CheckDensity && (BytesInBG * 100U) / AllocatedGroupSize <
(100U - 1U - BlockSize / 16U)) { (100U - 1U - BlockSize / 16U)) {
continue; continue;
Show All 28 Lines