This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Extract steps releaseToOSMaybe into functions in
ClosedPublic

Authored by Chia-hungDuan on Jun 9 2023, 5:08 PM.

Details

Summary

This refactor helps us identify which steps need FLLock so that we can
reduce the holding time of FLLock in SizeClassAllocator64.

Also move the data members to the end of class to align the style in
SizeClassAllocator32.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Jun 9 2023, 5:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2023, 5:08 PM
Chia-hungDuan requested review of this revision.Jun 9 2023, 5:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2023, 5:08 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Update comment

Chia-hungDuan edited the summary of this revision. (Show Details)Jun 9 2023, 5:10 PM
cferris requested changes to this revision.Jun 26 2023, 7:19 PM
cferris added inline comments.
compiler-rt/lib/scudo/standalone/primary64.h
912–913

Should maybe say:

in step 3 & 4 described in the comment below.

922

worth doing a page

925

It feels like there is a subtle difference between this and the previous code. If BytesInFreeList is zero, we return no matter the value of ReleaseType. Now, if ReleaseType is Force, then we will continue even if BytesInFreeList is zero. Is that expected?

This check feels like it's not quite right, it feels like it should be checking ForceAll instead.

963

can release pages.

964

I think you mean that the groups have pages that can be released?

The second sentence might be better written as:

Use a heuristic to gather groups that are candidates for doing a release.

Or maybe something similar.

968

It's not clear what the difference is between Normal, Force and ForceAll. Why is using the Force option doing this? It probably warrants a comment.

This revision now requires changes to proceed.Jun 26 2023, 7:19 PM
Chia-hungDuan marked 4 inline comments as done.

Address review comments

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

You're right, the Force here and below should be ForceAll instead.

About the BytesInFreeList == 0, you're right, it should be checked for all ReleaseType.

After the fixes above, there is one slightly difference is

if (BytesInFreeList <=
    Region->ReleaseInfo.BytesInFreeListAtLastCheckpoint) {
  Region->ReleaseInfo.BytesInFreeListAtLastCheckpoint = BytesInFreeList;
}

It seems that we bypass the update for ForceAll, however, this variable will be updated after releaseFreeMemoryToOS()

releaseFreeMemoryToOS(Context, Recorder, SkipRegion);

if (Recorder.getReleasedRangesCount() > 0) {
  Region->ReleaseInfo.BytesInFreeListAtLastCheckpoint = BytesInFreeList;
  Region->ReleaseInfo.RangesReleased += Recorder.getReleasedRangesCount();
  Region->ReleaseInfo.LastReleasedBytes = Recorder.getReleasedBytes();
}

Given that ForceAll is likely to release some memory, I just lift the ForceAll check out the hasChanceToReleasePages and think it will align the ForceAll concept more.
"ForceAll doesn't take any heuristic, just release the pages (except the zero free byte)"

Same reason for the GroupsToRelease below.

968

This should be ForceAll. I guess using ForceAll will be self-explained here. Let me know if you think a comment will make it more clear.

Chia-hungDuan added inline comments.Jun 26 2023, 9:03 PM
compiler-rt/lib/scudo/standalone/primary64.h
968

Add one more explanation here,
In the past, even if ForceAll, we still do the CheckDensity part in collecting GroupsToRelease,

if (CheckDensity) {
  const uptr ReleaseThreshold =
      (AllocatedGroupSize * (100 - 1U - BlockSize / 16U)) / 100U;
  const bool HighDensity = BytesInBG >= ReleaseThreshold;
  const bool MayHaveReleasedAll = NumBlocks >= (GroupSize / BlockSize);

I think this was against our expectation for ForceAll so I also fix it here.

1091–1092

This is another stuff that ForceAll won't check after. I think this is a dead condition (if the AllocatedGroupSize is zero, it shouldn't have any BatchGroup created. I feel like this is a legacy check.

Will remove it along with next round review

cferris requested changes to this revision.Jun 28 2023, 6:35 PM
cferris added inline comments.
compiler-rt/lib/scudo/standalone/primary64.h
969

I presume that if force all, then doing this make sense since you are going to operate on all of the blocks. Is there any chance that you could call this but not do a release? For example, would it ever be legal for hasBlockMarked to return false when BlockList is not empty?

It seems at least one potential issue is the page map cannot be created in markFreeBlocks. But maybe that causes problems in both cases.

1063

Isn't this clear unneeded? The class initializes Size, First and Last to the same values as clear does.

This revision now requires changes to proceed.Jun 28 2023, 6:35 PM
Chia-hungDuan marked an inline comment as done.

Address review comment

Chia-hungDuan added inline comments.Jun 28 2023, 10:39 PM
compiler-rt/lib/scudo/standalone/primary64.h
969

Nice Catch! This is a bug that has existed since we allow failure of markFreeBlocks(). Will fix that in a separate CL (will be the parent CL of this) and also restore the freelist in this CL

1063

hmm, I do have the impression that this won't clear. Maybe I messed it up with TransferBatch which doesn't do clear().

Removed.

cferris accepted this revision.Jun 29 2023, 3:22 PM

LGTM.

This revision is now accepted and ready to land.Jun 29 2023, 3:22 PM