This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Make the boundary of memory group aligned with region begin
ClosedPublic

Authored by Chia-hungDuan on Jan 30 2023, 1:41 PM.

Details

Summary

This alignment guarantee enables simpler group range check while page
releasing and a potential optimization which is, now all the pointers
from the same group are also inth same region, that means the complexity
in markFreeBlocks() can be reduced as well.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Jan 30 2023, 1:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 1:41 PM
Chia-hungDuan requested review of this revision.Jan 30 2023, 1:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 1:41 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Add one more context, on SizeClassAllocator64, now the pointer always gets compacted at least by using the offset from the region begin. This means the PrimaryCompactPtrT is able to use u32 in DefaultConfig as well. This will be done in later metadata size reduction work.

cferris added inline comments.Feb 2 2023, 4:27 PM
compiler-rt/lib/scudo/standalone/primary32.h
116–117

Would it be better to name this compactPtrGroupBase? And use Base instead of Beg for all of the variable names?

You use base in a few other places, and I think it makes it easier to understand the code since you are currently using both beg and base and sometimes comparing the values.

121–125

Since this function doesn't do anything, do you need it any more? Or do you want to keep it so that the primary32 and primary64 are the same?

Chia-hungDuan marked 2 inline comments as done.
  1. Address review comment
  2. Restrict (GroupSize == RegionSize) in SizeClassAllocator32 so that the logic of page releasing will be easier
Chia-hungDuan added inline comments.
compiler-rt/lib/scudo/standalone/primary32.h
116–117

Agree, done!

121–125

Right, it tries to align the style with primary64

cferris requested changes to this revision.Feb 14 2023, 12:01 PM

Only a couple of nits.

compiler-rt/lib/scudo/standalone/local_cache.h
65

base

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

What is RegionBeg? I think the names have changed so I'm not sure if this should be RegionSize instead?

This revision now requires changes to proceed.Feb 14 2023, 12:01 PM
Chia-hungDuan marked 2 inline comments as done.

Address review comment

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

Oops, forgot to update the log. Done.

cferris accepted this revision.Feb 23 2023, 6:19 PM

LGTM

This revision is now accepted and ready to land.Feb 23 2023, 6:19 PM